Introduce WorkerBackingThread

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 f9f67b0cf5944f3188743bafedacbb4d0c8aa788..50f4af9a0a09974b4ca49269b4c4a3137ff6e757 100644
--- a/third_party/WebKit/Source/core/workers/WorkerThread.cpp
+++ b/third_party/WebKit/Source/core/workers/WorkerThread.cpp
@@ -35,6 +35,7 @@
 #include "core/inspector/InspectorTaskRunner.h"
 #include "core/inspector/WorkerThreadDebugger.h"
 #include "core/workers/DedicatedWorkerGlobalScope.h"
+#include "core/workers/WorkerBackingThread.h"
 #include "core/workers/WorkerClients.h"
 #include "core/workers/WorkerReportingProxy.h"
 #include "core/workers/WorkerThreadStartupData.h"
@@ -43,7 +44,6 @@
 #include "platform/heap/SafePoint.h"
 #include "platform/heap/ThreadState.h"
 #include "platform/weborigin/KURL.h"
-#include "public/platform/Platform.h"
 #include "public/platform/WebScheduler.h"
 #include "public/platform/WebThread.h"
 #include "wtf/Functional.h"
@@ -143,7 +143,6 @@ WorkerThread::WorkerThread(PassRefPtr<WorkerLoaderProxy> workerLoaderProxy, Work
     , m_workerLoaderProxy(workerLoaderProxy)
     , m_workerReportingProxy(workerReportingProxy)
     , m_webScheduler(nullptr)
-    , m_isolate(nullptr)
     , m_shutdownEvent(adoptPtr(new WaitableEvent(
         WaitableEvent::ResetPolicy::Manual,
         WaitableEvent::InitialState::NonSignaled)))
@@ -170,14 +169,14 @@ void WorkerThread::start(PassOwnPtr<WorkerThreadStartupData> startupData)
         return;
 
     m_started = true;
-    backingThread().postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerThread::initialize, AllowCrossThreadAccess(this), startupData));
+    workerBackingThread().backingThread().postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerThread::initialize, AllowCrossThreadAccess(this), startupData));
 }
 
 PlatformThreadId WorkerThread::platformThreadId()
 {
     if (!m_started)
         return 0;
-    return backingThread().platformThread().threadId();
+    return workerBackingThread().backingThread().platformThread().threadId();
 }
 
 void WorkerThread::initialize(PassOwnPtr<WorkerThreadStartupData> startupData)
@@ -187,7 +186,7 @@ void WorkerThread::initialize(PassOwnPtr<WorkerThreadStartupData> startupData)
     WorkerThreadStartMode startMode = startupData->m_startMode;
     OwnPtr<Vector<char>> cachedMetaData = startupData->m_cachedMetaData.release();
     V8CacheOptions v8CacheOptions = startupData->m_v8CacheOptions;
-    m_webScheduler = backingThread().platformThread().scheduler();
+    m_webScheduler = workerBackingThread().backingThread().platformThread().scheduler();
 
     {
         MutexLocker lock(m_threadStateMutex);
@@ -202,18 +201,18 @@ void WorkerThread::initialize(PassOwnPtr<WorkerThreadStartupData> startupData)
             return;
         }
 
+        workerBackingThread().attach();
+
+        if (shouldAttachThreadDebugger())
+            V8PerIsolateData::from(isolate())->setThreadDebugger(adoptPtr(new WorkerThreadDebugger(this, isolate())));
         m_microtaskRunner = adoptPtr(new WorkerMicrotaskRunner(this));
-        initializeBackingThread();
-        backingThread().addTaskObserver(m_microtaskRunner.get());
+        workerBackingThread().backingThread().addTaskObserver(m_microtaskRunner.get());
 
-        m_isolate = initializeIsolate();
         // Optimize for memory usage instead of latency for the worker isolate.
-        m_isolate->IsolateInBackgroundNotification();
+        isolate()->IsolateInBackgroundNotification();
         m_workerGlobalScope = createWorkerGlobalScope(startupData);
         m_workerGlobalScope->scriptLoaded(sourceCode.length(), cachedMetaData.get() ? cachedMetaData->size() : 0);
 
-        didStartWorkerThread();
-
         // Notify proxy that a new WorkerGlobalScope has been created and started.
         m_workerReportingProxy.workerGlobalScopeStarted(m_workerGlobalScope.get());
 
@@ -252,11 +251,7 @@ void WorkerThread::shutdown()
 
     workerGlobalScope()->dispose();
 
-    // This should be called after the WorkerGlobalScope's disposed (which may
-    // trigger some last-minutes cleanups) and before the thread actually stops.
-    willStopWorkerThread();
-
-    backingThread().removeTaskObserver(m_microtaskRunner.get());
+    workerBackingThread().backingThread().removeTaskObserver(m_microtaskRunner.get());
     postTask(BLINK_FROM_HERE, createSameThreadTask(&WorkerThread::performShutdownTask, this));
 }
 
@@ -268,10 +263,8 @@ void WorkerThread::performShutdownTask()
     m_workerGlobalScope->notifyContextDestroyed();
     m_workerGlobalScope = nullptr;
 
-    willDestroyIsolate();
-    shutdownBackingThread();
-    destroyIsolate();
-    m_isolate = nullptr;
+    workerBackingThread().detach();
+    // We must not touch workerBackingThread() from now on.
 
     m_microtaskRunner = nullptr;
 
@@ -336,29 +329,34 @@ void WorkerThread::terminateInternal()
     // 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.
     m_workerGlobalScope->scriptController()->willScheduleExecutionTermination();
 
-    // 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.
-    if (m_runningDebuggerTask)
-        m_shouldTerminateV8Execution = true;
-    else
-        terminateV8Execution();
+    if (workerBackingThread().workerScriptCount() == 1) {

[haraken, 2016/04/07 11:54:45]

I don't yet understand why we want to have this check. Before this CL,
TerminateExecution was unconditionally sent when
WorkerThread::terminateInternal() is called. Why do we want to make the change
about the condition?

[kinuko, 2016/04/07 12:34:17]

WorkerThread::terminateV8Execution() was virtual method and overridden in
CompositorWorker code to call TerminateExecution conditionally.  My
understanding is that WorkerBackingThread is an attempt to make the
script:thread relationship more explicit in the core code, thus we have this
here, I think it kinda makes sense.  (Back then it was hidden in
CompositorWorkerThread, which had its own benefit as we didn't need to expose
this in core code, but instead we needed really a lot of virtual methods that
get overridden in subclasses, which was making the overall code structure less
clear-- that's my feeling)

[haraken, 2016/04/07 13:55:08]

I still think it's not a good idea to expose the concept of workerScriptCount()
to outside WorkerBackingThread. Would it be an option to move the termination
logic into WorkerBackingThread?

[haraken, 2016/04/07 13:55:08]

Ah, thanks, got it. I understand this CL doesn't change the existing behavior.

BTW, why do you not send the termination signal when workerScriptCount > 1? I
think the intention of the termination signal is to kill the target thread
immediately in any event.

[kinuko, 2016/04/07 14:37:00]

As far as the thread is shared we can't really forcibly kill each script
execution without stopping other scripts in the current model, and sending
immediate kill signal is not the only purpose of terminate(), as we should still
be able to stop the script execution unless the script has infinite loop etc
(and that's what we're considering applying to other workers too in graceful
shutdown mode).  We'll still want to (immediately) kill the thread's script
execution in blink shutdown sequence, and in the case we call
terminateAndWaitForAllworkers so we should be eventually reaching
workerScriptCount == 1.

[kinuko, 2016/04/07 14:37:00]

I basically agree with this (I had the same request/question), while given that
we have clear TODO comment and this logic is to be revisited / cleaned up soon
again having this code as is in this patch works for me too, this still looks
better than the existing code.  (Leaving the option to make further changes in
this patch up to hirano-san and you)

+        // 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|.
+        // TODO(yhirano): TerminateExecution should be called more carefully.
+        // https://crbug.com/413518
+        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 {
+            isolate()->TerminateExecution();
+        }
+    }
 
     InspectorInstrumentation::allAsyncTasksCanceled(m_workerGlobalScope.get());
     m_inspectorTaskRunner->kill();
-    backingThread().postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerThread::shutdown, AllowCrossThreadAccess(this)));
+    workerBackingThread().backingThread().postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerThread::shutdown, AllowCrossThreadAccess(this)));
 }
 
-void WorkerThread::didStartWorkerThread()
+v8::Isolate* WorkerThread::isolate()
 {
-    ASSERT(isCurrentThread());
-    Platform::current()->didStartWorkerThread();
-}
-
-void WorkerThread::willStopWorkerThread()
-{
-    ASSERT(isCurrentThread());
-    Platform::current()->willStopWorkerThread();
+    return workerBackingThread().isolate();
 }
 
 void WorkerThread::terminateAndWaitForAllWorkers()
@@ -375,58 +373,12 @@ void WorkerThread::terminateAndWaitForAllWorkers()
 
 bool WorkerThread::isCurrentThread()
 {
-    return m_started && backingThread().isCurrentThread();
+    return m_started && workerBackingThread().backingThread().isCurrentThread();
 }
 
 void WorkerThread::postTask(const WebTraceLocation& location, PassOwnPtr<ExecutionContextTask> task)
 {
-    backingThread().postTask(location, createWorkerThreadTask(task, true));
-}
-
-void WorkerThread::initializeBackingThread()
-{
-    ASSERT(isCurrentThread());
-    backingThread().initialize();
-}
-
-void WorkerThread::shutdownBackingThread()
-{
-    ASSERT(isCurrentThread());
-    backingThread().shutdown();
-}
-
-v8::Isolate* WorkerThread::initializeIsolate()
-{
-    ASSERT(isCurrentThread());
-    ASSERT(!m_isolate);
-    v8::Isolate* isolate = V8PerIsolateData::initialize();
-    V8Initializer::initializeWorker(isolate);
-
-    OwnPtr<V8IsolateInterruptor> interruptor = adoptPtr(new V8IsolateInterruptor(isolate));
-    ThreadState::current()->addInterruptor(interruptor.release());
-    ThreadState::current()->registerTraceDOMWrappers(isolate, V8GCController::traceDOMWrappers);
-    if (RuntimeEnabledFeatures::v8IdleTasksEnabled())
-        V8PerIsolateData::enableIdleTasks(isolate, adoptPtr(new V8IdleTaskRunner(m_webScheduler)));
-    V8PerIsolateData::from(isolate)->setThreadDebugger(adoptPtr(new WorkerThreadDebugger(this, isolate)));
-    return isolate;
-}
-
-void WorkerThread::willDestroyIsolate()
-{
-    ASSERT(isCurrentThread());
-    ASSERT(m_isolate);
-    V8PerIsolateData::willBeDestroyed(m_isolate);
-}
-
-void WorkerThread::destroyIsolate()
-{
-    ASSERT(isCurrentThread());
-    V8PerIsolateData::destroy(m_isolate);
-}
-
-void WorkerThread::terminateV8Execution()
-{
-    m_isolate->TerminateExecution();
+    workerBackingThread().backingThread().postTask(location, createWorkerThreadTask(task, true));
 }
 
 void WorkerThread::runDebuggerTaskDontWait()
@@ -446,10 +398,10 @@ void WorkerThread::appendDebuggerTask(PassOwnPtr<CrossThreadClosure> task)
     m_inspectorTaskRunner->appendTask(threadSafeBind(&WorkerThread::runDebuggerTask, AllowCrossThreadAccess(this), task));
     {
         MutexLocker lock(m_threadStateMutex);
-        if (m_isolate)
-            m_inspectorTaskRunner->interruptAndRunAllTasksDontWait(m_isolate);
+        if (isolate())
+            m_inspectorTaskRunner->interruptAndRunAllTasksDontWait(isolate());
     }
-    backingThread().postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerThread::runDebuggerTaskDontWait, AllowCrossThreadAccess(this)));
+    workerBackingThread().backingThread().postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerThread::runDebuggerTaskDontWait, AllowCrossThreadAccess(this)));
 }
 
 void WorkerThread::runDebuggerTask(PassOwnPtr<CrossThreadClosure> task)
@@ -468,7 +420,7 @@ void WorkerThread::runDebuggerTask(PassOwnPtr<CrossThreadClosure> task)
         m_runningDebuggerTask = false;
         if (m_shouldTerminateV8Execution) {
             m_shouldTerminateV8Execution = false;
-            terminateV8Execution();
+            isolate()->TerminateExecution();
         }
     }
 }