From c8872347c90cc1e812462928e18e76ee05b029bb Mon Sep 17 00:00:00 2001
From: Bobby Holley <bobbyholley@gmail.com>
Date: Tue, 17 Sep 2013 09:46:32 -0700
Subject: [PATCH] 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 |  9 +++------
 js/xpconnect/src/nsXPConnect.cpp       | 11 ++++++++---
 js/xpconnect/src/xpcprivate.h          |  2 --
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/js/xpconnect/src/XPCJSContextStack.cpp b/js/xpconnect/src/XPCJSContextStack.cpp
index d9d5fa322bf..476a296b334 100644
--- a/js/xpconnect/src/XPCJSContextStack.cpp
+++ b/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;
 }
diff --git a/js/xpconnect/src/nsXPConnect.cpp b/js/xpconnect/src/nsXPConnect.cpp
index 2398afc344f..e9cb74035c8 100644
--- a/js/xpconnect/src/nsXPConnect.cpp
+++ b/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();
diff --git a/js/xpconnect/src/xpcprivate.h b/js/xpconnect/src/xpcprivate.h
index ecc00090026..70897698c2d 100644
--- a/js/xpconnect/src/xpcprivate.h
+++ b/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;
 };
 
 /***************************************************************************/