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

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

 /*
  * Copyright (C) 2008 Apple Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 125 matching lines @@
     // 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;
 
-    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();
     workerBackingThread().backingThread().postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerThread::prepareForShutdownOnWorkerThread, AllowCrossThreadAccess(this)));
     workerBackingThread().backingThread().postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerThread::performShutdownOnWorkerThread, AllowCrossThreadAccess(this)));
 }
 
 void WorkerThread::terminateAndWait()
 {
     DCHECK(isMainThread());
@@ skipping 255 matching lines @@
     {
         MutexLocker lock(m_threadStateMutex);
         m_runningDebuggerTask = true;
     }
     ThreadDebugger::idleFinished(isolate());
     (*task)();
     ThreadDebugger::idleStarted(isolate());
     {
         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.

     }
 }
 
 void WorkerThread::runDebuggerTaskDontWaitOnWorkerThread()
 {
     DCHECK(isCurrentThread());
     std::unique_ptr<CrossThreadClosure> task = m_inspectorTaskRunner->takeNextTask(InspectorTaskRunner::DontWaitForTask);
     if (task)
         (*task)();
 }
 
 } // namespace blink