Skip installing fields which have no text. Bug 397924, r+sr+a=sicking

2007-10-02 14:38:35 +00:00 · 2007-10-02 14:38:35 +00:00 · 7bacde9da4
--- a/content/xbl/src/nsXBLBinding.cpp
+++ b/content/xbl/src/nsXBLBinding.cpp
@ -204,9 +204,10 @@ XBLResolve(JSContext *cx, JSObject *obj, jsval id, uintN flags,


  // Now we either resolve or fail
-  *objp = origObj;
+  PRBool didInstall;
  nsresult rv = field->InstallField(context, origObj,
-                                    protoBinding->DocURI());
+                                    protoBinding->DocURI(),
+                                    &didInstall);
  if (NS_FAILED(rv)) {
    if (!::JS_IsExceptionPending(cx)) {
      nsDOMClassInfo::ThrowJSException(cx, rv);
@ -215,6 +216,11 @@ XBLResolve(JSContext *cx, JSObject *obj, jsval id, uintN flags,
    return JS_FALSE;
  }

+  if (didInstall) {
+    *objp = origObj;
+  }
+  // else we didn't resolve this field after all
+
  return JS_TRUE;
 }

--- a/content/xbl/src/nsXBLProtoImplField.cpp
+++ b/content/xbl/src/nsXBLProtoImplField.cpp
@ -94,13 +94,20 @@ nsXBLProtoImplField::AppendFieldText(const nsAString& aText)
 nsresult
 nsXBLProtoImplField::InstallField(nsIScriptContext* aContext,
                                  JSObject* aBoundNode,
-                                  nsIURI* aBindingDocURI) const
+                                  nsIURI* aBindingDocURI,
+                                  PRBool* aDidInstall) const
 {
  NS_PRECONDITION(aBoundNode,
                  "uh-oh, bound node should NOT be null or bad things will "
                  "happen");

-  jsval result = JSVAL_VOID;
+  *aDidInstall = PR_FALSE;
+
+  if (mFieldTextLength == 0) {
+    return NS_OK;
+  }
+
+  jsval result = JSVAL_NULL;
  
  // EvaluateStringWithValue and JS_DefineUCProperty can both trigger GC, so
  // protect |result| here.
@ -109,27 +116,25 @@ nsXBLProtoImplField::InstallField(nsIScriptContext* aContext,
  if (NS_FAILED(rv))
    return rv;

-  if (mFieldTextLength != 0) {
-    nsCAutoString uriSpec;
-    aBindingDocURI->GetSpec(uriSpec);
+  nsCAutoString uriSpec;
+  aBindingDocURI->GetSpec(uriSpec);
  
-    // compile the literal string
-    // XXX Could we produce a better principal here?  Should be able
-    // to, really!
-    PRBool undefined;
-    nsCOMPtr<nsIScriptContext> context = aContext;
-    rv = context->EvaluateStringWithValue(nsDependentString(mFieldText,
-                                                            mFieldTextLength), 
-                                          aBoundNode,
-                                          nsnull, uriSpec.get(),
-                                          mLineNumber, nsnull,
-                                          (void*) &result, &undefined);
-    if (NS_FAILED(rv))
-      return rv;
+  // compile the literal string
+  // XXX Could we produce a better principal here?  Should be able
+  // to, really!
+  PRBool undefined;
+  nsCOMPtr<nsIScriptContext> context = aContext;
+  rv = context->EvaluateStringWithValue(nsDependentString(mFieldText,
+                                                          mFieldTextLength), 
+                                        aBoundNode,
+                                        nsnull, uriSpec.get(),
+                                        mLineNumber, nsnull,
+                                        (void*) &result, &undefined);
+  if (NS_FAILED(rv))
+    return rv;

-    if (undefined) {
-      result = JSVAL_VOID;
-    }
+  if (undefined) {
+    result = JSVAL_VOID;
  }

  // Define the evaluated result as a JS property
@ -142,6 +147,7 @@ nsXBLProtoImplField::InstallField(nsIScriptContext* aContext,
                             mJSAttributes)) {
    return NS_ERROR_OUT_OF_MEMORY;
  }
-  
+
+  *aDidInstall = PR_TRUE;
  return NS_OK;
 }
--- a/content/xbl/src/nsXBLProtoImplField.h
+++ b/content/xbl/src/nsXBLProtoImplField.h
@ -63,8 +63,9 @@ public:
  void SetNext(nsXBLProtoImplField* aNext) { mNext = aNext; }

  nsresult InstallField(nsIScriptContext* aContext,
-                        JSObject* aBoundNode, nsIURI*
-                        aBindingDocURI) const;
+                        JSObject* aBoundNode,
+                        nsIURI* aBindingDocURI,
+                        PRBool* aDidInstall) const;

  const PRUnichar* GetName() const { return mName; }

--- a/content/xbl/test/Makefile.in
+++ b/content/xbl/test/Makefile.in
@ -51,6 +51,7 @@ _TEST_FILES =	\
 		test_bug366770.html \
 		test_bug371724.xhtml \
 		test_bug372769.xhtml \
+		test_bug397934.xhtml \
 		$(NULL)

 libs:: $(_TEST_FILES)
--- a/content/xbl/test/test_bug397934.xhtml
+++ b/content/xbl/test/test_bug397934.xhtml
@ -0,0 +1,116 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=397934
+-->
+<head>
+  <title>Test for Bug 397934</title>
+  <script type="text/javascript" src="/MochiKit/packed.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+  <bindings xmlns="http://www.mozilla.org/xbl">
+    <binding id="ancestor">
+      <implementation>
+        <field name="testAncestor">"ancestor"</field>
+      </implementation>
+    </binding>
+    <binding id="test1" extends="#ancestor">
+      <implementation>
+        <constructor>
+          this.storage = window;
+        </constructor>
+        <field name="window"></field>
+        <field name="storage">null</field>
+        <field name="parentNode"></field>
+        <field name="ownerDocument">"ownerDocument"</field>
+        <field name="emptyTest1"></field>
+        <field name="emptyTest1">"empty1"</field>
+        <field name="emptyTest2">"empty2"</field>        
+        <field name="emptyTest2"></field>
+        <field name="testAncestor"></field>
+        <field name="testEvalOnce">window.counter++; undefined</field>
+      </implementation>
+    </binding>
+    <binding id="test2" extends="#ancestor">
+      <implementation>
+        <constructor>
+          this.storage = window;
+        </constructor>
+        <field name="window">undefined</field>
+        <field name="storage">null</field>
+        <field name="parentNode">undefined</field>
+        <field name="ownerDocument">"ownerDocument"</field>
+        <field name="emptyTest1">undefined</field>
+        <field name="emptyTest1">"empty1"</field>
+        <field name="emptyTest2">"empty2"</field>        
+        <field name="emptyTest2">undefined</field>
+        <field name="testAncestor">undefined</field>
+      </implementation>
+    </binding>
+  </bindings>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=397934">Mozilla Bug 397934</a>
+<p id="display1" style="-moz-binding: url(#test1)"></p>
+<p id="display2" style="-moz-binding: url(#test2)"></p>
+<div id="content" style="display: none">
+  
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+<![CDATA[
+
+/** Test for Bug 397934 **/
+SimpleTest.waitForExplicitFinish();
+addLoadEvent(function() {
+  var d;
+  d = $("display1");
+  is(d.storage, window,
+     "test1" +
+       ": |window| in the constructor should have resolved to our window");
+  is(d.ownerDocument, "ownerDocument",
+     "test1" + ": Control to test field overriding DOM prop");
+  is(d.parentNode, document.body, "test1" + ": unexpected parent");
+  is(d.emptyTest1, undefined,
+     "test1" + ": First field wins even if undefined");
+  is(typeof(d.emptyTest1), "undefined",
+     "test1" + ": First field wins even if undefined, double-check");
+  is(d.emptyTest2, "empty2",
+     "test1" + ": First field wins");
+  is(d.testAncestor, "ancestor",
+     "test1" + ": Empty field should not override ancestor binding");
+
+  window.counter = 0;
+  d.testEvalOnce;
+  d.testEvalOnce;
+  is(window.counter, 1, "Field should have evaluated once and only once");
+
+  d = $("display2");
+  is(d.storage, undefined,
+     "test2" +
+       ": |window| in the constructor should have resolved to undefined");
+  is(typeof(d.storage), "undefined",
+     "test2" +
+       ": |window| in the constructor should really have resolved to undefined");
+  is(d.ownerDocument, "ownerDocument",
+     "test2" + ": Control to test field overriding DOM prop");
+  is(d.parentNode, undefined, "test2" + ": unexpected parent");
+  is(typeof(d.parentNode), "undefined", "test2" + ": unexpected parent for real");
+  is(d.emptyTest1, undefined,
+     "test2" + ": First field wins even if undefined");
+  is(typeof(d.emptyTest1), "undefined",
+     "test2" + ": First field wins even if undefined, double-check");
+  is(d.emptyTest2, "empty2",
+     "test2" + ": First field wins");
+  is(d.testAncestor, undefined,
+     "test2" + ": Undefined field should override ancestor binding");
+  is(typeof(d.testAncestor), "undefined",
+     "test2" + ": Undefined field should really override ancestor binding");
+});
+addLoadEvent(SimpleTest.finish);
+
+]]>
+</script>
+</pre>
+</body>
+</html>
+