378866 - Loop in event target chain because insertion parents aren't updated properly when modifying DOM

Status: VERIFIED FIXED

(Core :: XBL, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, on trunk, it hangs Mozilla within 400ms and uses all available memory (so beware, before you open it!)

On branch, it crashes:
Talkback ID: TB31554630Z
nsGenericElement::HandleDOMEvent  [mozilla/content/base/src/nsGenericElement.cpp, line 2080]
nsGenericElement::HandleDOMEvent  [mozilla/content/base/src/nsGenericElement.cpp, line 2180]
nsXULElement::HandleDOMEvent  [mozilla/content/xul/content/src/nsXULElement.cpp, line 2209]
nsGenericElement::HandleDOMEvent  [mozilla/content/base/src/nsGenericElement.cpp, line 2180]
nsGenericElement::HandleDOMEvent  [mozilla/content/base/src/nsGenericElement.cpp, line 2180]
nsGenericElement::HandleDOMEvent  [mozilla/content/base/src/nsGenericElement.cpp, line 2180]
nsXULElement::HandleDOMEvent  [mozilla/content/xul/content/src/nsXULElement.cpp, line 2209]
nsGenericElement::HandleDOMEvent  [mozilla/content/base/src/nsGenericElement.cpp, line 2180]
nsGenericElement::HandleDOMEvent  [mozilla/content/base/src/nsGenericElement.cpp, line 2180]
nsGenericElement::HandleDOMEvent  [mozilla/content/base/src/nsGenericElement.cpp, line 2180]
etc

Comment 1

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

I was testing this on trunk, and to me it looks like we get a loop in 
the event target chain, but not in DOM. That is strange.
(Because of the strangeness, marking security sensitive)

Comment 2

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

Attachment: testing code for nsEventDispatcher.cpp

Comment 3

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

er, maybe not a loop. Something similar to the .innerHTML recursion crash?

Comment 4

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

Note to myself, remember to check whether insertionParent is updated properly when
an anonymous child is moved. (That might cause a loop in ETC)

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Note to myself, remember to check whether insertionParent is updated properly

See bug 375299?

Comment 6

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

bug 375299 didn't help here

Comment 7

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

Attachment: testcase

This is a testcase without marquee. This also shows that the bug
is XBL bug, not events bug.

Comment 8

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

And the problem seems to be the same both on branches and on trunk.
Trunk just stays alive longer because event dispatching happens more in heap. So trunk will die because of OOM, not because of stack overflow like branches do.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So the issue is that moving a <children> around in the DOM doesn't update any of the insertion point info?

Arguably, that appendChild call should throw, right?  Or at the very least removal of an insertion point ancestor should blow away the insertion point?

Comment 10

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

Attachment: possible patch

I think we could and should limit what can be done to anonymous content.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 274972 [details] [diff] [review]
possible patch

>Index: content/base/src/nsGenericElement.cpp
>+      nsIContent* bindingParent = newContent->GetBindingParent();
>+      if (removeIndex < 0 ||
>+          (bindingParent && aParent &&
>+           bindingParent != aParent->GetBindingParent())) {

How about:

if (removeIndex < 0 || 
    !aParent ||
    !nsContentUtils::IsInSameAnonymousSubtree(aParent, newContent)) {

That way when we insert under a Document node (which implies becoming completely non-anonymous) we'll correctly bail, and we'll be using the same check we use elsewhere to detect mutually-anonymous content.
    
r+sr=bzbarsky with that change.

Comment 12

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

(In reply to comment #11) 
> How about:
> 
> if (removeIndex < 0 || 
>     !aParent ||
>     !nsContentUtils::IsInSameAnonymousSubtree(aParent, newContent)) {
> 

Hmm, why !aParent? Maybe: 
if (removeIndex < 0 || 
    !nsContentUtils::IsInSameAnonymousSubtree(container, newContent)

That should take care of document too.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, right.  The first arg to IsInSameAnonymousTree is an nsINode, precisely for this reason.  So yeah, the !aParent thing is not needed.  ;)

Comment 14

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

Attachment: v2

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 274987 [details] [diff] [review]
v2

Looks great.

Comment 16

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

Attachment: testcase; removeChild/appendChild

The patch doesn't handle this case :(

Comment 17

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

I guess I need to keep track on possible insertionparents and when
an insertionparent is unbound, remove all the references to it.

Comment 18

[Boris Zbarsky [:bzbarsky]]

I'm pretty sure we have existing bugs about insertion point tables not getting updated right... that said, we _do_ have code for that.  Is it not working?

Comment 19

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

When unbinding an insertion parent, only its explicit child nodes are
unbound too. Element which have it as a insertion parent aren't updated
in any way, as far as I see.
So nsBindingManager::ChangeDocumentFor should probably check whether
aContent is an insertion parent and remove all entries pointing to it
from mInsertionParentTable.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Oh, hmmm.  I had it backwards.  Removing non-anon nodes removes them from the insertion point data... but yes, removing insertion point parents doesn't remove the insertion point.  :(

Comment 21

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

Attachment: possible patch

This is a bit ugly, but should work. Just keep the NODE_IS_INSERTION_PARENT
when content object is an insertion parent.
I tried to remove content from the hashtables which have (owning) pointers 
to it, but that caused  RECURSION_LEVEL assertions in PLDHash, so just tracking
NODE_IS_INSERTION_PARENT flag seemed an easier and safer way to fix this.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Hmm...  Could you have sicking look?  I'm kinda swamped right this moment.  :(

Also, I'd love to see the stack to one of the recursion asserts.

Comment 23

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

Comment on attachment 275581 [details] [diff] [review]
possible patch

I'll post a stack for assertions tomorrow.

Comment 24

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

This assertion comes if I try to remove insertion parent from binding's mInsertionPointTable. I have an alternative patch to fix the problem.

#0  PL_DHashTableOperate (table=0x782598, key=0x1047d90, op=PL_DHASH_REMOVE) at pldhash.c:587
#1  0x00002aaab119c21f in SetOrRemoveObject (table=@0x31a834a840, aKey=0x1047d90, aValue=0x0)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xbl/src/nsBindingManager.cpp:281
#2  0x00002aaab119c9d4 in nsBindingManager::SetAnonymousNodesFor (this=0x782520, aContent=0x1047d90, aList=0x0)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xbl/src/nsBindingManager.cpp:653
#3  0x00002aaab119d574 in nsBindingManager::SetBinding (this=0x782520, aContent=0x1047d90, aBinding=0x0)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xbl/src/nsBindingManager.cpp:451
#4  0x00002aaab119f2a6 in nsBindingManager::ChangeDocumentFor (this=0x782520, aContent=0x1047d90, aOldDocument=0x7176a0, 
    aNewDocument=0x0) at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xbl/src/nsBindingManager.cpp:534
#5  0x00002aaab0fb7ef2 in nsGenericElement::UnbindFromTree (this=0x1047d90, aDeep=0, aNullParent=1)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/base/src/nsGenericElement.cpp:2088
#6  0x00002aaab12ecbae in nsXULElement::UnbindFromTree (this=<value optimized out>, aDeep=0, aNullParent=1)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xul/content/src/nsXULElement.cpp:818
#7  0x00002aaab0f37af3 in nsAttrAndChildArray::Clear (this=0x1046d50)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/base/src/nsAttrAndChildArray.cpp:650
#8  0x00002aaab0f37b33 in ~nsAttrAndChildArray (this=0x31a834a840)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/base/src/nsAttrAndChildArray.cpp:133
#9  0x00002aaab0fb1624 in ~nsGenericElement (this=0x1046d20)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/base/src/nsGenericElement.cpp:1107
#10 0x00002aaab10f2b32 in ~nsXMLElement (this=0x31a834a840)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xml/content/src/nsXMLElement.h:49
#11 0x00002aaab0fd240a in nsNodeUtils::LastRelease (aNode=0x1046d20)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/base/src/nsNodeUtils.cpp:228
#12 0x00002aaab0fb4755 in nsGenericElement::Release (this=0x1046d20)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/base/src/nsGenericElement.cpp:3397
#13 0x00002aaab10f1810 in nsXMLElement::Release (this=0x31a834a840)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xml/content/src/nsXMLElement.cpp:99
#14 0x00002aaab11a0bfb in ~nsXBLInsertionPoint (this=0x1012710) at ../../../dist/include/xpcom/nsCOMPtr.h:583
#15 0x00002aaab11a0c65 in nsXBLInsertionPoint::Release (this=0x1012710)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xbl/src/nsXBLInsertionPoint.cpp:61
#16 0x00002aaab119a836 in ~nsAnonymousContentList (this=0x101b560) at ../../../dist/include/xpcom/nsAutoPtr.h:956
#17 0x00002aaab119ad05 in nsAnonymousContentList::Release (this=0x101b560)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xbl/src/nsBindingManager.cpp:128
#18 0x00002aaaab2675d5 in ~nsCOMPtr_base (this=0x1242cb0) at nsCOMPtr.cpp:81
#19 0x00002aaab119a8b7 in ClearObjectEntry (table=<value optimized out>, entry=0x1242ca0)
    at ../../../dist/include/xpcom/nsCOMPtr.h:929
#20 0x00002aaaab264124 in PL_DHashTableRawRemove (table=0x782598, entry=0x1242ca0) at pldhash.c:696
#21 0x00002aaaab264b64 in PL_DHashTableOperate (table=0x782598, key=0x1023bd0, op=PL_DHASH_REMOVE) at pldhash.c:664
#22 0x00002aaab119c21f in SetOrRemoveObject (table=@0x31a834a840, aKey=0x1023bd0, aValue=0x0)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xbl/src/nsBindingManager.cpp:281
#23 0x00002aaab119c9d4 in nsBindingManager::SetAnonymousNodesFor (this=0x782520, aContent=0x1023bd0, aList=0x0)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xbl/src/nsBindingManager.cpp:653
#24 0x00002aaab119d574 in nsBindingManager::SetBinding (this=0x782520, aContent=0x1023bd0, aBinding=0x0)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xbl/src/nsBindingManager.cpp:451
#25 0x00002aaab119f2a6 in nsBindingManager::ChangeDocumentFor (this=0x782520, aContent=0x1023bd0, aOldDocument=0x7176a0, 
    aNewDocument=0x0) at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xbl/src/nsBindingManager.cpp:534
#26 0x00002aaab0fb7ef2 in nsGenericElement::UnbindFromTree (this=0x1023bd0, aDeep=1, aNullParent=1)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/base/src/nsGenericElement.cpp:2088
#27 0x00002aaab12ecbae in nsXULElement::UnbindFromTree (this=<value optimized out>, aDeep=1, aNullParent=1)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/xul/content/src/nsXULElement.cpp:818
#28 0x00002aaab0f61842 in nsContentUtils::DestroyAnonymousContent (aContent=0xdd4050)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/content/base/src/nsContentUtils.cpp:3504
#29 0x00002aaab0f0f318 in nsDocElementBoxFrame::Destroy (this=0xdd3fa8)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/xul/base/src/nsDocElementBoxFrame.cpp:108
#30 0x00002aaab0d99d6b in nsFrameList::DestroyFrames (this=0xdd3908)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/generic/nsFrameList.cpp:67
#31 0x00002aaab0d7f158 in nsContainerFrame::Destroy (this=0xdd38b0)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/generic/nsContainerFrame.cpp:252
#32 0x00002aaab0ef5d0f in nsBoxFrame::Destroy (this=0xdd38b0)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/xul/base/src/nsBoxFrame.cpp:949
#33 0x00002aaab0d99d6b in nsFrameList::DestroyFrames (this=0xdd3800)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/generic/nsFrameList.cpp:67
#34 0x00002aaab0d7f158 in nsContainerFrame::Destroy (this=0xdd37a8)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/generic/nsContainerFrame.cpp:252
#35 0x00002aaab0e0a040 in ViewportFrame::Destroy (this=0xdd37a8)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/generic/nsViewportFrame.cpp:68
#36 0x00002aaab0d2ae7f in nsFrameManager::Destroy (this=0xdf98e8)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/base/nsFrameManager.cpp:281
#37 0x00002aaab0d45a8f in PresShell::Destroy (this=0xdf98b0)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/base/nsPresShell.cpp:1651
#38 0x00002aaab0d2516b in DocumentViewerImpl::Destroy (this=0x71a6b0)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/base/nsDocumentViewer.cpp:1492
#39 0x00002aaab3a86fa2 in nsDocShell::Destroy (this=0xc7a580)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/docshell/base/nsDocShell.cpp:3521
#40 0x00002aaab4e75984 in nsXULWindow::Destroy (this=0xc3f160)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/xpfe/appshell/src/nsXULWindow.cpp:506
#41 0x00002aaab4e8304e in nsWebShellWindow::Destroy (this=0xc3f160)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/xpfe/appshell/src/nsWebShellWindow.cpp:826
#42 0x00002aaab4e83bbc in nsWebShellWindow::HandleEvent (aEvent=<value optimized out>)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/xpfe/appshell/src/nsWebShellWindow.cpp:383
#43 0x00002aaab3d477c1 in nsCommonWidget::DispatchEvent (this=0xc2fc60, aEvent=0x7fffd06d1c20, aStatus=@0x7fffd06d1c6c)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/widget/src/gtk2/nsCommonWidget.cpp:225
#44 0x00002aaab3d3ed5c in nsWindow::OnDeleteEvent (this=0x31a834a840, aWidget=<value optimized out>, 
    aEvent=<value optimized out>) at /home/smaug/mozilla/mozilla_cvs/mozilla/widget/src/gtk2/nsWindow.cpp:1882

Comment 25

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

Attachment: possible patch, v2

Even simpler, clearing hashtables. Keeping objects alive when calling
RemoveObjectEntry removes the assertions.

Comment 26

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

Comment on attachment 275759 [details] [diff] [review]
possible patch, v2

peterv's review queue is to terribly long :)

Comment 27

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

bah, s/to/not.

Comment 28

[Boris Zbarsky [:bzbarsky]]

I probably won't get to this until after I get back...

Comment 29

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

Jonas, will you have time to sr this?

Comment 30

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

Hmm.. actually, can a node be the insertion parent in several bindings? I.e. if <children> if a direct child of the binding that is inserted. If so, wouldn't you reset the IS_INSERTION_PARENT flag too soon?

Comment 31

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

(In reply to comment #30)
>  I.e. if <children> if a direct child of the binding that is inserted. 

Um, could you rephrase? An example would be great, even just pseudo-code.

Comment 32

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

Ah, I think I understand it know. Shouldn't be a problem, but better to clear
insertion parent table also in the insertion parent's binging, not only its binding parent's binding.

Comment 33

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

Something like:

bound element:
<div style="-moz-binding: url('#binding1')><span/></div>

binding1:
<content>
  <foo style="-moz-binding: url('#binding2')><children/></foo>
</content>

binding2:
<content>
  <bar/>
  <children/>
</content>

the <foo> element would be the insertion parent both for the children of the <div> element, and the children in binding 2, i.e. both for the <span> and the <bar>.

If binding2 is made to no longer be bound to <foo> we shouldn't unflag <foo> as insertion parent as it will still be the insertion parent for the <span>.

At least in theory, I don't know how well that would work in practice anyway.

Comment 34

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

Actually, it looks like this patch only deals with the <div> being moved out of the document. Should you also deal with things like style changes causing bindings to be removed?

Comment 35

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

I'll check that out. Will re-ask review.

Comment 36

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

Attachment: clear the flag if bindingparent's binding nor binding has references to content

this is ugly. But to be safe it is better to clear references to the old
insertion parent.
The main functional change here is that insertion parent is cleared also
SetBinding (not only in ChangeDocumentFor)
Added also few assertions.

Comment 37

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

Comment on attachment 284000 [details] [diff] [review]
clear the flag if bindingparent's binding nor binding has references to content

>Index: content/base/public/nsINode.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/public/nsINode.h,v
>retrieving revision 3.58
>diff -u -8 -p -r3.58 nsINode.h
>--- content/base/public/nsINode.h	18 Sep 2007 08:38:24 -0000	3.58
>+++ content/base/public/nsINode.h	8 Oct 2007 14:49:37 -0000
>@@ -93,18 +93,20 @@ enum {
>   NODE_IS_EDITABLE =             0x00000100U,
> 
>   // Optimizations to quickly check whether element may have ID, class or style
>   // attributes. Not all element implementations may use these!
>   NODE_MAY_HAVE_ID =             0x00000200U,
>   NODE_MAY_HAVE_CLASS =          0x00000400U,
>   NODE_MAY_HAVE_STYLE =          0x00000800U,
> 
>+  NODE_IS_INSERTION_PARENT =     0x00001000U,
>+
>   // Four bits for the script-type ID
>-  NODE_SCRIPT_TYPE_OFFSET =               12,
>+  NODE_SCRIPT_TYPE_OFFSET =               13,
> 
>   NODE_SCRIPT_TYPE_SIZE =                  4,
> 
>   // Remaining bits are node type specific.
>   NODE_TYPE_SPECIFIC_BITS_OFFSET =
>     NODE_SCRIPT_TYPE_OFFSET + NODE_SCRIPT_TYPE_SIZE
> };
> 
>Index: content/xbl/src/nsBindingManager.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xbl/src/nsBindingManager.cpp,v
>retrieving revision 1.186
>diff -u -8 -p -r1.186 nsBindingManager.cpp
>--- content/xbl/src/nsBindingManager.cpp	3 Oct 2007 23:38:32 -0000	1.186
>+++ content/xbl/src/nsBindingManager.cpp	8 Oct 2007 14:49:38 -0000
>@@ -82,36 +82,43 @@
> #include "nsBindingManager.h"
> 
> #include "nsThreadUtils.h"
> 
> // ==================================================================
> // = nsAnonymousContentList 
> // ==================================================================
> 
>+#define NS_ANONYMOUS_CONTENT_LIST_IID \
>+  { 0xa29df1f8, 0xaeca, 0x4356, \
>+    { 0xa8, 0xc2, 0xa7, 0x24, 0xa2, 0x11, 0x73, 0xac } }
>+
> class nsAnonymousContentList : public nsIDOMNodeList
> {
> public:
>   nsAnonymousContentList(nsInsertionPointList* aElements);
>   virtual ~nsAnonymousContentList();
> 
>   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>   NS_DECL_CYCLE_COLLECTION_CLASS(nsAnonymousContentList)
>   // nsIDOMNodeList interface
>   NS_DECL_NSIDOMNODELIST
> 
>   PRInt32 GetInsertionPointCount() { return mElements->Length(); }
> 
>   nsXBLInsertionPoint* GetInsertionPointAt(PRInt32 i) { return static_cast<nsXBLInsertionPoint*>(mElements->ElementAt(i)); }
>   void RemoveInsertionPointAt(PRInt32 i) { mElements->RemoveElementAt(i); }
>-
>+  NS_DECLARE_STATIC_IID_ACCESSOR(NS_ANONYMOUS_CONTENT_LIST_IID)
> private:
>   nsInsertionPointList* mElements;
> };
> 
>+NS_DEFINE_STATIC_IID_ACCESSOR(nsAnonymousContentList,
>+                              NS_ANONYMOUS_CONTENT_LIST_IID)
>+
> nsAnonymousContentList::nsAnonymousContentList(nsInsertionPointList* aElements)
>   : mElements(aElements)
> {
>   MOZ_COUNT_CTOR(nsAnonymousContentList);
> 
>   // We don't reference count our Anonymous reference (to avoid circular
>   // references). We'll be told when the Anonymous goes away.
> }
>@@ -124,16 +131,17 @@ nsAnonymousContentList::~nsAnonymousCont
> 
> NS_IMPL_CYCLE_COLLECTION_CLASS(nsAnonymousContentList)
> 
> NS_IMPL_CYCLE_COLLECTING_ADDREF(nsAnonymousContentList)
> NS_IMPL_CYCLE_COLLECTING_RELEASE(nsAnonymousContentList)
> 
> NS_INTERFACE_MAP_BEGIN(nsAnonymousContentList)
>   NS_INTERFACE_MAP_ENTRY(nsIDOMNodeList)
>+  NS_INTERFACE_MAP_ENTRY(nsAnonymousContentList)
>   NS_INTERFACE_MAP_ENTRY(nsISupports)
>   NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO(NodeList)
>   NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(nsAnonymousContentList)
> NS_INTERFACE_MAP_END
> 
> NS_IMPL_CYCLE_COLLECTION_UNLINK_0(nsAnonymousContentList)
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsAnonymousContentList)
>   {
>@@ -294,18 +302,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;
> }
> 
> // Implementation /////////////////////////////////////////////////////////////////
> 
> // Static member variable initialization
> 
> // Implement our nsISupports methods
>@@ -397,22 +414,70 @@ nsBindingManager::nsBindingManager(nsIDo
> }
> 
> nsBindingManager::~nsBindingManager(void)
> {
>   if (mContentListTable.ops)
>     PL_DHashTableFinish(&mContentListTable);
>   if (mAnonymousNodesTable.ops)
>     PL_DHashTableFinish(&mAnonymousNodesTable);
>+  NS_ASSERTION(!mInsertionParentTable.ops || !mInsertionParentTable.entryCount,
>+               "Insertion parent table isn't empty!");
>   if (mInsertionParentTable.ops)
>     PL_DHashTableFinish(&mInsertionParentTable);
>   if (mWrapperTable.ops)
>     PL_DHashTableFinish(&mWrapperTable);
> }
> 
>+PLDHashOperator
>+PR_CALLBACK RemoveInsertionParentCB(PLDHashTable* aTable, PLDHashEntryHdr* aEntry,
>+                                  PRUint32 aNumber, void* aArg)
>+{
>+  return (static_cast<ObjectEntry*>(aEntry)->GetValue() ==
>+          static_cast<nsISupports*>(aArg)) ? PL_DHASH_REMOVE : PL_DHASH_NEXT;
>+}
>+
>+static void
>+RemoveInsertionParentForNodeList(nsIDOMNodeList* aList, nsIContent* aParent)
>+{
>+  nsCOMPtr<nsAnonymousContentList> list = do_QueryInterface(aList);
>+  if (list) {
>+    PRInt32 count = list->GetInsertionPointCount();
>+    for (PRInt32 i = 0; i < count; ++i) {
>+      nsRefPtr<nsXBLInsertionPoint> currPoint = list->GetInsertionPointAt(i);
>+      nsCOMPtr<nsIContent> defContent = currPoint->GetDefaultContent();
>+      if (defContent) {
>+        defContent->UnbindFromTree();
>+      }
>+#ifdef DEBUG
>+      nsCOMPtr<nsIContent> parent = currPoint->GetInsertionParent();
>+      NS_ASSERTION(!parent || parent == aParent, "Wrong insertion parent!");
>+#endif
>+      currPoint->ClearInsertionParent();
>+    }
>+  }
>+}
>+
>+void
>+nsBindingManager::RemoveInsertionParent(nsIContent* aParent)
>+{
>+  nsCOMPtr<nsIDOMNodeList> contentlist;
>+  GetContentListFor(aParent, getter_AddRefs(contentlist));
>+  RemoveInsertionParentForNodeList(contentlist, aParent);

Do you really want to unbind all nodes in GetContentListFor? Will that not contain any of the DOM children? I can never remember what these lists contain. Would be great to have them better documented in nsBindingManager.h

r/sr=me with that checked/fixed.

Comment 38

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

Where am I unbinding "all nodes". Only if the domnodelist is anonymous, 
insertionpoints' defaultcontent is unbound.
There is one comment about content lists:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xbl/src/nsXBLBinding.cpp&rev=1.239&mark=674-679#670

Comment 39

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

I need to still mochitestify the tests.

Comment 40

[Martijn Wargers (dead)]

Verified fixed, with all the testcases attached to this bug, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007101905 Minefield/3.0a9pre