New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 638877 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 641215



Sign in to add a comment

WorkerThread::m_globalScope is nullified without a lock on a worker thread

Project Member Reported by nhiroki@chromium.org, Aug 18 2016

Issue description

WorkerThread::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)
 
NOTE: This is safe because it's guaranteed that the main thread never accesses m_workerGlobalScope after it's nullified on the worker thread (of course we should do it in a saner way though)
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.
Summary: WorkerThread::m_globalScope is nullified without a lock on a worker thread (was: WorkerThread::m_workerGlobalScope is nullified without a lock on a worker thread)
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Status: Started (was: Assigned)
I'm now working on the ideal-fix.
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Project Member

Comment 8 by bugdroid1@chromium.org, 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

Blockedon: 641215
Project Member

Comment 10 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
All accesses to WorkerThread::m_globalScope from the main thread are gone :D
Labels: M-55

Sign in to add a comment