360078 - Crash [@ nsHTMLAnchorElement::UnbindFromTree] with XBL, XUL, and unused cloneNode

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Jesse Ruderman]

Loading the testcase makes Firefox crash after 5-15 seconds.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

Attachment: stack trace (mac debug / gdb)

This crash happens with a lot of destructors on the stack, e.g. nsDocument::~nsDocument.  Sometimes it happens during GC, sometimes not, but the top line of the stack is always the same.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000

0x081c6060 in nsHTMLAnchorElement::UnbindFromTree (this=0x248f7ee0, aDeep=0, aNullParent=1) at /Users/admin/trunk/mozilla/content/html/content/src/nsHTMLAnchorElement.cpp:201

201         GetCurrentDoc()->ForgetLink(this);

(gdb) p IsInDoc()
$2 = 1

(gdb) p GetCurrentDoc()
$1 = (nsIDocument *) 0x0

Comment 3

[Jesse Ruderman]

I spent 2 days (!) making the reduced testcase after bc pointed the crash out to me, so I hope it gets fixed :)

Comment 4

[Bernd]

but now your testcase works cross platform winxp 2006110704

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm... So the basic problem here is bug 53901.  I would guess that the sequence of events is the following:

1)  We clone the XUL node.  Due to bug 53901 it flags itself as being in
    document.
2)  We attach the XBL binding.  The anonymous nodes now think they're in the
    document.
3)  The document is destroyed.  It notifies the nodeinfo manager, so now all
    nodes with that document have a null GetOwnerDoc()/GetCurrentDoc().
4)  Destruction of the document destroys the binding manager, which tears down 
    the binding, releases the anonymous content, and enters the code in
    question.

We could change step 3 to release the binding manager before notifying the nodeinfo manager, but that would sorta violate he invariant that we have a binding manager for the lifetime of the document.  I think it also wouldn't help in the case when step 2 is replaced by an explicit DOM appendChild call (followed by holding to the little subtree until after the document has gone away; could happen either on purpose or due to the ordering of GC finalization).

So really, we need to either fix bug 53901, fix what happens when nodes are appended to such a lying XUL node, or add null-checks everywhere where we assume that IsInDoc() means GetCurrentDoc() is non-null.

I vote we fix bug 53901, except that I doubt we can do that on branches.... and I'm not sure we can do it for 1.9.  :(

Comment 6

[Jesse Ruderman]

> or add null-checks everywhere where we
> assume that IsInDoc() means GetCurrentDoc() is non-null

Or change IsInDoc to check that GetOwnerDoc() != nsnull?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Or change IsInDoc to check that GetOwnerDoc() != nsnull?

Or that for now, yeah....  That might work...  Sorta.

Comment 8

[Jesse Ruderman]

Bug 53901 is fixed now, so should I expect this to be fixed?

The testcase in this bug doesn't work any more because I'm not allowed to load XBL cross-site (e.g. from software.hixie.ch).  If I change the testcase to use a data: URL (which is possible now that bug 366770 is fixed), I don't get a crash, but I do get a lot of

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

Comment 9

[Jesse Ruderman]

Attachment: testcase using data: URL

Comment 10

[Jesse Ruderman]

Should I morph this bug to be about the assertion in comment 8 or should I make that a new bug?

Comment 11

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

Please file a new bug on the assertion.

Comment 12

[Jesse Ruderman]

Ok, filed bug 397849.

Comment 13

[Jesse Ruderman]

Crashtest checked in.