Bug 1013576 - Guard against duplicate property holder creation in XBL. r=billm
authorBobby Holley <bobbyholley@gmail.com>
Thu, 22 May 2014 18:44:03 -0700
changeset 197046 aa8381640f9ead1c05503e107e29e122203a6ddc
parent 197045 3078d2486cab285f624e371716864a5a43700a67
child 197047 e2ee59a9f90f2fe0695275bd023c8f1ba4c35b91
push idunknown
push userunknown
push dateunknown
reviewersbillm
bugs1013576
milestone32.0a1
Bug 1013576 - Guard against duplicate property holder creation in XBL. r=billm
dom/xbl/nsXBLProtoImpl.cpp
dom/xbl/test/file_bug821850.xhtml
--- a/dom/xbl/nsXBLProtoImpl.cpp
+++ b/dom/xbl/nsXBLProtoImpl.cpp
@@ -76,39 +76,59 @@ nsXBLProtoImpl::InstallImplementation(ns
   // 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,
     GetGlobalForObjectCrossCompartment(targetClassObject));
   JS::Rooted<JSObject*> scopeObject(cx, xpc::GetXBLScopeOrGlobal(cx, globalObject));
   NS_ENSURE_TRUE(scopeObject, NS_ERROR_OUT_OF_MEMORY);
   JSAutoCompartment ac(cx, scopeObject);
 
-  // If they're different, create our safe holder object in the XBL scope.
+  // Determine the appropriate property holder.
+  //
+  // Note: If |targetIsNew| is false, we'll early-return above. However, that only
+  // tells us if the content-side object is new, which may be the case even if
+  // we've already set up the binding on the XBL side. For example, if we apply
+  // a binding #foo to a <span> when we've already applied it to a <div>, we'll
+  // end up with a different content prototype, but we'll already have a property
+  // holder called |foo| in the XBL scope. Check for that to avoid wasteful and
+  // weird property holder duplication.
+  const char* className = aPrototypeBinding->ClassName().get();
   JS::Rooted<JSObject*> propertyHolder(cx);
-  if (scopeObject != globalObject) {
+  JS::Rooted<JSPropertyDescriptor> existingHolder(cx);
+  if (scopeObject != globalObject &&
+      !JS_GetOwnPropertyDescriptor(cx, scopeObject, className, &existingHolder)) {
+    return NS_ERROR_FAILURE;
+  }
+  bool propertyHolderIsNew = !existingHolder.object() || !existingHolder.value().isObject();
+
+  if (!propertyHolderIsNew) {
+    propertyHolder = &existingHolder.value().toObject();
+  } else if (scopeObject != globalObject) {
 
     // This is just a property holder, so it doesn't need any special JSClass.
     propertyHolder = JS_NewObjectWithGivenProto(cx, nullptr, JS::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, aPrototypeBinding->ClassName().get(),
-                                propertyHolder, JSPROP_PERMANENT | JSPROP_READONLY,
+    bool ok = JS_DefineProperty(cx, scopeObject, className, propertyHolder,
+                                JSPROP_PERMANENT | JSPROP_READONLY,
                                 JS_PropertyStub, JS_StrictPropertyStub);
     NS_ENSURE_TRUE(ok, NS_ERROR_UNEXPECTED);
   } else {
     propertyHolder = targetClassObject;
   }
 
   // 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);
+  if (propertyHolderIsNew) {
+    for (nsXBLProtoImplMember* curr = mMembers;
+         curr;
+         curr = curr->GetNext())
+      curr->InstallMember(cx, propertyHolder);
+  }
 
   // Now, if we're using a separate XBL scope, enter the compartment of the
   // bound node and copy exposable 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);
--- a/dom/xbl/test/file_bug821850.xhtml
+++ b/dom/xbl/test/file_bug821850.xhtml
@@ -7,17 +7,17 @@ https://bugzilla.mozilla.org/show_bug.cg
     <binding id="testBinding">
       <implementation>
         <constructor>
           // Store a property as an expando on the bound element.
           this._prop = "propVal";
 
           // Wait for both constructors to fire.
           window.constructorCount = (window.constructorCount + 1) || 1;
-          if (window.constructorCount != 2)
+          if (window.constructorCount != 3)
             return;
 
           // Grab some basic infrastructure off the content window.
           var win = XPCNativeWrapper.unwrap(window);
           SpecialPowers = win.SpecialPowers;
           Cu = SpecialPowers.Cu;
           is = win.is;
           ok = win.ok;
@@ -32,16 +32,18 @@ https://bugzilla.mozilla.org/show_bug.cg
           win.functionExpando = function() { return "called" };
           win.functionExpando.prop = 2;
 
           // Make sure we're Xraying.
           ok(Cu.isXrayWrapper(window), "Window is Xrayed");
           ok(Cu.isXrayWrapper(document), "Document is Xrayed");
 
           var bound = document.getElementById('bound');
+          var bound2 = document.getElementById('bound2');
+          var bound3 = document.getElementById('bound3');
           ok(bound, "bound is non-null");
           is(bound.method('baz'), "method:baz", "Xray methods work");
           is(bound.prop, "propVal", "Property Xrays work");
           is(bound.primitiveField, undefined, "Xrays don't show fields");
           is(bound.wrappedJSObject.primitiveField, 2, "Waiving Xrays show fields");
 
           // Check exposure behavior.
           is(typeof bound.unexposedMethod, 'function',
@@ -63,16 +65,23 @@ https://bugzilla.mozilla.org/show_bug.cg
           // Make sure standard constructors work right in the presence of
           // sandboxPrototype and Xray-resolved constructors.
           is(window.Function, XPCNativeWrapper(window.wrappedJSObject.Function),
              "window.Function comes from the window, not the global");
           ok(Function != window.Function, "Function constructors are distinct");
           is(Object.getPrototypeOf(Function.prototype), Object.getPrototypeOf({foo: 42}),
              "Function constructor is local");
 
+          ok(Object.getPrototypeOf(bound.wrappedJSObject) == Object.getPrototypeOf(bound2.wrappedJSObject),
+             "Div and div should have the same content-side prototype");
+          ok(Object.getPrototypeOf(bound.wrappedJSObject) != Object.getPrototypeOf(bound3.wrappedJSObject),
+             "Div and span should have different content-side prototypes");
+          ok(bound.wrappedJSObject.method == bound3.wrappedJSObject.method,
+             "Methods should be shared");
+
           // This gets invoked by an event handler.
           window.finish = function() {
             // Content messed with stuff. Make sure we still see the right thing.
             is(bound.method('bay'), "method:bay", "Xray methods work");
             is(bound.wrappedJSObject.method('bay'), "hah", "Xray waived methods work");
             is(bound.prop, "set:someOtherVal", "Xray props work");
             is(bound.wrappedJSObject.prop, "redefined", "Xray waived props work");
             is(bound.wrappedJSObject.primitiveField, 321, "Can't do anything about redefined fields");
@@ -274,25 +283,27 @@ https://bugzilla.mozilla.org/show_bug.cg
     catch (e) { ok(false, desc + ": Threw: " + e); }
   }
 
   function setup() {
     // When the bindings are applied, the constructor will be invoked and the
     // test will continue.
     document.getElementById('bound').style.MozBinding = 'url(#testBinding)';
     document.getElementById('bound2').style.MozBinding = 'url(#testBinding)';
+    document.getElementById('bound3').style.MozBinding = 'url(#testBinding)';
   }
 
   ]]>
 </script>
 </head>
 <body onload="setup()">
 <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=821850">Mozilla Bug 821850</a>
 <p id="display"></p>
 <div id="content">
   <div id="bound">Bound element</div>
   <div id="bound2">Bound element</div>
+  <span id="bound3">Bound element</span>
   <img/>
 </div>
 <pre id="test">
 </pre>
 </body>
 </html>