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..bbeef18005882e5b10e3bd522c8f00eca9687102 100644
--- a/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp
+++ b/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp
@@ -18,10 +18,12 @@ namespace blink {
 
 namespace {
 
+// Tracks whether we have instantiated a BackingThreadHolder.
+static bool s_hasBackingThreadHolder = false;
+
 // 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.
+// renderrer process. BackingThreadHolst::m_thread is cleared by

[haraken, 2016/05/09 02:00:53]

renderer

[flackr, 2016/05/10 18:07:39]

Done.

+// ModulesInitializer::shutdown.
 // See WorkerThread::terminateAndWaitForAllWorkers for the process shutdown
 // case.
 class BackingThreadHolder {
@@ -29,10 +31,18 @@ public:
     static BackingThreadHolder& instance()
     {
         DEFINE_THREAD_SAFE_STATIC_LOCAL(BackingThreadHolder, holder, new BackingThreadHolder);
+        s_hasBackingThreadHolder = true;
         return holder;
     }
 
     WorkerBackingThread* thread() { return m_thread.get(); }
+    static void clear()
+    {
+        // Calling instance() if it has not already been called would create
+        // a backing thread so check if we have an instance first.
+        if (s_hasBackingThreadHolder)

[haraken, 2016/05/09 02:00:53]

Just to confirm: The access to s_hasBackingThreadHolder doesn't cause threading
race because it's guaranteed that the BackingThreadHolder is instantiated before
any compositor thread starts running and the BackingThreadHolder is destructed
after all compositor threads finished, right?

If my understanding is correct, it would be better to make
s_hasBackingThreadHolder a member variable of BackingThreadHolder. Then the call
site can just call instance().clear().

If my understanding is wrong, you need to address the threading race.

[flackr, 2016/05/10 18:07:39]

Right.
The problem is that calling instance() creates the BackingThreadHolder (and
WorkerBackingThread) if we haven't yet. This means that during shutdown we would
unnecessarily create the WorkerBackingThread just to clear it. This is why
yhirano removed the call to clearSharedBackingThread() in
https://codereview.chromium.org/1897143002. The problem is that we can't call
ThreadState::detach until we are no longer referencing oilpan objects from the
compositor thread (CompositorMutatorImpl).

+            instance().m_thread = nullptr;
+    }
     void resetForTest()
     {
         ASSERT(!m_thread || (m_thread->workerScriptCount() == 0));
@@ -77,6 +87,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();