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}

third_party/WebKit/Source/core/workers/WorkerThread.cpp

@@ -366,7 +366,6 @@ void WorkerThread::terminateInternal(TerminationMode mode)
 {
     DCHECK(isMainThread());
     DCHECK(m_requestedToStart);
-    bool hasBeenInitialized = true;
 
     {
         // Prevent the deadlock between GC and an attempt to terminate a thread.
@@ -376,8 +375,6 @@ void WorkerThread::terminateInternal(TerminationMode mode)
         // termination via the global scope racing each other.
         MutexLocker lock(m_threadStateMutex);
 
-        hasBeenInitialized = (m_threadState != ThreadState::NotStarted);
-
         // If terminate has already been called.
         if (m_requestedToTerminate) {
             if (m_runningDebuggerTask) {
@@ -400,12 +397,7 @@ void WorkerThread::terminateInternal(TerminationMode mode)
         }
         m_requestedToTerminate = true;
 
-        if (!hasBeenInitialized) {
-            // If the worker thread was never initialized, don't start another
-            // shutdown, but still wait for the thread to signal when shutdown
-            // has completed on initializeOnWorkerThread().
-            setExitCode(lock, ExitCode::GracefullyTerminated);
-        } else if (shouldScheduleToTerminateExecution(lock)) {
+        if (shouldScheduleToTerminateExecution(lock)) {
             switch (mode) {
             case TerminationMode::Forcible:
                 forciblyTerminateExecution(lock, ExitCode::SyncForciblyTerminated);
@@ -420,10 +412,8 @@ void WorkerThread::terminateInternal(TerminationMode mode)
     }
 
     m_workerThreadLifecycleContext->notifyContextDestroyed();
-    if (!hasBeenInitialized)
-        return;
-
     m_inspectorTaskRunner->kill();
+
     workerBackingThread().backingThread().postTask(BLINK_FROM_HERE, crossThreadBind(&WorkerThread::prepareForShutdownOnWorkerThread, crossThreadUnretained(this)));
     workerBackingThread().backingThread().postTask(BLINK_FROM_HERE, crossThreadBind(&WorkerThread::performShutdownOnWorkerThread, crossThreadUnretained(this)));
 }
@@ -480,6 +470,8 @@ bool WorkerThread::isInShutdown()
 void WorkerThread::initializeOnWorkerThread(std::unique_ptr<WorkerThreadStartupData> startupData)
 {
     DCHECK(isCurrentThread());
+    DCHECK_EQ(ThreadState::NotStarted, m_threadState);
+
     KURL scriptURL = startupData->m_scriptURL;
     String sourceCode = startupData->m_sourceCode;
     WorkerThreadStartMode startMode = startupData->m_startMode;
@@ -489,21 +481,6 @@ void WorkerThread::initializeOnWorkerThread(std::unique_ptr<WorkerThreadStartupD
     {
         MutexLocker lock(m_threadStateMutex);
 
-        // The worker was terminated before the thread had a chance to run.
-        if (m_requestedToTerminate) {
-            DCHECK_EQ(ExitCode::GracefullyTerminated, m_exitCode);
-
-            // Notify the proxy that the WorkerOrWorkletGlobalScope has been
-            // disposed of. This can free this thread object, hence it must not
-            // be touched afterwards.
-            m_workerReportingProxy.didTerminateWorkerThread();
-
-            // Notify the main thread that it is safe to deallocate our
-            // resources.
-            m_shutdownEvent->signal();
-            return;
-        }
-
         if (isOwningBackingThread())
             workerBackingThread().initialize();
 
@@ -520,6 +497,8 @@ void WorkerThread::initializeOnWorkerThread(std::unique_ptr<WorkerThreadStartupD
         m_workerReportingProxy.didCreateWorkerGlobalScope(globalScope());
         m_workerInspectorController = WorkerInspectorController::create(this);
 
+        // TODO(nhiroki): Handle a case where the script controller fails to
+        // initialize the context.
         globalScope()->scriptController()->initializeContextIfNeeded();
 
         // If Origin Trials have been registered before the V8 context was ready,
@@ -531,14 +510,15 @@ void WorkerThread::initializeOnWorkerThread(std::unique_ptr<WorkerThreadStartupD
         setThreadState(lock, ThreadState::Running);
     }
 
-    if (startMode == PauseWorkerGlobalScopeOnStart) {
+    if (startMode == PauseWorkerGlobalScopeOnStart)
         startRunningDebuggerTasksOnPauseOnWorkerThread();
 
-        // WorkerThread may be ready to shut down at this point if termination
-        // is requested while the debugger task is running. Shutdown sequence
-        // will start soon.
-        if (m_threadState == ThreadState::ReadyToShutdown)
-            return;
+    if (checkRequestedToTerminateOnWorkerThread()) {
+        // Stop further worker tasks from running after this point. WorkerThread
+        // was requested to terminate before initialization or during running
+        // debugger tasks. performShutdownOnWorkerThread() will be called soon.
+        prepareForShutdownOnWorkerThread();
+        return;
     }
 
     if (globalScope()->scriptController()->isContextInitialized()) {
@@ -584,12 +564,7 @@ void WorkerThread::prepareForShutdownOnWorkerThread()
 void WorkerThread::performShutdownOnWorkerThread()
 {
     DCHECK(isCurrentThread());
-#if DCHECK_IS_ON()
-    {
-        MutexLocker lock(m_threadStateMutex);
-        DCHECK(m_requestedToTerminate);
-    }
-#endif
+    DCHECK(checkRequestedToTerminateOnWorkerThread());
     DCHECK_EQ(ThreadState::ReadyToShutdown, m_threadState);
 
     // The below assignment will destroy the context, which will in turn notify
@@ -645,16 +620,14 @@ void WorkerThread::performDebuggerTaskOnWorkerThread(std::unique_ptr<CrossThread
     ThreadDebugger::idleStarted(isolate());
     {
         MutexLocker lock(m_threadStateMutex);
-        if (!m_requestedToTerminate) {
-            m_runningDebuggerTask = false;
+        m_runningDebuggerTask = false;
+        if (!m_requestedToTerminate)
             return;
-        }
-        // terminate() was called. Shutdown sequence will start soon.
-        // Keep |m_runningDebuggerTask| to prevent forcible termination from the
-        // main thread before shutdown preparation.
+        // termiante() was called while a debugger task is running. Shutdown
+        // sequence will start soon.
     }
-    // Stop further worker tasks to run after this point.
-    prepareForShutdownOnWorkerThread();
+    // Stop further debugger tasks from running after this point.
+    m_inspectorTaskRunner->kill();
 }
 
 void WorkerThread::performDebuggerTaskDontWaitOnWorkerThread()
@@ -701,6 +674,12 @@ bool WorkerThread::isThreadStateMutexLocked(const MutexLocker& /* unused */)
 #endif
 }
 
+bool WorkerThread::checkRequestedToTerminateOnWorkerThread()
+{
+    MutexLocker lock(m_threadStateMutex);
+    return m_requestedToTerminate;
+}
+
 ExitCode WorkerThread::getExitCodeForTesting()
 {
     MutexLocker lock(m_threadStateMutex);

third_party/WebKit/Source/core/workers/WorkerThread.h

@@ -225,6 +225,11 @@ private:
     void setExitCode(const MutexLocker&, ExitCode);
     bool isThreadStateMutexLocked(const MutexLocker&);
 
+    // This internally acquires |m_threadStateMutex|. If you already have the
+    // lock or you're on the main thread, you should consider directly accessing
+    // |m_requestedToTerminate|.
+    bool checkRequestedToTerminateOnWorkerThread();
+
     ExitCode getExitCodeForTesting();
 
     // Accessed only on the main thread.
@@ -233,14 +238,13 @@ private:
     // Set on the main thread and checked on both the main and worker threads.
     bool m_requestedToTerminate = false;
 
-    ThreadState m_threadState = ThreadState::NotStarted;
-
     // Accessed only on the worker thread.
     bool m_pausedInDebugger = false;
 
     // Set on the worker thread and checked on both the main and worker threads.
     bool m_runningDebuggerTask = false;
 
+    ThreadState m_threadState = ThreadState::NotStarted;
     ExitCode m_exitCode = ExitCode::NotTerminated;
 
     long long m_forceTerminationDelayInMs;

third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp

@@ -85,7 +85,7 @@ protected:
 
     void expectReportingCallsForWorkerPossiblyTerminatedBeforeInitialization()
     {
-        EXPECT_CALL(*m_reportingProxy, didCreateWorkerGlobalScope(_)).Times(AtMost(1));
+        EXPECT_CALL(*m_reportingProxy, didCreateWorkerGlobalScope(_)).Times(1);
         EXPECT_CALL(*m_reportingProxy, didInitializeWorkerContext()).Times(AtMost(1));
         EXPECT_CALL(*m_reportingProxy, willEvaluateWorkerScriptMock(_, _)).Times(AtMost(1));
         EXPECT_CALL(*m_reportingProxy, didEvaluateWorkerScript(_)).Times(AtMost(1));
@@ -174,15 +174,11 @@ TEST_F(WorkerThreadTest, AsyncTerminate_ImmediatelyAfterStart)
     expectReportingCallsForWorkerPossiblyTerminatedBeforeInitialization();
     start();
 
-    // There are two possible cases depending on timing:
-    // (1) If the thread hasn't been initialized on the worker thread yet,
-    // terminate() should not attempt to shut down the thread.
-    // (2) If the thread has already been initialized on the worker thread,
-    // terminate() should gracefully shut down the thread.
+    // The worker thread is not being blocked, so the worker thread should be
+    // gracefully shut down.
     m_workerThread->terminate();
     m_workerThread->waitForShutdownForTesting();
-    ExitCode exitCode = getExitCode();
-    EXPECT_EQ(ExitCode::GracefullyTerminated, exitCode);
+    EXPECT_EQ(ExitCode::GracefullyTerminated, getExitCode());
 }
 
 TEST_F(WorkerThreadTest, SyncTerminate_ImmediatelyAfterStart)
@@ -192,10 +188,14 @@ TEST_F(WorkerThreadTest, SyncTerminate_ImmediatelyAfterStart)
 
     // There are two possible cases depending on timing:
     // (1) If the thread hasn't been initialized on the worker thread yet,
-    // terminateAndWait() should not attempt to shut down the thread.
+    // terminateAndWait() should wait for initialization and shut down the
+    // thread immediately after that.
     // (2) If the thread has already been initialized on the worker thread,
     // terminateAndWait() should synchronously forcibly terminates the worker
     // execution.
+    // TODO(nhiroki): Make this test deterministically pass through the case 1),
+    // that is, terminateAndWait() is called before initializeOnWorkerThread().
+    // Then, rename this test to SyncTerminate_BeforeInitialization.
     m_workerThread->terminateAndWait();
     ExitCode exitCode = getExitCode();
     EXPECT_TRUE(ExitCode::GracefullyTerminated == exitCode || ExitCode::SyncForciblyTerminated == exitCode);