Bug 975419 - Trace the Incumbent Global from a CallbackObject (but check it too, just to be safe). r=bz,mccr8
authorBobby Holley <bobbyholley@gmail.com>
Mon, 03 Mar 2014 08:53:42 -0800
changeset 189797 1742ecba9c09b5456777f025431932263942d5bb
parent 189796 058ed6c240bbd1abbf7518b72a07c26d996e50b8
child 189798 572ba53870303db3c5d7e9d3acb20a49df3d52a3
push id474
push userasasaki@mozilla.com
push dateMon, 02 Jun 2014 21:01:02 +0000
treeherdermozilla-release@967f4cf1b31c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz, mccr8
bugs975419
milestone30.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 975419 - Trace the Incumbent Global from a CallbackObject (but check it too, just to be safe). r=bz,mccr8
dom/bindings/CallbackObject.cpp
dom/bindings/CallbackObject.h
--- a/dom/bindings/CallbackObject.cpp
+++ b/dom/bindings/CallbackObject.cpp
@@ -30,25 +30,26 @@ NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(
 NS_INTERFACE_MAP_END
 
 NS_IMPL_CYCLE_COLLECTING_ADDREF(CallbackObject)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(CallbackObject)
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(CallbackObject)
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(CallbackObject)
-  tmp->DropCallback();
+  tmp->DropJSObjects();
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mIncumbentGlobal)
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(CallbackObject)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIncumbentGlobal)
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(CallbackObject)
   NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mCallback)
+  NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mIncumbentJSGlobal)
 NS_IMPL_CYCLE_COLLECTION_TRACE_END
 
 CallbackObject::CallSetup::CallSetup(CallbackObject* aCallback,
                                      ErrorResult& aRv,
                                      ExceptionHandling aExceptionHandling,
                                      JSCompartment* aCompartment,
                                      bool aIsJSImplementedWebIDL)
   : mCx(nullptr)
@@ -117,18 +118,27 @@ CallbackObject::CallSetup::CallSetup(Cal
     // on gaia-ui tests, probably because nsInProcessTabChildGlobal is returning
     // null in some kind of teardown state.
     if (!globalObject->GetGlobalJSObject()) {
       return;
     }
 
     mAutoEntryScript.construct(globalObject, mIsMainThread, cx);
     mAutoEntryScript.ref().SetWebIDLCallerPrincipal(webIDLCallerPrincipal);
-    if (aCallback->IncumbentGlobalOrNull()) {
-      mAutoIncumbentScript.construct(aCallback->IncumbentGlobalOrNull());
+    nsIGlobalObject* incumbent = aCallback->IncumbentGlobalOrNull();
+    if (incumbent) {
+      // The callback object traces its incumbent JS global, so in general it
+      // should be alive here. However, it's possible that we could run afoul
+      // of the same IPC global weirdness described above, wherein the
+      // nsIGlobalObject has severed its reference to the JS global. Let's just
+      // be safe here, so that nobody has to waste a day debugging gaia-ui tests.
+      if (!incumbent->GetGlobalJSObject()) {
+        return;
+      }
+      mAutoIncumbentScript.construct(incumbent);
     }
 
     // Unmark the callable (by invoking Callback() and not the CallbackPreserveColor()
     // variant), and stick it in a Rooted before it can go gray again.
     // Nothing before us in this function can trigger a CC, so it's safe to wait
     // until here it do the unmark. This allows us to order the following two
     // operations _after_ the Push() above, which lets us take advantage of the
     // JSAutoRequest embedded in the pusher.
--- a/dom/bindings/CallbackObject.h
+++ b/dom/bindings/CallbackObject.h
@@ -52,17 +52,17 @@ public:
   // caller should pass null.
   explicit CallbackObject(JS::Handle<JSObject*> aCallback, nsIGlobalObject *aIncumbentGlobal)
   {
     Init(aCallback, aIncumbentGlobal);
   }
 
   virtual ~CallbackObject()
   {
-    DropCallback();
+    DropJSObjects();
   }
 
   JS::Handle<JSObject*> Callback() const
   {
     JS::ExposeObjectToActiveJS(mCallback);
     return CallbackPreserveColor();
   }
 
@@ -116,38 +116,51 @@ protected:
       js::UncheckedUnwrap(aOther.CallbackPreserveColor());
     return thisObj == otherObj;
   }
 
 private:
   inline void Init(JSObject* aCallback, nsIGlobalObject* aIncumbentGlobal)
   {
     MOZ_ASSERT(aCallback && !mCallback);
-    // Set mCallback before we hold, on the off chance that a GC could somehow
-    // happen in there... (which would be pretty odd, granted).
+    // Set script objects before we hold, on the off chance that a GC could
+    // somehow happen in there... (which would be pretty odd, granted).
     mCallback = aCallback;
+    if (aIncumbentGlobal) {
+      mIncumbentGlobal = aIncumbentGlobal;
+      mIncumbentJSGlobal = aIncumbentGlobal->GetGlobalJSObject();
+    }
     mozilla::HoldJSObjects(this);
-
-    mIncumbentGlobal = aIncumbentGlobal;
   }
 
   CallbackObject(const CallbackObject&) MOZ_DELETE;
   CallbackObject& operator =(const CallbackObject&) MOZ_DELETE;
 
 protected:
-  void DropCallback()
+  void DropJSObjects()
   {
+    MOZ_ASSERT_IF(mIncumbentJSGlobal, mCallback);
     if (mCallback) {
       mCallback = nullptr;
+      mIncumbentJSGlobal = nullptr;
       mozilla::DropJSObjects(this);
     }
   }
 
   JS::Heap<JSObject*> mCallback;
+  // Ideally, we'd just hold a reference to the nsIGlobalObject, since that's
+  // what we need to pass to AutoIncumbentScript. Unfortunately, that doesn't
+  // hold the actual JS global alive. So we maintain an additional pointer to
+  // the JS global itself so that we can trace it.
+  //
+  // At some point we should consider trying to make native globals hold their
+  // scripted global alive, at which point we can get rid of the duplication
+  // here.
   nsCOMPtr<nsIGlobalObject> mIncumbentGlobal;
+  JS::TenuredHeap<JSObject*> mIncumbentJSGlobal;
 
   class MOZ_STACK_CLASS CallSetup
   {
     /**
      * A class that performs whatever setup we need to safely make a
      * call while this class is on the stack, After the constructor
      * returns, the call is safe to make if GetContext() returns
      * non-null.