From a80267e9e88b3dcebf7f8f1f6f10931a99ddf2ca Mon Sep 17 00:00:00 2001 From: janekptacijarabaci Date: Thu, 5 Jul 2018 13:04:44 +0200 Subject: Bug 604026 - Sync event loops in workers should be created only if compatible with the worker shutdown status --- dom/base/File.cpp | 2 +- dom/base/WebSocket.cpp | 8 +++---- dom/broadcastchannel/BroadcastChannel.cpp | 2 +- dom/canvas/ImageBitmap.cpp | 11 +++++---- dom/fetch/Request.cpp | 2 +- dom/notification/Notification.cpp | 6 ++--- dom/quota/StorageManager.cpp | 2 +- dom/url/URL.cpp | 20 +++++++++++++---- dom/workers/RuntimeService.cpp | 2 +- dom/workers/ScriptLoader.cpp | 14 +++++++----- dom/workers/WorkerNavigator.cpp | 2 +- dom/workers/WorkerPrivate.cpp | 10 ++++++++- dom/workers/WorkerPrivate.h | 16 ++++++++----- dom/workers/WorkerRunnable.cpp | 13 +++++++---- dom/workers/WorkerRunnable.h | 9 +++++--- dom/xhr/XMLHttpRequestWorker.cpp | 37 ++++++++++++++++++------------- 16 files changed, 101 insertions(+), 55 deletions(-) (limited to 'dom') diff --git a/dom/base/File.cpp b/dom/base/File.cpp index 8602a3064..7d86dfe8a 100755 --- a/dom/base/File.cpp +++ b/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(); } diff --git a/dom/base/WebSocket.cpp b/dom/base/WebSocket.cpp index d85bae82b..af4b7858b 100644 --- a/dom/base/WebSocket.cpp +++ b/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 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 runnable = new AsyncOpenRunnable(webSocket->mImpl, aRv); - runnable->Dispatch(aRv); + runnable->Dispatch(Terminating, aRv); } if (NS_WARN_IF(aRv.Failed())) { diff --git a/dom/broadcastchannel/BroadcastChannel.cpp b/dom/broadcastchannel/BroadcastChannel.cpp index 874212db7..d154b6562 100644 --- a/dom/broadcastchannel/BroadcastChannel.cpp +++ b/dom/broadcastchannel/BroadcastChannel.cpp @@ -369,7 +369,7 @@ BroadcastChannel::Constructor(const GlobalObject& aGlobal, RefPtr runnable = new InitializeRunnable(workerPrivate, origin, principalInfo, aRv); - runnable->Dispatch(aRv); + runnable->Dispatch(Closing, aRv); } if (aRv.Failed()) { diff --git a/dom/canvas/ImageBitmap.cpp b/dom/canvas/ImageBitmap.cpp index 6588e0aa3..e45cdfc6f 100644 --- a/dom/canvas/ImageBitmap.cpp +++ b/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 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)) { diff --git a/dom/fetch/Request.cpp b/dom/fetch/Request.cpp index c119a503e..d3836cda1 100644 --- a/dom/fetch/Request.cpp +++ b/dom/fetch/Request.cpp @@ -403,7 +403,7 @@ Request::Constructor(const GlobalObject& aGlobal, // this work in a single sync loop. RefPtr checker = new ReferrerSameOriginChecker(worker, referrerURL, rv); - checker->Dispatch(aRv); + checker->Dispatch(Terminating, aRv); if (aRv.Failed() || NS_FAILED(rv)) { aRv.ThrowTypeError(referrer, worker->GetLocationInfo().mOrigin); diff --git a/dom/notification/Notification.cpp b/dom/notification/Notification.cpp index 9c0ce2f18..1dd5724e4 100644 --- a/dom/notification/Notification.cpp +++ b/dom/notification/Notification.cpp @@ -1881,7 +1881,7 @@ Notification::GetPermission(nsIGlobalObject* aGlobal, ErrorResult& aRv) MOZ_ASSERT(worker); RefPtr 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 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 loadChecker = new CheckLoadRunnable(worker, NS_ConvertUTF16toUTF8(aScope)); - loadChecker->Dispatch(aRv); + loadChecker->Dispatch(Terminating, aRv); if (aRv.Failed()) { return nullptr; } diff --git a/dom/quota/StorageManager.cpp b/dom/quota/StorageManager.cpp index c8455f0fe..4e9f0cf8c 100644 --- a/dom/quota/StorageManager.cpp +++ b/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; } diff --git a/dom/url/URL.cpp b/dom/url/URL.cpp index c8724c359..04f5ec137 100644 --- a/dom/url/URL.cpp +++ b/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 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 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 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 runnable = new IsValidURLRunnable(workerPrivate, aUrl); - runnable->Dispatch(aRv); + runnable->Dispatch(Terminating, aRv); if (NS_WARN_IF(aRv.Failed())) { return false; } diff --git a/dom/workers/RuntimeService.cpp b/dom/workers/RuntimeService.cpp index ad95d4896..e1910536f 100644 --- a/dom/workers/RuntimeService.cpp +++ b/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(); } diff --git a/dom/workers/ScriptLoader.cpp b/dom/workers/ScriptLoader.cpp index 46545e737..56b18441e 100644 --- a/dom/workers/ScriptLoader.cpp +++ b/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 syncLoopTarget = syncLoop.GetEventTarget(); + if (!syncLoopTarget) { + aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); + return; + } RefPtr 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(); diff --git a/dom/workers/WorkerNavigator.cpp b/dom/workers/WorkerNavigator.cpp index 682c7a22c..f79896881 100644 --- a/dom/workers/WorkerNavigator.cpp +++ b/dom/workers/WorkerNavigator.cpp @@ -152,7 +152,7 @@ WorkerNavigator::GetUserAgent(nsString& aUserAgent, ErrorResult& aRv) const RefPtr runnable = new GetUserAgentRunnable(workerPrivate, aUserAgent); - runnable->Dispatch(aRv); + runnable->Dispatch(Terminating, aRv); } uint64_t diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp index 8848e881a..612090027 100644 --- a/dom/workers/WorkerPrivate.cpp +++ b/dom/workers/WorkerPrivate.cpp @@ -5446,10 +5446,18 @@ WorkerPrivate::CancelAllTimeouts() } already_AddRefed -WorkerPrivate::CreateNewSyncLoop() +WorkerPrivate::CreateNewSyncLoop(Status aFailStatus) { AssertIsOnWorkerThread(); + { + MutexAutoLock lock(mMutex); + + if (mStatus >= aFailStatus) { + return nullptr; + } + } + nsCOMPtr thread = do_QueryInterface(NS_GetCurrentThread()); MOZ_ASSERT(thread); diff --git a/dom/workers/WorkerPrivate.h b/dom/workers/WorkerPrivate.h index 28283bed7..20a530205 100644 --- a/dom/workers/WorkerPrivate.h +++ b/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 - 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; } }; diff --git a/dom/workers/WorkerRunnable.cpp b/dom/workers/WorkerRunnable.cpp index 1e16d7254..6bbe40f66 100644 --- a/dom/workers/WorkerRunnable.cpp +++ b/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 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; diff --git a/dom/workers/WorkerRunnable.h b/dom/workers/WorkerRunnable.h index c65060f44..8249a8053 100644 --- a/dom/workers/WorkerRunnable.h +++ b/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; diff --git a/dom/xhr/XMLHttpRequestWorker.cpp b/dom/xhr/XMLHttpRequestWorker.cpp index e7193a279..b5f853a50 100644 --- a/dom/xhr/XMLHttpRequestWorker.cpp +++ b/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 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 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 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 runnable = new SetWithCredentialsRunnable(mWorkerPrivate, mProxy, aWithCredentials); - runnable->Dispatch(aRv); + runnable->Dispatch(Terminating, aRv); } void @@ -1997,7 +2002,7 @@ XMLHttpRequestWorker::SetMozBackgroundRequest(bool aBackgroundRequest, RefPtr 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 runnable = new AbortRunnable(mWorkerPrivate, mProxy); - runnable->Dispatch(aRv); + runnable->Dispatch(Terminating, aRv); } void @@ -2288,7 +2293,7 @@ XMLHttpRequestWorker::GetResponseHeader(const nsACString& aHeader, RefPtr 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 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 runnable = new OverrideMimeTypeRunnable(mWorkerPrivate, mProxy, aMimeType); - runnable->Dispatch(aRv); + runnable->Dispatch(Terminating, aRv); } void @@ -2382,7 +2387,7 @@ XMLHttpRequestWorker::SetResponseType(XMLHttpRequestResponseType aResponseType, RefPtr runnable = new SetResponseTypeRunnable(mWorkerPrivate, mProxy, aResponseType); - runnable->Dispatch(aRv); + runnable->Dispatch(Terminating, aRv); if (aRv.Failed()) { return; } -- cgit v1.2.3