397849 - "ASSERTION: Shallow unbind won't clear document and binding parent on kids!"

Status: RESOLVED WORKSFORME

(Core :: XBL, defect, P4)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Shallow unbind won't clear document and binding parent on kids!: 'aDeep || (!GetCurrentDoc() && !GetBindingParent())', file /Users/jruderman/trunk/mozilla/content/base/src/nsGenericElement.cpp, line 2086

Split from bug 360078.  The testcase is the same as the testcase from that bug; I haven't attempted to reduce it further.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is kinda fun.  We have a node with NODE_FORCE_XBL_BINDINGS set on it.  It never gets inserted into a DOM.  Then its ownerDocument dies, which kills the binding manager, which releases the <content> node from the binding.... which still has all the anon content as kids (though their parent is the bound node).  So we assert because those kids have a bindingParent that's not null, and we're doing a shallow unbind on them.

I think the right thing to do when a binding goes away would be to unbind all that content... or something.  Jonas?

Comment 2

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

"that content" being the anonymous content in the binding? If so yes. Can this cause badness if we don't fix for 1.9? If so we should consider blocking.

Ideally those nodes shouldn't even be children of the <content>, but instead kept alive using a simple nsCOMArray<nsIContent> somewhere.

Comment 3

[Boris Zbarsky [:bzbarsky]]

"that content" is the anonymous content of the binding, yes.

I think we might use the fact that they're kids of the <content> in XBL code, so switching that would not be all that simple.  But I may be wrong... I haven't looked at that code recently.

Not sure about badness.  It does violate some expectations we seem to have (e.g. that a binding won't outlive the binding manager).

Comment 4

[Mats Palmgren (inactive)]

The testcase in bug 394800 also triggers this assertion.

Comment 5

[Martijn Wargers (dead)]

I can't reproduce this assertion with this testcase or the one in bug 394800.

Comment 6

[Jesse Ruderman]

I still hit it with this testcase, but not as often as when I initially reported the bug.  I hit the assertion within a few minutes just now.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

A clean Mac trunk Sunbird debug build outputs the following on startup:

###!!! ASSERTION: Shallow unbind won't clear document and binding parent on kids!: 'aDeep || (!GetCurrentDoc() && !GetBindingParent())', file /Users/skywalker/Desktop/Mozilla/cvs/mozilla/content/base/src/nsGenericElement.cpp, line 2177

Comment 8

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

I do think we want this since potentially this can cause inconsistencies leading to crashes. Keeping low priority though until we've actually seen that.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Same happens on Windows when restarting Firefox after a crash and the Session Restore error page is loading.

###!!! ASSERTION: Shallow unbind won't clear document and binding parent on kids
!: 'aDeep || (!GetCurrentDoc() && !GetBindingParent())', file c:/mozilla/src/con
tent/base/src/nsGenericElement.cpp, line 2654

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Same happens on Windows when restarting Firefox after a crash and the Session
> Restore error page is loading.

Are you sure it's "same"?  This assertion will fire any time someone screws up binding/unbinding using nsIContent APIs.  This bug describes one way it can happen, but there are plenty of others.  I strongly suggest that unless you have some data indicating that the other screwup is the exact XBL screwup described here (which, honestly, I doubt) filing a bug to investigate the issue is a lot more productive than assuming that it's the same as this one.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sorry, filed that as bug 460686.

Comment 12

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

I can't reproduce this (on OSX) anymore.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Me too. Smaug, is it also fixed by the patch on bug 461027?

Comment 14

[Jesse Ruderman]

This testcase WFM, but I still see the assertion frequently.

Comment 15

[Jesse Ruderman]

in-testsuite-: very slow testcase