Nakuru Wubni added 1 comment

Line 115, Patchset 2 (Latest):

Related details

Attention is currently required from:

Adam Langley
Tom Sepez

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tom Sepez (Gerrit)

unread,

Apr 11, 2024, 1:15:45 PMApr 11

to Nakuru Wubni, Mikhail Khokhlov, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley, Mikhail Khokhlov and Nakuru Wubni

Tom Sepez added 3 comments

File base/trace_event/process_memory_dump.cc

  std::unique_ptr<PSAPI_WORKING_SET_EX_INFORMATION[]> vec(
      new PSAPI_WORKING_SET_EX_INFORMATION[max_vec_size]);

Tom Sepez . unresolved

why didn't we update this one, too?

Line 118, Patchset 2 (Latest):

  auto vec = base::HeapArray<char>::WithSize(max_vec_size);
#elif BUILDFLAG(IS_POSIX) || BUILDFLAG(IS_FUCHSIA)
  auto vec = base::HeapArray<unsigned char>::WithSize(max_vec_size);

Tom Sepez . unresolved

we should probably just merge these cases into a HeapArray<uint8_t> and deal with consequences later, perhaps converting as needed.

Line 130, Patchset 2 (Latest): vec.data()[i].VirtualAddress =

Tom Sepez . unresolved

Shouldn't need to call .data() here, doing so skirts safety provided by heap array's operator[].

Related details

Attention is currently required from:

Adam Langley
Mikhail Khokhlov
Nakuru Wubni

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nakuru Wubni (Gerrit)

unread,

Apr 12, 2024, 5:38:41 AMApr 12

to Mikhail Khokhlov, Tom Sepez, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley, Mikhail Khokhlov and Tom Sepez

Nakuru Wubni added 4 comments

Patchset-level comments

Nakuru Wubni . resolved

Addressed reviews.
Please check my code updates.

File base/trace_event/process_memory_dump.cc

Line 115, Patchset 2:

  std::unique_ptr<PSAPI_WORKING_SET_EX_INFORMATION[]> vec(
      new PSAPI_WORKING_SET_EX_INFORMATION[max_vec_size]);

Tom Sepez . resolved

why didn't we update this one, too?

Nakuru Wubni

I have merged lines 114-121 into:
auto vec = base::HeapArray<uint8_t>::WithSize(max_vec_size);

Line 118, Patchset 2: auto vec = base::HeapArray<char>::WithSize(max_vec_size);


#elif BUILDFLAG(IS_POSIX) || BUILDFLAG(IS_FUCHSIA)
  auto vec = base::HeapArray<unsigned char>::WithSize(max_vec_size);

Tom Sepez . resolved

we should probably just merge these cases into a HeapArray<uint8_t> and deal with consequences later, perhaps converting as needed.

Nakuru Wubni

I have merged lines 114-121 into:
auto vec = base::HeapArray<uint8_t>::WithSize(max_vec_size);

Line 130, Patchset 2: vec.data()[i].VirtualAddress =

Tom Sepez . resolved

Shouldn't need to call .data() here, doing so skirts safety provided by heap array's operator[].

Nakuru Wubni

Changed vec.data()[i] to vec[i].

Related details

Attention is currently required from:

Adam Langley
Mikhail Khokhlov
Tom Sepez

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nakuru Wubni (Gerrit)

unread,

Apr 18, 2024, 10:06:25 AMApr 18

to Mikhail Khokhlov, Tom Sepez, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley, Mikhail Khokhlov and Tom Sepez

Nakuru Wubni added 1 comment

Patchset-level comments

Nakuru Wubni . resolved

Kindly review my updates.

satisfied_requirement

unsatisfied_requirement

open

diffy

Nakuru Wubni (Gerrit)

unread,

May 14, 2024, 3:07:50 AMMay 14

to Mikhail Khokhlov, Tom Sepez, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley, Mikhail Khokhlov and Tom Sepez

Nakuru Wubni added 1 comment

Patchset-level comments

Nakuru Wubni . unresolved

This CL needs review.

Related details

Attention is currently required from:

Adam Langley
Mikhail Khokhlov
Tom Sepez

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Mikhail Khokhlov (Gerrit)

unread,

May 14, 2024, 5:35:01 AMMay 14

to Nakuru Wubni, Chromium LUCI CQ, Tom Sepez, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley, Nakuru Wubni and Tom Sepez

Mikhail Khokhlov added 1 comment

Patchset-level comments

Nakuru Wubni . unresolved

This CL needs review.

Mikhail Khokhlov

Please take a look at build failures on Mac and Win, you'll need to cast pointers to the right types there.

File-level comment, Patchset 5:

Related details

Attention is currently required from:

Adam Langley
Nakuru Wubni
Tom Sepez

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nakuru Wubni (Gerrit)

unread,

May 15, 2024, 4:58:21 AMMay 15

to Chromium LUCI CQ, Mikhail Khokhlov, Tom Sepez, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley, Mikhail Khokhlov and Tom Sepez

Nakuru Wubni added 1 comment

Patchset-level comments

Nakuru Wubni . resolved

This CL needs review.

Mikhail Khokhlov

Please take a look at build failures on Mac and Win, you'll need to cast pointers to the right types there.

Nakuru Wubni

I have addressed them.
Please review and try building it again.

File-level comment, Patchset 6 (Latest):

Related details

Attention is currently required from:

Adam Langley
Mikhail Khokhlov
Tom Sepez

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Mikhail Khokhlov (Gerrit)

unread,

May 15, 2024, 8:51:43 AMMay 15

to Nakuru Wubni, Chromium LUCI CQ, Tom Sepez, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley, Nakuru Wubni and Tom Sepez

Mikhail Khokhlov voted and added 1 comment

Votes added by Mikhail Khokhlov

Code-Review

+1

1 comment

Patchset-level comments

Mikhail Khokhlov . resolved

LGTM from my side, but please also wait for someone who has a broader context on this change to approve.

Related details

Attention is currently required from:

Adam Langley
Nakuru Wubni
Tom Sepez

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nakuru Wubni (Gerrit)

unread,

May 21, 2024, 1:19:09 PMMay 21

to Mikhail Khokhlov, Chromium LUCI CQ, Tom Sepez, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley and Tom Sepez

Nakuru Wubni added 1 comment

File base/trace_event/process_memory_dump.cc

Line 118, Patchset 2:

  auto vec = base::HeapArray<char>::WithSize(max_vec_size);
#elif BUILDFLAG(IS_POSIX) || BUILDFLAG(IS_FUCHSIA)
  auto vec = base::HeapArray<unsigned char>::WithSize(max_vec_size);

Tom Sepez . resolved

we should probably just merge these cases into a HeapArray<uint8_t> and deal with consequences later, perhaps converting as needed.

Nakuru Wubni

I have merged lines 114-121 into:
auto vec = base::HeapArray<uint8_t>::WithSize(max_vec_size);

Nakuru Wubni

Hello, Tom.
This approach didn't work.
I had to revert your suggestion to the way I had it before.
All the checks are fine now.

Line 134, Patchset 6 (Latest):

Related details

Attention is currently required from:

Adam Langley
Tom Sepez

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tom Sepez (Gerrit)

unread,

May 21, 2024, 2:13:22 PMMay 21

to Nakuru Wubni, Mikhail Khokhlov, Chromium LUCI CQ, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley and Nakuru Wubni

Tom Sepez added 1 comment

File base/trace_event/process_memory_dump.cc

    DWORD vec_size = static_cast<DWORD>(
        page_count * sizeof(PSAPI_WORKING_SET_EX_INFORMATION));

Tom Sepez . unresolved

we can make a span from vec.first(static_cast<DWORD>(...)) then pass span.data(), span.size() on the next line.

Related details

Attention is currently required from:

Adam Langley
Nakuru Wubni

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nakuru Wubni (Gerrit)

unread,

May 23, 2024, 2:08:47 AMMay 23

to Mikhail Khokhlov, Chromium LUCI CQ, Tom Sepez, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley and Tom Sepez

Nakuru Wubni added 1 comment

File base/trace_event/process_memory_dump.cc

Line 134, Patchset 6:

    DWORD vec_size = static_cast<DWORD>(
        page_count * sizeof(PSAPI_WORKING_SET_EX_INFORMATION));

Tom Sepez . resolved

we can make a span from vec.first(static_cast<DWORD>(...)) then pass span.data(), span.size() on the next line.

Nakuru Wubni

I have applied your suggestion.
Please confirm.

Related details

Attention is currently required from:

Adam Langley
Tom Sepez

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tom Sepez (Gerrit)

unread,

May 23, 2024, 6:33:09 PMMay 23

to Nakuru Wubni, Mikhail Khokhlov, Chromium LUCI CQ, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley and Nakuru Wubni

Tom Sepez added 1 comment

File base/trace_event/process_memory_dump.cc

Line 137, Patchset 7 (Latest): static_cast<DWORD>(span.size()));

Tom Sepez . unresolved

This would seem to be a byte count in the old code, but an element count in the new code.

Related details

Attention is currently required from:

Adam Langley
Nakuru Wubni

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nakuru Wubni (Gerrit)

unread,

May 24, 2024, 10:03:56 AMMay 24

to Mikhail Khokhlov, Chromium LUCI CQ, Tom Sepez, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley and Tom Sepez

Nakuru Wubni added 1 comment

File base/trace_event/process_memory_dump.cc

Line 137, Patchset 7: static_cast<DWORD>(span.size()));

Tom Sepez . unresolved

This would seem to be a byte count in the old code, but an element count in the new code.

Nakuru Wubni

Hi, Tom.
I removed the static_cast<DWORD>.
Does having just span.size() fix it?

Related details

Attention is currently required from:

Adam Langley
Tom Sepez

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tom Sepez (Gerrit)

unread,

May 28, 2024, 11:38:38 AMMay 28

to Nakuru Wubni, Mikhail Khokhlov, Chromium LUCI CQ, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley and Nakuru Wubni

Tom Sepez added 1 comment

File base/trace_event/process_memory_dump.cc

Line 137, Patchset 7: static_cast<DWORD>(span.size()));

Tom Sepez . unresolved

This would seem to be a byte count in the old code, but an element count in the new code.

Nakuru Wubni

Hi, Tom.
I removed the static_cast<DWORD>.
Does having just span.size() fix it?

Tom Sepez

No, there's a multiplication by sizeof() that is missing. size_bytes() does this for us.

Related details

Attention is currently required from:

Adam Langley
Nakuru Wubni

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nakuru Wubni (Gerrit)

unread,

Jun 3, 2024, 8:18:46 AMJun 3

to Mikhail Khokhlov, Chromium LUCI CQ, Tom Sepez, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley and Tom Sepez

Nakuru Wubni added 2 comments

Patchset-level comments

Nakuru Wubni . resolved

Please review changes.

File base/trace_event/process_memory_dump.cc

Line 137, Patchset 7: static_cast<DWORD>(span.size()));

Tom Sepez . resolved

This would seem to be a byte count in the old code, but an element count in the new code.

Nakuru Wubni

Hi, Tom.
I removed the static_cast<DWORD>.
Does having just span.size() fix it?

Tom Sepez

No, there's a multiplication by sizeof() that is missing. size_bytes() does this for us.

Nakuru Wubni

Updated to span.size_bytes()

Related details

Attention is currently required from:

Adam Langley
Tom Sepez

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tom Sepez (Gerrit)

unread,

Jun 3, 2024, 12:46:33 PMJun 3

to Nakuru Wubni, Mikhail Khokhlov, Chromium LUCI CQ, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley and Nakuru Wubni

Tom Sepez voted Code-Review+1

Code-Review

+1

Related details

Attention is currently required from:

Adam Langley
Nakuru Wubni

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Nakuru Wubni (Gerrit)

unread,

Jun 4, 2024, 3:49:44 AMJun 4

to Tom Sepez, Mikhail Khokhlov, Chromium LUCI CQ, Adam Langley, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Adam Langley

Nakuru Wubni voted Commit-Queue+2

Commit-Queue

+2

Related details

Attention is currently required from:

Adam Langley

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Adam Langley (Gerrit)

unread,

Jun 4, 2024, 9:10:32 AMJun 4

to Nakuru Wubni, Tom Sepez, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Nakuru Wubni

Adam Langley voted and added 1 comment

Votes added by Adam Langley

Code-Review

+1

1 comment

Patchset-level comments

Adam Langley . resolved

(p.s. wrong Adam Langley was set as a reviewer.)

Related details

Attention is currently required from:

Nakuru Wubni

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Nakuru Wubni (Gerrit)

unread,

Jun 7, 2024, 5:15:16 AMJun 7

to Adam Langley, Tom Sepez, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Nakuru Wubni added 1 comment

Patchset-level comments

Nakuru Wubni . resolved

Hi,
This CL is ready to submit.

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Nakuru Wubni (Gerrit)

unread,

Jun 12, 2024, 1:19:28 PMJun 12

to Adam Langley, Tom Sepez, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Nakuru Wubni added 1 comment

Patchset-level comments

Nakuru Wubni . resolved

I don't have the rights to commit. Please any committer should submit.

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Adam Langley (Gerrit)

unread,

Jun 17, 2024, 8:37:41 PM (14 days ago) Jun 17

to Nakuru Wubni, Tom Sepez, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Nakuru Wubni

Adam Langley voted Commit-Queue+2

Commit-Queue

+2

Related details

Attention is currently required from:

Nakuru Wubni

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Adam Langley (Gerrit)

unread,

Jun 17, 2024, 10:04:36 PM (14 days ago) Jun 17

to Nakuru Wubni, Tom Sepez, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jun 17, 2024, 10:33:28 PM (14 days ago) Jun 17

to Nakuru Wubni, Adam Langley, Tom Sepez, Mikhail Khokhlov, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

Replaced unique_ptr with HeapArray in
base/trace_event/process_memory_dump.cc

This cl changed occurences of unique_ptr to HeapArray

Bug: 326458682, 326458967

Change-Id: I76a2443217f4bd1ecb9c63b97e54606ac4cd57a8

Fixed: 326458682, 326458967

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

Reviewed-by: Tom Sepez <tse...@chromium.org>

Reviewed-by: Mikhail Khokhlov <khok...@google.com>

Reviewed-by: Adam Langley <a...@chromium.org>

Commit-Queue: Adam Langley <a...@chromium.org>

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

Files:

M base/trace_event/process_memory_dump.cc

Change size: S

Delta: 1 file changed, 13 insertions(+), 10 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Adam Langley, +1 by Tom Sepez, +1 by Mikhail Khokhlov