Bug 883450 - Re-order stuff in CallSetup so that we construct the RootedObject after the Push. r=bz
authorBobby Holley <bobbyholley@gmail.com>
Thu, 20 Jun 2013 11:05:34 -0700
changeset 135852 aae5d429b8d47f1ee92dfd9825e24851e9a1b209
parent 135851 9e6fa71b9f7292274d9a70fffdd0ba93abfd82da
child 135853 dd4b059747b807dbc6a2fcb005709291e4da3177
push id24852
push userryanvm@gmail.com
push dateThu, 20 Jun 2013 23:22:28 +0000
treeherdermozilla-central@b3cbafd5eb99 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs883450
milestone24.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 883450 - Re-order stuff in CallSetup so that we construct the RootedObject after the Push. r=bz
dom/bindings/CallbackObject.cpp
dom/bindings/CallbackObject.h
--- a/dom/bindings/CallbackObject.cpp
+++ b/dom/bindings/CallbackObject.cpp
@@ -38,18 +38,16 @@ NS_IMPL_CYCLE_COLLECTION_TRACE_END
 
 CallbackObject::CallSetup::CallSetup(JS::Handle<JSObject*> aCallback,
                                      ErrorResult& aRv,
                                      ExceptionHandling aExceptionHandling)
   : mCx(nullptr)
   , 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,
   // we'll need to do the script entry point bits once we have an actual
   // callable.
 
   // First, find the real underlying callback.
   JSObject* realCallback = js::UncheckedUnwrap(aCallback);
@@ -85,24 +83,30 @@ CallbackObject::CallSetup::CallSetup(JS:
   }
 
   if (!cx) {
     // We didn't manage to hunt down a script global to work with.  Just fall
     // back on using the safe context.
     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
   // nsJSContext::CallEventHandler used to work, but is it required?
   // FIXME: Bug 807369.
   mCtx = ctx;
 
   // Check that it's ok to run this callback at all.
--- a/dom/bindings/CallbackObject.h
+++ b/dom/bindings/CallbackObject.h
@@ -140,23 +140,22 @@ protected:
     nsCOMPtr<nsIScriptContext> mCtx;
 
     // And now members whose construction/destruction order we need to control.
 
     // Put our nsAutoMicrotask first, so it gets destroyed after everything else
     // 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.
+    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;
 
-    nsCxPusher mCxPusher;
-
     // 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
     // things.
     Maybe<JSAutoCompartment> mAc;
 
     // An ErrorResult to possibly re-throw exceptions on and whether
     // we should re-throw them.