Bug 720679 - 'Crash @ WorkerPrivate::CancelAllTimeouts while closing Firefox'. r=khuey, a=akeybl
authorBen Turner <bent.mozilla@gmail.com>
Mon, 26 Mar 2012 21:05:09 -0700
changeset 90629 b0a19a26150c
parent 90628 196ad34ad49a
child 90630 4874cec6f1c8
push id1047
push userbturner@mozilla.com
push dateMon, 02 Apr 2012 17:48:37 +0000
treeherdermozilla-aurora@b0a19a26150c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskhuey, akeybl
bugs720679
milestone13.0a2
Bug 720679 - 'Crash @ WorkerPrivate::CancelAllTimeouts while closing Firefox'. r=khuey, a=akeybl
dom/workers/WorkerPrivate.cpp
dom/workers/test/Makefile.in
dom/workers/test/clearTimeouts_worker.js
dom/workers/test/test_clearTimeouts.html
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -3156,35 +3156,43 @@ WorkerPrivate::NotifyFeatures(JSContext*
       NS_WARNING("Failed to notify child worker!");
     }
   }
 }
 
 void
 WorkerPrivate::CancelAllTimeouts(JSContext* aCx)
 {
+  AssertIsOnWorkerThread();
+
   if (mTimerRunning) {
     NS_ASSERTION(mTimer, "Huh?!");
     NS_ASSERTION(!mTimeouts.IsEmpty(), "Huh?!");
 
     if (NS_FAILED(mTimer->Cancel())) {
       NS_WARNING("Failed to cancel timer!");
     }
 
     for (PRUint32 index = 0; index < mTimeouts.Length(); index++) {
       mTimeouts[index]->mCanceled = true;
     }
 
-    RunExpiredTimeouts(aCx);
-
-    mTimer = nsnull;
+    if (!RunExpiredTimeouts(aCx)) {
+      JS_ReportPendingException(aCx);
+    }
+
+    mTimerRunning = false;
   }
-  else {
+#ifdef DEBUG
+  else if (!mRunningExpiredTimeouts) {
     NS_ASSERTION(mTimeouts.IsEmpty(), "Huh?!");
   }
+#endif
+
+  mTimer = nsnull;
 }
 
 PRUint32
 WorkerPrivate::CreateNewSyncLoop()
 {
   AssertIsOnWorkerThread();
 
   NS_ASSERTION(mSyncQueues.Length() < PR_UINT32_MAX,
@@ -3503,18 +3511,25 @@ WorkerPrivate::SetTimeout(JSContext* aCx
   const PRUint32 timerId = mNextTimeoutId++;
 
   Status currentStatus;
   {
     MutexAutoLock lock(mMutex);
     currentStatus = mStatus;
   }
 
-  if (currentStatus > Running) {
+  // It's a script bug if setTimeout/setInterval are called from a close handler
+  // so throw an exception.
+  if (currentStatus == Closing) {
     JS_ReportError(aCx, "Cannot schedule timeouts from the close handler!");
+  }
+
+  // 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) {
     return false;
   }
 
   nsAutoPtr<TimeoutInfo> newInfo(new TimeoutInfo());
   newInfo->mIsInterval = aIsInterval;
   newInfo->mId = timerId;
 
   if (NS_UNLIKELY(timerId == PR_UINT32_MAX)) {
@@ -3647,16 +3662,17 @@ WorkerPrivate::RunExpiredTimeouts(JSCont
 
   // We may be called recursively (e.g. close() inside a timeout) or we could
   // have been canceled while this event was pending, bail out if there is
   // nothing to do.
   if (mRunningExpiredTimeouts || !mTimerRunning) {
     return true;
   }
 
+  NS_ASSERTION(mTimer, "Must have a timer!");
   NS_ASSERTION(!mTimeouts.IsEmpty(), "Should have some work to do!");
 
   bool retval = true;
 
   AutoPtrComparator<TimeoutInfo> comparator = GetAutoPtrComparator(mTimeouts);
   JSObject* global = JS_GetGlobalObject(aCx);
   JSPrincipals* principal = GetWorkerPrincipal();
 
@@ -3711,22 +3727,26 @@ WorkerPrivate::RunExpiredTimeouts(JSCont
                                 info->mExtraArgVals.Length(),
                                 info->mExtraArgVals.Elements(), &rval) &&
           !JS_ReportPendingException(aCx)) {
         retval = false;
         break;
       }
     }
 
+    NS_ASSERTION(mRunningExpiredTimeouts, "Someone changed this!");
+
     // Reschedule intervals.
-    if (info->mIsInterval) {
+    if (info->mIsInterval && !info->mCanceled) {
       PRUint32 timeoutIndex = mTimeouts.IndexOf(info);
       NS_ASSERTION(timeoutIndex != PRUint32(-1),
                    "Should still be in the main list!");
 
+      // This is nasty but we have to keep the old nsAutoPtr from deleting the
+      // info we're about to re-add.
       mTimeouts[timeoutIndex].forget();
       mTimeouts.RemoveElementAt(timeoutIndex);
 
       NS_ASSERTION(!mTimeouts.Contains(info), "Shouldn't have duplicates!");
 
       // NB: We must ensure that info->mTargetTime > now (where now is the
       // now above, not literally TimeStamp::Now()) or we will remove the
       // interval in the next loop below.
@@ -3747,18 +3767,18 @@ WorkerPrivate::RunExpiredTimeouts(JSCont
                    "Interval timers can only be removed when canceled!");
       mTimeouts.RemoveElement(info);
     }
     else {
       index++;
     }
   }
 
-  // Signal the parent that we're no longer using timeouts or reschedule the
-  // timer.
+  // Either signal the parent that we're no longer using timeouts or reschedule
+  // the timer.
   if (mTimeouts.IsEmpty()) {
     if (!ModifyBusyCountFromWorker(aCx, false)) {
       retval = false;
     }
     mTimerRunning = false;
   }
   else if (retval && !RescheduleTimeoutTimer(aCx)) {
     retval = false;
--- a/dom/workers/test/Makefile.in
+++ b/dom/workers/test/Makefile.in
@@ -51,16 +51,18 @@ DIRS = \
 
 include $(topsrcdir)/config/rules.mk
 
 _TEST_FILES = \
   test_404.html \
   test_atob.html \
   atob_worker.js \
   test_blobWorkers.html \
+  test_clearTimeouts.html \
+  clearTimeouts_worker.js \
   test_close.html \
   close_worker.js \
   test_closeOnGC.html \
   closeOnGC_worker.js \
   closeOnGC_server.sjs \
   test_dataURLWorker.html \
   test_errorPropagation.html \
   errorPropagation_iframe.html \
new file mode 100644
--- /dev/null
+++ b/dom/workers/test/clearTimeouts_worker.js
@@ -0,0 +1,12 @@
+var count = 0;
+function timerFunction() {
+  if (++count == 30) {
+    close();
+    postMessage("ready");
+    while (true) { }
+  }
+}
+
+for (var i = 0; i < 10; i++) {
+  setInterval(timerFunction, 500);
+}
new file mode 100644
--- /dev/null
+++ b/dom/workers/test/test_clearTimeouts.html
@@ -0,0 +1,30 @@
+<!--
+  Any copyright is dedicated to the Public Domain.
+  http://creativecommons.org/publicdomain/zero/1.0/
+-->
+<!DOCTYPE HTML>
+<html>
+<head>
+  <title>Test for DOM Worker Threads</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<p id="display"></p>
+<div id="content" style="display: none"></div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+
+  new Worker("clearTimeouts_worker.js").onmessage = function(event) {
+    event.target.terminate();
+
+    is(event.data, "ready", "Correct message");
+    setTimeout(function() { SimpleTest.finish(); }, 1000);
+  }
+
+  SimpleTest.waitForExplicitFinish();
+
+</script>
+</pre>
+</body>
+</html>