Bug 957693: Fix a worker hang and other related bugs. r=bent
authorKyle Huey <khuey@kylehuey.com>
Fri, 10 Jan 2014 16:37:47 -0800
changeset 179029 5cae5d9ef313aded50e3ed3023a07afdd73f6cd0
parent 179028 3f8990e34520df619b8219353b05e825f8dcc1a1
child 179030 ab1d9c6b581a8d7d4410075d66772adfee089078
push id3343
push userffxbld
push dateMon, 17 Mar 2014 21:55:32 +0000
treeherdermozilla-beta@2f7d3415f79f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbent
bugs957693
milestone29.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 957693: Fix a worker hang and other related bugs. r=bent
dom/workers/ScriptLoader.cpp
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerPrivate.h
dom/workers/XMLHttpRequest.cpp
--- a/dom/workers/ScriptLoader.cpp
+++ b/dom/workers/ScriptLoader.cpp
@@ -172,16 +172,23 @@ private:
   { }
 
   virtual bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) MOZ_OVERRIDE;
 
   virtual void
   PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate, bool aRunResult)
           MOZ_OVERRIDE;
+
+  NS_DECL_NSICANCELABLERUNNABLE
+
+  void
+  ShutdownScriptLoader(JSContext* aCx,
+                       WorkerPrivate* aWorkerPrivate,
+                       bool aResult);
 };
 
 class ScriptLoaderRunnable MOZ_FINAL : public WorkerFeature,
                                        public nsIRunnable,
                                        public nsIStreamLoaderObserver
 {
   friend class ScriptExecutorRunnable;
 
@@ -735,21 +742,36 @@ ScriptExecutorRunnable::PostRun(JSContex
     bool result = true;
     for (uint32_t index = 0; index < loadInfos.Length(); index++) {
       if (!loadInfos[index].mExecutionResult) {
         result = false;
         break;
       }
     }
 
-    aWorkerPrivate->RemoveFeature(aCx, &mScriptLoader);
-    aWorkerPrivate->StopSyncLoop(mSyncLoopTarget, result);
+    ShutdownScriptLoader(aCx, aWorkerPrivate, result);
   }
 }
 
+NS_IMETHODIMP
+ScriptExecutorRunnable::Cancel()
+{
+  ShutdownScriptLoader(mWorkerPrivate->GetJSContext(), mWorkerPrivate, false);
+  return NS_OK;
+}
+
+void
+ScriptExecutorRunnable::ShutdownScriptLoader(JSContext* aCx,
+                                             WorkerPrivate* aWorkerPrivate,
+                                             bool aResult)
+{
+  aWorkerPrivate->RemoveFeature(aCx, &mScriptLoader);
+  aWorkerPrivate->StopSyncLoop(mSyncLoopTarget, aResult);
+}
+
 bool
 LoadAllScripts(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
                nsTArray<ScriptLoadInfo>& aLoadInfos, bool aIsWorkerScript)
 {
   aWorkerPrivate->AssertIsOnWorkerThread();
   NS_ASSERTION(!aLoadInfos.IsEmpty(), "Bad arguments!");
 
   AutoSyncLoopHolder syncLoop(aWorkerPrivate);
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -1062,18 +1062,18 @@ class NotifyRunnable MOZ_FINAL : public 
 {
   Status mStatus;
 
 public:
   NotifyRunnable(WorkerPrivate* aWorkerPrivate, Status aStatus)
   : WorkerControlRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount),
     mStatus(aStatus)
   {
-    MOZ_ASSERT(aStatus == Terminating || aStatus == Canceling ||
-               aStatus == Killing);
+    MOZ_ASSERT(aStatus == Closing || aStatus == Terminating ||
+               aStatus == Canceling || aStatus == Killing);
   }
 
 private:
   virtual bool
   PreDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate) MOZ_OVERRIDE
   {
     // Modify here, but not in PostRun! This busy count addition will be matched
     // by the CloseEventRunnable.
@@ -4868,39 +4868,54 @@ WorkerPrivate::RunCurrentSyncLoop()
 
     MOZ_ALWAYS_TRUE(NS_ProcessNextEvent(thread, false));
 
     // Now *might* be a good time to GC. Let the JS engine make the decision.
     JS_MaybeGC(cx);
   }
 
   // Make sure that the stack didn't change underneath us.
+  MOZ_ASSERT(mSyncLoopStack[currentLoopIndex] == loopInfo);
+
+  return DestroySyncLoop(currentLoopIndex);
+}
+
+bool
+WorkerPrivate::DestroySyncLoop(uint32_t aLoopIndex, nsIThreadInternal* aThread)
+{
   MOZ_ASSERT(!mSyncLoopStack.IsEmpty());
-  MOZ_ASSERT(mSyncLoopStack.Length() - 1 == currentLoopIndex);
-  MOZ_ASSERT(mSyncLoopStack[currentLoopIndex] == loopInfo);
-
-  // We're about to delete |loop|, stash its event target and result.
+  MOZ_ASSERT(mSyncLoopStack.Length() - 1 == aLoopIndex);
+
+  if (!aThread) {
+    nsCOMPtr<nsIThreadInternal> thread = do_QueryInterface(mThread);
+    MOZ_ASSERT(thread);
+
+    aThread = thread.get();
+  }
+
+  // We're about to delete the loop, stash its event target and result.
+  SyncLoopInfo* loopInfo = mSyncLoopStack[aLoopIndex];
   nsIEventTarget* nestedEventTarget =
     loopInfo->mEventTarget->GetWeakNestedEventTarget();
   MOZ_ASSERT(nestedEventTarget);
 
   bool result = loopInfo->mResult;
 
   {
     // Modifications must be protected by mMutex in DEBUG builds, see comment
     // about mSyncLoopStack in WorkerPrivate.h.
 #ifdef DEBUG
     MutexAutoLock lock(mMutex);
 #endif
 
-    // This will delete |loop|!
-    mSyncLoopStack.RemoveElementAt(currentLoopIndex);
-  }
-
-  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(thread->PopEventQueue(nestedEventTarget)));
+    // This will delete |loopInfo|!
+    mSyncLoopStack.RemoveElementAt(aLoopIndex);
+  }
+
+  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(aThread->PopEventQueue(nestedEventTarget)));
 
   return result;
 }
 
 void
 WorkerPrivate::StopSyncLoop(nsIEventTarget* aSyncLoopTarget, bool aResult)
 {
   AssertIsOnWorkerThread();
--- a/dom/workers/WorkerPrivate.h
+++ b/dom/workers/WorkerPrivate.h
@@ -29,16 +29,17 @@
 
 class JSAutoStructuredCloneBuffer;
 class nsIChannel;
 class nsIDocument;
 class nsIEventTarget;
 class nsIPrincipal;
 class nsIScriptContext;
 class nsIThread;
+class nsIThreadInternal;
 class nsITimer;
 class nsIURI;
 
 namespace JS {
 class RuntimeStats;
 }
 
 namespace mozilla {
@@ -1093,16 +1094,19 @@ private:
   }
 
   already_AddRefed<nsIEventTarget>
   CreateNewSyncLoop();
 
   bool
   RunCurrentSyncLoop();
 
+  bool
+  DestroySyncLoop(uint32_t aLoopIndex, nsIThreadInternal* aThread = nullptr);
+
   void
   InitializeGCTimers();
 
   void
   SetGCTimerMode(GCTimerMode aMode);
 
   void
   ShutdownGCTimers();
@@ -1152,29 +1156,33 @@ WorkerStructuredCloneCallbacks(bool aMai
 
 JSStructuredCloneCallbacks*
 ChromeWorkerStructuredCloneCallbacks(bool aMainRuntime);
 
 class AutoSyncLoopHolder
 {
   WorkerPrivate* mWorkerPrivate;
   nsCOMPtr<nsIEventTarget> mTarget;
+  uint32_t mIndex;
 
 public:
   AutoSyncLoopHolder(WorkerPrivate* aWorkerPrivate)
-  : mWorkerPrivate(aWorkerPrivate), mTarget(aWorkerPrivate->CreateNewSyncLoop())
+  : mWorkerPrivate(aWorkerPrivate)
+  , mTarget(aWorkerPrivate->CreateNewSyncLoop())
+  , mIndex(aWorkerPrivate->mSyncLoopStack.Length() - 1)
   {
     aWorkerPrivate->AssertIsOnWorkerThread();
   }
 
   ~AutoSyncLoopHolder()
   {
     if (mWorkerPrivate) {
       mWorkerPrivate->AssertIsOnWorkerThread();
       mWorkerPrivate->StopSyncLoop(mTarget, false);
+      mWorkerPrivate->DestroySyncLoop(mIndex);
     }
   }
 
   bool
   Run()
   {
     WorkerPrivate* workerPrivate = mWorkerPrivate;
     mWorkerPrivate = nullptr;
--- a/dom/workers/XMLHttpRequest.cpp
+++ b/dom/workers/XMLHttpRequest.cpp
@@ -503,18 +503,18 @@ private:
     ~ResponseRunnable()
     { }
 
     virtual void
     MaybeSetException(JSContext* aCx) MOZ_OVERRIDE
     {
       MOZ_ASSERT(NS_FAILED(mErrorCode));
 
-        Throw(aCx, mErrorCode);
-      }
+      Throw(aCx, mErrorCode);
+    }
   };
 
 public:
   WorkerThreadProxySyncRunnable(WorkerPrivate* aWorkerPrivate, Proxy* aProxy)
   : mWorkerPrivate(aWorkerPrivate), mProxy(aProxy)
   {
     MOZ_ASSERT(aWorkerPrivate);
     MOZ_ASSERT(aProxy);
@@ -1802,24 +1802,16 @@ XMLHttpRequest::SendInternal(const nsASt
   }
 
   if (!isSyncXHR)  {
     autoUnpin.Clear();
     MOZ_ASSERT(autoSyncLoop.empty());
     return;
   }
 
-  // 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;
-  }
-
   autoUnpin.Clear();
 
   if (!autoSyncLoop.ref().Run()) {
     aRv.Throw(NS_ERROR_FAILURE);
   }
 }
 
 bool