From e8d47ac312e3f5740057e96f6783e13e50dce9ff Mon Sep 17 00:00:00 2001 From: Andrea Marchesini Date: Wed, 31 Aug 2016 21:33:05 -0700 Subject: [PATCH] Bug 790919 - Don't dispatch close event in Workers, r=bkelly --- dom/webidl/WorkerGlobalScope.webidl | 1 - dom/workers/RuntimeService.h | 12 - dom/workers/WorkerPrivate.cpp | 289 ++---------------- dom/workers/WorkerPrivate.h | 32 +- dom/workers/WorkerScope.h | 1 - dom/workers/test/closeOnGC_server.sjs | 23 -- dom/workers/test/closeOnGC_worker.js | 11 - dom/workers/test/close_worker.js | 21 -- dom/workers/test/mochitest.ini | 5 - dom/workers/test/terminate_worker.js | 4 - dom/workers/test/test_close.html | 29 -- dom/workers/test/test_closeOnGC.html | 57 ---- dom/workers/test/test_terminate.html | 4 +- .../subprocess/subprocess_worker_common.js | 6 - .../subprocess/subprocess_worker_unix.js | 1 + .../subprocess/subprocess_worker_win.js | 1 + 16 files changed, 30 insertions(+), 467 deletions(-) delete mode 100644 dom/workers/test/closeOnGC_server.sjs delete mode 100644 dom/workers/test/closeOnGC_worker.js delete mode 100644 dom/workers/test/close_worker.js delete mode 100644 dom/workers/test/test_close.html delete mode 100644 dom/workers/test/test_closeOnGC.html diff --git a/dom/webidl/WorkerGlobalScope.webidl b/dom/webidl/WorkerGlobalScope.webidl index ef61a18c1715..32648a8c526f 100644 --- a/dom/webidl/WorkerGlobalScope.webidl +++ b/dom/webidl/WorkerGlobalScope.webidl @@ -53,7 +53,6 @@ WorkerGlobalScope implements ImageBitmapFactories; // Mozilla extensions partial interface WorkerGlobalScope { - attribute EventHandler onclose; void dump(optional DOMString str); diff --git a/dom/workers/RuntimeService.h b/dom/workers/RuntimeService.h index 2de91c810a1b..2e5cc1dad581 100644 --- a/dom/workers/RuntimeService.h +++ b/dom/workers/RuntimeService.h @@ -215,18 +215,6 @@ public: void UpdateAllWorkerMemoryParameter(JSGCParamKey aKey, uint32_t aValue); - static uint32_t - GetContentCloseHandlerTimeoutSeconds() - { - return sDefaultJSSettings.content.maxScriptRuntime; - } - - static uint32_t - GetChromeCloseHandlerTimeoutSeconds() - { - return sDefaultJSSettings.chrome.maxScriptRuntime; - } - #ifdef JS_GC_ZEAL static void SetDefaultGCZeal(uint8_t aGCZeal, uint32_t aFrequency) diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp index 1099cac9aa73..4680d560f861 100644 --- a/dom/workers/WorkerPrivate.cpp +++ b/dom/workers/WorkerPrivate.cpp @@ -603,64 +603,6 @@ private: } }; -class CloseEventRunnable final : public WorkerRunnable -{ -public: - explicit CloseEventRunnable(WorkerPrivate* aWorkerPrivate) - : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount) - { } - -private: - virtual bool - PreDispatch(WorkerPrivate* aWorkerPrivate) override - { - MOZ_CRASH("Don't call Dispatch() on CloseEventRunnable!"); - } - - virtual void - PostDispatch(WorkerPrivate* aWorkerPrivate, bool aDispatchResult) override - { - MOZ_CRASH("Don't call Dispatch() on CloseEventRunnable!"); - } - - virtual bool - WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override - { - aWorkerPrivate->CloseHandlerStarted(); - - WorkerGlobalScope* globalScope = aWorkerPrivate->GlobalScope(); - - RefPtr event = NS_NewDOMEvent(globalScope, nullptr, nullptr); - - event->InitEvent(NS_LITERAL_STRING("close"), false, false); - event->SetTrusted(true); - - globalScope->DispatchDOMEvent(nullptr, event, nullptr, nullptr); - - return true; - } - - nsresult Cancel() override - { - // We need to run regardless. - Run(); - return WorkerRunnable::Cancel(); - } - - virtual void - PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate, bool aRunResult) - override - { - // Report errors. - WorkerRunnable::PostRun(aCx, aWorkerPrivate, aRunResult); - - // Match the busy count increase from NotifyRunnable. - aWorkerPrivate->ModifyBusyCountFromWorker(false); - - aWorkerPrivate->CloseHandlerFinished(); - } -}; - class MessageEventRunnable final : public WorkerRunnable , public StructuredCloneHolder { @@ -899,8 +841,6 @@ private: PreDispatch(WorkerPrivate* aWorkerPrivate) override { aWorkerPrivate->AssertIsOnParentThread(); - // Modify here, but not in PostRun! This busy count addition will be matched - // by the CloseEventRunnable. return aWorkerPrivate->ModifyBusyCount(true); } @@ -915,6 +855,14 @@ private: } } + virtual void + PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate, bool aRunResult) + override + { + aWorkerPrivate->ModifyBusyCountFromWorker(false); + return; + } + virtual bool WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override { @@ -935,9 +883,7 @@ private: virtual bool WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override { - // This busy count will be matched by the CloseEventRunnable. - return aWorkerPrivate->ModifyBusyCount(true) && - aWorkerPrivate->Close(); + return aWorkerPrivate->Close(); } }; @@ -1356,111 +1302,6 @@ DummyCallback(nsITimer* aTimer, void* aClosure) // Nothing! } -class KillCloseEventRunnable final : public WorkerRunnable -{ - nsCOMPtr mTimer; - - class KillScriptRunnable final : public WorkerControlRunnable - { - public: - explicit KillScriptRunnable(WorkerPrivate* aWorkerPrivate) - : WorkerControlRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount) - { } - - private: - virtual bool - PreDispatch(WorkerPrivate* aWorkerPrivate) override - { - // Silence bad assertions, this is dispatched from the timer thread. - return true; - } - - virtual void - PostDispatch(WorkerPrivate* aWorkerPrivate, bool aDispatchResult) override - { - // Silence bad assertions, this is dispatched from the timer thread. - } - - virtual bool - WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override - { - // Kill running script. - return false; - } - }; - -public: - explicit KillCloseEventRunnable(WorkerPrivate* aWorkerPrivate) - : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount) - { } - - bool - SetTimeout(uint32_t aDelayMS) - { - nsCOMPtr timer = do_CreateInstance(NS_TIMER_CONTRACTID); - if (!timer) { - return false; - } - - RefPtr runnable = - new KillScriptRunnable(mWorkerPrivate); - - RefPtr target = - new TimerThreadEventTarget(mWorkerPrivate, runnable); - - if (NS_FAILED(timer->SetTarget(target))) { - return false; - } - - if (NS_FAILED(timer->InitWithNamedFuncCallback( - DummyCallback, nullptr, aDelayMS, nsITimer::TYPE_ONE_SHOT, - "dom::workers::DummyCallback(1)"))) { - return false; - } - - mTimer.swap(timer); - return true; - } - -private: - ~KillCloseEventRunnable() - { - if (mTimer) { - mTimer->Cancel(); - } - } - - virtual bool - PreDispatch(WorkerPrivate* aWorkerPrivate) override - { - MOZ_CRASH("Don't call Dispatch() on KillCloseEventRunnable!"); - } - - virtual void - PostDispatch(WorkerPrivate* aWorkerPrivate, bool aDispatchResult) override - { - MOZ_CRASH("Don't call Dispatch() on KillCloseEventRunnable!"); - } - - virtual bool - WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override - { - if (mTimer) { - mTimer->Cancel(); - mTimer = nullptr; - } - - return true; - } - - nsresult Cancel() override - { - // We need to run regardless. - Run(); - return WorkerRunnable::Cancel(); - } -}; - class UpdateContextOptionsRunnable final : public WorkerControlRunnable { JS::ContextOptions mContextOptions; @@ -4035,8 +3876,6 @@ WorkerPrivate::WorkerPrivate(WorkerPrivate* aParent, , mFrozen(false) , mTimerRunning(false) , mRunningExpiredTimeouts(false) - , mCloseHandlerStarted(false) - , mCloseHandlerFinished(false) , mPendingEventQueueClearing(false) , mMemoryReporterRunning(false) , mBlockedForMemoryReporter(false) @@ -4258,7 +4097,6 @@ WorkerPrivate::GetLoadInfo(JSContext* aCx, nsPIDOMWindowInner* aWindow, } if (parentStatus > Running) { - NS_WARNING("Cannot create child workers from the close handler!"); return NS_ERROR_FAILURE; } @@ -4536,12 +4374,13 @@ WorkerPrivate::DoRunLoop(JSContext* aCx) Maybe workerCompartment; for (;;) { - Status currentStatus; + Status currentStatus, previousStatus; bool debuggerRunnablesPending = false; bool normalRunnablesPending = false; { MutexAutoLock lock(mMutex); + previousStatus = mStatus; while (mControlQueue.IsEmpty() && !(debuggerRunnablesPending = !mDebuggerQueue.IsEmpty()) && @@ -4562,10 +4401,11 @@ WorkerPrivate::DoRunLoop(JSContext* aCx) currentStatus = mStatus; } - // If the close handler has finished and all holders are done then we can - // kill this thread. + // if all holders are done then we can kill this thread. if (currentStatus != Running && !HasActiveHolders()) { - if (mCloseHandlerFinished && currentStatus != Killing) { + + // If we just changed status, we must schedule the current runnables. + if (previousStatus != Running && currentStatus != Killing) { NotifyInternal(aCx, Killing); MOZ_ASSERT(!JS_IsExceptionPending(aCx)); @@ -4646,10 +4486,8 @@ WorkerPrivate::DoRunLoop(JSContext* aCx) JS_MaybeGC(aCx); } } else if (normalRunnablesPending) { - MOZ_ASSERT(NS_HasPendingEvents(mThread)); - // Process a single runnable from the main queue. - MOZ_ALWAYS_TRUE(NS_ProcessNextEvent(mThread, false)); + NS_ProcessNextEvent(mThread, false); normalRunnablesPending = NS_HasPendingEvents(mThread); if (normalRunnablesPending && GlobalScope()) { @@ -4851,7 +4689,7 @@ WorkerPrivate::InterruptCallback(JSContext* aCx) break; } - WaitForWorkerEvents(PR_MillisecondsToInterval(RemainingRunTimeMS())); + WaitForWorkerEvents(PR_MillisecondsToInterval(UINT32_MAX)); } } @@ -5153,17 +4991,6 @@ WorkerPrivate::ClearDebuggerEventQueue() } } -uint32_t -WorkerPrivate::RemainingRunTimeMS() const -{ - if (mKillTime.IsNull()) { - return UINT32_MAX; - } - TimeDuration runtime = mKillTime - TimeStamp::Now(); - double ms = runtime > TimeDuration(0) ? runtime.ToMilliseconds() : 0; - return ms > double(UINT32_MAX) ? UINT32_MAX : uint32_t(ms); -} - bool WorkerPrivate::FreezeInternal() { @@ -5802,8 +5629,6 @@ WorkerPrivate::NotifyInternal(JSContext* aCx, Status aStatus) MOZ_ASSERT(previousStatus != Pending); - MOZ_ASSERT(previousStatus >= Canceling || mKillTime.IsNull()); - // Let all our holders know the new status. NotifyHolders(aCx, aStatus); MOZ_ASSERT(!JS_IsExceptionPending(aCx)); @@ -5820,28 +5645,12 @@ WorkerPrivate::NotifyInternal(JSContext* aCx, Status aStatus) } } - // If we've run the close handler, we don't need to do anything else. - if (mCloseHandlerFinished) { - return true; - } - // If the worker script never ran, or failed to compile, we don't need to do - // anything else, except pretend that we ran the close handler. + // anything else. if (!GlobalScope()) { - mCloseHandlerStarted = true; - mCloseHandlerFinished = true; return true; } - // If this is the first time our status has changed we also need to schedule - // the close handler unless we're being shut down. - if (previousStatus == Running && aStatus != Killing) { - MOZ_ASSERT(!mCloseHandlerStarted && !mCloseHandlerFinished); - - RefPtr closeRunnable = new CloseEventRunnable(this); - MOZ_ALWAYS_SUCCEEDS(NS_DispatchToCurrentThread(closeRunnable)); - } - if (aStatus == Closing) { // Notify parent to stop sending us messages and balance our busy count. RefPtr runnable = new CloseRunnable(this); @@ -5853,60 +5662,14 @@ WorkerPrivate::NotifyInternal(JSContext* aCx, Status aStatus) return true; } - if (aStatus == Terminating) { - // Only abort the script if we're not yet running the close handler. - return mCloseHandlerStarted; - } - - if (aStatus == Canceling) { - // We need to enforce a timeout on the close handler. - MOZ_ASSERT(previousStatus >= Running && previousStatus <= Terminating); - - uint32_t killSeconds = IsChromeWorker() ? - RuntimeService::GetChromeCloseHandlerTimeoutSeconds() : - RuntimeService::GetContentCloseHandlerTimeoutSeconds(); - - if (killSeconds) { - mKillTime = TimeStamp::Now() + TimeDuration::FromSeconds(killSeconds); - - if (!mCloseHandlerFinished && !ScheduleKillCloseEventRunnable()) { - return false; - } - } - - // Only abort the script if we're not yet running the close handler. - return mCloseHandlerStarted; - } - - MOZ_ASSERT(aStatus == Killing); - - mKillTime = TimeStamp::Now(); - - if (mCloseHandlerStarted && !mCloseHandlerFinished) { - ScheduleKillCloseEventRunnable(); - } + MOZ_ASSERT(aStatus == Terminating || + aStatus == Canceling || + aStatus == Killing); // Always abort the script. return false; } -bool -WorkerPrivate::ScheduleKillCloseEventRunnable() -{ - AssertIsOnWorkerThread(); - MOZ_ASSERT(!mKillTime.IsNull()); - - RefPtr killCloseEventRunnable = - new KillCloseEventRunnable(this); - if (!killCloseEventRunnable->SetTimeout(RemainingRunTimeMS())) { - return false; - } - - MOZ_ALWAYS_SUCCEEDS(NS_DispatchToCurrentThread(killCloseEventRunnable)); - - return true; -} - void WorkerPrivate::ReportError(JSContext* aCx, const char* aFallbackMessage, JSErrorReport* aReport) @@ -5965,9 +5728,8 @@ WorkerPrivate::ReportError(JSContext* aCx, const char* aFallbackMessage, mErrorHandlerRecursionCount++; // Don't want to run the scope's error handler if this is a recursive error or - // if there was an error in the close handler or if we ran out of memory. + // if we ran out of memory. bool fireAtScope = mErrorHandlerRecursionCount == 1 && - !mCloseHandlerStarted && errorNumber != JSMSG_OUT_OF_MEMORY && JS::CurrentGlobalOrNull(aCx); @@ -6008,13 +5770,6 @@ WorkerPrivate::SetTimeout(JSContext* aCx, currentStatus = mStatus; } - // It's a script bug if setTimeout/setInterval are called from a close handler - // so throw an exception. - if (currentStatus == Closing) { - aRv.Throw(NS_ERROR_FAILURE); - return 0; - } - // If the worker is trying to call setTimeout/setInterval and the parent // thread has initiated the close process then just silently fail. if (currentStatus >= Closing) { diff --git a/dom/workers/WorkerPrivate.h b/dom/workers/WorkerPrivate.h index 6f8c2615c8da..0a0e8e30d245 100644 --- a/dom/workers/WorkerPrivate.h +++ b/dom/workers/WorkerPrivate.h @@ -950,8 +950,6 @@ class WorkerPrivate : public WorkerPrivateParent bool mFrozen; bool mTimerRunning; bool mRunningExpiredTimeouts; - bool mCloseHandlerStarted; - bool mCloseHandlerFinished; bool mPendingEventQueueClearing; bool mMemoryReporterRunning; bool mBlockedForMemoryReporter; @@ -1155,20 +1153,6 @@ public: bool RescheduleTimeoutTimer(JSContext* aCx); - void - CloseHandlerStarted() - { - AssertIsOnWorkerThread(); - mCloseHandlerStarted = true; - } - - void - CloseHandlerFinished() - { - AssertIsOnWorkerThread(); - mCloseHandlerFinished = true; - } - void UpdateContextOptionsInternal(JSContext* aCx, const JS::ContextOptions& aContextOptions); @@ -1369,24 +1353,16 @@ private: status = mStatus; } - if (status >= Killing) { - return false; + if (status < Terminating) { + return true; } - if (status >= Running) { - return mKillTime.IsNull() || RemainingRunTimeMS() > 0; - } - return true; - } - uint32_t - RemainingRunTimeMS() const; + return false; + } void CancelAllTimeouts(); - bool - ScheduleKillCloseEventRunnable(); - enum class ProcessAllControlRunnablesResult { // We did not process anything. diff --git a/dom/workers/WorkerScope.h b/dom/workers/WorkerScope.h index d0f9a800909d..56df9e4c3197 100644 --- a/dom/workers/WorkerScope.h +++ b/dom/workers/WorkerScope.h @@ -148,7 +148,6 @@ public: IMPL_EVENT_HANDLER(online) IMPL_EVENT_HANDLER(offline) - IMPL_EVENT_HANDLER(close) void Dump(const Optional& aString) const; diff --git a/dom/workers/test/closeOnGC_server.sjs b/dom/workers/test/closeOnGC_server.sjs deleted file mode 100644 index 55d0402a6afd..000000000000 --- a/dom/workers/test/closeOnGC_server.sjs +++ /dev/null @@ -1,23 +0,0 @@ -/** - * Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ - */ -function handleRequest(request, response) -{ - response.setHeader("Content-Type", "text/plain", false); - response.setHeader("Cache-Control", "no-cache", false); - - if (request.method == "POST") { - setState("seenPost" + request.queryString, "1"); - return; - } - - if (request.method == "GET") { - if (getState("seenPost" + request.queryString) == "1") { - response.write("closed"); - } - return; - } - - response.setStatusLine(request.httpVersion, 404, "Not found"); -} diff --git a/dom/workers/test/closeOnGC_worker.js b/dom/workers/test/closeOnGC_worker.js deleted file mode 100644 index 3d7f3edd2ab7..000000000000 --- a/dom/workers/test/closeOnGC_worker.js +++ /dev/null @@ -1,11 +0,0 @@ -/** - * Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ - */ -onclose = function() { - var xhr = new XMLHttpRequest(); - xhr.open("POST", "closeOnGC_server.sjs" + location.search, false); - xhr.send(); -}; - -postMessage("ready"); diff --git a/dom/workers/test/close_worker.js b/dom/workers/test/close_worker.js deleted file mode 100644 index 26714bfed6c7..000000000000 --- a/dom/workers/test/close_worker.js +++ /dev/null @@ -1,21 +0,0 @@ -/** - * Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ - */ -onclose = function() { - postMessage("closed"); - // Try to open a new worker. - try { - var worker = new Worker("close_worker.js"); - throw new Error("We shouldn't get here!"); - } catch (e) { - // pass - } -}; - -setTimeout(function () { - setTimeout(function () { - throw new Error("I should never run!"); - }, 1000); - close(); -}, 1000); diff --git a/dom/workers/test/mochitest.ini b/dom/workers/test/mochitest.ini index 57625bc6da3f..d1ee29f58b1d 100644 --- a/dom/workers/test/mochitest.ini +++ b/dom/workers/test/mochitest.ini @@ -11,9 +11,6 @@ support-files = bug998474_worker.js bug1063538_worker.js clearTimeouts_worker.js - closeOnGC_server.sjs - closeOnGC_worker.js - close_worker.js content_worker.js console_worker.js consoleReplaceable_worker.js @@ -146,8 +143,6 @@ skip-if = true # bug 1176225 [test_bug1132924.html] [test_chromeWorker.html] [test_clearTimeouts.html] -[test_close.html] -[test_closeOnGC.html] [test_console.html] [test_consoleAndBlobs.html] [test_consoleReplaceable.html] diff --git a/dom/workers/test/terminate_worker.js b/dom/workers/test/terminate_worker.js index ad4e8252773d..f1a49e03266c 100644 --- a/dom/workers/test/terminate_worker.js +++ b/dom/workers/test/terminate_worker.js @@ -2,10 +2,6 @@ * Any copyright is dedicated to the Public Domain. * http://creativecommons.org/publicdomain/zero/1.0/ */ -onclose = function() { - postMessage("Closed!"); -} - onmessage = function(event) { throw "No messages should reach me!"; } diff --git a/dom/workers/test/test_close.html b/dom/workers/test/test_close.html deleted file mode 100644 index 2fc546c416b7..000000000000 --- a/dom/workers/test/test_close.html +++ /dev/null @@ -1,29 +0,0 @@ - - - - - Test for DOM Worker Threads - - - - -

- -
-
-
- - diff --git a/dom/workers/test/test_closeOnGC.html b/dom/workers/test/test_closeOnGC.html deleted file mode 100644 index c1b8f83cef78..000000000000 --- a/dom/workers/test/test_closeOnGC.html +++ /dev/null @@ -1,57 +0,0 @@ - - - - - Test for DOM Worker Threads - - - - -

- -
-
-
- - diff --git a/dom/workers/test/test_terminate.html b/dom/workers/test/test_terminate.html index 0d060dc58b42..5d31bd16523a 100644 --- a/dom/workers/test/test_terminate.html +++ b/dom/workers/test/test_terminate.html @@ -30,7 +30,7 @@ Tests of DOM Worker terminate feature function messageListener(event) { is(event.data, "Still alive!", "Correct message!"); - if (messageCount++ == 20) { + if (++messageCount == 20) { ok(worker.onmessage === messageListener, "Correct listener before terminate"); @@ -82,7 +82,7 @@ Tests of DOM Worker terminate feature } function testCount() { - is(messageCount, 21, "Received another message after terminated!"); + is(messageCount, 20, "Received another message after terminated!"); if (intervalCount++ == 5) { clearInterval(interval); SimpleTest.finish(); diff --git a/toolkit/modules/subprocess/subprocess_worker_common.js b/toolkit/modules/subprocess/subprocess_worker_common.js index 095bb6fb9ae2..2b681e737884 100644 --- a/toolkit/modules/subprocess/subprocess_worker_common.js +++ b/toolkit/modules/subprocess/subprocess_worker_common.js @@ -227,9 +227,3 @@ onmessage = event => { }); }); }; - -onclose = event => { - io.shutdown(); - - self.postMessage({msg: "close"}); -}; diff --git a/toolkit/modules/subprocess/subprocess_worker_unix.js b/toolkit/modules/subprocess/subprocess_worker_unix.js index 1a2595c286fb..1a6118707536 100644 --- a/toolkit/modules/subprocess/subprocess_worker_unix.js +++ b/toolkit/modules/subprocess/subprocess_worker_unix.js @@ -500,6 +500,7 @@ io = { this.signal.cleanup(); this.signal = null; + self.postMessage({msg: "close"}); self.close(); } }, diff --git a/toolkit/modules/subprocess/subprocess_worker_win.js b/toolkit/modules/subprocess/subprocess_worker_win.js index a12dd83ff9e4..b0756dac344e 100644 --- a/toolkit/modules/subprocess/subprocess_worker_win.js +++ b/toolkit/modules/subprocess/subprocess_worker_win.js @@ -599,6 +599,7 @@ io = { this.signal.cleanup(); this.signal = null; + self.postMessage({msg: "close"}); self.close(); } },