Last Comment Bug 508927 - "ASSERTION: returning frame that is not in childlist" with xul:listboxbody, XBL
: "ASSERTION: returning frame that is not in childlist" with xul:listboxbody, XBL
Status: RESOLVED FIXED
: [sg:critical?]
: assertion, testcase, verified1.9.0.15, verified1.9.1
Product: Core
Classification: Components
Component: XUL
: Trunk
: x86 Mac OS X
: P2 normal (vote)
: ---
Assigned To: Timothy Nikkel (:tn)
: xptoolkit.widgets
:
:
: 348483 488210
  Show dependency treegraph
 
Reported: 2009-08-06 17:25 PDT by Jesse Ruderman
Modified: 2010-02-08 19:49 PST (History)
12 users (show)
roc: blocking1.9.2+
samuel.sidler+old: blocking1.9.0.15+
jruderman: in‑testsuite+
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta1-fixed
  .4+
  .4-fixed


Attachments
testcase (514 bytes, application/vnd.mozilla.xul+xml)
2009-08-06 17:25 PDT, Jesse Ruderman
no flags Details
patch (10.03 KB, patch)
2009-08-10 14:52 PDT, Timothy Nikkel (:tn)
bzbarsky: review-
Details | Diff | Splinter Review
minor content debug listing nit (723 bytes, patch)
2009-08-10 14:54 PDT, Timothy Nikkel (:tn)
bzbarsky: review+
Details | Diff | Splinter Review
patch v2 (12.97 KB, patch)
2009-08-12 19:46 PDT, Timothy Nikkel (:tn)
bzbarsky: review+
Details | Diff | Splinter Review
testcase that crashes as well as asserts (756 bytes, application/vnd.mozilla.xul+xml)
2009-08-13 22:12 PDT, Timothy Nikkel (:tn)
no flags Details
followup patch (2.40 KB, patch)
2009-08-17 21:44 PDT, Timothy Nikkel (:tn)
bzbarsky: review+
Details | Diff | Splinter Review
followup patch v2 (3.57 KB, patch)
2009-08-24 11:52 PDT, Timothy Nikkel (:tn)
no flags Details | Diff | Splinter Review
followup patch v3 (2.46 KB, patch)
2009-08-24 15:34 PDT, Timothy Nikkel (:tn)
roc: approval1.9.2+
dveditz: approval1.9.1.4+
dveditz: approval1.9.0.15+
Details | Diff | Splinter Review
Merged to 1.9.1 and 1.9.0 (2.37 KB, patch)
2009-08-25 18:40 PDT, Boris Zbarsky (:bz)
no flags Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2009-08-06 17:25:09 PDT
Created attachment 393075 [details]
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 (:bz) 2009-08-06 20:05:07 PDT
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 (:bz) 2009-08-06 20:07:06 PDT
Er.. how do we have two different nsGridRowLeaf frames for the same content node?
Comment 3 Boris Zbarsky (:bz) 2009-08-06 20:13:31 PDT
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 neil@parkwaycc.co.uk 2009-08-07 03:00:52 PDT
Seems to me that it could use BindingManager()->GetXBLChildNodesFor instead.
Comment 5 Boris Zbarsky (:bz) 2009-08-07 06:02:30 PDT
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 (:tn) 2009-08-10 14:52:45 PDT
Created attachment 393619 [details] [review]
patch

Instead of looking at the binding parent, use ChildIterator to access the XBL nodes of the listboxbody content.
Comment 7 Timothy Nikkel (:tn) 2009-08-10 14:54:59 PDT
Created attachment 393620 [details] [review]
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 (:bz) 2009-08-12 12:57:28 PDT
Comment on attachment 393619 [details] [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 (:tn) 2009-08-12 17:35:15 PDT
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 (:bz) 2009-08-12 19:06:09 PDT
That's not a bad idea at all.  Let's do that.
Comment 11 Timothy Nikkel (:tn) 2009-08-12 19:46:47 PDT
Created attachment 394205 [details] [review]
patch v2

Pass aContainer from the frame constructor to OnContentRemoved and use that instead.
Comment 12 Boris Zbarsky (:bz) 2009-08-12 20:25:06 PDT
Comment on attachment 394205 [details] [review]
patch v2

r=bzbarsky
Comment 13 Boris Zbarsky (:bz) 2009-08-13 07:48:21 PDT
Hmm.  Do we need this on 1.9.1 or earlier branches, do you think?
Comment 14 Mike Beltzner [:beltzner] 2009-08-13 07:57:58 PDT
Comment on attachment 394205 [details] [review]
patch v2

a=beltzner for mozilla-central and mozilla-1.9.2
Comment 16 Timothy Nikkel (:tn) 2009-08-13 22:05:39 PDT
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 (:tn) 2009-08-13 22:12:45 PDT
Created attachment 394458 [details]
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) 2009-08-17 16:45:52 PDT
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 (:tn) 2009-08-17 21:44:06 PDT
Created attachment 394997 [details] [review]
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 (:tn) 2009-08-17 21:47:31 PDT
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 (:tn) 2009-08-18 13:39:45 PDT
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 (:bz) 2009-08-24 09:20:46 PDT
Comment on attachment 394997 [details] [review]
followup patch

Looks good, if you use NS_ERROR instead of NS_ASSERTION(PR_FALSE
Comment 23 Timothy Nikkel (:tn) 2009-08-24 11:52:12 PDT
Created attachment 396259 [details] [review]
followup patch v2

Used NS_ERROR.

Also included the crashing testcase as a crashtest.
Comment 24 Boris Zbarsky (:bz) 2009-08-24 13:10:17 PDT
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 (:tn) 2009-08-24 15:34:24 PDT
Created attachment 396318 [details] [review]
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 (:tn) 2009-08-24 15:35:26 PDT
Setting testsuite back to ? so the crashtest gets in later.
Comment 27 Boris Zbarsky (:bz) 2009-08-25 05:38:00 PDT
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 (:bz) 2009-08-25 05:38:50 PDT
Removing the fixed1.9.2 flag too, in case we want to land the followup there.
Comment 29 Daniel Veditz 2009-08-25 16:17:34 PDT
Comment on attachment 396318 [details] [review]
followup patch v3

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz for release-drivers
Comment 30 Boris Zbarsky (:bz) 2009-08-25 18:40:24 PDT
Created attachment 396635 [details] [review]
Merged to 1.9.1 and 1.9.0
Comment 31 Boris Zbarsky (:bz) 2009-08-25 18:45:03 PDT
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] 2009-09-17 11:35:08 PDT
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 (:tn) 2009-09-21 18:41:36 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5c81af01b507
Comment 34 Daniel Veditz 2009-09-23 14:57:32 PDT
Assuming this bug was potentially "bad".
Comment 35 Jesse Ruderman 2009-12-06 17:06:19 PST
The first testcase is already checked in as a crashtest.  I just checked in the second:

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

Note You need to log in before you can comment on or make changes to this bug.