Closed Bug 508927 Opened 15 years ago Closed 15 years ago

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


(Core :: XUL, defect, P2)




Tracking Status
status1.9.2 --- beta1-fixed
blocking1.9.1 --- .4+
status1.9.1 --- .4-fixed


(Reporter: jruderman, Assigned: tnikkel)



(4 keywords, Whiteboard: [sg:critical?])


(6 files, 3 obsolete files)

Attached file 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.)
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!
Er.. how do we have two different nsGridRowLeaf frames for the same content node?
Group: core-security
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?
Seems to me that it could use BindingManager()->GetXBLChildNodesFor instead.
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.
Attached patch patch (obsolete) — Splinter Review
Instead of looking at the binding parent, use ChildIterator to access the XBL nodes of the listboxbody content.
Assignee: nobody → tnikkel
Attachment #393619 - Flags: review?(bzbarsky)
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.
Attachment #393620 - Flags: review?(bzbarsky)
Attachment #393620 - Flags: review?(bzbarsky) → review+
Comment on attachment 393619 [details] [diff] [review]

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.
Attachment #393619 - Flags: review?(bzbarsky) → review-
Hmm, so just pass aContainer from the frame constructor to OnContentRemoved and use that to ensure we get the right container?
That's not a bad idea at all.  Let's do that.
Attached patch patch v2Splinter Review
Pass aContainer from the frame constructor to OnContentRemoved and use that instead.
Attachment #393619 - Attachment is obsolete: true
Attachment #394205 - Flags: review?(bzbarsky)
Comment on attachment 394205 [details] [diff] [review]
patch v2

Attachment #394205 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Hmm.  Do we need this on 1.9.1 or earlier branches, do you think?
Flags: blocking1.9.2?
Comment on attachment 394205 [details] [diff] [review]
patch v2

a=beltzner for mozilla-central and mozilla-1.9.2
Pushed the debug fixup:

And the main patch:
Closed: 15 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Keywords: fixed1.9.2
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).
blocking1.9.1: --- → ?
status1.9.1: --- → ?
Flags: blocking1.9.0.15?
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?
blocking1.9.1: ? → .4+
Flags: blocking1.9.0.15? → blocking1.9.0.15+
Attached patch followup patch (obsolete) — Splinter Review
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.
Attachment #394997 - Flags: review?(bzbarsky)
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.
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?
Attachment #394997 - Flags: review?(bzbarsky) → review+
Comment on attachment 394997 [details] [diff] [review]
followup patch

Looks good, if you use NS_ERROR instead of NS_ASSERTION(PR_FALSE
Attached patch followup patch v2 (obsolete) — Splinter Review

Also included the crashing testcase as a crashtest.
Attachment #394997 - Attachment is obsolete: true
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?
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.
Attachment #396259 - Attachment is obsolete: true
Setting testsuite back to ? so the crashtest gets in later.
Flags: in-testsuite+ → in-testsuite?
Pushed followup patch as

Timothy, please request branch approval flags as needed on the patches you think should go on branches?
Removing the fixed1.9.2 flag too, in case we want to land the followup there.
Keywords: fixed1.9.2
Attachment #396318 - Flags: approval1.9.2?
Attachment #396318 - Flags: approval1.9.1.4?
Attachment #396318 - Flags: approval1.9.0.15?
Attachment #396318 - Flags: approval1.9.1.4?
Attachment #396318 - Flags: approval1.9.1.4+
Attachment #396318 - Flags: approval1.9.0.15?
Attachment #396318 - Flags: approval1.9.0.15+
Comment on attachment 396318 [details] [diff] [review]
followup patch v3

Approved for and, a=dveditz for release-drivers
Attachment #396635 - Attachment description: Merged to 1.9.0 → Merged to 1.9.1 and 1.9.0
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

Attachment #396318 - Flags: approval1.9.2? → approval1.9.2+
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Keywords: checkin-needed
Whiteboard: [needs 192 landing]
Verified for using testcase and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2009091321 GranParadiso/3.0.15pre (.NET CLR 3.5.30729).

Verified for with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv: Gecko/20090914 Shiretoko/3.5.4pre.
Assuming this bug was potentially "bad".
Whiteboard: [sg:critical?]
Group: core-security
The first testcase is already checked in as a crashtest.  I just checked in the second:
Flags: in-testsuite? → in-testsuite+
Depends on: 545061
No longer depends on: 545061
You need to log in before you can comment on or make changes to this bug.