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..41078dfbed15f785983d4798909201bb4d365742 100644
--- a/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp
+++ b/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp
@@ -12,36 +12,80 @@
 #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;

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

Can we make this a static member variable of BackingThreadHolder?

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

Done.

+
+static Mutex& holderInstanceMutex()
+{
+    DEFINE_THREAD_SAFE_STATIC_LOCAL(Mutex, holderMutex, new Mutex);
+    return holderMutex;
+}
+
 // 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;
+        MutexLocker locker(holderInstanceMutex());
+        if (!s_holderInstance)

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

Can you create another function for ensureBackingThread and move this if-branch
to that function?

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

Done.

+            s_holderInstance = new BackingThreadHolder;
+        return *s_holderInstance;
+    }
+
+    static void clear()
+    {
+        MutexLocker locker(holderInstanceMutex());
+        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)));

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

Add DCHECK(isMainThread()) (for documentation purpose).

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

Done.

+    }
+
+    void initializeOnThread()
+    {
+        DCHECK_EQ(0u, m_thread->workerScriptCount()) << "BackingThreadHolder should be the first to attach to WorkerBackingThread";

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

Once we land this CL, it should be guaranteed that BackingThreadHolder is the
first/last thread that gets attached/detached to/from the WorkerBackingThread.
Then maybe we no longer need to reference-count the WorkerBackingThread? i.e.,
once we land this CL, can we remove the concept of workerScriptCount() entirely?

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

It's now used to see if we need call TerminateExecution. Graceful shutdown will
solve that problem.

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

But this CL makes the TerminateExecution useless, doesn't it? Given that the
compositor thread keeps one reference count to the backing thread until
blink::shutdown, it won't happen that workerScriptCount() returns 1 when
processing the TerminateExecution (workerScriptCount() returns a number larger
than 2).

[yhirano, 2016/05/20 03:10:15]

It depends on whether we want to keep the "current" behavior. I thought we
wanted to (see the first comment in
https://codereview.chromium.org/1955693003/#msg18). If we don't have to, it's
great in terms of code simplicity.

+        m_thread->attach();
+    }
+
+    void shutdownAndWait()
+    {
+        WaitableEvent doneEvent;

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

Add DCHECK(isMainThread()).

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

Done.

+        Platform::current()->compositorThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, threadSafeBind(&BackingThreadHolder::shutdownOnThread, AllowCrossThreadAccess(this), AllowCrossThreadAccess(&doneEvent)));
+        doneEvent.wait();
+    }
+
+    void shutdownOnThread(WaitableEvent* doneEvent)
+    {
+        DCHECK_EQ(1u, m_thread->workerScriptCount()) << "BackingThreadHolder should be the last to detach from WorkerBackingThread";
+        m_thread->detach();
+        doneEvent->signal();
+    }
 
     OwnPtr<WorkerBackingThread> m_thread;
 };
@@ -77,6 +121,16 @@ WorkerGlobalScope*CompositorWorkerThread::createWorkerGlobalScope(PassOwnPtr<Wor
     return CompositorWorkerGlobalScope::create(this, std::move(startupData), m_timeOrigin);
 }
 
+void CompositorWorkerThread::ensureSharedBackingThread()
+{
+    BackingThreadHolder::instance();

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

Add DCHECK(isMainThread()).

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

Done.

+}
+
+void CompositorWorkerThread::clearSharedBackingThread()
+{
+    BackingThreadHolder::clear();

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

Add DCHECK(isMainThread()).

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

Done.

+}
+
 void CompositorWorkerThread::resetSharedBackingThreadForTest()
 {
     BackingThreadHolder::instance().resetForTest();