Worker ScriptWrappables aren't marking their wrapper objects.
Reported by
sigbjo...@opera.com,
Jan 29 2017
|
|||||||
Issue descriptionThe marking of a ScriptWrappable's v8 wrapper object currently assumes that the ScriptWrappable contains the wrapper reference. This is not the case in worker contexts, where the wrapper reference is kept in the worker's DOMWrapperWorld, causing ScriptWrappables with pending activity to be GCed prematurely. The missing piece was noticed and reported by https://bugs.chromium.org/p/chromium/issues/detail?id=683406#c19 , see that issue for additional discussion and details. To ease any backporting, handle the fix for the shortcoming via this issue.
,
Jan 30 2017
Which OSs does this affect?
,
Jan 30 2017
Affects all operating systems. Still exploring potential solutions in https://codereview.chromium.org/2665733002/. The problems are: - We cannot call GetCurrentContext as this requires a new handle, which we shouldn't create during a GC. - AFAICS we also allow Worklets on the main thread by using MainThreadWorkletGlobalScope, which makes my solution of always providing a TLS-based DOMWrapperWorld a bit tricky.
,
Jan 31 2017
,
Jan 31 2017
A TLS-based solution should still work, as we could use it to store the worker world (on all threads) and just process it if it exists.
,
Jan 31 2017
Alright, this is all complicated. So here is what is going on - We have worker threads that can create DOMWrapperWorlds on the main thread, and worker threads. - If on the main thread and not a worker thread, we should go to the object directly, otherwise we should mark through the DOMWrapperWorld - On the main thread, without being a worker, we also have the additional isolated worlds. This is all super convoluted. On top of that, we query DOMWrapperWorlds for native contexts which can be different all the time. We rely on the fact that we will bail out to the object on a very low level (DOMDataStore::set). I need to think about where we put some abstraction for this.
,
Feb 1 2017
I checked what happens with patch set 3 (Store worker world in TLS). It's still not working in all cases, clicking on the trash icon in devconsole still kills the XHR. DOMWrapperWorld::markWrappersInAllWorlds() is called, but isolate->InContext() returns false, so it's not marked in the data store. I also checked what happens if I remove that InContext check, and it seems to work for me then (but I do not know what side-effects it may have). Oh and one more thing: I think that the destructor of DOMWrapperWorld should contain something like (to avoid possible UAFs): if (m_worldId == WorkerWorldId) workerWorld() = nullptr; Here is a stack trace where the gc is called without being InContext: #2 0x56468c9597b5 blink::DOMWrapperWorld::markWrappersInAllWorlds() #3 0x56468c99fe40 blink::TraceTrait<>::traceMarkedWrapper() #4 0x56468c964fb5 blink::ScriptWrappableVisitor::AdvanceTracing() #5 0x564689c7ae6c v8::internal::MarkCompactCollector::ProcessEphemeralMarking() #6 0x564689c7559e v8::internal::MarkCompactCollector::MarkLiveObjects() #7 0x564689c74d09 v8::internal::MarkCompactCollector::CollectGarbage() #8 0x564689c57183 v8::internal::Heap::MarkCompact() #9 0x564689c55ae3 v8::internal::Heap::PerformGarbageCollection() #10 0x564689c5510c v8::internal::Heap::CollectGarbage() #11 0x564689c54c18 v8::internal::Heap::CollectAllAvailableGarbage() #12 0x5646898f232f v8::Isolate::LowMemoryNotification() #13 0x56468a0afc5d v8_inspector::V8HeapProfilerAgentImpl::collectGarbage() #14 0x56468a05c808 v8_inspector::protocol::HeapProfiler::DispatcherImpl::collectGarbage() #15 0x56468a03e30b v8_inspector::protocol::Console::DispatcherImpl::dispatch() #16 0x56468a03648a v8_inspector::protocol::UberDispatcher::dispatch() #17 0x56468a0b9ab1 v8_inspector::V8InspectorSessionImpl::dispatchProtocolMessage() #18 0x56468cfe46f9 blink::InspectorSession::dispatchProtocolMessage() #19 0x56468e5d8a57 blink::WorkerInspectorController::dispatchMessageFromFrontend() #20 0x56468d2c33c9 blink::WorkerThread::performDebuggerTaskOnWorkerThread() #21 0x56468d2c4c39 _ZN4base8internal7InvokerINS0_9BindStateIMN5blink12WorkerThreadEFvSt10unique_ptrIN3WTF8FunctionIFvvELNS6_22FunctionThreadAffinityE0EEESt14default_deleteISA_EEEJNS6_17UnretainedWrapperIS4_LS9_0EEENS6_13PassedWrapperISD_EEEEES8_E3RunEPNS0_13BindStateBaseE #22 0x56468d2c3468 blink::WorkerThread::performDebuggerTaskDontWaitOnWorkerThread() #23 0x56468c8666b7 _ZN4base8internal7InvokerINS0_9BindStateIPFvSt10unique_ptrIN3WTF8FunctionIFvvELNS4_22FunctionThreadAffinityE0EEESt14default_deleteIS8_EEEJNS0_13PassedWrapperISB_EEEEES6_E3RunEPNS0_13BindStateBaseE #24 0x56468ac9688e base::debug::TaskAnnotator::RunTask() #25 0x56468c8a7912 blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue() #26 0x56468c8a64d6 blink::scheduler::TaskQueueManager::DoWork() #27 0x56468ac9688e base::debug::TaskAnnotator::RunTask() #28 0x56468ac2d85d base::MessageLoop::RunTask() #29 0x56468ac2df95 base::MessageLoop::DoWork() #30 0x56468ac2ef7a base::MessagePumpDefault::Run() #31 0x56468ac2d587 base::MessageLoop::RunHandler() #32 0x56468ac4b54e base::RunLoop::Run() #33 0x56468ac6c677 base::Thread::ThreadMain() #34 0x56468ac683b3 base::(anonymous namespace)::ThreadFunc() #35 0x7f2d5403a550 <unknown> #36 0x7f2d4d7fa5fd clone
,
Feb 1 2017
Thanks a lot! The stacktrace makes sense as we can also call through a stackless heap (task) which isn't in a context. Will fix and get it reviewed.
,
Feb 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae599161857edda9ecbed70494c220f7f711600f commit ae599161857edda9ecbed70494c220f7f711600f Author: mlippautz <mlippautz@chromium.org> Date: Wed Feb 01 21:01:14 2017 [wrapper-tracing] For a worker, mark relative to its wrapper map. If marking a ScriptWrappable's wrapper object in a worker context, do that by consulting the worker's underlying DOMWrapperMap. Store the worker world in TLS, as we cannot rely on the context of the isolate during marking as we might mark without a stack (and thus context). BUG= chromium:686563 patch from issue 2663643002 at patchset 140001 (http://crrev.com/2663643002#ps140001) Review-Url: https://codereview.chromium.org/2665733002 Cr-Commit-Position: refs/heads/master@{#447604} [add] https://crrev.com/ae599161857edda9ecbed70494c220f7f711600f/third_party/WebKit/LayoutTests/fast/workers/resources/worker-xhr-onerror.js [add] https://crrev.com/ae599161857edda9ecbed70494c220f7f711600f/third_party/WebKit/LayoutTests/fast/workers/worker-gc-alive.html [modify] https://crrev.com/ae599161857edda9ecbed70494c220f7f711600f/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp [modify] https://crrev.com/ae599161857edda9ecbed70494c220f7f711600f/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h
,
Feb 2 2017
mlippautz@; Could you please update the Issue status as Fixed if there is no further work to be done and request merge into M57, as its marked as Release Blocker Stable.
,
Feb 2 2017
,
Feb 2 2017
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 2 2017
Please merge your change to M57 branch 2987 by 5:00 PM PT, Friday (02/03) so we can pick it for next week M57 Beta release. Thank you.
,
Feb 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/730c14f17cf3e89fbc1749abb053965cbc41f25a commit 730c14f17cf3e89fbc1749abb053965cbc41f25a Author: mlippautz <mlippautz@chromium.org> Date: Fri Feb 03 16:35:04 2017 [wrapper-tracing] For a worker, mark relative to its wrapper map. If marking a ScriptWrappable's wrapper object in a worker context, do that by consulting the worker's underlying DOMWrapperMap. Store the worker world in TLS, as we cannot rely on the context of the isolate during marking as we might mark without a stack (and thus context). BUG= chromium:686563 patch from issue 2663643002 at patchset 140001 (http://crrev.com/2663643002#ps140001) Review-Url: https://codereview.chromium.org/2665733002 Cr-Commit-Position: refs/heads/master@{#447604} (cherry picked from commit ae599161857edda9ecbed70494c220f7f711600f) NOTRY=true NOPRESUBMIT=true TBR=haraken@chromium.org Review-Url: https://codereview.chromium.org/2678473002 Cr-Commit-Position: refs/branch-heads/2987@{#288} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [add] https://crrev.com/730c14f17cf3e89fbc1749abb053965cbc41f25a/third_party/WebKit/LayoutTests/fast/workers/resources/worker-xhr-onerror.js [add] https://crrev.com/730c14f17cf3e89fbc1749abb053965cbc41f25a/third_party/WebKit/LayoutTests/fast/workers/worker-gc-alive.html [modify] https://crrev.com/730c14f17cf3e89fbc1749abb053965cbc41f25a/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp [modify] https://crrev.com/730c14f17cf3e89fbc1749abb053965cbc41f25a/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by jochen@chromium.org
, Jan 29 2017