Bug 823953: Improve sync queue handling. r=bent

--HG--
extra : rebase_source : 04c9eb2a7462d2c94c66b8126507fa690df0f13d

dom/workers/RuntimeService.cpp

@@ -306,14 +306,15 @@ public:
   bool
   Dispatch(JSContext* aCx)
   {
-    mSyncQueueKey = mWorkerPrivate->CreateNewSyncLoop();
+    AutoSyncLoopHolder syncLoop(mWorkerPrivate);
+    mSyncQueueKey = syncLoop.SyncQueueKey();
 
     if (NS_FAILED(NS_DispatchToMainThread(this, NS_DISPATCH_NORMAL))) {
       JS_ReportError(aCx, "Failed to dispatch to main thread!");
       return false;
     }
 
-    return mWorkerPrivate->RunSyncLoop(aCx, mSyncQueueKey);
+    return syncLoop.RunAndForget(aCx);
   }
 
   NS_IMETHOD

dom/workers/ScriptLoader.cpp

@@ -666,11 +666,11 @@ LoadAllScripts(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
   aWorkerPrivate->AssertIsOnWorkerThread();
   NS_ASSERTION(!aLoadInfos.IsEmpty(), "Bad arguments!");
 
-  uint32_t syncQueueKey = aWorkerPrivate->CreateNewSyncLoop();
+  AutoSyncLoopHolder syncLoop(aWorkerPrivate);
 
   nsRefPtr<ScriptLoaderRunnable> loader =
-    new ScriptLoaderRunnable(aWorkerPrivate, syncQueueKey, aLoadInfos,
+    new ScriptLoaderRunnable(aWorkerPrivate, syncLoop.SyncQueueKey(),
-                             aIsWorkerScript);
+                             aLoadInfos, aIsWorkerScript);
 
   NS_ASSERTION(aLoadInfos.IsEmpty(), "Should have swapped!");
 
@@ -685,7 +685,7 @@ LoadAllScripts(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
     return false;
   }
 
-  return aWorkerPrivate->RunSyncLoop(aCx, syncQueueKey);
+  return syncLoop.RunAndForget(aCx);
 }
 
 } /* anonymous namespace */

dom/workers/WorkerPrivate.cpp

@@ -3390,7 +3390,7 @@ WorkerPrivate::RunSyncLoop(JSContext* aCx, uint32_t aSyncLoopKey)
       NS_ASSERTION(syncQueue->mQueue.IsEmpty(), "Unprocessed sync events!");
 
       bool result = syncQueue->mResult;
-      mSyncQueues.RemoveElementAt(aSyncLoopKey);
+      DestroySyncLoop(aSyncLoopKey);
 
 #ifdef DEBUG
       syncQueue = nullptr;
@@ -3424,6 +3424,14 @@ WorkerPrivate::StopSyncLoop(uint32_t aSyncLoopKey, bool aSyncResult)
   syncQueue->mComplete = true;
 }
 
+void
+WorkerPrivate::DestroySyncLoop(uint32_t aSyncLoopKey)
+{
+  AssertIsOnWorkerThread();
+
+  mSyncQueues.RemoveElementAt(aSyncLoopKey);
+}
+
 bool
 WorkerPrivate::PostMessageToParent(JSContext* aCx, jsval aMessage,
                                    jsval aTransferable)

dom/workers/WorkerPrivate.h

@@ -661,6 +661,9 @@ public:
   void
   StopSyncLoop(uint32_t aSyncLoopKey, bool aSyncResult);
 
+  void
+  DestroySyncLoop(uint32_t aSyncLoopKey);
+
   bool
   PostMessageToParent(JSContext* aCx, jsval aMessage,
                       jsval transferable);
@@ -848,6 +851,42 @@ WorkerStructuredCloneCallbacks(bool aMainRuntime);
 JSStructuredCloneCallbacks*
 ChromeWorkerStructuredCloneCallbacks(bool aMainRuntime);
 
+class AutoSyncLoopHolder
+{
+public:
+  AutoSyncLoopHolder(WorkerPrivate* aWorkerPrivate)
+  : mWorkerPrivate(aWorkerPrivate), mSyncLoopKey(UINT32_MAX)
+  {
+    mSyncLoopKey = mWorkerPrivate->CreateNewSyncLoop();
+  }
+
+  ~AutoSyncLoopHolder()
+  {
+    if (mWorkerPrivate) {
+      mWorkerPrivate->StopSyncLoop(mSyncLoopKey, false);
+      mWorkerPrivate->DestroySyncLoop(mSyncLoopKey);
+    }
+  }
+
+  bool
+  RunAndForget(JSContext* aCx)
+  {
+    WorkerPrivate* workerPrivate = mWorkerPrivate;
+    mWorkerPrivate = nullptr;
+    return workerPrivate->RunSyncLoop(aCx, mSyncLoopKey);
+  }
+
+  uint32_t
+  SyncQueueKey() const
+  {
+    return mSyncLoopKey;
+  }
+
+private:
+  WorkerPrivate* mWorkerPrivate;
+  uint32_t mSyncLoopKey;
+};
+
 END_WORKERS_NAMESPACE
 
 #endif /* mozilla_dom_workers_workerprivate_h__ */

dom/workers/XMLHttpRequest.cpp

@@ -830,18 +830,15 @@ public:
   {
     mWorkerPrivate->AssertIsOnWorkerThread();
 
-    mSyncQueueKey = mWorkerPrivate->CreateNewSyncLoop();
+    AutoSyncLoopHolder syncLoop(mWorkerPrivate);
+    mSyncQueueKey = syncLoop.SyncQueueKey();
 
     if (NS_FAILED(NS_DispatchToMainThread(this, NS_DISPATCH_NORMAL))) {
       JS_ReportError(aCx, "Failed to dispatch to main thread!");
       return false;
     }
 
-    if (!mWorkerPrivate->RunSyncLoop(aCx, mSyncQueueKey)) {
+    return syncLoop.RunAndForget(aCx);
-      return false;
-    }
-
-    return true;
   }
 
   virtual nsresult
@@ -1707,10 +1704,13 @@ XMLHttpRequest::SendInternal(const nsAString& aStringBody,
   }
 
   AutoUnpinXHR autoUnpin(this);
+  Maybe<AutoSyncLoopHolder> autoSyncLoop;
 
   uint32_t syncQueueKey = UINT32_MAX;
-  if (mProxy->mIsSyncXHR) {
+  bool isSyncXHR = mProxy->mIsSyncXHR;
-    syncQueueKey = mWorkerPrivate->CreateNewSyncLoop();
+  if (isSyncXHR) {
+    autoSyncLoop.construct(mWorkerPrivate);
+    syncQueueKey = autoSyncLoop.ref().SyncQueueKey();
   }
 
   mProxy->mOuterChannelId++;
@@ -1724,16 +1724,24 @@ XMLHttpRequest::SendInternal(const nsAString& aStringBody,
     return;
   }
 
-  autoUnpin.Clear();
+  if (!isSyncXHR)  {
+    autoUnpin.Clear();
+    MOZ_ASSERT(autoSyncLoop.empty());
+    return;
+  }
 
-  // The event loop was spun above, make sure we aren't canceled already.
+  // If our sync XHR was canceled during the send call the worker is going
+  // away.  We have no idea how far through the send call we got.  There may
+  // be a ProxyCompleteRunnable in the sync loop, but rather than run the loop
+  // to get it we just let our RAII helpers clean up.
   if (mCanceled) {
     return;
   }
 
-  if (mProxy->mIsSyncXHR && !mWorkerPrivate->RunSyncLoop(cx, syncQueueKey)) {
+  autoUnpin.Clear();
+
+  if (!autoSyncLoop.ref().RunAndForget(cx)) {
     aRv.Throw(NS_ERROR_FAILURE);
-    return;
   }
 }