Actually unhook the binding proto when we're tearing down the binding anonymous content. Hasn't worked in years, apparently. Bug 398135, r+sr=sicking

content/xbl/src/nsXBLBinding.cpp

@@ -1039,25 +1039,19 @@ void
 nsXBLBinding::ChangeDocument(nsIDocument* aOldDocument, nsIDocument* aNewDocument)
 {
   if (aOldDocument != aNewDocument) {
-    if (mNextBinding)
-      mNextBinding->ChangeDocument(aOldDocument, aNewDocument);
-
-    // Only style bindings get their prototypes unhooked.
+    // Only style bindings get their prototypes unhooked.  First do ourselves.
     if (mIsStyleBinding) {
       // Now the binding dies.  Unhook our prototypes.
-      nsIContent* interfaceElement =
-        mPrototypeBinding->GetImmediateChild(nsGkAtoms::implementation);
-
-      if (interfaceElement) { 
-        nsIScriptGlobalObject *global = aOldDocument->GetScriptGlobalObject();
+      if (mPrototypeBinding->HasImplementation()) { 
+        nsIScriptGlobalObject *global = aOldDocument->GetScopeObject();
         if (global) {
           nsIScriptContext *context = global->GetContext();
           if (context) {
-            JSContext *jscontext = (JSContext *)context->GetNativeContext();
+            JSContext *cx = (JSContext *)context->GetNativeContext();
  
             nsCOMPtr<nsIXPConnectJSObjectHolder> wrapper;
             nsresult rv = nsContentUtils::XPConnect()->
-              WrapNative(jscontext, global->GetGlobalJSObject(),
+              WrapNative(cx, global->GetGlobalJSObject(),
                          mBoundElement, NS_GET_IID(nsISupports),
                          getter_AddRefs(wrapper));
             if (NS_FAILED(rv))
@@ -1068,16 +1062,57 @@ nsXBLBinding::ChangeDocument(nsIDocument* aOldDocument, nsIDocument* aNewDocumen
             if (NS_FAILED(rv))
               return;
 
+            mPrototypeBinding->UndefineFields(cx, scriptObject);
+
             // XXX Stay in sync! What if a layered binding has an
             // <interface>?!
+            // XXXbz what does that comment mean, really?  It seems to date
+            // back to when there was such a thing as an <interface>, whever
+            // that was...
 
-            // XXX Sanity check to make sure our class name matches
-            // Pull ourselves out of the proto chain.
-            JSObject* ourProto = ::JS_GetPrototype(jscontext, scriptObject);
-            if (ourProto)
-            {
-              JSObject* grandProto = ::JS_GetPrototype(jscontext, ourProto);
-              ::JS_SetPrototype(jscontext, scriptObject, grandProto);
+            // Find the right prototype.
+            JSObject* base = scriptObject;
+            JSObject* proto;
+            JSAutoRequest ar(cx);
+            for ( ; true; base = proto) { // Will break out on null proto
+              proto = ::JS_GetPrototype(cx, base);
+              if (!proto) {
+                break;
+              }
+
+              JSClass* clazz = ::JS_GetClass(cx, proto);
+              if (!clazz ||
+                  (~clazz->flags &
+                   (JSCLASS_HAS_PRIVATE | JSCLASS_PRIVATE_IS_NSISUPPORTS)) ||
+                  JSCLASS_RESERVED_SLOTS(clazz) != 1) {
+                // Clearly not the right class
+                continue;
+              }
+
+              nsCOMPtr<nsIXBLDocumentInfo> docInfo =
+                do_QueryInterface(static_cast<nsISupports*>
+                                             (::JS_GetPrivate(cx, proto)));
+              if (!docInfo) {
+                // Not the proto we seek
+                continue;
+              }
+              
+              jsval protoBinding;
+              if (!::JS_GetReservedSlot(cx, proto, 0, &protoBinding)) {
+                NS_ERROR("Really shouldn't happen");
+                continue;
+              }
+
+              if (JSVAL_TO_PRIVATE(protoBinding) != mPrototypeBinding) {
+                // Not the right binding
+                continue;
+              }
+
+              // Alright!  This is the right prototype.  Pull it out of the
+              // proto chain.
+              JSObject* grandProto = ::JS_GetPrototype(cx, proto);
+              ::JS_SetPrototype(cx, base, grandProto);
+              break;
             }
 
             // Don't remove the reference from the document to the
@@ -1088,7 +1123,14 @@ nsXBLBinding::ChangeDocument(nsIDocument* aOldDocument, nsIDocument* aNewDocumen
       }
     }
 
+    // Then do our ancestors.  This reverses the construction order, so that at
+    // all times things are consistent as far as everyone is concerned.
+    if (mNextBinding) {
+      mNextBinding->ChangeDocument(aOldDocument, aNewDocument);
+    }
+
     // Update the anonymous content.
+    // XXXbz why not only for style bindings?
     nsIContent *anonymous = mContent;
     if (anonymous) {
       // Also kill the default content within all our insertion points.

content/xbl/src/nsXBLProtoImpl.cpp

@@ -254,6 +254,19 @@ nsXBLProtoImpl::ResolveAllFields(JSContext *cx, JSObject *obj) const
   return PR_TRUE;
 }
 
+void
+nsXBLProtoImpl::UndefineFields(JSContext *cx, JSObject *obj) const
+{
+  JSAutoRequest ar(cx);
+  for (nsXBLProtoImplField* f = mFields; f; f = f->GetNext()) {
+    nsDependentString name(f->GetName());
+    jsval dummy;
+    ::JS_DeleteUCProperty2(cx, obj,
+                           reinterpret_cast<const jschar*>(name.get()),
+                           name.Length(), &dummy);
+  }
+}
+
 void
 nsXBLProtoImpl::DestroyMembers(nsXBLProtoImplMember* aBrokenMember)
 {

content/xbl/src/nsXBLProtoImpl.h

@@ -99,6 +99,10 @@ public:
   // return means a JS exception was set.
   PRBool ResolveAllFields(JSContext *cx, JSObject *obj) const;
 
+  // Undefine all our fields from object |obj| (which should be a
+  // JSObject for a bound element).
+  void UndefineFields(JSContext* cx, JSObject* obj) const;
+
   PRBool CompiledMembers() const {
     return mClassObject != nsnull;
   }

content/xbl/src/nsXBLPrototypeBinding.h

@@ -106,6 +106,14 @@ public:
     return !mImplementation || mImplementation->ResolveAllFields(cx, obj);
   }
 
+  // Undefine all our fields from object |obj| (which should be a
+  // JSObject for a bound element).
+  void UndefineFields(JSContext* cx, JSObject* obj) const {
+    if (mImplementation) {
+      mImplementation->UndefineFields(cx, obj);
+    }
+  }
+
   const nsCString& ClassName() const {
     return mImplementation ? mImplementation->mClassName : EmptyCString();
   }
@@ -118,6 +126,7 @@ public:
   
   void SetImplementation(nsXBLProtoImpl* aImpl) { mImplementation = aImpl; }
   nsresult InstallImplementation(nsIContent* aBoundElement);
+  PRBool HasImplementation() const { return mImplementation != nsnull; }
 
   void AttributeChanged(nsIAtom* aAttribute, PRInt32 aNameSpaceID,
                         PRBool aRemoveFlag, nsIContent* aChangedElement,

content/xbl/test/Makefile.in

@@ -53,6 +53,7 @@ _TEST_FILES =	\
 		test_bug372769.xhtml \
 		test_bug378866.xhtml \
 		test_bug397934.xhtml \
+		test_bug398135.xhtml \
 		$(NULL)
 
 libs:: $(_TEST_FILES)

content/xbl/test/test_bug398135.xhtml

@@ -0,0 +1,136 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=398135
+-->
+<head>
+  <title>Test for Bug 398135</title>
+  <script type="text/javascript" src="/MochiKit/packed.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript">window.log = ""</script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+  <bindings xmlns="http://www.mozilla.org/xbl">
+    <binding id="ancestor">
+      <implementation>
+        <constructor>
+          window.log += "ancestorConstructor:";
+        </constructor>
+        <destructor>
+          window.log += "ancestorDestructor:";
+        </destructor>
+        <field name="ancestorField">"ancestorField"</field>
+        <property name="ancestorProp" onget="return 'ancestorProp'"/>
+        <method name="ancestorMethod">
+          <body>
+            return "ancestorMethod";
+          </body>
+        </method>
+      </implementation>
+    </binding>
+    <binding id="test" extends="#ancestor">
+      <implementation>
+        <constructor>
+          window.log += "descendantConstructor:";
+        </constructor>
+        <destructor>
+          window.log += "descendantDestructor:";
+        </destructor>
+        <field name="descendantField">"descendantField"</field>
+        <field name="contentField">
+          document.getAnonymousNodes(this)[0];
+        </field>
+        <property name="descendantProp" onget="return 'descendantProp'"/>
+        <method name="descendantMethod">
+          <body>
+            return "descendantMethod";
+          </body>
+        </method>
+      </implementation>
+      <content>
+        <span/>
+        <children/>
+      </content>
+    </binding>
+  </bindings>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=398135">Mozilla Bug 398135</a>
+<p id="display" style="-moz-binding: url(#test)"></p>
+<div id="content" style="display: none">
+  
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+<![CDATA[
+/** Test for Bug 398135 **/
+SimpleTest.waitForExplicitFinish();
+addLoadEvent(function() {
+  var d;
+  d = $("display");
+
+  function testInTree(type) {
+    is(d.ancestorField, "ancestorField", "Wrong ancestor field " + type);
+    is(d.descendantField, "descendantField", "Wrong descendant field " + type);
+    is(d.ancestorProp, "ancestorProp", "Wrong ancestor prop " + type);
+    is(d.descendantProp, "descendantProp", "Wrong descendant prop " + type);
+    is(d.ancestorMethod(), "ancestorMethod", "Wrong ancestor method " + type);
+    is(d.descendantMethod(), "descendantMethod",
+       "Wrong descendant method " + type);
+    is(d.contentField, document.getAnonymousNodes(d)[0],
+       "Unexpected content field " + type);
+  }
+
+  function testNotInTree(type) {
+    is(typeof(d.ancestorField), "undefined", "Wrong ancestor field " + type);
+    is(typeof(d.descendantField), "undefined",
+       "Wrong descendant field " + type);
+    is(typeof(d.ancestorProp), "undefined", "Wrong ancestor prop " + type);
+    is(typeof(d.descendantProp), "undefined", "Wrong descendant prop " + type);
+    is(typeof(d.ancestorMethod), "undefined", "Wrong ancestor method " + type);
+    is(typeof(d.descendantMethod), "undefined",
+       "Wrong descendant method " + type);
+    is(typeof(d.contentField), "undefined",
+       "Unexpected content field " + type);
+  }
+
+  is(window.log, "ancestorConstructor:descendantConstructor:",
+     "Constructors did not fire?");
+  window.log = "";
+  testInTree("before removal");
+
+  var parent = d.parentNode;
+  var nextSibling = d.nextSibling;
+  parent.removeChild(d);
+  testNotInTree("after first removal");
+
+  todo(window.log == "descendantDestructor:ancestorDestructor:",
+     "Destructors did not fire");
+  window.log = "";
+  
+  parent.insertBefore(d, nextSibling);
+  is(window.log, "ancestorConstructor:descendantConstructor:",
+     "Constructors did not fire a second time?");
+  window.log = "";
+  testInTree("after reinsertion");
+
+  // Now munge the proto chain to test the robustness of the proto-unhooking
+  // code
+  var origProto = d.__proto__;
+  var origProtoProto = origProto.__proto__;
+  var newProto = new Object();
+  origProto.__proto__ = newProto;
+  newProto.__proto__ = origProtoProto;
+
+  parent.removeChild(d);
+  todo(window.log == "descendantDestructor:ancestorDestructor:",
+     "Destructors did not fire a second time?");
+
+  testNotInTree("after second removal");
+});
+addLoadEvent(SimpleTest.finish);
+
+]]>
+</script>
+</pre>
+</body>
+</html>
+