Bug 911864 - Install XBL members in the safe scope, then clone them into content (rather than the reverse). r=smaug
authorBobby Holley <bobbyholley@gmail.com>
Fri, 01 Nov 2013 15:31:57 +0100
changeset 153062 720546f7c02ddd42c234c58b07df9ede68c7a6e0
parent 153061 aad5b909924dbd9d069c1b91c9613c100284af1a
child 153063 fa25d7921c33ecd72f5f5ad2484693508d2d5bd4
push id25566
push userryanvm@gmail.com
push dateFri, 01 Nov 2013 18:40:05 +0000
treeherdermozilla-central@5bb07c1ae9f5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs911864
milestone28.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 911864 - Install XBL members in the safe scope, then clone them into content (rather than the reverse). r=smaug
content/xbl/src/nsXBLProtoImpl.cpp
content/xbl/src/nsXBLProtoImplMethod.cpp
content/xbl/src/nsXBLProtoImplProperty.cpp
js/xpconnect/src/XPCJSRuntime.cpp
js/xpconnect/src/xpcpublic.h
--- a/content/xbl/src/nsXBLProtoImpl.cpp
+++ b/content/xbl/src/nsXBLProtoImpl.cpp
@@ -17,16 +17,18 @@
 #include "nsIDOMNode.h"
 #include "nsXBLPrototypeBinding.h"
 #include "nsXBLProtoImplProperty.h"
 #include "nsIURI.h"
 #include "mozilla/dom/XULElementBinding.h"
 #include "xpcpublic.h"
 
 using namespace mozilla;
+using js::GetGlobalForObjectCrossCompartment;
+using js::AssertSameCompartment;
 
 nsresult
 nsXBLProtoImpl::InstallImplementation(nsXBLPrototypeBinding* aPrototypeBinding,
                                       nsXBLBinding* aBinding)
 {
   // This function is called to install a concrete implementation on a bound element using
   // this prototype implementation as a guide.  The prototype implementation is compiled lazily,
   // so for the first bound element that needs a concrete implementation, we also build the
@@ -64,59 +66,68 @@ nsXBLProtoImpl::InstallImplementation(ns
   aBinding->SetJSClass(nsXBLJSClass::fromJSClass(JS_GetClass(targetClassObject)));
 
   // If the prototype already existed, we don't need to install anything. return early.
   if (!targetObjectIsNew)
     return NS_OK;
 
   JS::Rooted<JSObject*> targetScriptObject(cx, holder->GetJSObject());
 
-  JSAutoCompartment ac(cx, targetClassObject);
+  // We want to define the canonical set of members in a safe place. If we're
+  // using a separate XBL scope, we want to define them there first (so that
+  // they'll be available for Xray lookups, among other things), and then copy
+  // the properties to the content-side prototype as needed. We don't need to
+  // bother about the field accessors here, since we don't use/support those
+  // for in-content bindings.
 
-  // Walk our member list and install each one in turn.
-  for (nsXBLProtoImplMember* curr = mMembers;
-       curr;
-       curr = curr->GetNext())
-    curr->InstallMember(cx, targetClassObject);
-
-  // If we're using a separate XBL scope, make a safe copy of the target class
-  // object in the XBL scope that we can use for Xray lookups. We don't need
-  // the field accessors, so do this before installing them.
+  // First, start by entering the compartment of the XBL scope. This may or may
+  // not be the same compartment as globalObject.
   JS::Rooted<JSObject*> globalObject(cx,
-    JS_GetGlobalForObject(cx, targetClassObject));
+    GetGlobalForObjectCrossCompartment(targetClassObject));
   JS::Rooted<JSObject*> scopeObject(cx, xpc::GetXBLScope(cx, globalObject));
   NS_ENSURE_TRUE(scopeObject, NS_ERROR_OUT_OF_MEMORY);
-  if (scopeObject != globalObject) {
-    JSAutoCompartment ac2(cx, scopeObject);
+  JSAutoCompartment ac(cx, scopeObject);
 
-    // Create the object. This is just a property holder, so it doesn't need
-    // any special JSClass.
-    JS::Rooted<JSObject*> shadowProto(cx,
-      JS_NewObjectWithGivenProto(cx, nullptr, nullptr, scopeObject));
-    NS_ENSURE_TRUE(shadowProto, NS_ERROR_OUT_OF_MEMORY);
+  // If they're different, create our safe holder object in the XBL scope.
+  JS::RootedObject propertyHolder(cx);
+  if (scopeObject != globalObject) {
+
+    // This is just a property holder, so it doesn't need any special JSClass.
+    propertyHolder = JS_NewObjectWithGivenProto(cx, nullptr, nullptr, scopeObject);
+    NS_ENSURE_TRUE(propertyHolder, NS_ERROR_OUT_OF_MEMORY);
 
     // Define it as a property on the scopeObject, using the same name used on
     // the content side.
     bool ok = JS_DefineProperty(cx, scopeObject,
                                 js::GetObjectClass(targetClassObject)->name,
-                                JS::ObjectValue(*shadowProto), JS_PropertyStub,
+                                JS::ObjectValue(*propertyHolder), JS_PropertyStub,
                                 JS_StrictPropertyStub,
                                 JSPROP_PERMANENT | JSPROP_READONLY);
     NS_ENSURE_TRUE(ok, NS_ERROR_UNEXPECTED);
+  } else {
+    propertyHolder = targetClassObject;
+  }
 
-    // Copy all the properties from the content-visible prototype to the shadow
-    // object. This rewraps them appropriately, which should result in vanilla
-    // functions, since the properties on the content prototype were cross-
-    // compartment wrappers.
-    ok = JS_CopyPropertiesFrom(cx, shadowProto, targetClassObject);
-    NS_ENSURE_TRUE(ok, NS_ERROR_UNEXPECTED);
+  // Walk our member list and install each one in turn on the XBL scope object.
+  for (nsXBLProtoImplMember* curr = mMembers;
+       curr;
+       curr = curr->GetNext())
+    curr->InstallMember(cx, propertyHolder);
+
+  // From here on out, work in the scope of the bound element.
+  JSAutoCompartment ac2(cx, targetClassObject);
 
-    // Content shouldn't have any way to touch this object, but freeze it just
-    // to be safe.
-    ok = JS_FreezeObject(cx, shadowProto);
+  // Now, if we're using a separate XBL scope, enter the compartment of the
+  // bound node and copy the properties to the prototype there. This rewraps
+  // them appropriately, which should result in cross-compartment function
+  // wrappers.
+  if (propertyHolder != targetClassObject) {
+    AssertSameCompartment(propertyHolder, scopeObject);
+    AssertSameCompartment(targetClassObject, globalObject);
+    bool ok = JS_CopyPropertiesFrom(cx, targetClassObject, propertyHolder);
     NS_ENSURE_TRUE(ok, NS_ERROR_UNEXPECTED);
   }
 
   // Install all of our field accessors.
   for (nsXBLProtoImplField* curr = mFields;
        curr;
        curr = curr->GetNext())
     curr->InstallAccessors(cx, targetClassObject);
--- a/content/xbl/src/nsXBLProtoImplMethod.cpp
+++ b/content/xbl/src/nsXBLProtoImplMethod.cpp
@@ -99,35 +99,25 @@ nsresult
 nsXBLProtoImplMethod::InstallMember(JSContext* aCx,
                                     JS::Handle<JSObject*> aTargetClassObject)
 {
   NS_PRECONDITION(IsCompiled(),
                   "Should not be installing an uncompiled method");
   MOZ_ASSERT(js::IsObjectInContextCompartment(aTargetClassObject, aCx));
 
   JS::Rooted<JSObject*> globalObject(aCx, JS_GetGlobalForObject(aCx, aTargetClassObject));
-  JS::Rooted<JSObject*> scopeObject(aCx, xpc::GetXBLScope(aCx, globalObject));
-  NS_ENSURE_TRUE(scopeObject, NS_ERROR_OUT_OF_MEMORY);
+  MOZ_ASSERT(xpc::IsInXBLScope(globalObject) ||
+             globalObject == xpc::GetXBLScope(aCx, globalObject));
 
   JS::Rooted<JSObject*> jsMethodObject(aCx, GetCompiledMethod());
   if (jsMethodObject) {
     nsDependentString name(mName);
 
-    // First, make the function in the compartment of the scope object.
-    JSAutoCompartment ac(aCx, scopeObject);
-    JS::Rooted<JSObject*> method(aCx, ::JS_CloneFunctionObject(aCx, jsMethodObject, scopeObject));
-    if (!method) {
-      return NS_ERROR_OUT_OF_MEMORY;
-    }
-
-    // Then, enter the content compartment, wrap the method pointer, and define
-    // the wrapped version on the class object.
-    JSAutoCompartment ac2(aCx, aTargetClassObject);
-    if (!JS_WrapObject(aCx, &method))
-      return NS_ERROR_OUT_OF_MEMORY;
+    JS::Rooted<JSObject*> method(aCx, JS_CloneFunctionObject(aCx, jsMethodObject, globalObject));
+    NS_ENSURE_TRUE(method, NS_ERROR_OUT_OF_MEMORY);
 
     JS::Rooted<JS::Value> value(aCx, JS::ObjectValue(*method));
     if (!::JS_DefineUCProperty(aCx, aTargetClassObject,
                                static_cast<const jschar*>(mName),
                                name.Length(), value,
                                nullptr, nullptr, JSPROP_ENUMERATE)) {
       return NS_ERROR_OUT_OF_MEMORY;
     }
--- a/content/xbl/src/nsXBLProtoImplProperty.cpp
+++ b/content/xbl/src/nsXBLProtoImplProperty.cpp
@@ -124,42 +124,34 @@ nsresult
 nsXBLProtoImplProperty::InstallMember(JSContext *aCx,
                                       JS::Handle<JSObject*> aTargetClassObject)
 {
   NS_PRECONDITION(mIsCompiled,
                   "Should not be installing an uncompiled property");
   MOZ_ASSERT(mGetter.IsCompiled() && mSetter.IsCompiled());
   MOZ_ASSERT(js::IsObjectInContextCompartment(aTargetClassObject, aCx));
   JS::Rooted<JSObject*> globalObject(aCx, JS_GetGlobalForObject(aCx, aTargetClassObject));
-  JS::Rooted<JSObject*> scopeObject(aCx, xpc::GetXBLScope(aCx, globalObject));
-  NS_ENSURE_TRUE(scopeObject, NS_ERROR_OUT_OF_MEMORY);
+  MOZ_ASSERT(xpc::IsInXBLScope(globalObject) ||
+             globalObject == xpc::GetXBLScope(aCx, globalObject));
 
   if (mGetter.GetJSFunction() || mSetter.GetJSFunction()) {
-    // First, enter the compartment of the scope object and clone the functions.
-    JSAutoCompartment ac(aCx, scopeObject);
-
     JS::Rooted<JSObject*> getter(aCx, nullptr);
     if (mGetter.GetJSFunction()) {
-      if (!(getter = ::JS_CloneFunctionObject(aCx, mGetter.GetJSFunction(), scopeObject)))
+      if (!(getter = ::JS_CloneFunctionObject(aCx, mGetter.GetJSFunction(), globalObject)))
         return NS_ERROR_OUT_OF_MEMORY;
     }
 
     JS::Rooted<JSObject*> setter(aCx, nullptr);
     if (mSetter.GetJSFunction()) {
-      if (!(setter = ::JS_CloneFunctionObject(aCx, mSetter.GetJSFunction(), scopeObject)))
+      if (!(setter = ::JS_CloneFunctionObject(aCx, mSetter.GetJSFunction(), globalObject)))
         return NS_ERROR_OUT_OF_MEMORY;
     }
 
-    // Now, enter the content compartment, wrap the getter/setter, and define
-    // them on the class object.
-    JSAutoCompartment ac2(aCx, aTargetClassObject);
     nsDependentString name(mName);
-    if (!JS_WrapObject(aCx, &getter) ||
-        !JS_WrapObject(aCx, &setter) ||
-        !::JS_DefineUCProperty(aCx, aTargetClassObject,
+    if (!::JS_DefineUCProperty(aCx, aTargetClassObject,
                                static_cast<const jschar*>(mName),
                                name.Length(), JSVAL_VOID,
                                JS_DATA_TO_FUNC_PTR(JSPropertyOp, getter.get()),
                                JS_DATA_TO_FUNC_PTR(JSStrictPropertyOp, setter.get()),
                                mJSAttributes))
       return NS_ERROR_OUT_OF_MEMORY;
   }
   return NS_OK;
--- a/js/xpconnect/src/XPCJSRuntime.cpp
+++ b/js/xpconnect/src/XPCJSRuntime.cpp
@@ -416,16 +416,22 @@ IsXBLScope(JSCompartment *compartment)
     // We always eagerly create compartment privates for XBL scopes.
     CompartmentPrivate *priv = GetCompartmentPrivate(compartment);
     if (!priv || !priv->scope)
         return false;
     return priv->scope->IsXBLScope();
 }
 
 bool
+IsInXBLScope(JSObject *obj)
+{
+    return IsXBLScope(js::GetObjectCompartment(obj));
+}
+
+bool
 IsUniversalXPConnectEnabled(JSCompartment *compartment)
 {
     CompartmentPrivate *priv = GetCompartmentPrivate(compartment);
     if (!priv)
         return false;
     return priv->universalXPConnectEnabled;
 }
 
--- a/js/xpconnect/src/xpcpublic.h
+++ b/js/xpconnect/src/xpcpublic.h
@@ -261,16 +261,17 @@ bool StringToJsval(JSContext* cx, mozill
         return true;
     }
     return NonVoidStringToJsval(cx, str, rval);
 }
 
 nsIPrincipal *GetCompartmentPrincipal(JSCompartment *compartment);
 
 bool IsXBLScope(JSCompartment *compartment);
+bool IsInXBLScope(JSObject *obj);
 
 void SetLocationForGlobal(JSObject *global, const nsACString& location);
 void SetLocationForGlobal(JSObject *global, nsIURI *locationURI);
 
 // ReportJSRuntimeExplicitTreeStats will expect this in the |extra| member
 // of JS::ZoneStats.
 class ZoneStatsExtras {
 public: