WorkerThread::m_globalScope is nullified without a lock on a worker thread |
||||||
Issue descriptionWorkerThread::m_workerGlobalScope can be accessed both main/worker threads, but it's nullified without a lock on the worker thread. This causes TSan failures as follow: (This was originally reported at https://bugs.chromium.org/p/chromium/issues/detail?id=345240#c26) [ RUN ] ServiceWorkerBlackBoxBrowserTest.Registration [7664:7664:0818/021615:37476997980:WARNING:audio_manager.cc(317)] Multiple instances of AudioManager detected [7664:7664:0818/021615:37476998140:WARNING:audio_manager.cc(278)] Multiple instances of AudioManager detected Xlib: extension "RANDR" missing on display ":9". [7664:7755:0818/021616:37478128060:WARNING:embedded_test_server.cc(202)] Request not handled. Returning 404: /does/not/exist ================== WARNING: ThreadSanitizer: data race (pid=7798) Write of size 8 at 0x7d340000b5a8 by thread T9: #0 blink::PersistentBase<blink::WorkerOrWorkletGlobalScope, (blink::WeaknessPersistentConfiguration)0, (blink::CrossThreadnessPersistentConfiguration)0>::assign(blink::WorkerOrWorkletGlobalScope*) third_party/WebKit/Source/platform/heap/Persistent.h:172:19 (content_browsertests+0x000004dd8bf4) #1 operator= third_party/WebKit/Source/platform/heap/Persistent.h:119:9 (content_browsertests+0x000004dd6650) #2 operator= third_party/WebKit/Source/platform/heap/Persistent.h:300 (content_browsertests+0x000004dd6650) #3 blink::WorkerThread::performShutdownOnWorkerThread() third_party/WebKit/Source/core/workers/WorkerThread.cpp:596 (content_browsertests+0x000004dd6650) #4 Invoke<blink::WorkerThread *> base/bind_internal.h:214:12 (content_browsertests+0x000004dd8015) #5 MakeItSo<void (blink::WorkerThread::*const &)(), blink::WorkerThread *> base/bind_internal.h:283 (content_browsertests+0x000004dd8015) #6 RunImpl<void (blink::WorkerThread::*const &)(), const std::__1::tuple<WTF::UnretainedWrapper<blink::WorkerThread, WTF::FunctionThreadAffinity::CrossThreadAffinity> > &, 0> base/bind_internal.h:346 (content_browsertests+0x000004dd8015) #7 base::internal::Invoker<base::internal::BindState<void (blink::WorkerThread::*)(), WTF::UnretainedWrapper<blink::WorkerThread, (WTF::FunctionThreadAffinity)0> >, void ()>::Run(base::internal::BindStateBase*) base/bind_internal.h:324 (content_browsertests+0x000004dd8015) #8 Run base/callback.h:388:12 (content_browsertests+0x000003e9c752) #9 operator() third_party/WebKit/Source/wtf/Functional.h:229 (content_browsertests+0x000003e9c752) #10 blink::CrossThreadTask::run() third_party/WebKit/Source/platform/WebTaskRunner.cpp:38 (content_browsertests+0x000003e9c752) #11 blink::scheduler::WebTaskRunnerImpl::runTask(std::__1::unique_ptr<blink::WebTaskRunner::Task, std::__1::default_delete<blink::WebTaskRunner::Task> >) third_party/WebKit/Source/platform/scheduler/child/web_task_runner_impl.cc:71:9 (content_browsertests+0x000003eded4d) #12 Invoke<std::__1::unique_ptr<blink::WebTaskRunner::Task, std::__1::default_delete<blink::WebTaskRunner::Task> > > base/bind_internal.h:164:12 (content_browsertests+0x000003edf1b1) #13 MakeItSo<void (*const &)(std::__1::unique_ptr<blink::WebTaskRunner::Task, std::__1::default_delete<blink::WebTaskRunner::Task> >), std::__1::unique_ptr<blink::WebTaskRunner::Task, std::__1::default_delete<blink::WebTaskRunner::Task> > > base/bind_internal.h:283 (content_browsertests+0x000003edf1b1) #14 RunImpl<void (*const &)(std::__1::unique_ptr<blink::WebTaskRunner::Task, std::__1::default_delete<blink::WebTaskRunner::Task> >), const std::__1::tuple<base::internal::PassedWrapper<std::__1::unique_ptr<blink::WebTaskRunner::Task, std::__1::default_delete<blink::WebTaskRunner::Task> > > > &, 0> base/bind_internal.h:346 (content_browsertests+0x000003edf1b1) #15 base::internal::Invoker<base::internal::BindState<void (*)(std::__1::unique_ptr<blink::WebTaskRunner::Task, std::__1::default_delete<blink::WebTaskRunner::Task> >), base::internal::PassedWrapper<std::__1::unique_ptr<blink::WebTaskRunner::Task, std::__1::default_delete<blink::WebTaskRunner::Task> > > >, void ()>::Run(base::internal::BindStateBase*) base/bind_internal.h:324 (content_browsertests+0x000003edf1b1) #16 Run base/callback.h:388:12 (content_browsertests+0x00000287ac49) #17 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask const&) base/debug/task_annotator.cc:54 (content_browsertests+0x00000287ac49) #18 blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue(blink::scheduler::internal::WorkQueue*, blink::scheduler::internal::TaskQueueImpl::Task*) third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:315:19 (content_browsertests+0x000003ef8ec5) #19 blink::scheduler::TaskQueueManager::DoWork(base::TimeTicks, bool) third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:218:13 (content_browsertests+0x000003ef7356) #20 Invoke<const base::WeakPtr<blink::scheduler::TaskQueueManager> &, const base::TimeTicks &, const bool &> base/bind_internal.h:214:12 (content_browsertests+0x000003efa03f) ... Previous read of size 8 at 0x7d340000b5a8 by main thread (mutexes: write M2799): #0 operator blink::WorkerOrWorkletGlobalScope * third_party/WebKit/Source/platform/heap/Persistent.h:106:34 (content_browsertests+0x000004dd426a) #1 blink::WorkerThread::terminateInternal(blink::WorkerThread::TerminationMode) third_party/WebKit/Source/core/workers/WorkerThread.cpp:368 (content_browsertests+0x000004dd426a) #2 blink::WorkerThread::terminateAndWait() third_party/WebKit/Source/core/workers/WorkerThread.cpp:216:5 (content_browsertests+0x000004dd4b1b) #3 blink::WebEmbeddedWorkerImpl::~WebEmbeddedWorkerImpl() third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:111:25 (content_browsertests+0x000003f9fdba) #4 blink::WebEmbeddedWorkerImpl::~WebEmbeddedWorkerImpl() third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:106:1 (content_browsertests+0x000003fa05d9) #5 operator() buildtools/third_party/libc++/trunk/include/memory:2529:13 (content_browsertests+0x000003ba6789) #6 reset buildtools/third_party/libc++/trunk/include/memory:2735 (content_browsertests+0x000003ba6789) #7 ~unique_ptr buildtools/third_party/libc++/trunk/include/memory:2703 (content_browsertests+0x000003ba6789) #8 ~WorkerWrapper content/renderer/service_worker/embedded_worker_dispatcher.cc:38 (content_browsertests+0x000003ba6789) #9 IDMap<content::EmbeddedWorkerDispatcher::WorkerWrapper, (IDMapOwnershipSemantics)1, int>::Releaser<(IDMapOwnershipSemantics)1, 0>::release_all(std::__1::unordered_map<int, content::EmbeddedWorkerDispatcher::WorkerWrapper*, base_hash::hash<int>, std::__1::equal_to<int>, std::__1::allocator<std::__1::pair<int const, content::EmbeddedWorkerDispatcher::WorkerWrapper*> > >*) base/id_map.h:250 (content_browsertests+0x000003ba6789) #10 ~IDMap base/id_map.h:59:5 (content_browsertests+0x000003ba4ae3) ... Location is heap block of size 200 at 0x7d340000b530 allocated by main thread: #0 operator new(unsigned long) <null> (content_browsertests+0x00000050f2c2) #1 blink::ServiceWorkerThread::create(WTF::PassRefPtr<blink::WorkerLoaderProxy>, blink::WorkerReportingProxy&) third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerThread.cpp:43:23 (content_browsertests+0x000005330339) #2 blink::WebEmbeddedWorkerImpl::startWorkerThread() third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:451:22 (content_browsertests+0x000003fa15ef) #3 blink::WebEmbeddedWorkerImpl::onScriptLoaderFinished() third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:398:5 (content_browsertests+0x000003fa2b9a) #4 Invoke<blink::WebEmbeddedWorkerImpl *> base/bind_internal.h:214:12 (content_browsertests+0x000003fa5ed5) #5 MakeItSo<void (blink::WebEmbeddedWorkerImpl::*const &)(), blink::WebEmbeddedWorkerImpl *> base/bind_internal.h:283 (content_browsertests+0x000003fa5ed5) #6 RunImpl<void (blink::WebEmbeddedWorkerImpl::*const &)(), const std::__1::tuple<WTF::UnretainedWrapper<blink::WebEmbeddedWorkerImpl, WTF::FunctionThreadAffinity::SameThreadAffinity> > &, 0> base/bind_internal.h:346 (content_browsertests+0x000003fa5ed5) #7 base::internal::Invoker<base::internal::BindState<void (blink::WebEmbeddedWorkerImpl::*)(), WTF::UnretainedWrapper<blink::WebEmbeddedWorkerImpl, (WTF::FunctionThreadAffinity)1> >, void ()>::Run(base::internal::BindStateBase*) base/bind_internal.h:324 (content_browsertests+0x000003fa5ed5) #8 Run base/callback.h:388:12 (content_browsertests+0x000004dd2841)
,
Aug 18 2016
Short-term fix: Make WorkerThread::performShutdownOnWorkerThread() acquire the lock when it nulliies the global scope. Ideal-fix: Stop WorkerThread::terminateInternal() and WorkerThread::forciblyTerminateExecution() accessing the global scope from the main thread.
,
Aug 22 2016
,
Aug 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/53752ac68bbd8b93650a9475c9b5b5f2e13a8e9a commit 53752ac68bbd8b93650a9475c9b5b5f2e13a8e9a Author: nhiroki <nhiroki@chromium.org> Date: Mon Aug 22 08:27:44 2016 Streams: Remove isTerminating() calls isTerminating() is used for making sure whether a worker execution context is still valid after V8 API calls, but this is no longer necessary thanks to graceful shutdown mechanism for WorkerThread ( https://crbug.com/487050 ) BUG=614272, 638877 Review-Url: https://codereview.chromium.org/2265863002 Cr-Commit-Position: refs/heads/master@{#413420} [modify] https://crrev.com/53752ac68bbd8b93650a9475c9b5b5f2e13a8e9a/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp [modify] https://crrev.com/53752ac68bbd8b93650a9475c9b5b5f2e13a8e9a/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h [modify] https://crrev.com/53752ac68bbd8b93650a9475c9b5b5f2e13a8e9a/third_party/WebKit/Source/core/streams/ReadableStreamController.h [modify] https://crrev.com/53752ac68bbd8b93650a9475c9b5b5f2e13a8e9a/third_party/WebKit/Source/core/streams/ReadableStreamOperations.cpp [modify] https://crrev.com/53752ac68bbd8b93650a9475c9b5b5f2e13a8e9a/third_party/WebKit/Source/core/workers/WorkerThread.cpp [modify] https://crrev.com/53752ac68bbd8b93650a9475c9b5b5f2e13a8e9a/third_party/WebKit/Source/core/workers/WorkerThread.h [modify] https://crrev.com/53752ac68bbd8b93650a9475c9b5b5f2e13a8e9a/third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp [modify] https://crrev.com/53752ac68bbd8b93650a9475c9b5b5f2e13a8e9a/third_party/WebKit/Source/core/workers/WorkletGlobalScope.cpp [modify] https://crrev.com/53752ac68bbd8b93650a9475c9b5b5f2e13a8e9a/third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp [modify] https://crrev.com/53752ac68bbd8b93650a9475c9b5b5f2e13a8e9a/third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
,
Aug 24 2016
,
Aug 24 2016
I'm now working on the ideal-fix.
,
Aug 24 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/360cedf2865c54108fa97f246737454ff68fc9ad commit 360cedf2865c54108fa97f246737454ff68fc9ad Author: nhiroki <nhiroki@chromium.org> Date: Wed Aug 24 13:31:10 2016 Revert of Streams: Remove isTerminating() calls (patchset #3 id:40001 of https://codereview.chromium.org/2265863002/ ) Reason for revert: This causes crashes (see https://bugs.chromium.org/p/chromium/issues/detail?id=640521) Original issue's description: > Streams: Remove isTerminating() calls > > isTerminating() is used for making sure whether a worker execution context is > still valid after V8 API calls, but this is no longer necessary thanks to > graceful shutdown mechanism for WorkerThread ( https://crbug.com/487050 ) > > BUG=614272, 638877 > > Committed: https://crrev.com/53752ac68bbd8b93650a9475c9b5b5f2e13a8e9a > Cr-Commit-Position: refs/heads/master@{#413420} TBR=yhirano@chromium.org,haraken@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=614272, 638877 Review-Url: https://codereview.chromium.org/2277703002 Cr-Commit-Position: refs/heads/master@{#414062} [modify] https://crrev.com/360cedf2865c54108fa97f246737454ff68fc9ad/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp [modify] https://crrev.com/360cedf2865c54108fa97f246737454ff68fc9ad/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h [modify] https://crrev.com/360cedf2865c54108fa97f246737454ff68fc9ad/third_party/WebKit/Source/core/streams/ReadableStreamController.h [modify] https://crrev.com/360cedf2865c54108fa97f246737454ff68fc9ad/third_party/WebKit/Source/core/streams/ReadableStreamOperations.cpp [modify] https://crrev.com/360cedf2865c54108fa97f246737454ff68fc9ad/third_party/WebKit/Source/core/workers/WorkerThread.cpp [modify] https://crrev.com/360cedf2865c54108fa97f246737454ff68fc9ad/third_party/WebKit/Source/core/workers/WorkerThread.h [modify] https://crrev.com/360cedf2865c54108fa97f246737454ff68fc9ad/third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp [modify] https://crrev.com/360cedf2865c54108fa97f246737454ff68fc9ad/third_party/WebKit/Source/core/workers/WorkletGlobalScope.cpp [modify] https://crrev.com/360cedf2865c54108fa97f246737454ff68fc9ad/third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp [modify] https://crrev.com/360cedf2865c54108fa97f246737454ff68fc9ad/third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
,
Aug 25 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aefae76fee3afe6cc9eabbe5d06cde009d317c16 commit aefae76fee3afe6cc9eabbe5d06cde009d317c16 Author: nhiroki <nhiroki@chromium.org> Date: Thu Aug 25 06:12:44 2016 Worker: Refine state management of WorkerThread This CL... 1) introduces ThreadState enum that represents a state of the worker thread, 2) stops accessing |m_globalScope| from the main thread for detecting worker thread initialization and instead checks ThreadState, 3) renames |m_started| and |m_terminated| with |m_requestedToStart| and |m_requestedToTerminate| in order to emphasize that these fields represent not a state of the worker thread but facts that the worker thread has been requested to start/terminate from the main thread, and 4) removes mention of |m_microtaskRunner| from a comment about |m_threadStateMutex| because the member is accessed only from the worker thread. BUG= 638877 , 640843 Review-Url: https://codereview.chromium.org/2268183005 Cr-Commit-Position: refs/heads/master@{#414336} [modify] https://crrev.com/aefae76fee3afe6cc9eabbe5d06cde009d317c16/third_party/WebKit/Source/core/workers/WorkerThread.cpp [modify] https://crrev.com/aefae76fee3afe6cc9eabbe5d06cde009d317c16/third_party/WebKit/Source/core/workers/WorkerThread.h
,
Aug 26 2016
,
Sep 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/01ff9de2e66ce93e2dc7bc9f0d7ee2681f32367c commit 01ff9de2e66ce93e2dc7bc9f0d7ee2681f32367c Author: nhiroki <nhiroki@chromium.org> Date: Wed Sep 14 15:41:51 2016 Worker: Check forcible termination by WorkerThread::isForciblyTerminated() Before this CL, WorkerThread checks whether forcible termination happened by WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL, WorkerThread checks it by WorkerThread::isForciblyTerminated() instead. This should be clearer than the previous way and enables to remove an access to WorkerThread::m_globalScope from the main thread. BUG= 638877 Review-Url: https://codereview.chromium.org/2324693003 Cr-Commit-Position: refs/heads/master@{#418571} [modify] https://crrev.com/01ff9de2e66ce93e2dc7bc9f0d7ee2681f32367c/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp [modify] https://crrev.com/01ff9de2e66ce93e2dc7bc9f0d7ee2681f32367c/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h [modify] https://crrev.com/01ff9de2e66ce93e2dc7bc9f0d7ee2681f32367c/third_party/WebKit/Source/core/workers/WorkerThread.cpp [modify] https://crrev.com/01ff9de2e66ce93e2dc7bc9f0d7ee2681f32367c/third_party/WebKit/Source/core/workers/WorkerThread.h [modify] https://crrev.com/01ff9de2e66ce93e2dc7bc9f0d7ee2681f32367c/third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp [modify] https://crrev.com/01ff9de2e66ce93e2dc7bc9f0d7ee2681f32367c/third_party/WebKit/Source/core/workers/WorkletGlobalScope.cpp
,
Sep 15 2016
All accesses to WorkerThread::m_globalScope from the main thread are gone :D
,
Sep 15 2016
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by nhiroki@chromium.org
, Aug 18 2016