Jesse McKenna (Gerrit)

unread,

May 15, 2026, 7:51:01 PMMay 15

to Etienne Pierre-Doray, Sean Maher, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Etienne Pierre-Doray and Sean Maher

Jesse McKenna added 1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Jesse McKenna . resolved

PTAL - any and all feedback is welcome!

I found that I had to finalize the previous packet and create a new one in order to add interned data to the trace packet, but I'm open to suggestions if there's a better way.

Open in Gerrit

Related details

Attention is currently required from:

Etienne Pierre-Doray
Sean Maher

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sean Maher (Gerrit)

unread,

May 16, 2026, 9:17:23 PMMay 16

to Jesse McKenna, Etienne Pierre-Doray, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Etienne Pierre-Doray and Jesse McKenna

Sean Maher added 1 comment

File components/tracing/common/etw_consumer_win.cc

Line 365, Patchset 7 (Latest):void EtwConsumer::HandleStackWalkEvent(const EVENT_HEADER& header,

Sean Maher . unresolved

naisu

Open in Gerrit

Related details

Attention is currently required from:

Etienne Pierre-Doray
Jesse McKenna

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Etienne Pierre-Doray (Gerrit)

unread,

May 19, 2026, 3:13:46 PMMay 19

to Jesse McKenna, Sean Maher, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Jesse McKenna

Etienne Pierre-Doray added 6 comments

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Etienne Pierre-Doray . resolved

Nice!

File components/tracing/common/etw_consumer_win.h

Line 252, Patchset 7 (Latest): InterningIndex<TypeList<uintptr_t>, SizeList<1024>> interned_frames_;

Etienne Pierre-Doray . unresolved

See my comment re. pointer_size below. This would need to be uint64_t to work in all cases.

Line 250, Patchset 7 (Latest): InterningIndex<TypeList<size_t>, SizeList<1024>> interned_callstacks_;

Etienne Pierre-Doray . unresolved

Interned data needs to reset on EtwSystemDataSource::WillClearIncrementalState, so it gets emitted again.

File components/tracing/common/etw_consumer_win.cc

Line 374, Patchset 7 (Latest):

  // Read and validate the contents of `packet_data`.
  base::BufferIterator<const uint8_t> iterator{packet_data};
  auto qpc_timestamp = *iterator.CopyObject<uint64_t>();
  auto process_id = *iterator.CopyObject<uint32_t>();
  auto thread_id = *iterator.CopyObject<uint32_t>();

Etienne Pierre-Doray . unresolved

This is missing the typical bound check:

```
if (packet_data.size() < kMinimumSize) {
  return false;
}
```

Line 384, Patchset 7 (Latest):

  base::span<const uintptr_t> call_stack =
      iterator.Span<const uintptr_t>(remaining_bytes / pointer_size);

Etienne Pierre-Doray . unresolved

I think this incorrectly assumes "Stack" members are uintptr_t, but they are documented as pointer_size.
To correctly handle this while keeping the zero copy impl, you could do a template <class T> HandleStackWalkEventImpl() and then

```
if (pointer_size == sizeof(uint32_t)) {
  HandleStackWalkEventImpl<uint32_t>(...);
) else {
  HandleStackWalkEventImpl<uint64_t>(...);
}
```

Line 1016, Patchset 7 (Latest): packet_handle_ = trace_writer_->NewTracePacket();

Etienne Pierre-Doray . unresolved

I think we need to unconditionally set SEQ_NEEDS_INCREMENTAL_STATE, since the packet may be reused for stackwalk after interned data was emitted in a previous packet.

Open in Gerrit

Related details

Attention is currently required from:

Jesse McKenna

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jesse McKenna (Gerrit)

unread,

May 29, 2026, 2:12:26 PM (13 days ago) May 29

to Chromium LUCI CQ, Etienne Pierre-Doray, Sean Maher, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Etienne Pierre-Doray and Sean Maher

Jesse McKenna voted and added 7 comments

Votes added by Jesse McKenna

Commit-Queue

+1

7 comments

Patchset-level comments

File-level comment, Patchset 8:

Jesse McKenna . resolved

Thanks so much for the thoughtful review!

I had to do some BUILD.gn changes to work around a circular dependency with using `InterningIndex`, so the latest patchset also includes those.

File components/tracing/common/etw_consumer_win.h

Line 252, Patchset 7: InterningIndex<TypeList<uintptr_t>, SizeList<1024>> interned_frames_;

Etienne Pierre-Doray . resolved

See my comment re. pointer_size below. This would need to be uint64_t to work in all cases.

Jesse McKenna

Done - changed to uint64_t

Line 250, Patchset 7: InterningIndex<TypeList<size_t>, SizeList<1024>> interned_callstacks_;

Etienne Pierre-Doray . resolved

Interned data needs to reset on EtwSystemDataSource::WillClearIncrementalState, so it gets emitted again.

Jesse McKenna

Done

File components/tracing/common/etw_consumer_win.cc

Line 365, Patchset 7:void EtwConsumer::HandleStackWalkEvent(const EVENT_HEADER& header,

Sean Maher . resolved

naisu

Jesse McKenna

Thanks!

Line 374, Patchset 7: // Read and validate the contents of `packet_data`.


  base::BufferIterator<const uint8_t> iterator{packet_data};
  auto qpc_timestamp = *iterator.CopyObject<uint64_t>();
  auto process_id = *iterator.CopyObject<uint32_t>();
  auto thread_id = *iterator.CopyObject<uint32_t>();

Etienne Pierre-Doray . resolved

This is missing the typical bound check:
```
if (packet_data.size() < kMinimumSize) {
  return false;
}
```

Jesse McKenna

Oops, done

Line 384, Patchset 7: base::span<const uintptr_t> call_stack =


      iterator.Span<const uintptr_t>(remaining_bytes / pointer_size);

Etienne Pierre-Doray . resolved

I think this incorrectly assumes "Stack" members are uintptr_t, but they are documented as pointer_size.
To correctly handle this while keeping the zero copy impl, you could do a template <class T> HandleStackWalkEventImpl() and then
```
if (pointer_size == sizeof(uint32_t)) {
  HandleStackWalkEventImpl<uint32_t>(...);
) else {
  HandleStackWalkEventImpl<uint64_t>(...);
}
```

Jesse McKenna

Thanks for pointing this out! For the sake of simplicity I opted to use `CopyPointer` rather than templating, but feel free to let me know if you disagree.

Line 1016, Patchset 7: packet_handle_ = trace_writer_->NewTracePacket();

Etienne Pierre-Doray . resolved

I think we need to unconditionally set SEQ_NEEDS_INCREMENTAL_STATE, since the packet may be reused for stackwalk after interned data was emitted in a previous packet.

Jesse McKenna

Done

Open in Gerrit

Related details

Attention is currently required from:

Etienne Pierre-Doray
Sean Maher

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Etienne Pierre-Doray (Gerrit)

unread,

Jun 1, 2026, 8:37:38 AM (10 days ago) Jun 1

to Jesse McKenna, Chromium LUCI CQ, Sean Maher, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Jesse McKenna and Sean Maher

Etienne Pierre-Doray voted and added 2 comments

Votes added by Etienne Pierre-Doray

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Etienne Pierre-Doray . resolved

LGTM % thread_id

File components/tracing/common/etw_consumer_win.cc

Line 401, Patchset 9 (Latest): (void)*iterator.CopyObject<uint32_t>(); // StackThread

Etienne Pierre-Doray . unresolved

We should write this to EtwTraceEvent::thread_id?

Open in Gerrit

Related details

Attention is currently required from:

Jesse McKenna
Sean Maher

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sean Maher (Gerrit)

unread,

Jun 3, 2026, 10:55:47 AM (8 days ago) Jun 3

to Jesse McKenna, Code Review Nudger, Etienne Pierre-Doray, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Jesse McKenna

Sean Maher voted and added 1 comment

Votes added by Sean Maher

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Sean Maher . resolved

lgtm, but i haven't done as thorough of a review as eti

Open in Gerrit

Related details

Attention is currently required from:

Jesse McKenna

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jesse McKenna (Gerrit)

unread,

Jun 4, 2026, 7:54:43 PM (6 days ago) Jun 4

to Sean Maher, Code Review Nudger, Etienne Pierre-Doray, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Jesse McKenna voted and added 2 comments

Votes added by Jesse McKenna

Commit-Queue

+2

2 comments

Patchset-level comments

File-level comment, Patchset 11 (Latest):

Jesse McKenna . resolved

Thank you for reviewing!

File components/tracing/common/etw_consumer_win.cc

Line 401, Patchset 9: (void)*iterator.CopyObject<uint32_t>(); // StackThread

Etienne Pierre-Doray . resolved

We should write this to EtwTraceEvent::thread_id?

Jesse McKenna

Done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jun 4, 2026, 9:33:42 PM (6 days ago) Jun 4

to Jesse McKenna, Sean Maher, Code Review Nudger, Etienne Pierre-Doray, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

9 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: components/tracing/common/etw_consumer_win.h
Insertions: 9, Deletions: 0.

The diff is too large to show. Please review the diff.
```
```
The name of the file: components/tracing/common/etw_consumer_win.cc
Insertions: 61, Deletions: 7.

The diff is too large to show. Please review the diff.
```

Change information

Commit message:

etw-stackwalk: add simple stack walk events

This change adds support for StackWalk Event Tracing for Windows (ETW)
events to Chrome's ETW consumer.

StackWalk events are not yet enabled, so this CL will not yet change
Chrome tracing's behavior. This is the first part of a sequence of CLs
to enable StackWalk events.

This is based on spvm@'s original prototype at
https://chromium-review.git.corp.google.com/c/chromium/src/+/7598112

Bug: 400769265

Change-Id: I0aeccd860eaadcc910ac4e2583f32f982dde537b

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7794326

Reviewed-by: Etienne Pierre-Doray <etie...@chromium.org>

Commit-Queue: Jesse McKenna <jessem...@google.com>

Reviewed-by: Sean Maher <sp...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1642073}

Files:

M components/tracing/BUILD.gn
M components/tracing/common/etw_consumer_win.cc
M components/tracing/common/etw_consumer_win.h
M components/tracing/common/etw_system_data_source_win.cc
M components/tracing/common/etw_system_data_source_win.h
M services/tracing/public/cpp/BUILD.gn

Change size: M

Delta: 6 files changed, 187 insertions(+), 9 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Etienne Pierre-Doray, +1 by Sean Maher

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

etw-stackwalk: add simple stack walk events [chromium/src : main]

Jesse McKenna (Gerrit)

Jesse McKenna added 1 comment

Related details

Sean Maher (Gerrit)

Sean Maher added 1 comment

Related details

Etienne Pierre-Doray (Gerrit)

Etienne Pierre-Doray added 6 comments

Related details

Jesse McKenna (Gerrit)

Jesse McKenna voted and added 7 comments

Votes added by Jesse McKenna

7 comments

Related details

Etienne Pierre-Doray (Gerrit)

Etienne Pierre-Doray voted and added 2 comments

Votes added by Etienne Pierre-Doray

2 comments

Related details

Sean Maher (Gerrit)

Sean Maher voted and added 1 comment

Votes added by Sean Maher

1 comment

Related details

Jesse McKenna (Gerrit)

Jesse McKenna voted and added 2 comments

Votes added by Jesse McKenna

2 comments

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information