398083 - [FIX]XBL unlinking triggered hash-table recursion assertion

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Jesse Ruderman]

Attachment: stack trace for the assertion

###!!! ASSERTION: op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0: 'op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0', file pldhash.c, line 585

pure virtual method called
terminate called without an active exception

I can't reproduce at will, but I did get a stack trace for the assertion.

Regression from bug 372769?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Possibly fixed by the patch (once I found the final fix) for the insertion 
parent loop. At least sounds similar to what I saw when making the current 
patch and why I added the following to the patch:
+++ content/xbl/src/nsBindingManager.cpp
@@ -292,18 +292,27 @@ SetOrRemoveObject(PLDHashTable& table, n
       table.ops = nsnull;
       return NS_ERROR_OUT_OF_MEMORY;
     }
     aKey->SetFlags(NODE_MAY_BE_IN_BINDING_MNGR);
     return AddObjectEntry(table, aKey, aValue);
   }
 
   // no value, so remove the key from the table
-  if (table.ops)
-    RemoveObjectEntry(table, aKey);
+  if (table.ops) {
+    ObjectEntry* entry =
+      static_cast<ObjectEntry*>
+        (PL_DHashTableOperate(&table, aKey, PL_DHASH_LOOKUP));
+    if (entry && PL_DHASH_ENTRY_IS_BUSY(entry)) {
+      // Keep key and value alive while removing the entry.
+      nsCOMPtr<nsISupports> key = entry->GetKey();
+      nsCOMPtr<nsISupports> value = entry->GetValue();
+      RemoveObjectEntry(table, aKey);
+    }
+  }
   return NS_OK;
 }

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible fix

There's no need to nuke the hashtables to unlink.  Just have to nuke the precompiled functions that are holding the JS scope alive.  Since we unlink event listeners off of nsGenericElement, holding all those refs to the content nodes from here is ok.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Don't you at least want to traverse the hashes and remove all values? Or will unlinking the elements do that?

Comment 4

[Boris Zbarsky [:bzbarsky]]

The hashes will get destroyed in ~nsXBLPrototypeBinding, as long as we unlink enough stuff.  Which we still do with this patch.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

But if we don't unlink enough stuff we won't destroy anything and no destructors will run. Or are you sure that we'll never end up with a cycle where this is the only breakable link?

Comment 6

[Boris Zbarsky [:bzbarsky]]

I'm pretty sure, yes.  The hashtables hold refs to things entirely within the XBL document, and the unlink methods of those content nodes will drop refs out into JS.  So at that point we just have refs into the XBL document DOM (but not out of it), and we should be good.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 282949 [details] [diff] [review]
Possible fix

Ok, sounds good.

I've actually come to realize that we need to be a little careful with what we put in Unlink for performance reasons. We're already calling unlink a lot, and it's only going to happen more.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 282949 [details] [diff] [review]
Possible fix

Requesting approval for crash regression fix.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Checked in.