Bug 911303 - Remove manual Destroy() routine from CycleCollectedJSRuntime. r=billm
authorBobby Holley <bobbyholley@gmail.com>
Wed, 27 Nov 2013 10:10:25 -0800
changeset 172466 50dcaa9d8d1a8791abdf504f6a3bdf805b35cfdb
parent 172465 862e92d55d50c92b6e4e4add441ba2060258f8fb
child 172467 fb6205ce52896ff45d6d459940d9a9e39dade95d
push id3224
push userlsblakk@mozilla.com
push dateTue, 04 Feb 2014 01:06:49 +0000
treeherdermozilla-beta@60c04d0987f1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbillm
bugs911303
milestone28.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 911303 - Remove manual Destroy() routine from CycleCollectedJSRuntime. r=billm
js/xpconnect/src/XPCJSRuntime.cpp
xpcom/base/CycleCollectedJSRuntime.cpp
xpcom/base/CycleCollectedJSRuntime.h
--- a/js/xpconnect/src/XPCJSRuntime.cpp
+++ b/js/xpconnect/src/XPCJSRuntime.cpp
@@ -604,19 +604,18 @@ WindowGlobalOrNull(JSObject *aObj)
     return static_cast<nsGlobalWindow*>(piWin.get());
 }
 
 }
 
 static void
 CompartmentDestroyedCallback(JSFreeOp *fop, JSCompartment *compartment)
 {
-    XPCJSRuntime* self = nsXPConnect::GetRuntimeInstance();
-    if (!self)
-        return;
+    // NB - This callback may be called in JS_DestroyRuntime, which happens
+    // after the XPCJSRuntime has been torn down.
 
     // Get the current compartment private into an AutoPtr (which will do the
     // cleanup for us), and null out the private (which may already be null).
     nsAutoPtr<CompartmentPrivate> priv(GetCompartmentPrivate(compartment));
     JS_SetCompartmentPrivate(compartment, nullptr);
 }
 
 void XPCJSRuntime::TraceNativeBlackRoots(JSTracer* trc)
@@ -1514,16 +1513,23 @@ void XPCJSRuntime::SystemIsBeingShutDown
 
     if (mDetachedWrappedNativeProtoMap)
         mDetachedWrappedNativeProtoMap->
             Enumerate(DetachedWrappedNativeProtoShutdownMarker, nullptr);
 }
 
 XPCJSRuntime::~XPCJSRuntime()
 {
+    // This destructor runs before ~CycleCollectedJSRuntime, which does the
+    // actual JS_DestroyRuntime() call. But destroying the runtime triggers
+    // one final GC, which can call back into the runtime with various
+    // callback if we aren't careful. Null out the relevant callbacks.
+    js::SetActivityCallback(Runtime(), nullptr, nullptr);
+    JS_SetFinalizeCallback(Runtime(), nullptr);
+
     // Clear any pending exception.  It might be an XPCWrappedJS, and if we try
     // to destroy it later we will crash.
     SetPendingException(nullptr);
 
     JS::SetGCSliceCallback(Runtime(), mPrevGCSliceCallback);
 
     xpc_DelocalizeRuntime(Runtime());
 
@@ -1532,51 +1538,62 @@ XPCJSRuntime::~XPCJSRuntime()
 
     if (mCallContext)
         mCallContext->SystemIsBeingShutDown();
 
     auto rtPrivate = static_cast<PerThreadAtomCache*>(JS_GetRuntimePrivate(Runtime()));
     delete rtPrivate;
     JS_SetRuntimePrivate(Runtime(), nullptr);
 
-    // Tell the superclas to destroy the JSRuntime. We need to do this here,
-    // because destroying the runtime runs one final rambo GC, which sometimes
-    // calls various finalizers that assume that XPCJSRuntime is still
-    // operational. We have bug 911303 on file for fixing this.
-    DestroyRuntime();
-
     // clean up and destroy maps...
     if (mWrappedJSMap) {
         mWrappedJSMap->ShutdownMarker();
         delete mWrappedJSMap;
+        mWrappedJSMap = nullptr;
     }
 
-    if (mWrappedJSClassMap)
+    if (mWrappedJSClassMap) {
         delete mWrappedJSClassMap;
-
-    if (mIID2NativeInterfaceMap)
+        mWrappedJSClassMap = nullptr;
+    }
+
+    if (mIID2NativeInterfaceMap) {
         delete mIID2NativeInterfaceMap;
-
-    if (mClassInfo2NativeSetMap)
+        mIID2NativeInterfaceMap = nullptr;
+    }
+
+    if (mClassInfo2NativeSetMap) {
         delete mClassInfo2NativeSetMap;
-
-    if (mNativeSetMap)
+        mClassInfo2NativeSetMap = nullptr;
+    }
+
+    if (mNativeSetMap) {
         delete mNativeSetMap;
-
-    if (mThisTranslatorMap)
+        mNativeSetMap = nullptr;
+    }
+
+    if (mThisTranslatorMap) {
         delete mThisTranslatorMap;
-
-    if (mNativeScriptableSharedMap)
+        mThisTranslatorMap = nullptr;
+    }
+
+    if (mNativeScriptableSharedMap) {
         delete mNativeScriptableSharedMap;
-
-    if (mDyingWrappedNativeProtoMap)
+        mNativeScriptableSharedMap = nullptr;
+    }
+
+    if (mDyingWrappedNativeProtoMap) {
         delete mDyingWrappedNativeProtoMap;
-
-    if (mDetachedWrappedNativeProtoMap)
+        mDyingWrappedNativeProtoMap = nullptr;
+    }
+
+    if (mDetachedWrappedNativeProtoMap) {
         delete mDetachedWrappedNativeProtoMap;
+        mDetachedWrappedNativeProtoMap = nullptr;
+    }
 
 #ifdef MOZ_ENABLE_PROFILER_SPS
     // Tell the profiler that the runtime is gone
     if (PseudoStack *stack = mozilla_get_pseudo_stack())
         stack->sampleRuntime(nullptr);
 #endif
 
 #ifdef DEBUG
--- a/xpcom/base/CycleCollectedJSRuntime.cpp
+++ b/xpcom/base/CycleCollectedJSRuntime.cpp
@@ -453,40 +453,30 @@ CycleCollectedJSRuntime::CycleCollectedJ
   JS_SetGCCallback(mJSRuntime, GCCallback, this);
   JS_SetContextCallback(mJSRuntime, ContextCallback, this);
   JS_SetDestroyZoneCallback(mJSRuntime, XPCStringConvert::FreeZoneCache);
   JS_SetSweepZoneCallback(mJSRuntime, XPCStringConvert::ClearZoneCache);
 
   nsCycleCollector_registerJSRuntime(this);
 }
 
-void
-CycleCollectedJSRuntime::DestroyRuntime()
+CycleCollectedJSRuntime::~CycleCollectedJSRuntime()
 {
-  if (!mJSRuntime) {
-    return;
-  }
-
+  MOZ_ASSERT(mJSRuntime);
   MOZ_ASSERT(!mDeferredFinalizerTable.Count());
   MOZ_ASSERT(!mDeferredSupports.Length());
 
   // Clear mPendingException first, since it might be cycle collected.
   mPendingException = nullptr;
 
   JS_DestroyRuntime(mJSRuntime);
   mJSRuntime = nullptr;
   nsCycleCollector_forgetJSRuntime();
 }
 
-CycleCollectedJSRuntime::~CycleCollectedJSRuntime()
-{
-  // Destroy our runtime if the subclass hasn't done it already.
-  DestroyRuntime();
-}
-
 size_t
 CycleCollectedJSRuntime::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const
 {
   size_t n = 0;
 
   // nullptr for the second arg;  we're not measuring anything hanging off the
   // entries in mJSHolders.
   n += mJSHolders.SizeOfExcludingThis(nullptr, aMallocSizeOf);
--- a/xpcom/base/CycleCollectedJSRuntime.h
+++ b/xpcom/base/CycleCollectedJSRuntime.h
@@ -99,20 +99,16 @@ class CycleCollectedJSRuntime
   friend class JSGCThingParticipant;
   friend class JSZoneParticipant;
   friend class IncrementalFinalizeRunnable;
 protected:
   CycleCollectedJSRuntime(uint32_t aMaxbytes,
                           JSUseHelperThreads aUseHelperThreads);
   virtual ~CycleCollectedJSRuntime();
 
-  // Idempotent. Subclasses may destroy their runtimes earlier in execution if
-  // they so desire.
-  void DestroyRuntime();
-
   size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
   void UnmarkSkippableJSHolders();
 
   virtual void TraverseAdditionalNativeRoots(nsCycleCollectionNoteRootCallback& aCb) {}
   virtual void TraceAdditionalNativeGrayRoots(JSTracer* aTracer) {}
 
   virtual void CustomGCCallback(JSGCStatus aStatus) {}
   virtual bool CustomContextCallback(JSContext* aCx, unsigned aOperation)