Bug 905926 - Be more explicit about GCing twice in the nsXPConnect destructor. r=billm
authorBobby Holley <bobbyholley@gmail.com>
Tue, 17 Sep 2013 09:46:32 -0700
changeset 161334 afa1358c1a8016bf40ce2f6af365d60dec6b6b5e
parent 161333 e78212e4cfa3a3854e92b769ab9dc98b8b766212
child 161335 5ffef86946ed2e72144b7ea2f8b9911dfd76091c
push id3066
push userakeybl@mozilla.com
push dateMon, 09 Dec 2013 19:58:46 +0000
treeherdermozilla-beta@a31a0dce83aa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbillm
bugs905926, 662444
milestone27.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 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
js/xpconnect/src/nsXPConnect.cpp
js/xpconnect/src/xpcprivate.h
--- a/js/xpconnect/src/XPCJSContextStack.cpp
+++ b/js/xpconnect/src/XPCJSContextStack.cpp
@@ -17,19 +17,19 @@ using namespace mozilla;
 using namespace JS;
 using namespace xpc;
 using mozilla::dom::DestroyProtoAndIfaceCache;
 
 /***************************************************************************/
 
 XPCJSContextStack::~XPCJSContextStack()
 {
-    if (mOwnSafeJSContext) {
-        JS_DestroyContext(mOwnSafeJSContext);
-        mOwnSafeJSContext = nullptr;
+    if (mSafeJSContext) {
+        JS_DestroyContextNoGC(mSafeJSContext);
+        mSafeJSContext = nullptr;
     }
 }
 
 JSContext*
 XPCJSContextStack::Pop()
 {
     MOZ_ASSERT(!mStack.IsEmpty());
 
@@ -176,13 +176,10 @@ XPCJSContextStack::GetSafeJSContext()
     // nsIScriptObjectPrincipal ownership is either handled by the
     // nsCOMPtr or dealt with, or we'll release in the finalize
     // hook.
     if (NS_FAILED(xpc->InitClasses(mSafeJSContext, glob)))
         MOZ_CRASH();
 
     JS_FireOnNewGlobalObject(mSafeJSContext, glob);
 
-    // Save it off so we can destroy it later.
-    mOwnSafeJSContext = mSafeJSContext;
-
     return mSafeJSContext;
 }
--- a/js/xpconnect/src/nsXPConnect.cpp
+++ b/js/xpconnect/src/nsXPConnect.cpp
@@ -91,20 +91,25 @@ nsXPConnect::nsXPConnect()
     char* reportableEnv = PR_GetEnv("MOZ_REPORT_ALL_JS_EXCEPTIONS");
     if (reportableEnv && *reportableEnv)
         gReportAllJSExceptions = 1;
 }
 
 nsXPConnect::~nsXPConnect()
 {
     mRuntime->DeleteJunkScope();
+    mRuntime->DestroyJSContextStack();
 
-    // 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();
 
     // The above causes us to clean up a bunch of XPConnect data structures,
     // after which point we need to GC to clean everything up. We need to do
     // this before deleting the XPCJSRuntime, because doing so destroys the
--- a/js/xpconnect/src/xpcprivate.h
+++ b/js/xpconnect/src/xpcprivate.h
@@ -3081,17 +3081,16 @@ void PopJSContextNoScriptContext();
 
 } /* namespace xpc */
 
 class XPCJSContextStack
 {
 public:
     XPCJSContextStack()
       : mSafeJSContext(NULL)
-      , mOwnSafeJSContext(NULL)
     { }
 
     virtual ~XPCJSContextStack();
 
     uint32_t Count()
     {
         return mStack.Length();
     }
@@ -3114,17 +3113,16 @@ private:
 
     // We make these private so that stack manipulation can only happen
     // through one of the above friends.
     JSContext *Pop();
     bool Push(JSContext *cx);
 
     AutoInfallibleTArray<XPCJSContextInfo, 16> mStack;
     JSContext*  mSafeJSContext;
-    JSContext*  mOwnSafeJSContext;
 };
 
 /***************************************************************************/
 // 'Components' object
 
 class nsXPCComponents : public nsIXPCComponents,
                         public nsIXPCScriptable,
                         public nsIClassInfo,