###!!! 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?

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; }

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.

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

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

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?

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 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 on attachment 282949 [details] [diff] [review] Possible fix Requesting approval for crash regression fix.

Checked in.