Oilpan: Prepare moving InspectorController and InspectorAgents to oilpan.

Source/core/workers/WorkerThread.cpp

Index: Source/core/workers/WorkerThread.cpp
diff --git a/Source/core/workers/WorkerThread.cpp b/Source/core/workers/WorkerThread.cpp
index b04391a18c04666cd2b1d94e7c2d2366d1203f90..5e5781c72e2eb783385b08c16e6cc3879eb809f0 100644
--- a/Source/core/workers/WorkerThread.cpp
+++ b/Source/core/workers/WorkerThread.cpp
@@ -30,6 +30,8 @@
 
 #include "bindings/v8/ScriptSourceCode.h"
 #include "core/inspector/InspectorInstrumentation.h"
+#include "core/inspector/WorkerDebuggerAgent.h"
+#include "core/inspector/WorkerInspectorController.h"
 #include "core/workers/DedicatedWorkerGlobalScope.h"
 #include "core/workers/WorkerClients.h"
 #include "core/workers/WorkerReportingProxy.h"
@@ -102,12 +104,20 @@ void WorkerThread::workerThreadStart(void* thread)
     static_cast<WorkerThread*>(thread)->workerThread();
 }
 
+void WorkerThread::interruptAndDispatchInspectorCommands()
+{
+    MutexLocker locker(m_threadShutdownMutex);
+    if (!m_workerGlobalScope)
+        return;
+    if (WorkerInspectorController* inspectorController = m_workerGlobalScope->workerInspectorController())

[yurys, 2014/06/26 11:04:30]

WorkerGlobalScope::clearInspector may be executing on the worker thread in
parallel and you may end up with non-null inspectorController instance here
which will be destroyed immediately after that on the worker thread resulting in
use after free.

Having said that, I don't think this may happen with dedicated workers as
clearInspector is called in the following two places:
1) From WorkerGlobalScope::dispose, which is called with the mutex locked
2) From WorkerThreadShutdownFinishTask::performTask where you don't lock the
mutex but the task is always a result of WorkerThread::stop, which in turn is
only invoked from WorkerMessagingProxy when m_askedToTerminate is true. If
m_askedToTerminate is true we will bail out from
WorkerMessagingProxy::sendMessageToInspector before calling
interruptAndDispatchInspectorCommands. The latter is not true though when
interruptAndDispatchInspectorCommands is invoked from
WebEmbeddedWorkerImpl::dispatchDevToolsMessage without checking
m_askedToTerminate. 

All of that seems very fragile so I believe we need to guard
WorkerInspectorController destruction inside WorkerGlobalObject::clearInspector
with the same mutex as we do when disposing m_workerGlobalScope.


+horo who may have additional input regarding WebEmbeddedWorkerImpl

[horo, 2014/06/27 06:36:05]

I agree with yurys.

But it may be good to add "if (m_askedToTerminate) return;" in
WebEmbeddedWorkerImpl::dispatchDevToolsMessage and
WebSharedWorkerImpl::dispatchDevToolsMessage.

+        inspectorController->interruptAndDispatchInspectorCommands();
+}
+
 void WorkerThread::workerThread()
 {
     KURL scriptURL = m_startupData->m_scriptURL;
     String sourceCode = m_startupData->m_sourceCode;
     WorkerThreadStartMode startMode = m_startupData->m_startMode;
-
     {
         MutexLocker lock(m_threadCreationMutex);
         ThreadState::attach();
@@ -140,14 +150,17 @@ void WorkerThread::workerThread()
 
     ThreadIdentifier threadID = m_threadID;
 
-    // The below assignment will destroy the context, which will in turn notify messaging proxy.
-    // We cannot let any objects survive past thread exit, because no other thread will run GC or otherwise destroy them.
-    // If Oilpan is enabled, we detach of the context/global scope, with the final heap cleanup below sweeping it out.
+    {
+        MutexLocker locker(m_threadShutdownMutex);
+        // The below assignment will destroy the context, which will in turn notify messaging proxy.
+        // We cannot let any objects survive past thread exit, because no other thread will run GC or otherwise destroy them.
+        // If Oilpan is enabled, we detach of the context/global scope, with the final heap cleanup below sweeping it out.
 #if !ENABLE(OILPAN)
-    ASSERT(m_workerGlobalScope->hasOneRef());
+        ASSERT(m_workerGlobalScope->hasOneRef());
 #endif
-    m_workerGlobalScope->dispose();
-    m_workerGlobalScope = nullptr;
+        m_workerGlobalScope->dispose();
+        m_workerGlobalScope = nullptr;
+    }
 
     // Detach the ThreadState, cleaning out the thread's heap by
     // performing a final GC. The cleanup operation will at the end