Worker: Stop calling Isolate::TerminateExecution() on a worker thread

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 09d432d27e07d2e84d786d659f64ba7650f45ca3..c735446fdf8da8e18e90434375acec4c42317a48 100644
--- a/third_party/WebKit/Source/core/workers/WorkerThread.cpp
+++ b/third_party/WebKit/Source/core/workers/WorkerThread.cpp
@@ -143,36 +143,32 @@ void WorkerThread::terminate()
     if (!m_workerGlobalScope)
         return;
 
-    if (!m_readyToShutdown) {
-        // Ensure that tasks are being handled by thread event loop. If script
-        // execution weren't forbidden, a while(1) loop in JS could keep the
-        // thread alive forever.
-        // If |m_readyToShutdown| 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
-        // execution.
+    // 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();
-
-        // 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|.
-        if (workerBackingThread().workerScriptCount() == 1) {
-            if (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 postpone termination after
-                // task has finished.
-                // Note: m_runningDebuggerTask and m_shouldTerminateV8Execution
-                // access must be guarded by the lock.
-                m_shouldTerminateV8Execution = true;
-            } else {
-                // TODO(yhirano): TerminateExecution should be called more
-                // carefully (https://crbug.com/413518).
-                isolate()->TerminateExecution();
-            }
-        }
+        isolate()->TerminateExecution();
     }
 
     m_inspectorTaskRunner->kill();
@@ -448,10 +444,16 @@ void WorkerThread::runDebuggerTaskOnWorkerThread(std::unique_ptr<CrossThreadClos
     {
         MutexLocker lock(m_threadStateMutex);
         m_runningDebuggerTask = false;
-        if (m_shouldTerminateV8Execution) {
-            m_shouldTerminateV8Execution = false;
-            isolate()->TerminateExecution();
-        }
+
+        if (!m_terminated)
+            return;
+        // terminate() was called. Shutdown sequence will start soon.
+
+        if (m_readyToShutdown)
+            return;
+        // Disposes WorkerGlobalScope to stop associated ActiveDOMObjects and
+        // close the event queue.
+        prepareForShutdownOnWorkerThread();

[yhirano, 2016/05/26 07:57:38]

m_threadStateMutex is not recursive, so you should not call this function here.

[yhirano, 2016/05/26 07:57:38]

I'm not sure why this is needed, because we've already posted
prepareForShutdownOnWorkerThread from WorkerThread::terminate.

[nhiroki, 2016/05/26 08:13:52]

Good point. Moved this to out of the critical section.

[nhiroki, 2016/05/26 08:13:52]

Other tasks that may block the worker thread could be interleaved between this
debugger task and that posted task. To avoid them, I think we should set
m_readyToShutdown and dispose WorkerGlobalScope here.

     }
 }