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

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.

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

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 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.

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.

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

Comment on attachment 394205 [details] [diff] [review] patch v2 r=bzbarsky

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

Comment on attachment 394205 [details] [diff] [review] patch v2 a=beltzner for mozilla-central and mozilla-1.9.2

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

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

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).

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?

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.

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?

Comment on attachment 394997 [details] [diff] [review] followup patch Looks good, if you use NS_ERROR instead of NS_ASSERTION(PR_FALSE

Used NS_ERROR. Also included the crashing testcase as a crashtest.

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.

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

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?

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

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

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

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.

Assuming this bug was potentially "bad".

The first testcase is already checked in as a crashtest. I just checked in the second: http://hg.mozilla.org/mozilla-central/rev/a86c5f1d3fef