Bug 726241: XHR should not double AddFeature itself. r=bent

dom/workers/XMLHttpRequestPrivate.cpp

@@ -349,8 +349,7 @@ class LoadStartDetectionRunnable : public nsIRunnable,
         aWorkerPrivate->StopSyncLoop(mSyncQueueKey, true);
       }
 
-      mXMLHttpRequestPrivate->UnrootJSObject(aCx);
-      aWorkerPrivate->RemoveFeature(aCx, mXMLHttpRequestPrivate);
+      mXMLHttpRequestPrivate->Unpin(aCx);
 
       return true;
     }
@@ -1183,6 +1182,29 @@ public:
   }
 };
 
+class AutoUnpinXHR {
+public:
+  AutoUnpinXHR(XMLHttpRequestPrivate* aXMLHttpRequestPrivate,
+               JSContext* aCx)
+  :  mXMLHttpRequestPrivate(aXMLHttpRequestPrivate), mCx(aCx)
+  { }
+
+  ~AutoUnpinXHR()
+  {
+    if (mXMLHttpRequestPrivate) {
+      mXMLHttpRequestPrivate->Unpin(mCx);
+    }
+  }
+
+  void Clear()
+  {
+    mXMLHttpRequestPrivate = nsnull;
+  }
+private:
+  XMLHttpRequestPrivate* mXMLHttpRequestPrivate;
+  JSContext* mCx;
+};
+
 } // anonymous namespace
 
 void
@@ -1346,7 +1368,7 @@ XMLHttpRequestPrivate::ReleaseProxy()
 }
 
 bool
-XMLHttpRequestPrivate::RootJSObject(JSContext* aCx)
+XMLHttpRequestPrivate::Pin(JSContext* aCx)
 {
   mWorkerPrivate->AssertIsOnWorkerThread();
 
@@ -1355,6 +1377,12 @@ XMLHttpRequestPrivate::RootJSObject(JSContext* aCx)
                                "XMLHttpRequestPrivate mJSObject")) {
       return false;
     }
+    if (!mWorkerPrivate->AddFeature(aCx, this)) {
+      if (!JS_RemoveObjectRoot(aCx, &mJSObject)) {
+        NS_ERROR("JS_RemoveObjectRoot failed!");
+      }
+      return false;
+    }
   }
 
   mJSObjectRootCount++;
@@ -1362,16 +1390,22 @@ XMLHttpRequestPrivate::RootJSObject(JSContext* aCx)
 }
 
 void
-XMLHttpRequestPrivate::UnrootJSObject(JSContext* aCx)
+XMLHttpRequestPrivate::Unpin(JSContext* aCx)
 {
   mWorkerPrivate->AssertIsOnWorkerThread();
 
-  NS_ASSERTION(mJSObjectRootCount, "Mismatched calls to UnrootJSObject!");
+  NS_ASSERTION(mJSObjectRootCount, "Mismatched calls to Unpin!");
   mJSObjectRootCount--;
 
-  if (!mJSObjectRootCount && !JS_RemoveObjectRoot(aCx, &mJSObject)) {
+  if (mJSObjectRootCount) {
+    return;
+  }
+
+  if (!JS_RemoveObjectRoot(aCx, &mJSObject)) {
     NS_ERROR("JS_RemoveObjectRoot failed!");
   }
+
+  mWorkerPrivate->RemoveFeature(aCx, this);
 }
 
 bool
@@ -1704,14 +1738,11 @@ XMLHttpRequestPrivate::Send(JSContext* aCx, bool aHasBody, jsval aBody)
     hasUploadListeners = target->HasListeners();
   }
 
-  if (!RootJSObject(aCx)) {
+  if (!Pin(aCx)) {
     return false;
   }
 
-  if (!mWorkerPrivate->AddFeature(aCx, this)) {
-    UnrootJSObject(aCx);
-    return false;
-  }
+  AutoUnpinXHR autoUnpin(this, aCx);
 
   PRUint32 syncQueueKey = PR_UINT32_MAX;
   if (mProxy->mIsSyncXHR) {
@@ -1722,11 +1753,11 @@ XMLHttpRequestPrivate::Send(JSContext* aCx, bool aHasBody, jsval aBody)
     new SendRunnable(mWorkerPrivate, mProxy, buffer, syncQueueKey,
                      hasUploadListeners);
   if (!runnable->Dispatch(aCx)) {
-    UnrootJSObject(aCx);
-    mWorkerPrivate->RemoveFeature(aCx, this);
     return false;
   }
 
+  autoUnpin.Clear();
+
   // The event loop was spun above, make sure we aren't canceled already.
   if (mCanceled) {
     return false;
@@ -1764,10 +1795,12 @@ XMLHttpRequestPrivate::SendAsBinary(JSContext* aCx, JSString* aBody)
     hasUploadListeners = target->HasListeners();
   }
 
-  if (!RootJSObject(aCx)) {
+  if (!Pin(aCx)) {
     return false;
   }
 
+  AutoUnpinXHR autoUnpin(this, aCx);
+
   PRUint32 syncQueueKey = PR_UINT32_MAX;
   if (mProxy->mIsSyncXHR) {
     syncQueueKey = mWorkerPrivate->CreateNewSyncLoop();
@@ -1777,10 +1810,11 @@ XMLHttpRequestPrivate::SendAsBinary(JSContext* aCx, JSString* aBody)
     new SendAsBinaryRunnable(mWorkerPrivate, mProxy, body, syncQueueKey,
                              hasUploadListeners);
   if (!runnable->Dispatch(aCx)) {
-    UnrootJSObject(aCx);
     return false;
   }
 
+  autoUnpin.Clear();
+
   // The event loop was spun above, make sure we aren't canceled already.
   if (mCanceled) {
     return false;

dom/workers/XMLHttpRequestPrivate.h

@@ -80,7 +80,7 @@ public:
   }
 
   void
-  UnrootJSObject(JSContext* aCx);
+  Unpin(JSContext* aCx);
 
   JSObject*
   GetJSObject()
@@ -152,7 +152,7 @@ private:
   ReleaseProxy();
 
   bool
-  RootJSObject(JSContext* aCx);
+  Pin(JSContext* aCx);
 
   bool
   MaybeDispatchPrematureAbortEvents(JSContext* aCx);

dom/workers/test/Makefile.in

@@ -120,6 +120,8 @@ _TEST_FILES = \
   WorkerTest_badworker.js \
   test_workersDisabled.html \
   workersDisabled_worker.js \
+  test_xhr_implicit_cancel.html \
+  xhr_implicit_cancel_worker.js \
   $(NULL)
 
 _SUBDIR_TEST_FILES = \

dom/workers/test/test_xhr_implicit_cancel.html

@@ -0,0 +1,44 @@
+<!--
+  Any copyright is dedicated to the Public Domain.
+  http://creativecommons.org/publicdomain/zero/1.0/
+-->
+<!DOCTYPE HTML>
+<html>
+<!--
+Tests of DOM Worker Threads XHR(Bug 450452 )
+-->
+<head>
+  <title>Test for DOM Worker Threads XHR (Bug 450452 )</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=450452">DOM Worker Threads XHR (Bug 450452)</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+
+  var worker = new Worker("xhr_implicit_cancel_worker.js");
+
+  worker.onmessage = function(event) {
+    is(event.target, worker);
+    ok(true, "Worker didn't have an error");
+    SimpleTest.finish();
+  };
+
+  worker.onerror = function(event) {
+    is(event.target, worker);
+    ok(false, "Worker had an error:" + event.data);
+    SimpleTest.finish();
+  }
+
+  SimpleTest.waitForExplicitFinish();
+
+</script>
+</pre>
+</body>
+</html>
+

dom/workers/test/xhr_implicit_cancel_worker.js

@@ -0,0 +1,10 @@
+/**
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+var xhr = new XMLHttpRequest();
+xhr.open("GET", "testXHR.txt");
+xhr.send(null);
+xhr.open("GET", "testXHR.txt");
+xhr.send(null);
+postMessage("done");