Bug 905926 - Be more explicit about GCing twice in the nsXPConnect destructor. r=billm

In the current setup, we'll end up GCing once when destroying the SafeJSContext,
and then once again immediately after in the explicit GC we do before destroying
the XPCJSRuntime. It would be nice to avoid the first GC entirely, but we
currently need both to avoid leaking.

All in all, these patches cause us to GC three times during shutdown, rather
than twice as we did before, because the second GC was rolled together with
the runtime destruction GC when we destroyed the last JSContext. There are a
number of ways to eliminate these, at least in opt builds, but mccr8 thinks
it probably doesn't matter, since there shouldn't be much left in the heap after
the second GC.

We can probably get away with eliminating rambo GC entirely at some point. But
this might become irrelevant for the browser if we end up doing bug 662444.

It would also be interesting to see what, if anything, the rambo GC actually
collects. We might even be able to get away with asserting that all the zones
are gone and removing the GC entirely.

We also take the opportunity to kill mOwnSafeJSContext, which no longer holds
any meaning.

js/xpconnect/src/XPCJSContextStack.cpp

@@ -22,9 +22,9 @@ using mozilla::dom::DestroyProtoAndIfaceCache;
 
 XPCJSContextStack::~XPCJSContextStack()
 {
-    if (mOwnSafeJSContext) {
-        JS_DestroyContext(mOwnSafeJSContext);
-        mOwnSafeJSContext = nullptr;
+    if (mSafeJSContext) {
+        JS_DestroyContextNoGC(mSafeJSContext);
+        mSafeJSContext = nullptr;
     }
 }
 
@@ -181,8 +181,5 @@ XPCJSContextStack::GetSafeJSContext()
 
     JS_FireOnNewGlobalObject(mSafeJSContext, glob);
 
-    // Save it off so we can destroy it later.
-    mOwnSafeJSContext = mSafeJSContext;
-
     return mSafeJSContext;
 }

js/xpconnect/src/nsXPConnect.cpp

@@ -96,11 +96,16 @@ nsXPConnect::nsXPConnect()
 nsXPConnect::~nsXPConnect()
 {
     mRuntime->DeleteJunkScope();
-
-    // This needs to happen exactly here, otherwise we leak at shutdown. I don't
-    // know why. :-(
     mRuntime->DestroyJSContextStack();
 
+    // In order to clean up everything properly, we need to GC twice: once now,
+    // to clean anything that can go away on its own (like the Junk Scope, which
+    // we unrooted above), and once after forcing a bunch of shutdown in
+    // XPConnect, to clean the stuff we forcibly disconnected. The forced
+    // shutdown code defaults to leaking in a number of situations, so we can't
+    // get by with only the second GC. :-(
+    JS_GC(mRuntime->Runtime());
+
     mShuttingDown = true;
     XPCWrappedNativeScope::SystemIsBeingShutDown();
     mRuntime->SystemIsBeingShutDown();

js/xpconnect/src/xpcprivate.h

@@ -3086,7 +3086,6 @@ class XPCJSContextStack
 public:
     XPCJSContextStack()
       : mSafeJSContext(NULL)
-      , mOwnSafeJSContext(NULL)
     { }
 
     virtual ~XPCJSContextStack();
@@ -3119,7 +3118,6 @@ private:
 
     AutoInfallibleTArray<XPCJSContextInfo, 16> mStack;
     JSContext*  mSafeJSContext;
-    JSContext*  mOwnSafeJSContext;
 };
 
 /***************************************************************************/