Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528053002:
Account only main WinHeap in MemoryDumpProvider on Windows. (Closed)
DescriptionAccount only main WinHeap in MemoryDumpProvider on Windows.
Querying private heaps not owned by your code is racy and crashy
task for WinAPI. See bug for more context.
Heads for for perf sheriffs:
---------------------------
This change will very likely decrease the malloc memory reported
on Windows bots and is WAI, however, not a real improvement, sorry.
BUG=665516
Committed: https://crrev.com/ad50729b102fa81b770559dbbd33109b7cb5d6f5
Cr-Commit-Position: refs/heads/master@{#434537}
Patch Set 1 #
Total comments: 1
Patch Set 2 : CHECK--; #
Total comments: 1
Messages
Total messages: 23 (16 generated)
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kraynov@chromium.org changed reviewers: + primiano@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
great thanks. LGTM with 1 minor comment. Also please add something like this to the commit message: Heads for for perf sheriffs: --------------------------- This will very likely shift down the malloc memory on windows bots and is WAI. https://codereview.chromium.org/2528053002/diff/1/base/trace_event/malloc_dum... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/2528053002/diff/1/base/trace_event/malloc_dum... base/trace_event/malloc_dump_provider.cc:114: CHECK(main_heap != NULL); no need for these two CHECKs, it will crash anyways without :)
Description was changed from ========== Account only main WinHeap in MemoryDumpProvider on Windows. Querying private heaps not owned by your code is racy and crashy task for WinAPI. See bug for more context. BUG=665516 ========== to ========== Account only main WinHeap in MemoryDumpProvider on Windows. Querying private heaps not owned by your code is racy and crashy task for WinAPI. See bug for more context. Heads for for perf sheriffs: --------------------------- This change will very likely decrease the malloc memory reported on Windows bots and is WAI, however, not a real improvement, sorry. BUG=665516 ==========
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kraynov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2528053002/#ps20001 (title: "CHECK--;")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480096630814480,
"parent_rev": "b3195007eceaf6de937d5b27aa9d2cb8364122d7", "commit_rev":
"14077e609cd0d987a5cbfdccf704dde67d8d0072"}
Message was sent while issue was closed.
Description was changed from ========== Account only main WinHeap in MemoryDumpProvider on Windows. Querying private heaps not owned by your code is racy and crashy task for WinAPI. See bug for more context. Heads for for perf sheriffs: --------------------------- This change will very likely decrease the malloc memory reported on Windows bots and is WAI, however, not a real improvement, sorry. BUG=665516 ========== to ========== Account only main WinHeap in MemoryDumpProvider on Windows. Querying private heaps not owned by your code is racy and crashy task for WinAPI. See bug for more context. Heads for for perf sheriffs: --------------------------- This change will very likely decrease the malloc memory reported on Windows bots and is WAI, however, not a real improvement, sorry. BUG=665516 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Account only main WinHeap in MemoryDumpProvider on Windows. Querying private heaps not owned by your code is racy and crashy task for WinAPI. See bug for more context. Heads for for perf sheriffs: --------------------------- This change will very likely decrease the malloc memory reported on Windows bots and is WAI, however, not a real improvement, sorry. BUG=665516 ========== to ========== Account only main WinHeap in MemoryDumpProvider on Windows. Querying private heaps not owned by your code is racy and crashy task for WinAPI. See bug for more context. Heads for for perf sheriffs: --------------------------- This change will very likely decrease the malloc memory reported on Windows bots and is WAI, however, not a real improvement, sorry. BUG=665516 Committed: https://crrev.com/ad50729b102fa81b770559dbbd33109b7cb5d6f5 Cr-Commit-Position: refs/heads/master@{#434537} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ad50729b102fa81b770559dbbd33109b7cb5d6f5 Cr-Commit-Position: refs/heads/master@{#434537}
Message was sent while issue was closed.
siggi@chromium.org changed reviewers: + siggi@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2528053002/diff/20001/base/trace_event/malloc... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/2528053002/diff/20001/base/trace_event/malloc... base/trace_event/malloc_dump_provider.cc:113: HANDLE main_heap = ::GetProcessHeap(); This relies on an implementation detail of the CRT using the process heap. This is not true for e.g. SyzyASAN, and I plan to change it. Look for a CL to use inline HANDLE get_heap_handle() { return reinterpret_cast<HANDLE>(_get_heap_handle()); } instead.
Message was sent while issue was closed.
Thanks siggi@ Essentially, it could happen that (heap supposed to be main in WinAPI's opinion) != (heap used by C runtime). Refering to allocator shim https://cs.chromium.org/chromium/src/base/allocator/winheap_stubs_win.cc it seems to be proper solution pointing to the main heap. |
