Bug 604026 - Sync event loops in workers should be created only if compatible with the worker shutdown status

dom/base/File.cpp

@@ -912,7 +912,7 @@ BlobImplFile::GetType(nsAString& aType)
         new GetTypeRunnable(workerPrivate, this);
 
       ErrorResult rv;
-      runnable->Dispatch(rv);
+      runnable->Dispatch(Terminating, rv);
       if (NS_WARN_IF(rv.Failed())) {
         rv.SuppressException();
       }

dom/base/WebSocket.cpp

@@ -340,7 +340,7 @@ WebSocketImpl::PrintErrorOnConsole(const char *aBundleURI,
       new PrintErrorOnConsoleRunnable(this, aBundleURI, aError, aFormatStrings,
                                       aFormatStringsLen);
     ErrorResult rv;
-    runnable->Dispatch(rv);
+    runnable->Dispatch(Killing, rv);
     // XXXbz this seems totally broken.  We should be propagating this out, but
     // none of our callers really propagate anything usefully.  Come to think of
     // it, why is this a syncrunnable anyway?  Can't this be a fire-and-forget
@@ -629,7 +629,7 @@ WebSocketImpl::Disconnect()
     RefPtr<DisconnectInternalRunnable> runnable =
       new DisconnectInternalRunnable(this);
     ErrorResult rv;
-    runnable->Dispatch(rv);
+    runnable->Dispatch(Killing, rv);
     // XXXbz this seems totally broken.  We should be propagating this out, but
     // where to, exactly?
     rv.SuppressException();
@@ -1293,7 +1293,7 @@ WebSocket::ConstructorCommon(const GlobalObject& aGlobal,
       new InitRunnable(webSocketImpl, !!aTransportProvider, aUrl,
                        protocolArray, nsDependentCString(file.get()), lineno,
                        column, aRv, &connectionFailed);
-    runnable->Dispatch(aRv);
+    runnable->Dispatch(Terminating, aRv);
   }
 
   if (NS_WARN_IF(aRv.Failed())) {
@@ -1377,7 +1377,7 @@ WebSocket::ConstructorCommon(const GlobalObject& aGlobal,
                "not yet implemented");
     RefPtr<AsyncOpenRunnable> runnable =
       new AsyncOpenRunnable(webSocket->mImpl, aRv);
-    runnable->Dispatch(aRv);
+    runnable->Dispatch(Terminating, aRv);
   }
 
   if (NS_WARN_IF(aRv.Failed())) {

dom/broadcastchannel/BroadcastChannel.cpp

@@ -369,7 +369,7 @@ BroadcastChannel::Constructor(const GlobalObject& aGlobal,
 
     RefPtr<InitializeRunnable> runnable =
       new InitializeRunnable(workerPrivate, origin, principalInfo, aRv);
-    runnable->Dispatch(aRv);
+    runnable->Dispatch(Closing, aRv);
   }
 
   if (aRv.Failed()) {

dom/canvas/ImageBitmap.cpp

@@ -950,7 +950,7 @@ ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, ImageData& aImageData,
                                                        imageSize,
                                                        aCropRect,
                                                        getter_AddRefs(data));
-    task->Dispatch(aRv);
+    task->Dispatch(Terminating, aRv);
   }
 
   if (NS_WARN_IF(!data)) {
@@ -1377,10 +1377,10 @@ private:
     RefPtr<DecodeBlobInMainThreadSyncTask> task =
       new DecodeBlobInMainThreadSyncTask(mWorkerPrivate, *mBlob, mCropRect,
                                          getter_AddRefs(data), sourceSize);
-    task->Dispatch(rv); // This is a synchronous call.
+    task->Dispatch(Terminating, rv); // This is a synchronous call.
 
+    // In case the worker is terminating, this rejection can be handled.
     if (NS_WARN_IF(rv.Failed())) {
-      // XXXbz does this really make sense if we're shutting down?  Ah, well.
       mPromise->MaybeReject(rv);
       return nullptr;
     }
@@ -2104,7 +2104,10 @@ ImageBitmap::Create(nsIGlobalObject* aGlobal,
                                                                  aFormat,
                                                                  aLayout,
                                                                  getter_AddRefs(data));
-    task->Dispatch(aRv);
+    task->Dispatch(Terminating, aRv);
+    if (aRv.Failed()) {
+      return promise.forget();
+    }
   }
 
   if (NS_WARN_IF(!data)) {

dom/fetch/Request.cpp

@@ -403,7 +403,7 @@ Request::Constructor(const GlobalObject& aGlobal,
           // this work in a single sync loop.
           RefPtr<ReferrerSameOriginChecker> checker =
             new ReferrerSameOriginChecker(worker, referrerURL, rv);
-          checker->Dispatch(aRv);
+          checker->Dispatch(Terminating, aRv);
           if (aRv.Failed() || NS_FAILED(rv)) {
             aRv.ThrowTypeError<MSG_CROSS_ORIGIN_REFERRER_URL>(referrer,
                                                               worker->GetLocationInfo().mOrigin);

dom/notification/Notification.cpp

@@ -1881,7 +1881,7 @@ Notification::GetPermission(nsIGlobalObject* aGlobal, ErrorResult& aRv)
     MOZ_ASSERT(worker);
     RefPtr<GetPermissionRunnable> r =
       new GetPermissionRunnable(worker);
-    r->Dispatch(aRv);
+    r->Dispatch(Terminating, aRv);
     if (aRv.Failed()) {
       return NotificationPermission::Denied;
     }
@@ -2484,7 +2484,7 @@ NotificationWorkerHolder::Notify(Status aStatus)
     RefPtr<CloseNotificationRunnable> r =
       new CloseNotificationRunnable(kungFuDeathGrip);
     ErrorResult rv;
-    r->Dispatch(rv);
+    r->Dispatch(Killing, rv);
     // XXXbz I'm told throwing and returning false from here is pointless (and
     // also that doing sync stuff from here is really weird), so I guess we just
     // suppress the exception on rv, if any.
@@ -2627,7 +2627,7 @@ Notification::ShowPersistentNotification(JSContext* aCx,
     worker->AssertIsOnWorkerThread();
     RefPtr<CheckLoadRunnable> loadChecker =
       new CheckLoadRunnable(worker, NS_ConvertUTF16toUTF8(aScope));
-    loadChecker->Dispatch(aRv);
+    loadChecker->Dispatch(Terminating, aRv);
     if (aRv.Failed()) {
       return nullptr;
     }

dom/quota/StorageManager.cpp

@@ -335,7 +335,7 @@ StorageManager::Estimate(ErrorResult& aRv)
     new EstimateWorkerMainThreadRunnable(promiseProxy->GetWorkerPrivate(),
                                          promiseProxy);
 
-  runnnable->Dispatch(aRv);
+  runnnable->Dispatch(Terminating, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }

dom/url/URL.cpp

@@ -1113,6 +1113,12 @@ public:
     return true;
   }
 
+  void
+  Dispatch(ErrorResult& aRv)
+  {
+    WorkerMainThreadRunnable::Dispatch(Terminating, aRv);
+  }
+
 private:
   nsAString& mValue;
   GetterType mType;
@@ -1213,6 +1219,12 @@ public:
     return mFailed;
   }
 
+  void
+  Dispatch(ErrorResult& aRv)
+  {
+    WorkerMainThreadRunnable::Dispatch(Terminating, aRv);
+  }
+
 private:
   const nsString mValue;
   SetterType mType;
@@ -1224,7 +1236,7 @@ already_AddRefed<URLWorker>
 FinishConstructor(JSContext* aCx, WorkerPrivate* aPrivate,
                   ConstructorRunnable* aRunnable, ErrorResult& aRv)
 {
-  aRunnable->Dispatch(aRv);
+  aRunnable->Dispatch(Terminating, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }
@@ -1302,7 +1314,7 @@ URLWorker::CreateObjectURL(const GlobalObject& aGlobal, Blob& aBlob,
   RefPtr<CreateURLRunnable> runnable =
     new CreateURLRunnable(workerPrivate, blobImpl, aOptions, aResult);
 
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
     return;
   }
@@ -1325,7 +1337,7 @@ URLWorker::RevokeObjectURL(const GlobalObject& aGlobal, const nsAString& aUrl,
   RefPtr<RevokeURLRunnable> runnable =
     new RevokeURLRunnable(workerPrivate, aUrl);
 
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
     return;
   }
@@ -1348,7 +1360,7 @@ URLWorker::IsValidURL(const GlobalObject& aGlobal, const nsAString& aUrl,
   RefPtr<IsValidURLRunnable> runnable =
     new IsValidURLRunnable(workerPrivate, aUrl);
 
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
     return false;
   }

dom/workers/RuntimeService.cpp

@@ -587,7 +587,7 @@ ContentSecurityPolicyAllows(JSContext* aCx)
         new LogViolationDetailsRunnable(worker, fileName, lineNum);
 
     ErrorResult rv;
-    runnable->Dispatch(rv);
+    runnable->Dispatch(Killing, rv);
     if (NS_WARN_IF(rv.Failed())) {
       rv.SuppressException();
     }

dom/workers/ScriptLoader.cpp

@@ -2118,12 +2118,16 @@ LoadAllScripts(WorkerPrivate* aWorkerPrivate,
   aWorkerPrivate->AssertIsOnWorkerThread();
   NS_ASSERTION(!aLoadInfos.IsEmpty(), "Bad arguments!");
 
-  AutoSyncLoopHolder syncLoop(aWorkerPrivate);
+  AutoSyncLoopHolder syncLoop(aWorkerPrivate, Terminating);
+  nsCOMPtr<nsIEventTarget> syncLoopTarget = syncLoop.GetEventTarget();
+  if (!syncLoopTarget) {
+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
+    return;
+  }
 
   RefPtr<ScriptLoaderRunnable> loader =
-    new ScriptLoaderRunnable(aWorkerPrivate, syncLoop.EventTarget(),
-                             aLoadInfos, aIsMainScript, aWorkerScriptType,
-                             aRv);
+    new ScriptLoaderRunnable(aWorkerPrivate, syncLoopTarget, aLoadInfos,
+                             aIsMainScript, aWorkerScriptType, aRv);
 
   NS_ASSERTION(aLoadInfos.IsEmpty(), "Should have swapped!");
 
@@ -2184,7 +2188,7 @@ ChannelFromScriptURLWorkerThread(JSContext* aCx,
     new ChannelGetterRunnable(aParent, aScriptURL, aChannel);
 
   ErrorResult rv;
-  getter->Dispatch(rv);
+  getter->Dispatch(Terminating, rv);
   if (rv.Failed()) {
     NS_ERROR("Failed to dispatch!");
     return rv.StealNSResult();

dom/workers/WorkerNavigator.cpp

@@ -152,7 +152,7 @@ WorkerNavigator::GetUserAgent(nsString& aUserAgent, ErrorResult& aRv) const
   RefPtr<GetUserAgentRunnable> runnable =
     new GetUserAgentRunnable(workerPrivate, aUserAgent);
 
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
 }
 
 uint64_t

dom/workers/WorkerPrivate.cpp

@@ -5446,10 +5446,18 @@ WorkerPrivate::CancelAllTimeouts()
 }
 
 already_AddRefed<nsIEventTarget>
-WorkerPrivate::CreateNewSyncLoop()
+WorkerPrivate::CreateNewSyncLoop(Status aFailStatus)
 {
   AssertIsOnWorkerThread();
 
+  {
+    MutexAutoLock lock(mMutex);
+
+    if (mStatus >= aFailStatus) {
+      return nullptr;
+    }
+  }
+
   nsCOMPtr<nsIThreadInternal> thread = do_QueryInterface(NS_GetCurrentThread());
   MOZ_ASSERT(thread);
 

dom/workers/WorkerPrivate.h

@@ -1442,8 +1442,11 @@ private:
     memcpy(aPreferences, mPreferences, WORKERPREF_COUNT * sizeof(bool));
   }
 
+  // If the worker shutdown status is equal or greater then aFailStatus, this
+  // operation will fail and nullptr will be returned. See WorkerHolder.h for
+  // more information about the correct value to use.
   already_AddRefed<nsIEventTarget>
-  CreateNewSyncLoop();
+  CreateNewSyncLoop(Status aFailStatus);
 
   bool
   RunCurrentSyncLoop();
@@ -1518,9 +1521,11 @@ class AutoSyncLoopHolder
   uint32_t mIndex;
 
 public:
-  explicit AutoSyncLoopHolder(WorkerPrivate* aWorkerPrivate)
+  // See CreateNewSyncLoop() for more information about the correct value to use
+  // for aFailStatus.
+  AutoSyncLoopHolder(WorkerPrivate* aWorkerPrivate, Status aFailStatus)
   : mWorkerPrivate(aWorkerPrivate)
-  , mTarget(aWorkerPrivate->CreateNewSyncLoop())
+  , mTarget(aWorkerPrivate->CreateNewSyncLoop(aFailStatus))
   , mIndex(aWorkerPrivate->mSyncLoopStack.Length() - 1)
   {
     aWorkerPrivate->AssertIsOnWorkerThread();
@@ -1528,7 +1533,7 @@ public:
 
   ~AutoSyncLoopHolder()
   {
-    if (mWorkerPrivate) {
+    if (mWorkerPrivate && mTarget) {
       mWorkerPrivate->AssertIsOnWorkerThread();
       mWorkerPrivate->StopSyncLoop(mTarget, false);
       mWorkerPrivate->DestroySyncLoop(mIndex);
@@ -1547,8 +1552,9 @@ public:
   }
 
   nsIEventTarget*
-  EventTarget() const
+  GetEventTarget() const
   {
+    // This can be null if CreateNewSyncLoop() fails.
     return mTarget;
   }
 };

dom/workers/WorkerRunnable.cpp

@@ -568,15 +568,20 @@ WorkerMainThreadRunnable::WorkerMainThreadRunnable(WorkerPrivate* aWorkerPrivate
 }
 
 void
-WorkerMainThreadRunnable::Dispatch(ErrorResult& aRv)
+WorkerMainThreadRunnable::Dispatch(Status aFailStatus, ErrorResult& aRv)
 {
   mWorkerPrivate->AssertIsOnWorkerThread();
 
   TimeStamp startTime = TimeStamp::NowLoRes();
 
-  AutoSyncLoopHolder syncLoop(mWorkerPrivate);
+  AutoSyncLoopHolder syncLoop(mWorkerPrivate, aFailStatus);
 
-  mSyncLoopTarget = syncLoop.EventTarget();
+  mSyncLoopTarget = syncLoop.GetEventTarget();
+  if (!mSyncLoopTarget) {
+    // SyncLoop creation can fail if the worker is shutting down.
+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
+    return;
+  }
 
   DebugOnly<nsresult> rv = mWorkerPrivate->DispatchToMainThread(this);
   MOZ_ASSERT(NS_SUCCEEDED(rv),
@@ -621,7 +626,7 @@ bool
 WorkerCheckAPIExposureOnMainThreadRunnable::Dispatch()
 {
   ErrorResult rv;
-  WorkerMainThreadRunnable::Dispatch(rv);
+  WorkerMainThreadRunnable::Dispatch(Terminating, rv);
   bool ok = !rv.Failed();
   rv.SuppressException();
   return ok;

dom/workers/WorkerRunnable.h

@@ -401,9 +401,12 @@ protected:
 
 public:
   // Dispatch the runnable to the main thread.  If dispatch to main thread
-  // fails, or if the worker is shut down while dispatching, an error will be
-  // reported on aRv.  In that case the error MUST be propagated out to script.
-  void Dispatch(ErrorResult& aRv);
+  // fails, or if the worker is in a state equal or greater of aFailStatus, an
+  // error will be reported on aRv. Normally you want to use 'Terminating' for
+  // aFailStatus, except if you want an infallible runnable. In this case, use
+  // 'Killing'.
+  // In that case the error MUST be propagated out to script.
+  void Dispatch(Status aFailStatus, ErrorResult& aRv);
 
 private:
   NS_IMETHOD Run() override;

dom/xhr/XMLHttpRequestWorker.cpp

@@ -208,9 +208,9 @@ public:
   }
 
   void
-  Dispatch(ErrorResult& aRv)
+  Dispatch(Status aFailStatus, ErrorResult& aRv)
   {
-    WorkerMainThreadRunnable::Dispatch(aRv);
+    WorkerMainThreadRunnable::Dispatch(aFailStatus, aRv);
     if (NS_WARN_IF(aRv.Failed())) {
       return;
     }
@@ -1634,7 +1634,8 @@ XMLHttpRequestWorker::ReleaseProxy(ReleaseType aType)
       mProxy = nullptr;
 
       ErrorResult forAssertionsOnly;
-      runnable->Dispatch(forAssertionsOnly);
+      // This runnable _must_ be executed.
+      runnable->Dispatch(Killing, forAssertionsOnly);
       if (forAssertionsOnly.Failed()) {
         NS_ERROR("Failed to dispatch teardown runnable!");
       }
@@ -1804,8 +1805,12 @@ XMLHttpRequestWorker::SendInternal(SendRunnable* aRunnable,
   nsCOMPtr<nsIEventTarget> syncLoopTarget;
   bool isSyncXHR = mProxy->mIsSyncXHR;
   if (isSyncXHR) {
-    autoSyncLoop.emplace(mWorkerPrivate);
-    syncLoopTarget = autoSyncLoop->EventTarget();
+    autoSyncLoop.emplace(mWorkerPrivate, Terminating);
+    syncLoopTarget = autoSyncLoop->GetEventTarget();
+    if (!syncLoopTarget) {
+      aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
+      return;
+    }
   }
 
   mProxy->mOuterChannelId++;
@@ -1815,7 +1820,7 @@ XMLHttpRequestWorker::SendInternal(SendRunnable* aRunnable,
 
   mStateData.mFlagSend = true;
 
-  aRunnable->Dispatch(aRv);
+  aRunnable->Dispatch(Terminating, aRv);
   if (aRv.Failed()) {
     // Dispatch() may have spun the event loop and we may have already unrooted.
     // If so we don't want autoUnpin to try again.
@@ -1889,7 +1894,7 @@ XMLHttpRequestWorker::Open(const nsACString& aMethod,
                      mTimeout, mResponseType);
 
   ++mProxy->mOpenCount;
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
   if (aRv.Failed()) {
     if (mProxy && !--mProxy->mOpenCount) {
       ReleaseProxy();
@@ -1926,7 +1931,7 @@ XMLHttpRequestWorker::SetRequestHeader(const nsACString& aHeader,
 
   RefPtr<SetRequestHeaderRunnable> runnable =
     new SetRequestHeaderRunnable(mWorkerPrivate, mProxy, aHeader, aValue);
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
 }
 
 void
@@ -1949,7 +1954,7 @@ XMLHttpRequestWorker::SetTimeout(uint32_t aTimeout, ErrorResult& aRv)
 
   RefPtr<SetTimeoutRunnable> runnable =
     new SetTimeoutRunnable(mWorkerPrivate, mProxy, aTimeout);
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
 }
 
 void
@@ -1972,7 +1977,7 @@ XMLHttpRequestWorker::SetWithCredentials(bool aWithCredentials, ErrorResult& aRv
 
   RefPtr<SetWithCredentialsRunnable> runnable =
     new SetWithCredentialsRunnable(mWorkerPrivate, mProxy, aWithCredentials);
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
 }
 
 void
@@ -1997,7 +2002,7 @@ XMLHttpRequestWorker::SetMozBackgroundRequest(bool aBackgroundRequest,
   RefPtr<SetBackgroundRequestRunnable> runnable =
     new SetBackgroundRequestRunnable(mWorkerPrivate, mProxy,
                                      aBackgroundRequest);
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
 }
 
 XMLHttpRequestUpload*
@@ -2265,7 +2270,7 @@ XMLHttpRequestWorker::Abort(ErrorResult& aRv)
   mProxy->mOuterEventStreamId++;
 
   RefPtr<AbortRunnable> runnable = new AbortRunnable(mWorkerPrivate, mProxy);
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
 }
 
 void
@@ -2288,7 +2293,7 @@ XMLHttpRequestWorker::GetResponseHeader(const nsACString& aHeader,
   RefPtr<GetResponseHeaderRunnable> runnable =
     new GetResponseHeaderRunnable(mWorkerPrivate, mProxy, aHeader,
                                   responseHeader);
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
   if (aRv.Failed()) {
     return;
   }
@@ -2314,7 +2319,7 @@ XMLHttpRequestWorker::GetAllResponseHeaders(nsACString& aResponseHeaders,
   nsCString responseHeaders;
   RefPtr<GetAllResponseHeadersRunnable> runnable =
     new GetAllResponseHeadersRunnable(mWorkerPrivate, mProxy, responseHeaders);
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
   if (aRv.Failed()) {
     return;
   }
@@ -2346,7 +2351,7 @@ XMLHttpRequestWorker::OverrideMimeType(const nsAString& aMimeType, ErrorResult&
 
   RefPtr<OverrideMimeTypeRunnable> runnable =
     new OverrideMimeTypeRunnable(mWorkerPrivate, mProxy, aMimeType);
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
 }
 
 void
@@ -2382,7 +2387,7 @@ XMLHttpRequestWorker::SetResponseType(XMLHttpRequestResponseType aResponseType,
 
   RefPtr<SetResponseTypeRunnable> runnable =
     new SetResponseTypeRunnable(mWorkerPrivate, mProxy, aResponseType);
-  runnable->Dispatch(aRv);
+  runnable->Dispatch(Terminating, aRv);
   if (aRv.Failed()) {
     return;
   }

gfx/thebes/gfxUtils.cpp

@@ -1477,7 +1477,7 @@ gfxUtils::ThreadSafeGetFeatureStatus(const nsCOMPtr<nsIGfxInfo>& gfxInfo,
                                    status);
 
     ErrorResult rv;
-    runnable->Dispatch(rv);
+    runnable->Dispatch(dom::workers::Terminating, rv);
     if (rv.Failed()) {
         // XXXbz This is totally broken, since we're supposed to just abort
         // everything up the callstack but the callers basically eat the