From f99b55852b0a08d57fcb5c0ea1cd04f2202416f6 Mon Sep 17 00:00:00 2001
From: Ben Turner <bent.mozilla@gmail.com>
Date: Mon, 26 Mar 2012 21:05:09 -0700
Subject: [PATCH] Bug 720679 - 'Crash @ WorkerPrivate::CancelAllTimeouts while
 closing Firefox'. r=khuey.

---
 dom/workers/WorkerPrivate.cpp            | 34 +++++++++++++++++++-----
 dom/workers/test/Makefile.in             |  2 ++
 dom/workers/test/clearTimeouts_worker.js | 12 +++++++++
 dom/workers/test/test_clearTimeouts.html | 30 +++++++++++++++++++++
 4 files changed, 71 insertions(+), 7 deletions(-)
 create mode 100644 dom/workers/test/clearTimeouts_worker.js
 create mode 100644 dom/workers/test/test_clearTimeouts.html

diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp
index 934ba10a73c..667db0b560d 100644
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -3164,6 +3164,8 @@ WorkerPrivate::NotifyFeatures(JSContext* aCx, Status aStatus)
 void
 WorkerPrivate::CancelAllTimeouts(JSContext* aCx)
 {
+  AssertIsOnWorkerThread();
+
   if (mTimerRunning) {
     NS_ASSERTION(mTimer, "Huh?!");
     NS_ASSERTION(!mTimeouts.IsEmpty(), "Huh?!");
@@ -3176,13 +3178,19 @@ WorkerPrivate::CancelAllTimeouts(JSContext* aCx)
       mTimeouts[index]->mCanceled = true;
     }
 
-    RunExpiredTimeouts(aCx);
+    if (!RunExpiredTimeouts(aCx)) {
+      JS_ReportPendingException(aCx);
+    }
 
-    mTimer = nsnull;
+    mTimerRunning = false;
   }
-  else {
+#ifdef DEBUG
+  else if (!mRunningExpiredTimeouts) {
     NS_ASSERTION(mTimeouts.IsEmpty(), "Huh?!");
   }
+#endif
+
+  mTimer = nsnull;
 }
 
 PRUint32
@@ -3511,8 +3519,15 @@ WorkerPrivate::SetTimeout(JSContext* aCx, unsigned aArgc, jsval* aVp,
     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;
   }
 
@@ -3655,6 +3670,7 @@ WorkerPrivate::RunExpiredTimeouts(JSContext* aCx)
     return true;
   }
 
+  NS_ASSERTION(mTimer, "Must have a timer!");
   NS_ASSERTION(!mTimeouts.IsEmpty(), "Should have some work to do!");
 
   bool retval = true;
@@ -3719,12 +3735,16 @@ WorkerPrivate::RunExpiredTimeouts(JSContext* aCx)
       }
     }
 
+    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);
 
@@ -3755,8 +3775,8 @@ WorkerPrivate::RunExpiredTimeouts(JSContext* aCx)
     }
   }
 
-  // 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;
diff --git a/dom/workers/test/Makefile.in b/dom/workers/test/Makefile.in
index efe31ede1d3..f577afd6bae 100644
--- a/dom/workers/test/Makefile.in
+++ b/dom/workers/test/Makefile.in
@@ -56,6 +56,8 @@ _TEST_FILES = \
   test_atob.html \
   atob_worker.js \
   test_blobWorkers.html \
+  test_clearTimeouts.html \
+  clearTimeouts_worker.js \
   test_close.html \
   close_worker.js \
   test_closeOnGC.html \
diff --git a/dom/workers/test/clearTimeouts_worker.js b/dom/workers/test/clearTimeouts_worker.js
new file mode 100644
index 00000000000..b471515b3ec
--- /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);
+}
diff --git a/dom/workers/test/test_clearTimeouts.html b/dom/workers/test/test_clearTimeouts.html
new file mode 100644
index 00000000000..5a6e83e5205
--- /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>