Bug 713069 - 'Remove AutoEnterCompartment calls from finalizers and assert'. r=mrbkap.
authorBen Turner <bent.mozilla@gmail.com>
Thu, 29 Dec 2011 13:28:13 -0500
changeset 84834 a895e9432ea4eebaf577c2e6937c7de15385bebc
parent 84833 af2659d377d40e24af4a9a550ae13cad47b6453c
child 84835 883815d2edb2e7104f7c0c7cf1164db083b2b4b8
push id805
push userakeybl@mozilla.com
push dateWed, 01 Feb 2012 18:17:35 +0000
treeherdermozilla-aurora@6fb3bf232436 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs713069
milestone12.0a1
Bug 713069 - 'Remove AutoEnterCompartment calls from finalizers and assert'. r=mrbkap.
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerPrivate.h
dom/workers/test/Makefile.in
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -896,33 +896,39 @@ public:
 
     bool dummy;
     return events::DispatchEventToTarget(aCx, target, event, &dummy);
   }
 };
 
 class NotifyRunnable : public WorkerControlRunnable
 {
+  bool mFromJSObjectFinalizer;
   Status mStatus;
 
 public:
-  NotifyRunnable(WorkerPrivate* aWorkerPrivate, Status aStatus)
+  NotifyRunnable(WorkerPrivate* aWorkerPrivate, bool aFromJSObjectFinalizer,
+                 Status aStatus)
   : WorkerControlRunnable(aWorkerPrivate, WorkerThread, UnchangedBusyCount),
-    mStatus(aStatus)
+    mFromJSObjectFinalizer(aFromJSObjectFinalizer), mStatus(aStatus)
   {
     NS_ASSERTION(aStatus == Terminating || aStatus == Canceling ||
                  aStatus == Killing, "Bad status!");
   }
 
   bool
   PreDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
   {
     // Modify here, but not in PostRun! This busy count addition will be matched
-    // by the CloseEventRunnable.
-    return aWorkerPrivate->ModifyBusyCount(aCx, true);
+    // by the CloseEventRunnable. If we're running from a finalizer there is no
+    // need to modify the count because future changes to the busy count will
+    // have no effect.
+    return mFromJSObjectFinalizer ?
+           true :
+           aWorkerPrivate->ModifyBusyCount(aCx, true);
   }
 
   bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
   {
     return aWorkerPrivate->NotifyInternal(aCx, mStatus);
   }
 };
@@ -1784,17 +1790,18 @@ WorkerPrivateParent<Derived>::Start()
     }
   }
 
   return false;
 }
 
 template <class Derived>
 bool
-WorkerPrivateParent<Derived>::Notify(JSContext* aCx, Status aStatus)
+WorkerPrivateParent<Derived>::NotifyPrivate(JSContext* aCx, Status aStatus,
+                                            bool aFromJSObjectFinalizer)
 {
   AssertIsOnParentThread();
 
   bool pending;
   {
     MutexAutoLock lock(mMutex);
 
     if (mParentStatus >= aStatus) {
@@ -1825,18 +1832,19 @@ WorkerPrivateParent<Derived>::Notify(JSC
 
   NS_ASSERTION(aStatus != Terminating || mQueuedRunnables.IsEmpty(),
                "Shouldn't have anything queued!");
 
   // Anything queued will be discarded.
   mQueuedRunnables.Clear();
 
   nsRefPtr<NotifyRunnable> runnable =
-    new NotifyRunnable(ParentAsWorkerPrivate(), aStatus);
-  return runnable->Dispatch(aCx);
+    new NotifyRunnable(ParentAsWorkerPrivate(), aFromJSObjectFinalizer,
+                       aStatus);
+  return runnable->Dispatch(aFromJSObjectFinalizer ? nsnull : aCx);
 }
 
 template <class Derived>
 bool
 WorkerPrivateParent<Derived>::Suspend(JSContext* aCx)
 {
   AssertIsOnParentThread();
   NS_ASSERTION(!mParentSuspended, "Suspended more than once!");
@@ -1901,33 +1909,34 @@ WorkerPrivateParent<Derived>::Resume(JSC
 template <class Derived>
 void
 WorkerPrivateParent<Derived>::FinalizeInstance(JSContext* aCx,
                                                bool aFromJSFinalizer)
 {
   AssertIsOnParentThread();
 
   if (mJSObject) {
-    // Make sure we're in the right compartment.
+    // Make sure we're in the right compartment, but only enter one if this is
+    // not running from a finalizer.
     JSAutoEnterCompartment ac;
-    if (!ac.enter(aCx, mJSObject)) {
+    if (!aFromJSFinalizer && !ac.enter(aCx, mJSObject)) {
       NS_ERROR("How can this fail?!");
       return;
     }
 
     // Decouple the object from the private now.
     worker::ClearPrivateSlot(aCx, mJSObject, !aFromJSFinalizer);
 
     // Clear the JS object.
     mJSObject = nsnull;
 
     // Unroot.
     RootJSObject(aCx, false);
 
-    if (!Terminate(aCx)) {
+    if (!TerminatePrivate(aCx, aFromJSFinalizer)) {
       NS_WARNING("Failed to terminate!");
     }
 
     events::EventTarget::FinalizeInstance(aCx);
   }
 }
 
 template <class Derived>
--- a/dom/workers/WorkerPrivate.h
+++ b/dom/workers/WorkerPrivate.h
@@ -236,24 +236,36 @@ protected:
 
 private:
   Derived*
   ParentAsWorkerPrivate() const
   {
     return static_cast<Derived*>(const_cast<WorkerPrivateParent*>(this));
   }
 
+  bool
+  NotifyPrivate(JSContext* aCx, Status aStatus, bool aFromJSFinalizer);
+
+  bool
+  TerminatePrivate(JSContext* aCx, bool aFromJSFinalizer)
+  {
+    return NotifyPrivate(aCx, Terminating, aFromJSFinalizer);
+  }
+
 public:
   // May be called on any thread...
   bool
   Start();
 
   // Called on the parent thread.
   bool
-  Notify(JSContext* aCx, Status aStatus);
+  Notify(JSContext* aCx, Status aStatus)
+  {
+    return NotifyPrivate(aCx, aStatus, false);
+  }
 
   bool
   Cancel(JSContext* aCx)
   {
     return Notify(aCx, Canceling);
   }
 
   bool
@@ -278,17 +290,17 @@ public:
   }
 
   void
   FinalizeInstance(JSContext* aCx, bool aFromJSFinalizer);
 
   bool
   Terminate(JSContext* aCx)
   {
-    return Notify(aCx, Terminating);
+    return TerminatePrivate(aCx, false);
   }
 
   bool
   Close(JSContext* aCx);
 
   bool
   ModifyBusyCount(JSContext* aCx, bool aIncrease);
 
--- a/dom/workers/test/Makefile.in
+++ b/dom/workers/test/Makefile.in
@@ -53,16 +53,19 @@ include $(topsrcdir)/config/rules.mk
 
 _TEST_FILES = \
   test_404.html \
   test_atob.html \
   atob_worker.js \
   test_blobWorkers.html \
   test_close.html \
   close_worker.js \
+  test_closeOnGC.html \
+  closeOnGC_worker.js \
+  closeOnGC_server.sjs \
   test_dataURLWorker.html \
   test_errorPropagation.html \
   errorPropagation_iframe.html \
   errorPropagation_worker.js \
   test_eventDispatch.html \
   eventDispatch_worker.js \
   test_importScripts.html \
   importScripts_worker.js \
@@ -153,24 +156,16 @@ include $(topsrcdir)/config/rules.mk
   WorkerTest_subworker.js \
   chromeWorker_worker.js \
   chromeWorker_subworker.js \
   test_workersDisabled.xul \
   workersDisabled_worker.js \
   dom_worker_helper.js \
   $(NULL)
 
-ifneq ($(OS_ARCH),WINNT)
-_TEST_FILES += \
-  test_closeOnGC.html \
-  closeOnGC_worker.js \
-  closeOnGC_server.sjs \
-  $(NULL)
-endif
-
 libs:: $(_TEST_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)
 
 libs:: $(_SUBDIR_TEST_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)/subdir
 
 libs:: $(_CHROME_TEST_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir)