Bug 883450 - Re-order stuff in CallSetup so that we construct the RootedObject after the Push. r=bz

dom/bindings/CallbackObject.cpp

@@ -43,8 +43,6 @@ CallbackObject::CallSetup::CallSetup(JS::Handle<JSObject*> aCallback,
   , mErrorResult(aRv)
   , mExceptionHandling(aExceptionHandling)
 {
-  xpc_UnmarkGrayObject(aCallback);
-
   // We need to produce a useful JSContext here.  Ideally one that the callback
   // is in some sense associated with, so that we can sort of treat it as a
   // "script entry point".  Though once we actually have script entry points,
@@ -90,14 +88,20 @@ CallbackObject::CallSetup::CallSetup(JS::Handle<JSObject*> aCallback,
     cx = nsContentUtils::GetSafeJSContext();
   }
 
-  // Go ahead and stick our callable in a Rooted, to make sure it can't go
-  // gray again.  We can do this even though we're not in the right compartment
-  // yet, because Rooted<> does not care about compartments.
-  mRootedCallable.construct(cx, aCallback);
-
   // Make sure our JSContext is pushed on the stack.
   mCxPusher.Push(cx);
 
+  // Unmark the callable, 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.
+  //
+  // We can do this even though we're not in the right compartment yet, because
+  // Rooted<> does not care about compartments.
+  xpc_UnmarkGrayObject(aCallback);
+  mRootedCallable.construct(cx, aCallback);
+
   // After this point we guarantee calling ScriptEvaluated() if we
   // have an nsIScriptContext.
   // XXXbz Why, if, say CheckFunctionAccess fails?  I know that's how

dom/bindings/CallbackObject.h

@@ -145,13 +145,12 @@ protected:
     // is gone
     nsAutoMicroTask mMt;
 
-    // We construct our JS::Rooted right after our JSAutoRequest; let's just
-    // hope that the change in ordering wrt the mCxPusher constructor here is
-    // ok.
-    Maybe<JS::Rooted<JSObject*> > mRootedCallable;
-
     nsCxPusher mCxPusher;
 
+    // Constructed the rooter within the scope of mCxPusher above, so that it's
+    // always within a request during its lifetime.
+    Maybe<JS::Rooted<JSObject*> > mRootedCallable;
+
     // Can't construct a JSAutoCompartment without a JSContext either.  Also,
     // Put mAc after mCxPusher so that we exit the compartment before we pop the
     // JSContext.  Though in practice we'll often manually order those two