508927 - "ASSERTION: returning frame that is not in childlist" with xul:listboxbody, XBL

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: returning frame that is not in childlist: '!result->IsBoxFrame() || result->GetParent() == this', file /Users/jruderman/central/layout/xul/base/src/nsListBoxBodyFrame.cpp, line 1263

This assertion was added as part of rev 4eb507b977b5 (Bug 488210.  Stop returning non-listitems' frames from GetNextItemBox.)

Comment 1

[Boris Zbarsky [:bzbarsky]]

nsGridRowGroup(listrows)(-1)@0x1eb0a4f8 {0,0,900,1920} [state=80841000] [content=0x1e2aa350] [sc=0x1eb0a49c]<
  XULScroll(listboxbody)(0)@0x1eb142c8 [view=0x1e2abca0] next=0x1eb148b4 {0,0,900,1560} [state=80843000] [content=0x1e2aa380] [sc=0x1eb0a564]<
    ScrollbarFrame(scrollbar)(-1)@0x1eb14390 next=0x1eb14210 {0,0,900,1560} [state=808c0000] [content=0x1e2abc30] [sc=0x1eb0a608]<
      SliderFrame(slider)(-1)@0x1eb145d8 [view=0x1e2ac270] next=0x1eb14730 {0,0,900,900} [state=80802000] [content=0x1e2abeb0] [sc=0x1eb144c4]<
        ButtonBoxFrame(thumb)(0)@0x1eb146c4 {0,360,900,49} [state=80000000] [content=0x1e2abee0] [sc=0x1eb14668]<>
      >
      ButtonBoxFrame(scrollbarbutton)(-1)@0x1eb14730 next=0x1eb147a0 {0,900,900,960} [state=80c00000] [content=0x1e2abf30] [sc=0x1eb14520]<>
      ButtonBoxFrame(scrollbarbutton)(-1)@0x1eb147a0 {0,1860,900,960} [state=80c00000] [content=0x1e2abf60] [sc=0x1eb1457c]<>
    >
    Box(listboxbody)(0)@0x1eb14210 [view=0x1e2ac0c0] {0,0,0,1680} [state=80803000] [content=0x1e2aa380] [sc=0x1eb14810] pst=:-moz-scrolled-content<
      nsGridRowLeaf(listitem)(0)@0x1eb15184 {0,0,0,0} [state=80c00412] [content=0x1e29b030] [sc=0x1eb15128]<
        Box(listcell)(-1)@0x12bf86c {0,0,0,0} [state=80400402] [content=0x1e2aaa90] [sc=0x12bf810]<
          XULLabel(label)(-1)@0x12bf934 {0,0,0,0} [state=00d00402] sc=0x12bf8d8(i=0,b=0)<
          >
        >
      >
    >
  >
  nsGridRowLeaf(listitem)(0)@0x1eb148b4 next=0x1eb14ed8 {0,1560,900,180} [state=80c00000] [content=0x1e29b030] [sc=0x1eb0a71c]<
    Box(listcell)(-1)@0x1eb14b28 {60,60,240,60} [state=80400000] [content=0x1e2aaa90] [sc=0x1eb14a38]<
      XULLabel(label)(-1)@0x1eb14e58 {0,0,240,60} [state=00d00000] sc=0x1eb14d68(i=0,b=0)<
      >
    >
  >
  nsGridRowLeaf(listitem)(1)@0x1eb14ed8 {0,1740,900,180} [state=80c00000] [content=0x1e29b080] [sc=0x1eb0a71c]<
    Box(listcell)(-1)@0x1eb14f44 {60,60,240,60} [state=80400000] [content=0x1e2abad0] [sc=0x1eb14a38]<
      XULLabel(label)(-1)@0x1eb14fb0 {0,0,240,60} [state=00d00000] sc=0x1eb14d68(i=0,b=0)<
      >
    >
  >
>
(gdb) p this
$4 = (nsListBoxBodyFrame *) 0x1eb14210
(gdb) p result->GetParent()
$5 = (nsGridRowGroupFrame *) 0x1eb0a4f8
(gdb) p result
$6 = (nsListItemFrame *) 0x1eb14ed8

So yeah... we're returning a sibling frame, effectively!

Comment 2

[Boris Zbarsky [:bzbarsky]]

Er.. how do we have two different nsGridRowLeaf frames for the same content node?

Comment 3

[Boris Zbarsky [:bzbarsky]]

So the key is that nsListBoxBodyFrame::GetListItemContentAt grabs the bindingParent of the listboxbody, then starts grabbing its listitem kids.  I have no idea why it's doing that, but it's clearly NOT the right thing to do in this case.

Neils, any idea why it's doing that bindingParent thing?

Comment 4

[[e-mail]]

Seems to me that it could use BindingManager()->GetXBLChildNodesFor instead.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, that's what Timothy and I were talking about last night.  Even better, it could just use nsChildIterator, right?  He's going to look into that, if I understood correctly.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Instead of looking at the binding parent, use ChildIterator to access the XBL nodes of the listboxbody content.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: minor content debug listing nit

In debug listing of XUL content get rid of the leading '<' so that angle brackets balance.

This is useful if you are looking at a big content tree dump and use a text editor that can match brackets.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 393619 [details] [diff] [review]
patch

If we're in OnContentRemoved, that means that ContentRemoved was called with the listbox as the aContainer.  So in that case the aIndex is in fact for the child list of the listbox, not for the flattened tree child list of the listboxbody...  So that part of the change doesn't look right to me, where we use aIndex to seek the child iterator.  The part where we're seeking to end, on the other hand, does need to use an ChildIterator.

Other than that, looks good.

Comment 9

[Timothy Nikkel (:tnikkel)]

Hmm, so just pass aContainer from the frame constructor to OnContentRemoved and use that to ensure we get the right container?

Comment 10

[Boris Zbarsky [:bzbarsky]]

That's not a bad idea at all.  Let's do that.

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Pass aContainer from the frame constructor to OnContentRemoved and use that instead.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 394205 [details] [diff] [review]
patch v2

r=bzbarsky

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.  Do we need this on 1.9.1 or earlier branches, do you think?

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 394205 [details] [diff] [review]
patch v2

a=beltzner for mozilla-central and mozilla-1.9.2

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pushed the debug fixup:
http://hg.mozilla.org/mozilla-central/rev/8c402112ba9c
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c7d1cd3d77a7

And the main patch:
http://hg.mozilla.org/mozilla-central/rev/a8f39e150cc1
http://hg.mozilla.org/mozilla-central/rev/a8f39e150cc1

Comment 16

[Timothy Nikkel (:tnikkel)]

Let me fix the links for the push so that the record is correct. They should be as follows.

Pushed the debug fixup:
http://hg.mozilla.org/mozilla-central/rev/8c402112ba9c
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c7d1cd3d77a7

And the main patch:
http://hg.mozilla.org/mozilla-central/rev/a8f39e150cc1
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ee13d35cf80e

Comment 17

[Timothy Nikkel (:tnikkel)]

Attachment: testcase that crashes as well as asserts

I think we do need this on the same branches we took bug 488210 (ie 1.9.1 and 1.9.0). Anytime the "returning frame that is not in childlist" assertion is triggered there is no reason why nsListBoxBodyFrame::RemoveChildFrame can not be called on whatever frame triggered the assertion, and hence we'll crash exactly the same way as bug 488210, with dangling pointers to a frame that has been destroyed. This testcase shows this is true, ie it crashes (I just added a bunch more listitems to Jesse's testcase so that RemoveChildFrame actually got called).

Comment 18

[Samuel Sidler (old account; do not CC)]

This is a decent-sized patch. Any comments on its safety for both branches? Does it apply as-is or will it need to be ported to 1.9.1 / 1.9.0?

Comment 19

[Timothy Nikkel (:tnikkel)]

Attachment: followup patch

Here is a much smaller patch which fixes the crash and I think should also be applied to trunk.

Returning a frame that is not in the childlist from GetNextItemBox can easily lead to a crash, so instead of checking for that in an assertion and proceeding, check for that in the code and don't return such a frame.

Also, like the assertion in RemoveChildFrame says, destroying a child frame we didn't remove will crash, so don't, just assert and return.

Comment 20

[Timothy Nikkel (:tnikkel)]

So the idea being we could apply this smaller patch to the branches. And as I said above, I also think it would be good on trunk.

Comment 21

[Timothy Nikkel (:tnikkel)]

David, would you be willing to give this patch some sort of review so it can get some bake time on trunk and then get a full review from bz when he gets back?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 394997 [details] [diff] [review]
followup patch

Looks good, if you use NS_ERROR instead of NS_ASSERTION(PR_FALSE

Comment 23

[Timothy Nikkel (:tnikkel)]

Attachment: followup patch v2

Used NS_ERROR.

Also included the crashing testcase as a crashtest.

Comment 24

[Boris Zbarsky [:bzbarsky]]

That last diff doesn't seem to apply to m-c.

And do we want to land the crashtest before we fix on all branches?

Comment 25

[Timothy Nikkel (:tnikkel)]

Attachment: followup patch v3

Whoops, sorry, more changed on m-c then I thought. Rebased to tip. And removed the crashtest so that we can add it once this is fixed everywhere.

Comment 26

[Timothy Nikkel (:tnikkel)]

Setting testsuite back to ? so the crashtest gets in later.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Pushed followup patch as http://hg.mozilla.org/mozilla-central/rev/2457c3db5ba6

Timothy, please request branch approval flags as needed on the patches you think should go on branches?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Removing the fixed1.9.2 flag too, in case we want to land the followup there.

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 396318 [details] [diff] [review]
followup patch v3

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz for release-drivers

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to 1.9.1 and 1.9.0

Comment 31

[Boris Zbarsky [:bzbarsky]]

Checking in layout/xul/base/src/nsListBoxBodyFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp,v  <--  nsListBoxBodyFrame.cpp
new revision: 1.103; previous revision: 1.102
done

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5d9e08875338

Comment 32

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.15 using testcase and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.15pre) Gecko/2009091321 GranParadiso/3.0.15pre (.NET CLR 3.5.30729).

Verified for 1.9.1.4 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.4pre) Gecko/20090914 Shiretoko/3.5.4pre.

Comment 33

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5c81af01b507

Comment 34

[Daniel Veditz [:dveditz]]

Assuming this bug was potentially "bad".

Comment 35

[Jesse Ruderman]

The first testcase is already checked in as a crashtest.  I just checked in the second:

http://hg.mozilla.org/mozilla-central/rev/a86c5f1d3fef