New issue
Advanced search Search tips

Issue 640843 link

Starred by 4 users

Issue metadata

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

Blocked on:
issue 639153



Sign in to add a comment

Refine state management of WorkerThread

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

Issue description

State management of WorkerThread is very complicated. It'd be nice if we can make it more straightforward, solid and readable.
 
Blockedon: 639153
Project Member

Comment 3 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

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ece89237c1fe0cf950bfb405d9c214b7b0fd7aff

commit ece89237c1fe0cf950bfb405d9c214b7b0fd7aff
Author: nhiroki <nhiroki@chromium.org>
Date: Mon Sep 12 23:31:07 2016

Worker: Factor out a part of WorkerThread::terminateInternal() into its own function for cleanup

Motivation of this change is to improve readability/testability of the current
complex critical section, and to make it more robust to future changes.

BUG= 640843 

Review-Url: https://codereview.chromium.org/2325923002
Cr-Commit-Position: refs/heads/master@{#418103}

[modify] https://crrev.com/ece89237c1fe0cf950bfb405d9c214b7b0fd7aff/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/ece89237c1fe0cf950bfb405d9c214b7b0fd7aff/third_party/WebKit/Source/core/workers/WorkerThread.h
[modify] https://crrev.com/ece89237c1fe0cf950bfb405d9c214b7b0fd7aff/third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ac6098db746f5939263b51863df6f061f0ebdb76

commit ac6098db746f5939263b51863df6f061f0ebdb76
Author: nhiroki <nhiroki@chromium.org>
Date: Tue Sep 13 13:48:16 2016

Worker: Notify WorkerThread lifecycle events via WorkerReportingProxy

Before this CL, script loading and evaluation are directly notified to
WorkerGlobalScope. After this CL, they are notified via WorkerReportingProxy
that is a proper route.

BUG= 519111 ,  640843 

Review-Url: https://codereview.chromium.org/2337003002
Cr-Commit-Position: refs/heads/master@{#418230}

[modify] https://crrev.com/ac6098db746f5939263b51863df6f061f0ebdb76/third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp
[modify] https://crrev.com/ac6098db746f5939263b51863df6f061f0ebdb76/third_party/WebKit/Source/core/workers/WorkerGlobalScope.h
[modify] https://crrev.com/ac6098db746f5939263b51863df6f061f0ebdb76/third_party/WebKit/Source/core/workers/WorkerReportingProxy.h
[modify] https://crrev.com/ac6098db746f5939263b51863df6f061f0ebdb76/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/ac6098db746f5939263b51863df6f061f0ebdb76/third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp
[modify] https://crrev.com/ac6098db746f5939263b51863df6f061f0ebdb76/third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h
[modify] https://crrev.com/ac6098db746f5939263b51863df6f061f0ebdb76/third_party/WebKit/Source/modules/compositorworker/AnimationWorkletThreadTest.cpp
[modify] https://crrev.com/ac6098db746f5939263b51863df6f061f0ebdb76/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
[modify] https://crrev.com/ac6098db746f5939263b51863df6f061f0ebdb76/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.h
[modify] https://crrev.com/ac6098db746f5939263b51863df6f061f0ebdb76/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp
[modify] https://crrev.com/ac6098db746f5939263b51863df6f061f0ebdb76/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h
[modify] https://crrev.com/ac6098db746f5939263b51863df6f061f0ebdb76/third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp
[modify] https://crrev.com/ac6098db746f5939263b51863df6f061f0ebdb76/third_party/WebKit/Source/web/WebSharedWorkerImpl.h

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71c9c5891cfe1cbc631823322894685cb7dc8975

commit 71c9c5891cfe1cbc631823322894685cb7dc8975
Author: nhiroki <nhiroki@chromium.org>
Date: Wed Sep 14 10:32:17 2016

Worker: Canonicalize names of WorkerThread lifecycle events in WorkerReportingProxy

This CL sorts handlers of the lifecycle events in call order and canonicalizes
their names.

BUG= 640843 

Review-Url: https://codereview.chromium.org/2337253005
Cr-Commit-Position: refs/heads/master@{#418530}

[modify] https://crrev.com/71c9c5891cfe1cbc631823322894685cb7dc8975/third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp
[modify] https://crrev.com/71c9c5891cfe1cbc631823322894685cb7dc8975/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
[modify] https://crrev.com/71c9c5891cfe1cbc631823322894685cb7dc8975/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h
[modify] https://crrev.com/71c9c5891cfe1cbc631823322894685cb7dc8975/third_party/WebKit/Source/core/workers/WorkerReportingProxy.h
[modify] https://crrev.com/71c9c5891cfe1cbc631823322894685cb7dc8975/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/71c9c5891cfe1cbc631823322894685cb7dc8975/third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp
[modify] https://crrev.com/71c9c5891cfe1cbc631823322894685cb7dc8975/third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h
[modify] https://crrev.com/71c9c5891cfe1cbc631823322894685cb7dc8975/third_party/WebKit/Source/modules/compositorworker/AnimationWorkletThreadTest.cpp
[modify] https://crrev.com/71c9c5891cfe1cbc631823322894685cb7dc8975/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp
[modify] https://crrev.com/71c9c5891cfe1cbc631823322894685cb7dc8975/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp
[modify] https://crrev.com/71c9c5891cfe1cbc631823322894685cb7dc8975/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h
[modify] https://crrev.com/71c9c5891cfe1cbc631823322894685cb7dc8975/third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp
[modify] https://crrev.com/71c9c5891cfe1cbc631823322894685cb7dc8975/third_party/WebKit/Source/web/WebSharedWorkerImpl.h

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8def110d7293b51fcdf0424a212b45d6ed33d24e

commit 8def110d7293b51fcdf0424a212b45d6ed33d24e
Author: nhiroki <nhiroki@chromium.org>
Date: Tue Sep 20 08:25:32 2016

Worker: Refine WorkerThreadTest

This CL...

- shortens variable names for improving readability,
- renames tests with more concise naming convention:
  <termination_mode>_<termination_timing>
- doesn't change test bodies other than Terminate_WhileDebuggerTaskIsRunning
  (StartAndTerminateOnScriptLoaded_TerminateWhileDebuggerTaskIsRunning), and
- makes Terminate_WhileDebuggerTaskIsRunning to run a real debugger task instead
  of simulating it to remove a test-only lifecycle observer implementation.

BUG= 640843 

Review-Url: https://codereview.chromium.org/2335343006
Cr-Commit-Position: refs/heads/master@{#419698}

[modify] https://crrev.com/8def110d7293b51fcdf0424a212b45d6ed33d24e/third_party/WebKit/Source/core/workers/WorkerThread.h
[modify] https://crrev.com/8def110d7293b51fcdf0424a212b45d6ed33d24e/third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 23 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/97f2c9054251e15ae8520cc7bf193e18c32eb6b3

commit 97f2c9054251e15ae8520cc7bf193e18c32eb6b3
Author: nhiroki <nhiroki@chromium.org>
Date: Fri Sep 23 02:21:55 2016

Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() was called during WorkerThread
initialization. This timing was late, but it was intended because one of
observers (ServiceWorker) wanted to access WorkerGlobalScope in the notification
that is created during WorkerThread initialization in order to record size of
worker scripts.

After this CL, recording the size is done in willEvaluateWorkerScript() that
would be more appropriate timing for the operation. willEvaluateWorkerScript()
is also useful for unit tests. It enables them to wait until the timing that
they really wait for.

BUG= 640843 

Review-Url: https://codereview.chromium.org/2355723005
Cr-Commit-Position: refs/heads/master@{#420546}

[modify] https://crrev.com/97f2c9054251e15ae8520cc7bf193e18c32eb6b3/third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp
[modify] https://crrev.com/97f2c9054251e15ae8520cc7bf193e18c32eb6b3/third_party/WebKit/Source/core/workers/WorkerReportingProxy.h
[modify] https://crrev.com/97f2c9054251e15ae8520cc7bf193e18c32eb6b3/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/97f2c9054251e15ae8520cc7bf193e18c32eb6b3/third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp
[modify] https://crrev.com/97f2c9054251e15ae8520cc7bf193e18c32eb6b3/third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h
[modify] https://crrev.com/97f2c9054251e15ae8520cc7bf193e18c32eb6b3/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
[modify] https://crrev.com/97f2c9054251e15ae8520cc7bf193e18c32eb6b3/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.h
[modify] https://crrev.com/97f2c9054251e15ae8520cc7bf193e18c32eb6b3/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp
[modify] https://crrev.com/97f2c9054251e15ae8520cc7bf193e18c32eb6b3/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h

Cc: horo@chromium.org nhiroki@chromium.org toyoshim@chromium.org kinuko@chromium.org
 Issue 519111  has been merged into this issue.
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 23 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cfa01627d191d2a1a801943994f287b8cf5b1c42

commit cfa01627d191d2a1a801943994f287b8cf5b1c42
Author: nhiroki <nhiroki@chromium.org>
Date: Fri Sep 23 11:29:28 2016

Worker: Unify worker thread shutdown sequences.

This CL unifies worker thread shutdown sequences to fix a crash and simplify
shutdown.

<Problem>

When termination is requested on the main thread before WorkerThread is
initialized on the worker thread, shutdown sequence runs in a different way from
regular shutdown sequence: initialization sequence seamlessly switches to
shutdown sequence and asks the main thread to destroy WorkerThread
(see WorkerThread::initializeOnWorkerThread).

This causes a crash in a following scenario:

  1) Request to start the worker thread from the main thread.
  2) Post a task to the worker thread from the main thread.
  3) Request to terminate the worker thread from the main thread.
  4) Start initialziation sequence and switch to shutdown sequence on the worker
     thread.
  5) WorkerThread is destroyed on the main thread.
  6) The posted task runs on the worker thread and crashes.

<Solution>

This CL makes the initialization sequence complete regardless of termination
request and defer to the regular shutdown task posted from the main thread.
Other tasks also posted from the main thread(*) are guaranteed to be drained
until the shutdown task runs.

<Appendix>

Regarding (*), you might wonder if tasks posted from/to the worker thread cannot
be guaranteed to be drained until the shutdown tasks run. This case is covered
by other mechanism. Let's consider it in following 2 cases:

  1) An uninitialized worker thread does not post a task to itself, so there
     should be no tasks when termination happens before initialization.
  2) Otherwise, WorkerBackingThread::shutdown drains all tasks.

Therefore, we can ensure that tasks posted from the worker thread also never run
after shutdown.

BUG=632810,  640843 

Review-Url: https://codereview.chromium.org/2280523002
Cr-Commit-Position: refs/heads/master@{#420592}

[modify] https://crrev.com/cfa01627d191d2a1a801943994f287b8cf5b1c42/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/cfa01627d191d2a1a801943994f287b8cf5b1c42/third_party/WebKit/Source/core/workers/WorkerThread.h
[modify] https://crrev.com/cfa01627d191d2a1a801943994f287b8cf5b1c42/third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp

Project Member

Comment 11 by bugdroid1@chromium.org, Sep 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/845fdc5871ba99d69966175052eb297e4c76b037

commit 845fdc5871ba99d69966175052eb297e4c76b037
Author: nhiroki <nhiroki@chromium.org>
Date: Fri Sep 30 05:02:29 2016

Worker: Notify WorkerReportingProxy::didInitializeWorkerContext() immediately after initialization

Before this CL, WorkerReportingProxy::didInitializeWorkerContext() is notified
after debugger tasks may run. This looks strange. The function should be called
immediately after the context initialization.

BUG= 640843 

Review-Url: https://codereview.chromium.org/2367783003
Cr-Commit-Position: refs/heads/master@{#422032}

[modify] https://crrev.com/845fdc5871ba99d69966175052eb297e4c76b037/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h
[modify] https://crrev.com/845fdc5871ba99d69966175052eb297e4c76b037/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/845fdc5871ba99d69966175052eb297e4c76b037/third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp

Labels: M-55
Status: Fixed (was: Started)
Most parts of the state management has been refined. I'll file a new issue if there is a room for improvement.

Sign in to add a comment