compositor-worker: Keep worker backing thread alive for the lifetime of the compositor thread.

third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp

Index: third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp
diff --git a/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp b/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp
index 78f1ed6beb27b0a324c26fab5deda377eb353b22..57dd7259f4be6f8256bbcd8ebbf351cd5c95785d 100644
--- a/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp
+++ b/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp
@@ -12,36 +12,70 @@
 #include "modules/compositorworker/CompositorWorkerGlobalScope.h"
 #include "platform/ThreadSafeFunctional.h"
 #include "platform/TraceEvent.h"
+#include "platform/WaitableEvent.h"
 #include "public/platform/Platform.h"
 
 namespace blink {
 
 namespace {
 
+class BackingThreadHolder;
+static BackingThreadHolder* s_holderInstance = nullptr;
+
 // This is a singleton class holding the compositor worker thread in this
-// renderrer process. BackingThreadHolst::m_thread will never be cleared,
-// but Oilpan and V8 are detached from the thread when the last compositor
-// worker thread is gone.
+// renderer process. BackingThreadHolder::m_thread is cleared by
+// ModulesInitializer::shutdown.
 // See WorkerThread::terminateAndWaitForAllWorkers for the process shutdown
 // case.
 class BackingThreadHolder {
 public:
     static BackingThreadHolder& instance()
     {
-        DEFINE_THREAD_SAFE_STATIC_LOCAL(BackingThreadHolder, holder, new BackingThreadHolder);
-        return holder;
+        if (!s_holderInstance)

[yhirano, 2016/05/19 05:35:37]

You need a mutex.

[flackr, 2016/05/19 22:24:16]

Done. But are you sure? It seems to me like instance() and clear() are always
accessed from the main thread.

[haraken, 2016/05/20 00:24:44]

Yeah, as I commented on
https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m...,
I don't think the mutex is needed.

[yhirano, 2016/05/20 02:24:54]

instance() can be called from worker threads via
WorkerThread::workerBackingThread().
I'm a bit nervous because the native thread exists before the first call of
ensureSharedBackingThread.

With enough comments and assertions it might be OK to remove mutex.

[haraken, 2016/05/20 02:35:24]

If I'm understanding correctly, it *should not* happen because the main thread
must call ensureBackingThread before any compositor worker thread starts
running.

[flackr, 2016/05/20 15:51:18]

Correct, that is how it is supposed to work.

I see your point on the Mutex, it shouldn't ever happen that there is contention
calling instance() since we ensure the instance exists before the first worker
is created and the workers should be shut down before we shut down the instance
but can't hurt to be safe.

+            s_holderInstance = new BackingThreadHolder;
+        return *s_holderInstance;
+    }
+
+    static void clear()
+    {
+        if (s_holderInstance) {
+            s_holderInstance->shutdownAndWait();
+            delete s_holderInstance;
+            s_holderInstance = nullptr;
+        }
     }
 
     WorkerBackingThread* thread() { return m_thread.get(); }
+
     void resetForTest()
     {
         ASSERT(!m_thread || (m_thread->workerScriptCount() == 0));
-        m_thread = nullptr;
         m_thread = WorkerBackingThread::createForTest(Platform::current()->compositorThread());
     }
 
 private:
-    BackingThreadHolder() : m_thread(WorkerBackingThread::create(Platform::current()->compositorThread())) {}
+    BackingThreadHolder() : m_thread(WorkerBackingThread::create(Platform::current()->compositorThread()))
+    {
+        Platform::current()->compositorThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, threadSafeBind(&BackingThreadHolder::initializeOnThread, AllowCrossThreadAccess(this)));
+    }
+
+    void initializeOnThread()

[yhirano, 2016/05/19 05:35:37]

DCHECK(0, m_thread->workerScriptCount());

[flackr, 2016/05/19 22:24:16]

Done.

+    {
+        m_thread->attach();
+    }
+
+    void shutdownAndWait()
+    {
+        WaitableEvent doneEvent;
+        Platform::current()->compositorThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, threadSafeBind(&BackingThreadHolder::shutdownOnThread, AllowCrossThreadAccess(&instance()), AllowCrossThreadAccess(&doneEvent)));
+        doneEvent.wait();
+    }
+
+    void shutdownOnThread(WaitableEvent* doneEvent)

[yhirano, 2016/05/19 05:35:37]

DCHECK(1, m_thread->workerScriptCount());

[flackr, 2016/05/19 22:24:16]

Done.

+    {
+        m_thread->detach();
+        doneEvent->signal();
+    }
 
     OwnPtr<WorkerBackingThread> m_thread;
 };
@@ -77,6 +111,16 @@ WorkerGlobalScope*CompositorWorkerThread::createWorkerGlobalScope(PassOwnPtr<Wor
     return CompositorWorkerGlobalScope::create(this, std::move(startupData), m_timeOrigin);
 }
 
+void CompositorWorkerThread::ensureSharedBackingThread()
+{
+    BackingThreadHolder::instance();
+}
+
+void CompositorWorkerThread::clearSharedBackingThread()
+{
+    BackingThreadHolder::clear();
+}
+
 void CompositorWorkerThread::resetSharedBackingThreadForTest()
 {
     BackingThreadHolder::instance().resetForTest();