514300 - Crash [@ FindBodyContent] with xul:listbox, XBL

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Boris Zbarsky [:bzbarsky]]

In FindBodyContent, we have aParent->IsInDoc() true, but childContent->IsInDoc() is false, so we crash when trying to get the binding manager off childContent->GetDocument() in FindBodyContent when recursing on it.

aParent above is a <xul:listitem> and the child is <html:span>.  It looks like this:

 document.getElementById('listbox').removeChild(document.getElementById('span'))

doesn't actually remove the <span> from the XBL child list for the <listitem> for some reason.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The xbl part is the usual mess where nsBindingManager::GetXBLChildNodesInternal will return the "other" content list, the one that doesn't get updated, if the GetAnonymousNodesInternal return value has length 0.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, I'm sick of the GetXBLChildNodes thing.  Patch coming up for it; that patch also fixes bug 415301 and bug 495354 by doing bug 495354 comment 9 item 1.

As a separate note, GetDocument here is wrong.  It should be GetOwnerDoc(), and that would also fix the crash.  I'll put up a patch for that too.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1: Use GetOwnerDoc

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2: remove from the binding manager's data structures better

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 with a needed ops check

Comment 8

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/mozilla-central/rev/7963a18d3373
  http://hg.mozilla.org/mozilla-central/rev/710cca46f64d

Jonas, Jesse, what do you think of taking this on 1.9.2 once it's baked a bit?

Comment 9

[Jesse Ruderman]

I would like that, given the kinds of assertions it fixes.  How much of a behavior change is it?

Comment 10

[Boris Zbarsky [:bzbarsky]]

And pushed http://hg.mozilla.org/mozilla-central/rev/52081e8986b7 to actually add the reftests to reftest.list.

As far as behavior change...  I think this should be pretty safe.  Offhand I can't think of a situation where it would break something (else I wouldn't have asked).  But this code is... finicky.  :(

Comment 11

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

I think we should take this on 1.9.2 after some m-c baking.