[webkit-changes] [125112] branches/safari-536.26-branch/Source/WebCore

Wed, 08 Aug 2012 16:41:59 -0700

Title: [125112] branches/safari-536.26-branch/Source/WebCore

Diff

Modified: branches/safari-536.26-branch/Source/WebCore/ChangeLog (125111 => 125112)


--- branches/safari-536.26-branch/Source/WebCore/ChangeLog	2012-08-08 23:39:50 UTC (rev 125111)
+++ branches/safari-536.26-branch/Source/WebCore/ChangeLog	2012-08-08 23:42:15 UTC (rev 125112)
@@ -1,5 +1,39 @@
 2012-08-07  Lucas Forschler  <[email protected]>
 
+    Merge 120328
+
+    2012-06-13  Mark Hahnenberg  <[email protected]>
+
+            Worker tear-down can re-enter JSC during GC finalization pt. 2
+            https://bugs.webkit.org/show_bug.cgi?id=88601
+
+            Reviewed by David Levin.
+
+            No new tests. Current regression tests are sufficient.
+
+            * workers/WorkerMessagingProxy.cpp:
+            (WebCore::WorkerMessagingProxy::WorkerMessagingProxy):
+            (WebCore::WorkerMessagingProxy::workerObjectDestroyed): We clear the m_workerObject here because 
+            we don't want anybody else trying to send messages to the Worker now that it has been destroyed.
+            We also queue the asynchronous task for the various other cleanup that still needs to be done. 
+            This allows us to avoid the problem of re-entrant JS code execution during GC.
+            (WebCore):
+            (WebCore::WorkerMessagingProxy::workerObjectDestroyedInternal): Here we set m_mayBeDestroyed to true.
+            This is the point after which deleting the WorkerMessagingProxy in workerContextDestroyedInternal()
+            is okay. It could happen during this function call if the worker thread has been shutdown already, or 
+            it could be called later after we shut down the worker thread.
+            (WebCore::WorkerMessagingProxy::workerContextDestroyedInternal): We check m_mayBeDestroyed here 
+            instead of checking m_workerObject. This change effectively orthogonalizes the roles that m_workerObject 
+            was filling. Since we were eagerly clearing m_workerObject, but we wanted to asynchronously call 
+            workerObjectDestroyed(), we needed to make sure we didn't accidentally try to delete the WorkerMessagingProxy
+            twice (once from destroying the Worker and once from destroying the WorkerContext). This boolean field 
+            should fix that issue--we set it lazily like we wanted to do without being in danger of causing use-after-free
+            issues with m_workerObject.
+            * workers/WorkerMessagingProxy.h: Added the new field and function.
+            (WorkerMessagingProxy):
+
+2012-08-07  Lucas Forschler  <[email protected]>
+
     Merge 119740
 
     2012-06-07  Mark Hahnenberg  <[email protected]>

Modified: branches/safari-536.26-branch/Source/WebCore/workers/WorkerMessagingProxy.cpp (125111 => 125112)


--- branches/safari-536.26-branch/Source/WebCore/workers/WorkerMessagingProxy.cpp	2012-08-08 23:39:50 UTC (rev 125111)
+++ branches/safari-536.26-branch/Source/WebCore/workers/WorkerMessagingProxy.cpp	2012-08-08 23:42:15 UTC (rev 125112)
@@ -250,6 +250,7 @@
 WorkerMessagingProxy::WorkerMessagingProxy(Worker* workerObject)
     : m_scriptExecutionContext(workerObject->scriptExecutionContext())
     , m_workerObject(workerObject)
+    , m_mayBeDestroyed(false)
     , m_unconfirmedMessageCount(0)
     , m_workerThreadHadPendingActivity(false)
     , m_askedToTerminate(false)
@@ -354,10 +355,16 @@
 void WorkerMessagingProxy::workerObjectDestroyed()
 {
     m_workerObject = 0;
-    if (m_workerThread)
-        terminateWorkerContext();
+    m_scriptExecutionContext->postTask(createCallbackTask(&workerObjectDestroyedInternal, AllowCrossThreadAccess(this)));
+}
+
+void WorkerMessagingProxy::workerObjectDestroyedInternal(ScriptExecutionContext*, WorkerMessagingProxy* proxy)
+{
+    proxy->m_mayBeDestroyed = true;
+    if (proxy->m_workerThread)
+        proxy->terminateWorkerContext();
     else
-        workerContextDestroyedInternal();
+        proxy->workerContextDestroyedInternal();
 }
 
 #if ENABLE(INSPECTOR)
@@ -438,7 +445,7 @@
 
     InspectorInstrumentation::workerContextTerminated(m_scriptExecutionContext.get(), this);
 
-    if (!m_workerObject)
+    if (m_mayBeDestroyed)
         delete this;
 }

Modified: branches/safari-536.26-branch/Source/WebCore/workers/WorkerMessagingProxy.h (125111 => 125112)


--- branches/safari-536.26-branch/Source/WebCore/workers/WorkerMessagingProxy.h	2012-08-08 23:39:50 UTC (rev 125111)
+++ branches/safari-536.26-branch/Source/WebCore/workers/WorkerMessagingProxy.h	2012-08-08 23:42:15 UTC (rev 125112)
@@ -99,11 +99,13 @@
         virtual ~WorkerMessagingProxy();
 
         void workerContextDestroyedInternal();
+        static void workerObjectDestroyedInternal(ScriptExecutionContext*, WorkerMessagingProxy*);
         void reportPendingActivityInternal(bool confirmingMessage, bool hasPendingActivity);
         Worker* workerObject() const { return m_workerObject; }
 
         RefPtr<ScriptExecutionContext> m_scriptExecutionContext;
         Worker* m_workerObject;
+        bool m_mayBeDestroyed;
         RefPtr<DedicatedWorkerThread> m_workerThread;
 
         unsigned m_unconfirmedMessageCount; // Unconfirmed messages from worker object to worker thread.

_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to