Worker: Attempt to gracefully terminate WorkerThread as much as possible

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

Index: third_party/WebKit/Source/core/workers/WorkerThread.cpp
diff --git a/third_party/WebKit/Source/core/workers/WorkerThread.cpp b/third_party/WebKit/Source/core/workers/WorkerThread.cpp
index 1b521bc58305b8d4b9f7db3e4605f4e817cdf609..958529529528185880c50c11b59be06013fca5bf 100644
--- a/third_party/WebKit/Source/core/workers/WorkerThread.cpp
+++ b/third_party/WebKit/Source/core/workers/WorkerThread.cpp
@@ -42,6 +42,7 @@
 #include "platform/WebThreadSupportingGC.h"
 #include "platform/heap/SafePoint.h"
 #include "platform/heap/ThreadState.h"
+#include "platform/scheduler/CancellableTaskFactory.h"
 #include "platform/weborigin/KURL.h"
 #include "public/platform/WebThread.h"
 #include "wtf/Functional.h"
@@ -51,6 +52,62 @@
 
 namespace blink {
 
+const long long kForceTerminationDelayInMs = 3000; // 3 secs
+
+// ForceTerminationTask is used for posting a delayed task to terminate an
+// isolate from the main thread. This task is expected to run when the shutdown
+// sequence does not start in a certain time period because of an inifite loop
+// in the JS execution context etc. When the shutdown sequence is started before
+// this task runs, the task is simply cancelled.
+class WorkerThread::ForceTerminationTask {

[yhirano, 2016/05/27 11:04:43]

+final

[nhiroki, 2016/05/30 06:18:47]

Done.

+public:
+    static PassOwnPtr<ForceTerminationTask> create(WorkerThread* workerThread)
+    {
+        return adoptPtr(new ForceTerminationTask(workerThread));
+    }
+
+    ~ForceTerminationTask()
+    {
+        DCHECK(isMainThread());
+        if (m_cancellableTaskFactory->isPending())
+            m_cancellableTaskFactory->cancel();

[yhirano, 2016/05/27 11:04:43]

Is this needed? Doesn't m_cancellableTaskFactory cancel tasks when destructed?

[nhiroki, 2016/05/30 06:18:47]

Ah, it's not necessary. Removed.

+    }
+
+    void schedule()
+    {
+        DCHECK(isMainThread());
+        Platform::current()->mainThread()->getWebTaskRunner()->postDelayedTask(BLINK_FROM_HERE, m_cancellableTaskFactory->cancelAndCreate(), m_workerThread->m_forceTerminationDelayInMs);
+    }
+
+private:
+    explicit ForceTerminationTask(WorkerThread* workerThread)
+        : m_workerThread(workerThread)
+    {
+        DCHECK(isMainThread());
+        m_cancellableTaskFactory = CancellableTaskFactory::create(this, &ForceTerminationTask::run);
+    }
+
+    void run()
+    {
+        DCHECK(isMainThread());
+        MutexLocker lock(m_workerThread->m_threadStateMutex);
+        if (m_workerThread->m_readyToShutdown) {
+            // Shutdown sequence is now running. Just return.
+            return;
+        }
+
+        // TODO(yhirano): TerminateExecution should be called more carefully.
+        // (https://crbug.com/413518)

[yhirano, 2016/05/27 11:04:43]

Is this comment still needed?

[nhiroki, 2016/05/30 06:18:47]

Removed.

+        m_workerThread->m_workerGlobalScope->scriptController()->willScheduleExecutionTermination();
+        m_workerThread->isolate()->TerminateExecution();
+        DCHECK_EQ(WorkerThread::ExitCode::NotTerminated, m_workerThread->m_exitCode);
+        m_workerThread->m_exitCode = WorkerThread::ExitCode::AsyncForciblyTerminated;
+    }
+
+    WorkerThread* m_workerThread;
+    OwnPtr<CancellableTaskFactory> m_cancellableTaskFactory;
+};
+
 class WorkerThread::WorkerMicrotaskRunner : public WebThread::TaskObserver {
 public:
     explicit WorkerMicrotaskRunner(WorkerThread* workerThread)
@@ -100,9 +157,14 @@ static HashSet<WorkerThread*>& workerThreads()
 
 WorkerThread::~WorkerThread()
 {
+    DCHECK(isMainThread());
     MutexLocker lock(threadSetMutex());
     DCHECK(workerThreads().contains(this));
     workerThreads().remove(this);
+
+    // TODO(nhiroki): Record how this thread is terminated (i.e. m_exitCode)
+    // in UMA.
+    DCHECK_NE(ExitCode::NotTerminated, m_exitCode);
 }
 
 void WorkerThread::start(PassOwnPtr<WorkerThreadStartupData> startupData)
@@ -119,66 +181,16 @@ void WorkerThread::start(PassOwnPtr<WorkerThreadStartupData> startupData)
 void WorkerThread::terminate()
 {
     DCHECK(isMainThread());
-
-    // Prevent the deadlock between GC and an attempt to terminate a thread.
-    SafePointScope safePointScope(BlinkGC::HeapPointersOnStack);
-
-    // Protect against this method, initializeOnWorkerThread() or termination
-    // via the global scope racing each other.
-    MutexLocker lock(m_threadStateMutex);
-
-    // If terminate has already been called, just return.
-    if (m_terminated)
-        return;
-    m_terminated = true;
-
-    // Signal the thread to notify that the thread's stopping.
-    if (m_terminationEvent)
-        m_terminationEvent->signal();
-
-    // 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().
-    if (!m_workerGlobalScope)
-        return;
-
-    // Determine if we should terminate the isolate so that the task can
-    // be handled by thread event loop. If script execution weren't forbidden,
-    // a while(1) loop in JS could keep the thread alive forever.
-    //
-    // (1) |m_readyToShutdown|: It this is set, the worker thread has already
-    // noticed that the thread is about to be terminated and the worker global
-    // scope is already disposed, so we don't have to explicitly terminate the
-    // isolate.
-    //
-    // (2) |workerScriptCount() == 1|: If other WorkerGlobalScopes are running
-    // on the worker thread, we should not terminate the isolate. This condition
-    // is not entirely correct because other scripts can be being initialized or
-    // terminated simuletaneously. Though this function itself is protected by a
-    // mutex, it is possible that |workerScriptCount()| here is not consistent
-    // with that in |initialize| and |shutdown|.
-    //
-    // (3) |m_runningDebuggerTask|: Terminating during debugger task may lead to
-    // crash due to heavy use of v8 api in debugger. Any debugger task is
-    // guaranteed to finish, so we can wait for the completion.
-    bool shouldTerminateIsolate = !m_readyToShutdown && (workerBackingThread().workerScriptCount() == 1) && !m_runningDebuggerTask;
-
-    if (shouldTerminateIsolate) {
-        // TODO(yhirano): TerminateExecution should be called more
-        // carefully (https://crbug.com/413518).
-        m_workerGlobalScope->scriptController()->willScheduleExecutionTermination();
-        isolate()->TerminateExecution();
-    }
-
-    m_inspectorTaskRunner->kill();
-    workerBackingThread().backingThread().postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerThread::prepareForShutdownOnWorkerThread, AllowCrossThreadAccess(this)));
-    workerBackingThread().backingThread().postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerThread::performShutdownOnWorkerThread, AllowCrossThreadAccess(this)));
+    terminateInternal(TerminationMode::Graceful);
 }
 
 void WorkerThread::terminateAndWait()
 {
     DCHECK(isMainThread());
-    terminate();
+
+    // The main thread will be blocked, so asynchronous graceful shutdown does
+    // not work.
+    terminateInternal(TerminationMode::Force);
     m_shutdownEvent->wait();
 }
 
@@ -189,8 +201,11 @@ void WorkerThread::terminateAndWaitForAllWorkers()
     // Keep this lock to prevent WorkerThread instances from being destroyed.
     MutexLocker lock(threadSetMutex());
     HashSet<WorkerThread*> threads = workerThreads();
+
+    // The main thread will be blocked, so asynchronous graceful shutdown does
+    // not work.
     for (WorkerThread* thread : threads)
-        thread->terminate();
+        thread->terminateInternal(TerminationMode::Force);
 
     for (WorkerThread* thread : threads)
         thread->m_shutdownEvent->wait();
@@ -275,7 +290,8 @@ PlatformThreadId WorkerThread::platformThreadId()
 }
 
 WorkerThread::WorkerThread(PassRefPtr<WorkerLoaderProxy> workerLoaderProxy, WorkerReportingProxy& workerReportingProxy)
-    : m_inspectorTaskRunner(adoptPtr(new InspectorTaskRunner()))
+    : m_forceTerminationDelayInMs(kForceTerminationDelayInMs)
+    , m_inspectorTaskRunner(adoptPtr(new InspectorTaskRunner()))
     , m_workerLoaderProxy(workerLoaderProxy)
     , m_workerReportingProxy(workerReportingProxy)
     , m_terminationEvent(adoptPtr(new WaitableEvent(
@@ -361,6 +377,75 @@ void WorkerThread::initializeOnWorkerThread(PassOwnPtr<WorkerThreadStartupData>
     postInitialize();
 }
 
+void WorkerThread::terminateInternal(TerminationMode mode)
+{
+    // Prevent the deadlock between GC and an attempt to terminate a thread.
+    SafePointScope safePointScope(BlinkGC::HeapPointersOnStack);
+
+    // Protect against this method, initializeOnWorkerThread() or termination
+    // via the global scope racing each other.
+    MutexLocker lock(m_threadStateMutex);
+
+    // If terminate has already been called, just return.
+    if (m_terminated)
+        return;
+    m_terminated = true;
+
+    // Signal the thread to notify that the thread's stopping.
+    if (m_terminationEvent)
+        m_terminationEvent->signal();
+
+    // 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().
+    if (!m_workerGlobalScope) {
+        DCHECK_EQ(ExitCode::NotTerminated, m_exitCode);
+        m_exitCode = ExitCode::GracefullyTerminated;
+        return;
+    }
+
+    // Determine if we should synchronously terminate or schedule to terminate
+    // the isolate so that the task can be handled by thread event loop. If
+    // script execution weren't forbidden, a while(1) loop in JS could keep
+    // the thread alive forever.
+    //
+    // (1) |m_readyToShutdown|: It this is set, the worker thread has already
+    // noticed that the thread is about to be terminated and the worker global
+    // scope is already disposed, so we don't have to explicitly terminate the
+    // isolate.
+    //
+    // (2) |workerScriptCount() == 1|: If other WorkerGlobalScopes are running
+    // on the worker thread, we should not terminate the isolate. This condition
+    // is not entirely correct because other scripts can be being initialized or
+    // terminated simuletaneously. Though this function itself is protected by a
+    // mutex, it is possible that |workerScriptCount()| here is not consistent
+    // with that in |initialize| and |shutdown|.
+    //
+    // (3) |m_runningDebuggerTask|: Terminating during debugger task may lead to
+    // crash due to heavy use of v8 api in debugger. Any debugger task is
+    // guaranteed to finish, so we can wait for the completion.
+    bool shouldScheduleToTerminateIsolate = !m_readyToShutdown && (workerBackingThread().workerScriptCount() == 1) && !m_runningDebuggerTask;
+
+    if (shouldScheduleToTerminateIsolate) {
+        if (mode == TerminationMode::Force) {
+            // TODO(yhirano): TerminateExecution should be called more
+            // carefully (https://crbug.com/413518).
+            m_workerGlobalScope->scriptController()->willScheduleExecutionTermination();
+            isolate()->TerminateExecution();
+            DCHECK_EQ(ExitCode::NotTerminated, m_exitCode);
+            m_exitCode = ExitCode::SyncForciblyTerminated;
+        } else {
+            DCHECK_EQ(TerminationMode::Graceful, mode);
+            m_scheduledForceTerminationTask = ForceTerminationTask::create(this);

[yhirano, 2016/05/27 11:04:43]

I'm not sure if we should avoid scheduling the task on compositor workers.

[nhiroki, 2016/05/30 06:18:47]

If we should avoid scheduling it on CW, we could add |virtual bool
WorkerThread::needsGracefulTermination()|, make CompositorWorkerThread return
false and check it here.

If the design of CW lifecycle hasn't clearly been determined yet, I'd like to
keep this CL as is for now and add the check function later if necessary.

flackr@, any thoughts?

[flackr, 2016/05/30 14:41:38]

It seems like compositor worker currently isn't ever going to trigger this
because we manually attach without a worker thread on the BackingThreadHolder
and manually detach last (i.e. shouldScheduleToTerminateIsolate will be false
because workerScriptCount > 1) and we manually call TerminateExecution. That
being said, if we did change this, this scheduled forced termination task would
never get a chance to run on compositor worker since at shutdown it waits on the
termination of the backing thread.

All in all, I think it's fine to keep as is for now from the perspective of
compositor worker.

[nhiroki, 2016/05/30 22:15:12]

I understand the current CW behavior. Thank you for checking this.

+            m_scheduledForceTerminationTask->schedule();
+        }
+    }
+
+    m_inspectorTaskRunner->kill();
+    workerBackingThread().backingThread().postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerThread::prepareForShutdownOnWorkerThread, AllowCrossThreadAccess(this)));
+    workerBackingThread().backingThread().postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerThread::performShutdownOnWorkerThread, AllowCrossThreadAccess(this)));
+}
+
 void WorkerThread::prepareForShutdownOnWorkerThread()
 {
     DCHECK(isCurrentThread());
@@ -369,6 +454,8 @@ void WorkerThread::prepareForShutdownOnWorkerThread()
         if (m_readyToShutdown)
             return;
         m_readyToShutdown = true;
+        if (m_exitCode == ExitCode::NotTerminated)
+            m_exitCode = ExitCode::GracefullyTerminated;
     }
 
     workerReportingProxy().willDestroyWorkerGlobalScope();
@@ -460,4 +547,10 @@ void WorkerThread::runDebuggerTaskDontWaitOnWorkerThread()
         (*task)();
 }
 
+WorkerThread::ExitCode WorkerThread::exitCode()
+{
+    MutexLocker lock(m_threadStateMutex);
+    return m_exitCode;
+}
+
 } // namespace blink