Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2415363002:
Unpoison CrossThreadPersistents (Closed)
DescriptionUnpoison CrossThreadPersistents
CrossThreadPersistents may be accessed from CrossThreadPersistent Region::shouldTracePersistent after the containing object is scheduled for collection and poisoned. This unpoison CrossThreadPersistents after we poison unmarked objects.
BUG=655293
Committed: https://crrev.com/6bac7b20289981b7fdb186a0735c6044a691be09
Cr-Commit-Position: refs/heads/master@{#425667}
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : fix #
Total comments: 1
Patch Set 4 : fix #
Total comments: 5
Patch Set 5 : added comment #Patch Set 6 : fix #Patch Set 7 : fix #
Messages
Total messages: 34 (15 generated)
Description was changed from ========== Disable ASAN for CrossThreadPersistent::get() during shouldTracePersistent Per thread heap GC means CrossThreadPersistentRegion::shouldTracePersistent may access CrossThreadPersistents scheduled for sweeping. This disables ASAN for the nested function calls from shouldTracePersistent. BUG=655293 ========== to ========== Disable ASAN for CrossThreadPersistent::get() during shouldTracePersistent Per thread heap GC means CrossThreadPersistentRegion::shouldTracePersistent may access CrossThreadPersistents scheduled for sweeping. This disables ASAN for the nested function calls from shouldTracePersistent. BUG=655293 ==========
keishi@chromium.org changed reviewers: + haraken@chromium.org, oilpan-reviews@chromium.org
PTAL
The ASAN problem was reported again. I was able to reproduce it locally too. Locally this seems to work.
I'm not sure if I'm understanding the problem accurately, but is there any option to simply disable poisoning of CrossThreadPersistents? Poisoning CrossThreadPersistents doesn't make sense in the first place because it's totally valid for other threads to access the CrossThreadPersistents...
On 2016/10/14 10:55:09, haraken wrote: > I'm not sure if I'm understanding the problem accurately, but is there any > option to simply disable poisoning of CrossThreadPersistents? > > Poisoning CrossThreadPersistents doesn't make sense in the first place because > it's totally valid for other threads to access the CrossThreadPersistents... We poison the heap page first with the CrossThreadPersistents in it, so we need to unpoison during tracing like Patch Set 3. I was worried if it puts too much stress on ASAN but it seems to work.
https://codereview.chromium.org/2415363002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (right): https://codereview.chromium.org/2415363002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/PersistentNode.cpp:91: sizeof(CrossThreadPersistent<void*>)); Would you help me understand who poisons the memory region of node->self()? I'm wondering if there's an option to simply stop poisoning heap pages for CrossThreadPersistents.
On 2016/10/17 03:55:22, haraken wrote: > https://codereview.chromium.org/2415363002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (right): > > https://codereview.chromium.org/2415363002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/PersistentNode.cpp:91: > sizeof(CrossThreadPersistent<void*>)); > > Would you help me understand who poisons the memory region of node->self()? > > I'm wondering if there's an option to simply stop poisoning heap pages for > CrossThreadPersistents. CrossThreadPersistent can be a member of a GarbageCollected object (e.g. WorkerThreadableLoader::m_mainThreadLoaderHolder) so when NormalPage::poisonUnmarkedObjects() poisons the GarbageCollected object, the CrossThreadPersistent is poisoned together. Avoid poisoning CrossThreadPersistents is ideally the best way, but when NormalPage::poisonUnmarkedObjects is called, we can't figure out the position of CrossThreadPersistents.
On 2016/10/17 04:02:18, keishi wrote: > On 2016/10/17 03:55:22, haraken wrote: > > > https://codereview.chromium.org/2415363002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (right): > > > > > https://codereview.chromium.org/2415363002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/heap/PersistentNode.cpp:91: > > sizeof(CrossThreadPersistent<void*>)); > > > > Would you help me understand who poisons the memory region of node->self()? > > > > I'm wondering if there's an option to simply stop poisoning heap pages for > > CrossThreadPersistents. > > CrossThreadPersistent can be a member of a GarbageCollected object (e.g. > WorkerThreadableLoader::m_mainThreadLoaderHolder) so when > NormalPage::poisonUnmarkedObjects() poisons the GarbageCollected object, the > CrossThreadPersistent is poisoned together. Avoid poisoning > CrossThreadPersistents is ideally the best way, but when > NormalPage::poisonUnmarkedObjects is called, we can't figure out the position of > CrossThreadPersistents. Ah, that makes sense :D Then would it an option to unpoison all the CrossThreadPersistents after NormalPage::poisonUnmarkedObjects is called? PS3 is not really nice in a sense that we're unpoisoning non-cross-thread Persistents too...
On 2016/10/17 04:10:51, haraken wrote: > On 2016/10/17 04:02:18, keishi wrote: > > On 2016/10/17 03:55:22, haraken wrote: > > > > > > https://codereview.chromium.org/2415363002/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (right): > > > > > > > > > https://codereview.chromium.org/2415363002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/heap/PersistentNode.cpp:91: > > > sizeof(CrossThreadPersistent<void*>)); > > > > > > Would you help me understand who poisons the memory region of node->self()? > > > > > > I'm wondering if there's an option to simply stop poisoning heap pages for > > > CrossThreadPersistents. > > > > CrossThreadPersistent can be a member of a GarbageCollected object (e.g. > > WorkerThreadableLoader::m_mainThreadLoaderHolder) so when > > NormalPage::poisonUnmarkedObjects() poisons the GarbageCollected object, the > > CrossThreadPersistent is poisoned together. Avoid poisoning > > CrossThreadPersistents is ideally the best way, but when > > NormalPage::poisonUnmarkedObjects is called, we can't figure out the position > of > > CrossThreadPersistents. > > Ah, that makes sense :D > > Then would it an option to unpoison all the CrossThreadPersistents after > NormalPage::poisonUnmarkedObjects is called? > > PS3 is not really nice in a sense that we're unpoisoning non-cross-thread > Persistents too... Done. PS4 unpoisons CrossThreadPersistents after poisoning.
LGTM https://codereview.chromium.org/2415363002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (right): https://codereview.chromium.org/2415363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/PersistentNode.cpp:182: for (int i = 0; i < PersistentNodeSlots::slotCount; ++i) { Maybe can you simply unpoison m_slot[0 : slotCount] in one call, regardless of whether the slots are unused or not? https://codereview.chromium.org/2415363002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/2415363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1142: ProcessHeap::crossThreadPersistentRegion().unpoisonCrossThreadPersistents(); Add a comment about why we do this.
https://codereview.chromium.org/2415363002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (right): https://codereview.chromium.org/2415363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/PersistentNode.cpp:182: for (int i = 0; i < PersistentNodeSlots::slotCount; ++i) { On 2016/10/17 07:32:40, haraken wrote: > > Maybe can you simply unpoison m_slot[0 : slotCount] in one call, regardless of > whether the slots are unused or not? We are unpoisoning the persistents, not the persistent nodes, so we cannot do it in one call. We need to call isUnused because if it is true, m_self points to a PersistentNode instead of a persistent. https://codereview.chromium.org/2415363002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/2415363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1142: ProcessHeap::crossThreadPersistentRegion().unpoisonCrossThreadPersistents(); On 2016/10/17 07:32:40, haraken wrote: > > Add a comment about why we do this. Done.
https://codereview.chromium.org/2415363002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (right): https://codereview.chromium.org/2415363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/PersistentNode.cpp:182: for (int i = 0; i < PersistentNodeSlots::slotCount; ++i) { On 2016/10/17 07:43:00, keishi wrote: > On 2016/10/17 07:32:40, haraken wrote: > > > > Maybe can you simply unpoison m_slot[0 : slotCount] in one call, regardless of > > whether the slots are unused or not? > > We are unpoisoning the persistents, not the persistent nodes, so we cannot do it > in one call. We need to call isUnused because if it is true, m_self points to a > PersistentNode instead of a persistent. Ah, thanks, makes sense.
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2415363002/#ps80001 (title: "added comment")
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 keishi@chromium.org
Description was changed from ========== Disable ASAN for CrossThreadPersistent::get() during shouldTracePersistent Per thread heap GC means CrossThreadPersistentRegion::shouldTracePersistent may access CrossThreadPersistents scheduled for sweeping. This disables ASAN for the nested function calls from shouldTracePersistent. BUG=655293 ========== to ========== Unpoison CrossThreadPersistents CrossThreadPersistents may be accessed from CrossThreadPersistent Region::shouldTracePersistent after the containing object is scheduled for collection and poisoned. This unpoison CrossThreadPersistents after we poison unmarked objects. BUG=655293 ==========
The CQ bit was checked by keishi@chromium.org
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
Exceeded global retry quota
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2415363002/#ps100001 (title: "fix")
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
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2415363002/#ps120001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Unpoison CrossThreadPersistents CrossThreadPersistents may be accessed from CrossThreadPersistent Region::shouldTracePersistent after the containing object is scheduled for collection and poisoned. This unpoison CrossThreadPersistents after we poison unmarked objects. BUG=655293 ========== to ========== Unpoison CrossThreadPersistents CrossThreadPersistents may be accessed from CrossThreadPersistent Region::shouldTracePersistent after the containing object is scheduled for collection and poisoned. This unpoison CrossThreadPersistents after we poison unmarked objects. BUG=655293 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Unpoison CrossThreadPersistents CrossThreadPersistents may be accessed from CrossThreadPersistent Region::shouldTracePersistent after the containing object is scheduled for collection and poisoned. This unpoison CrossThreadPersistents after we poison unmarked objects. BUG=655293 ========== to ========== Unpoison CrossThreadPersistents CrossThreadPersistents may be accessed from CrossThreadPersistent Region::shouldTracePersistent after the containing object is scheduled for collection and poisoned. This unpoison CrossThreadPersistents after we poison unmarked objects. BUG=655293 Committed: https://crrev.com/6bac7b20289981b7fdb186a0735c6044a691be09 Cr-Commit-Position: refs/heads/master@{#425667} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6bac7b20289981b7fdb186a0735c6044a691be09 Cr-Commit-Position: refs/heads/master@{#425667} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
