Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2552333002:
Change the CRT initialization to create a new heap for Chrome.dll. (Closed)
|
Created:
4 years ago by Sigurður Ásgeirsson Modified:
4 years ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, Dai Mikurube (NOT FULLTIME), vmpstr+watch_chromium.org, kraynov Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange the CRT initialization to create a new heap for Chrome.dll.
Also change the malloc provider to iterate over the CRT's heap.
BUG=671653
Committed: https://crrev.com/82535f6c4c3d582c7a60d941b8402a45c9e99e6b
Cr-Commit-Position: refs/heads/master@{#436760}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add a test per Will's request. #
Messages
Total messages: 27 (17 generated)
The CQ bit was checked by siggi@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...
Description was changed from ========== Change the CRT initialization to create a new heap for Chrome.dll. BUG= ========== to ========== Change the CRT initialization to create a new heap for Chrome.dll. BUG=671653 ==========
siggi@chromium.org changed reviewers: + primiano@chromium.org, wfh@google.com
Description was changed from ========== Change the CRT initialization to create a new heap for Chrome.dll. BUG=671653 ========== to ========== Change the CRT initialization to create a new heap for Chrome.dll. Also change the malloc provider to iterate over the CRT's heap. BUG=671653 ==========
siggi@chromium.org changed reviewers: + wfh@chromium.org - wfh@google.com
Will for the functionals of hooking the _acrt_ functions. Primiano for owners, pretty please?
I have no idea what acrt is and does, but the changes in base/trace_event/malloc_dump_provider.cc LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
seems okay to me... any chance of a test? e.g. heap should be different for each DLL now, right? https://codereview.chromium.org/2552333002/diff/1/base/allocator/allocator_sh... File base/allocator/allocator_shim_override_ucrt_symbols_win.h (right): https://codereview.chromium.org/2552333002/diff/1/base/allocator/allocator_sh... base/allocator/allocator_shim_override_ucrt_symbols_win.h:76: __acrt_heap = ::HeapCreate(0, 0, 0); do these flags match the process heap?
Thanks - added a test that the CRT heap is different from the process heap when the shim is enabled. https://codereview.chromium.org/2552333002/diff/1/base/allocator/allocator_sh... File base/allocator/allocator_shim_override_ucrt_symbols_win.h (right): https://codereview.chromium.org/2552333002/diff/1/base/allocator/allocator_sh... base/allocator/allocator_shim_override_ucrt_symbols_win.h:76: __acrt_heap = ::HeapCreate(0, 0, 0); On 2016/12/06 19:46:35, Will Harris wrote: > do these flags match the process heap? I believe so, the only flags documented are HEAP_CREATE_ENABLE_EXECUTE HEAP_GENERATE_EXCEPTIONS HEAP_NO_SERIALIZE We still have the HeapSetInformation calls in base to set the per-process heap corruption termination policy.
The CQ bit was checked by siggi@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...
https://codereview.chromium.org/2552333002/diff/1/base/allocator/allocator_sh... File base/allocator/allocator_shim_override_ucrt_symbols_win.h (right): https://codereview.chromium.org/2552333002/diff/1/base/allocator/allocator_sh... base/allocator/allocator_shim_override_ucrt_symbols_win.h:76: __acrt_heap = ::HeapCreate(0, 0, 0); On 2016/12/06 20:28:15, Sigurður Ásgeirsson wrote: > On 2016/12/06 19:46:35, Will Harris wrote: > > do these flags match the process heap? > > I believe so, the only flags documented are > > HEAP_CREATE_ENABLE_EXECUTE > HEAP_GENERATE_EXCEPTIONS > HEAP_NO_SERIALIZE > > We still have the HeapSetInformation calls in base to set the per-process heap > corruption termination policy. is HeapSetInformation called on this heap as well?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It's per process. Null goes in for heap handle. On Tue, Dec 6, 2016, 16:33 <wfh@chromium.org> wrote: > > > https://codereview.chromium.org/2552333002/diff/1/base/allocator/allocator_sh... > File base/allocator/allocator_shim_override_ucrt_symbols_win.h (right): > > > https://codereview.chromium.org/2552333002/diff/1/base/allocator/allocator_sh... > base/allocator/allocator_shim_override_ucrt_symbols_win.h:76: > __acrt_heap = ::HeapCreate(0, 0, 0); > On 2016/12/06 20:28:15, Sigurður Ásgeirsson wrote: > > On 2016/12/06 19:46:35, Will Harris wrote: > > > do these flags match the process heap? > > > > I believe so, the only flags documented are > > > > HEAP_CREATE_ENABLE_EXECUTE > > HEAP_GENERATE_EXCEPTIONS > > HEAP_NO_SERIALIZE > > > > We still have the HeapSetInformation calls in base to set the > per-process heap > > corruption termination policy. > > is HeapSetInformation called on this heap as well? > > https://codereview.chromium.org/2552333002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm then
The CQ bit was checked by siggi@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/2552333002/#ps20001 (title: "Add a test per Will's request.")
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": 1481062481014890,
"parent_rev": "734e4f8a74faa8d7b04f05d0fb902ea7383e706c", "commit_rev":
"932976f8c62b060ccd4ae61d89dce11a214b9f0e"}
Message was sent while issue was closed.
Description was changed from ========== Change the CRT initialization to create a new heap for Chrome.dll. Also change the malloc provider to iterate over the CRT's heap. BUG=671653 ========== to ========== Change the CRT initialization to create a new heap for Chrome.dll. Also change the malloc provider to iterate over the CRT's heap. BUG=671653 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Change the CRT initialization to create a new heap for Chrome.dll. Also change the malloc provider to iterate over the CRT's heap. BUG=671653 ========== to ========== Change the CRT initialization to create a new heap for Chrome.dll. Also change the malloc provider to iterate over the CRT's heap. BUG=671653 Committed: https://crrev.com/82535f6c4c3d582c7a60d941b8402a45c9e99e6b Cr-Commit-Position: refs/heads/master@{#436760} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/82535f6c4c3d582c7a60d941b8402a45c9e99e6b Cr-Commit-Position: refs/heads/master@{#436760} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
