Bug 958011: Fix worker object cycle collection to not rely on the JSObject being finalized, which is a bogus assumption. r=bent,mccr8

dom/bindings/Bindings.conf

@@ -27,11 +27,6 @@
 #   * workers - Indicates whether the descriptor is intended to be used solely
 #               for worker threads (defaults to false). If true the interface
 #               will not be made available on the main thread.
-#   * customFinalize - The native class will use a custom finalize hook
-#                      (defaults to true for workers, false otherwise).
-#   * customWrapperManagement - The native class will be responsible for
-#                               preserving its own wrapper (no AddProperty
-#                               hook will be generated, defaults to false).
 #   * notflattened - The native type does not have nsIClassInfo, so when
 #                    wrapping it the right IID needs to be passed in.
 #   * register - True if this binding should be registered.  Defaults to true.
@@ -221,8 +216,6 @@ DOMInterfaces = {
 'ChromeWorker': {
     'headerFile': 'mozilla/dom/WorkerPrivate.h',
     'nativeType': 'mozilla::dom::workers::ChromeWorkerPrivate',
-    'customFinalize': True,
-    'customWrapperManagement': True,
 },
 
 'DOMRectList': {
@@ -1526,8 +1519,6 @@ DOMInterfaces = {
     'implicitJSContext': [
         'terminate',
     ],
-    'customFinalize': True,
-    'customWrapperManagement': True,
 },
 
 'WorkerGlobalScope': {

dom/bindings/Codegen.py

@@ -45,7 +45,7 @@ def isTypeCopyConstructible(type):
 
 def wantsAddProperty(desc):
     return desc.concrete and \
-           desc.wrapperCache and not desc.customWrapperManagement and \
+           desc.wrapperCache and \
            not desc.interface.getExtendedAttribute("Global")
 
 class CGThing():
@@ -1078,18 +1078,15 @@ def DeferredFinalizeSmartPtr(descriptor):
     return smartPtr
 
 def finalizeHook(descriptor, hookName, freeOp):
-    if descriptor.customFinalize:
-        finalize = "self->%s(CastToJSFreeOp(%s));" % (hookName, freeOp)
-    else:
-        finalize = "JSBindingFinalized<%s>::Finalized(self);\n" % descriptor.nativeType
-        if descriptor.wrapperCache:
-            finalize += "ClearWrapper(self, self);\n"
-        if descriptor.interface.getExtendedAttribute('OverrideBuiltins'):
-            finalize += "self->mExpandoAndGeneration.expando = JS::UndefinedValue();\n"
-        if descriptor.interface.getExtendedAttribute("Global"):
-            finalize += "mozilla::dom::FinalizeGlobal(CastToJSFreeOp(%s), obj);\n" % freeOp
-        finalize += ("AddForDeferredFinalization<%s, %s >(self);" %
-            (descriptor.nativeType, DeferredFinalizeSmartPtr(descriptor)))
+    finalize = "JSBindingFinalized<%s>::Finalized(self);\n" % descriptor.nativeType
+    if descriptor.wrapperCache:
+        finalize += "ClearWrapper(self, self);\n"
+    if descriptor.interface.getExtendedAttribute('OverrideBuiltins'):
+        finalize += "self->mExpandoAndGeneration.expando = JS::UndefinedValue();\n"
+    if descriptor.interface.getExtendedAttribute("Global"):
+        finalize += "mozilla::dom::FinalizeGlobal(CastToJSFreeOp(%s), obj);\n" % freeOp
+    finalize += ("AddForDeferredFinalization<%s, %s >(self);" %
+        (descriptor.nativeType, DeferredFinalizeSmartPtr(descriptor)))
     return CGIfWrapper(CGGeneric(finalize), "self")
 
 class CGClassFinalizeHook(CGAbstractClassHook):

dom/bindings/Configuration.py

@@ -368,17 +368,11 @@ class Descriptor(DescriptorProvider):
             raise TypeError("Descriptor for %s has unrecognized value (%s) "
                             "for nativeOwnership" %
                             (self.interface.identifier.name, self.nativeOwnership))
-        self.customFinalize = desc.get('customFinalize', False)
-        self.customWrapperManagement = desc.get('customWrapperManagement', False)
         if desc.get('wantsQI', None) != None:
             self._wantsQI = desc.get('wantsQI', None)
         self.wrapperCache = (not self.interface.isCallback() and
                              (self.nativeOwnership != 'owned' and
                               desc.get('wrapperCache', True)))
-        if self.customWrapperManagement and not self.wrapperCache:
-            raise TypeError("Descriptor for %s has customWrapperManagement "
-                            "but is not wrapperCached." %
-                            (self.interface.identifier.name))
 
         def make_name(name):
             return name + "_workers" if self.workers else name

dom/workers/WorkerPrivate.cpp

@@ -769,14 +769,12 @@ private:
       NS_WARNING("Failed to dispatch, going to leak!");
     }
 
-    mFinishedWorker->Finish(aCx);
-
     RuntimeService* runtime = RuntimeService::GetService();
     NS_ASSERTION(runtime, "This should never be null!");
 
     runtime->UnregisterWorker(aCx, mFinishedWorker);
 
-    mFinishedWorker->Release();
+    mFinishedWorker->ClearSelfRef();
     return true;
   }
 };
@@ -800,14 +798,12 @@ private:
   {
     AssertIsOnMainThread();
 
+    RuntimeService* runtime = RuntimeService::GetService();
+    MOZ_ASSERT(runtime);
+
     AutoSafeJSContext cx;
     JSAutoRequest ar(cx);
 
-    mFinishedWorker->Finish(cx);
-
-    RuntimeService* runtime = RuntimeService::GetService();
-    NS_ASSERTION(runtime, "This should never be null!");
-
     runtime->UnregisterWorker(cx, mFinishedWorker);
 
     nsTArray<nsCOMPtr<nsISupports> > doomed;
@@ -822,8 +818,7 @@ private:
       NS_WARNING("Failed to dispatch, going to leak!");
     }
 
-    mFinishedWorker->Release();
-
+    mFinishedWorker->ClearSelfRef();
     return NS_OK;
   }
 };
@@ -2109,7 +2104,7 @@ WorkerPrivateParent<Derived>::WorkerPrivateParent(
   mMemoryReportCondVar(mMutex, "WorkerPrivateParent Memory Report CondVar"),
   mParent(aParent), mScriptURL(aScriptURL),
   mSharedWorkerName(aSharedWorkerName), mBusyCount(0), mMessagePortSerial(0),
-  mParentStatus(Pending), mRooted(false), mParentSuspended(false),
+  mParentStatus(Pending), mParentSuspended(false),
   mIsChromeWorker(aIsChromeWorker), mMainThreadObjectsForgotten(false),
   mWorkerType(aWorkerType)
 {
@@ -2143,8 +2138,6 @@ WorkerPrivateParent<Derived>::WorkerPrivateParent(
 template <class Derived>
 WorkerPrivateParent<Derived>::~WorkerPrivateParent()
 {
-  MOZ_ASSERT(!mRooted);
-
   DropJSObjects(this);
 }
 
@@ -2158,13 +2151,7 @@ WorkerPrivateParent<Derived>::WrapObject(JSContext* aCx,
 
   AssertIsOnParentThread();
 
-  JS::Rooted<JSObject*> obj(aCx, WorkerBinding::Wrap(aCx, aScope, ParentAsWorkerPrivate()));
-
-  if (mRooted) {
-    PreserveWrapper(this);
-  }
-
-  return obj;
+  return WorkerBinding::Wrap(aCx, aScope, ParentAsWorkerPrivate());
 }
 
 template <class Derived>
@@ -2606,25 +2593,6 @@ WorkerPrivateParent<Derived>::SynchronizeAndResume(
   return true;
 }
 
-template <class Derived>
-void
-WorkerPrivateParent<Derived>::_finalize(JSFreeOp* aFop)
-{
-  AssertIsOnParentThread();
-
-  MOZ_ASSERT(!mRooted);
-
-  ClearWrapper();
-
-  // Ensure that we're held alive across the TerminatePrivate call, and then
-  // release the reference our wrapper held to us.
-  nsRefPtr<WorkerPrivateParent<Derived> > kungFuDeathGrip = dont_AddRef(this);
-
-  if (!TerminatePrivate(nullptr)) {
-    NS_WARNING("Failed to terminate!");
-  }
-}
-
 template <class Derived>
 bool
 WorkerPrivateParent<Derived>::Close(JSContext* aCx)
@@ -2651,14 +2619,11 @@ WorkerPrivateParent<Derived>::ModifyBusyCount(JSContext* aCx, bool aIncrease)
   NS_ASSERTION(aIncrease || mBusyCount, "Mismatched busy count mods!");
 
   if (aIncrease) {
-    if (mBusyCount++ == 0) {
-      Root(true);
-    }
+    mBusyCount++;
     return true;
   }
 
   if (--mBusyCount == 0) {
-    Root(false);
 
     bool shouldCancel;
     {
@@ -2674,32 +2639,6 @@ WorkerPrivateParent<Derived>::ModifyBusyCount(JSContext* aCx, bool aIncrease)
   return true;
 }
 
-template <class Derived>
-void
-WorkerPrivateParent<Derived>::Root(bool aRoot)
-{
-  AssertIsOnParentThread();
-
-  if (aRoot == mRooted) {
-    return;
-  }
-
-  if (aRoot) {
-    NS_ADDREF_THIS();
-    if (GetWrapperPreserveColor()) {
-      PreserveWrapper(this);
-    }
-  }
-  else {
-    if (GetWrapperPreserveColor()) {
-      ReleaseWrapper(this);
-    }
-    NS_RELEASE_THIS();
-  }
-
-  mRooted = aRoot;
-}
-
 template <class Derived>
 void
 WorkerPrivateParent<Derived>::ForgetMainThreadObjects(
@@ -3512,16 +3451,28 @@ NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)
 template <class Derived>
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(WorkerPrivateParent<Derived>,
                                                   nsDOMEventTargetHelper)
-  // Nothing else to traverse
+  tmp->AssertIsOnParentThread();
+
+  // The WorkerPrivate::mSelfRef has a reference to itself, which is really
+  // held by the worker thread.  We traverse this reference if and only if our
+  // busy count is zero and we have not released the main thread reference.
+  // We do not unlink it.  This allows the CC to break cycles involving the
+  // WorkerPrivate and begin shutting it down (which does happen in unlink) but
+  // ensures that the WorkerPrivate won't be deleted before we're done shutting
+  // down the thread.
+
+  if (!tmp->mBusyCount && !tmp->mMainThreadObjectsForgotten) {
+    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSelfRef)
+  }
+
   // The various strong references in LoadInfo are managed manually and cannot
   // be cycle collected.
-  tmp->AssertIsOnParentThread();
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 
 template <class Derived>
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(WorkerPrivateParent<Derived>,
                                                 nsDOMEventTargetHelper)
-  tmp->AssertIsOnParentThread();
+  tmp->Terminate(nullptr);
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 
 template <class Derived>
@@ -3721,10 +3672,7 @@ WorkerPrivate::Constructor(const GlobalObject& aGlobal,
     return nullptr;
   }
 
-  // The worker will be owned by its JSObject (via the reference we return from
-  // this function), but it also needs to be owned by its thread, so AddRef it
-  // again.
-  NS_ADDREF(worker.get());
+  worker->mSelfRef = worker;
 
   return worker.forget();
 }

dom/workers/WorkerPrivate.h

@@ -232,13 +232,17 @@ private:
   uint64_t mBusyCount;
   uint64_t mMessagePortSerial;
   Status mParentStatus;
-  bool mRooted;
   bool mParentSuspended;
   bool mIsChromeWorker;
   bool mMainThreadObjectsForgotten;
   WorkerType mWorkerType;
 
 protected:
+  // The worker is owned by its thread, which is represented here.  This is set
+  // in Construct() and emptied by WorkerFinishedRunnable, and conditionally
+  // traversed by the cycle collector if the busy count is zero.
+  nsRefPtr<WorkerPrivate> mSelfRef;
+
   WorkerPrivateParent(JSContext* aCx, WorkerPrivate* aParent,
                       const nsAString& aScriptURL, bool aIsChromeWorker,
                       WorkerType aWorkerType,
@@ -281,6 +285,14 @@ public:
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(WorkerPrivateParent,
                                                          nsDOMEventTargetHelper)
 
+  void
+  ClearSelfRef()
+  {
+    AssertIsOnParentThread();
+    MOZ_ASSERT(mSelfRef);
+    mSelfRef = nullptr;
+  }
+
   nsresult
   Dispatch(WorkerRunnable* aRunnable)
   {
@@ -329,20 +341,10 @@ public:
   SynchronizeAndResume(JSContext* aCx, nsPIDOMWindow* aWindow,
                        nsIScriptContext* aScriptContext);
 
-  void
-  _finalize(JSFreeOp* aFop);
-
-  void
-  Finish(JSContext* aCx)
-  {
-    Root(false);
-  }
-
   bool
   Terminate(JSContext* aCx)
   {
     AssertIsOnParentThread();
-    Root(false);
     return TerminatePrivate(aCx);
   }
 
@@ -352,9 +354,6 @@ public:
   bool
   ModifyBusyCount(JSContext* aCx, bool aIncrease);
 
-  void
-  Root(bool aRoot);
-
   void
   ForgetMainThreadObjects(nsTArray<nsCOMPtr<nsISupports> >& aDoomed);
 

dom/workers/test/closeOnGC_server.sjs

@@ -8,12 +8,12 @@ function handleRequest(request, response)
   response.setHeader("Cache-Control", "no-cache", false);
 
   if (request.method == "POST") {
-    setState("seenPost", "1");
+    setState("seenPost" + request.queryString, "1");
     return;
   }
 
   if (request.method == "GET") {
-    if (getState("seenPost") == "1") {
+    if (getState("seenPost" + request.queryString) == "1") {
       response.write("closed");
     }
     return;

dom/workers/test/closeOnGC_worker.js

@@ -4,7 +4,7 @@
  */
 onclose = function() {
   var xhr = new XMLHttpRequest();
-  xhr.open("POST", "closeOnGC_server.sjs", false);
+  xhr.open("POST", "closeOnGC_server.sjs" + location.search, false);
   xhr.send();
 };
 

dom/workers/test/test_closeOnGC.html

@@ -15,24 +15,39 @@
 <pre id="test">
 <script class="testbody" type="text/javascript">
 
-  var worker = new Worker("closeOnGC_worker.js");
-  worker.onmessage = function(event) {
-    is(event.data, "ready");
-    worker = null;
+  var count = 0;
+
+  function testWorker(queryString) {
+    ++count;
+    var worker = new Worker("closeOnGC_worker.js?" + queryString);
+    worker.onmessage = function(event) {
+      is(event.data, "ready");
+      worker = null;
+    }
+
+    var interval = setInterval(function() {
+      var xhr = new XMLHttpRequest();
+      xhr.open("GET", "closeOnGC_server.sjs?" + queryString, false);
+      xhr.send();
+      if (xhr.responseText != "closed") {
+        SpecialPowers.gc();
+        return;
+      }
+      clearInterval(interval);
+      ok(true, "xhr correctly closed");
+      if (--count == 0) {
+        SimpleTest.finish();
+      }
+    }, 500);
+
+    return worker;
   }
 
-  var interval = setInterval(function() {
-    var xhr = new XMLHttpRequest();
-    xhr.open("GET", "closeOnGC_server.sjs", false);
-    xhr.send();
-    if (xhr.responseText != "closed") {
-      SpecialPowers.gc();
-      return;
-    }
-    clearInterval(interval);
-    ok(true, "xhr correctly closed");
-    SimpleTest.finish();
-  }, 500);
+  testWorker("white");
+  var worker = testWorker("gray");
+  worker.onerror = function() {};
+  worker.onerror.foo = worker;
+  worker = null;
 
   SimpleTest.waitForExplicitFinish();