Last Comment Bug 472950 - "ASSERTION: child list is not empty for initial reflow" with :first-letter, rtl, pre
: "ASSERTION: child list is not empty for initial reflow" with :first-letter, r...
Status: VERIFIED FIXED
[sg:critical?]
: assertion, fixed1.9.0.12, mlk, testcase, verified1.9.1
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Mac OS X
: P2 normal (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
Mentors:
Depends on:
Blocks: randomclasses 5588 framedest
  Show dependency treegraph
 
Reported: 2009-01-09 19:09 PST by Jesse Ruderman
Modified: 2015-04-16 11:12 PDT (History)
12 users (show)
roc: blocking1.9.1+
dveditz: blocking1.9.0.12+
dveditz: wanted1.9.0.x+
stransky: wanted1.8.1.x-
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (409 bytes, text/html)
2009-01-09 19:09 PST, Jesse Ruderman
no flags Details
fix (2.20 KB, patch)
2009-02-05 03:25 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
Details | Diff | Splinter Review
possible fix for the remaining assertion (902 bytes, patch)
2009-02-05 03:34 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review

Description Jesse Ruderman 2009-01-09 19:09:11 PST
Created attachment 356288 [details]
testcase

###!!! ASSERTION: child list is not empty for initial reflow: 'mFrames.IsEmpty()', file /Users/jruderman/central/layout/generic/nsInlineFrame.cpp, line 324

###!!! ASSERTION: Some objects allocated with AllocateFrame were not freed: 'mFrameCount == 0', file /Users/jruderman/central/layout/base/nsPresShell.cpp, line 673

These are the same assertions as in bug 429968, which was fixed recently.  This is probably related to bug 429969.  Security-sensitive because of the second assertion and the relation to those two bugs.
Comment 1 Boris Zbarsky [:bz] (TPAC) 2009-01-19 19:26:40 PST
Yeah, this looks a _lot_ like bug 429968, comeplete with white-space:pre newlines inside first-letter, etc...

roc, care to take a look?
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-01-19 19:36:40 PST
yeah
Comment 3 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-01-27 17:18:17 PST
This is NOT fixed by the patch in bug 460389, for what it's worth.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-04 20:50:28 PST
OK. After the boom() script fires, before we reflow the document after the style change, the relevant part of the frame tree looks like

              Block(div)(1)@0x9245a24 next=0x9245cbc {0,0,59520,1152} [state=a0100000] sc=0x91ec084(i=1,b=0)<
                line 0x9245e18: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4100] {0,0,1521,1152} <
                  Inline(span)(0)@0x9245b1c {0,96,1521,960} [state=00e00000] [content=0xe3d34f0] [sc=0x91ec0f8]<
                    Inline(span)(0)@0x9245ba4 next=0x9245ddc {0,720,0,0} [state=00e00000] [content=0xc5bb2e0] [sc=0x91ec174]<
                      Text(0)@0x9245c2c[0,12,T]  {0,0,0,0} [state=08220000] [content=0x7e210b0] sc=0x91ec1c4 pst=:-moz-non-element<
                        "\n      \n    "
                      >
                    >
                    Letter(span)(0)@0x9245ddc next=0x92460b0 next-continuation=0x92460b0 {0,0,641,960} [content=0xe3d34f0] [sc=0x9245bdc] pst=:first-letter<
                      Text(1)@0x9245d9c[0,1,F]  next-continuation=0x924606c {0,0,641,960} [state=80320200] SELECTED [content=0xc5e78f0] sc=0x9245b54 pst=:-moz-non-element<
                        "A"
                      >
                    >
                    Letter(span)(0)@0x92460b0 prev-continuation=0x9245ddc {641,0,880,960} [state=00000004] [content=0xe3d34f0] [sc=0x9245d4c] pst=:-moz-non-element<
                      Text(1)@0x924606c[1,2,T]  prev-continuation=0x9245d9c {0,0,880,960} [state=90420204] SELECTED [content=0xc5e78f0] sc=0x9245d4c pst=:-moz-non-element<
                        " B"
                      >
                    >
                  >
                >
              >

But by the time we get to reflowing the first inline frame (0x9245b1c), the frame tree looks like

              Block(div)(1)@0x9245a24 next=0x9245cbc {0,0,59520,1152} [state=a0000401] sc=0x91ec084(i=1,b=0)<
                line 0x9245e18: count=3 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0xc040] {0,0,1521,1152} <
                  Inline(span)(0)@0x9245b1c next=0x91ec214 next-continuation=0x91ec214 {0,96,1521,960} [state=00e00401] [content=0xe3d34f0] [sc=0x91ec0f8]<
                    Inline(span)(0)@0x9245ba4 {0,720,0,0} [state=00e00000] [content=0xc5bb2e0] [sc=0x91ec174]<
                      Text(0)@0x9245c2c[0,12,T]  {0,0,0,0} [state=08220000] [content=0x7e210b0] sc=0x91ec1c4 pst=:-moz-non-element<
                        "\n      \n    "
                      >
                    >
                  >
                  Inline(span)(0)@0x91ec214 next=0x91ec24c prev-continuation=0x9245b1c next-continuation=0x91ec24c {0,0,0,0} [state=00000406] [content=0xe3d34f0] [sc=0x91ec0f8]<
                    Letter(span)(0)@0x9245ddc next-continuation=0x92460b0 {0,0,641,960} [content=0xe3d34f0] [sc=0x9245bdc] pst=:first-letter<
                      Text(1)@0x9245d9c[0,3,T]  next-continuation=0x924606c {0,0,641,960} [state=80320200] SELECTED [content=0xc5e78f0] sc=0x9245b54 pst=:-moz-non-element<
                        "A B"
                      >
                    >
                  >
                  Inline(span)(0)@0x91ec24c prev-continuation=0x91ec214 {0,0,0,0} [state=00000402] [content=0xe3d34f0] [sc=0x91ec0f8]<
                    Letter(span)(0)@0x92460b0 prev-continuation=0x9245ddc {641,0,880,960} [state=00000004] [content=0xe3d34f0] [sc=0x9245d4c] pst=:-moz-non-element<
                      Text(1)@0x924606c[3,0,T]  prev-continuation=0x9245d9c {0,0,880,960} [state=90420204] SELECTED [content=0xc5e78f0] sc=0x9245d4c pst=:-moz-non-element<
                        ""
                      >
                    >
                  >

It looks like the bidi resolver has split the inline frame into three parts. The new continuations, in particular 0x91ec214, have NS_FRAME_FIRST_REFLOW set, but 0x91ec214 does of course have children. Next, the text frame 0x9245c2c (which is of course white-space:pre now) splits after the first \n, the inline frame 0x9245b1c gets some overflow frames, and then when we reflow 0x91ec214 we assert because it's prevInFlow is non-null, there are prevOverflowFrames, its NS_FRAME_FIRST_REFLOW is set, and there are child frames. Then it proceeds to ignore the child frames, just doing mFrames.SetFrames which leaks the existing child frames.

As far as I can tell, there is nothing particularly wrong with what the bidi resolver is doing and the situation we get into here is legitimate, so we should just handle it without asserting or leaking.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-05 03:25:09 PST
Created attachment 360691 [details] [diff] [review]
fix

This is pretty straightforward and safe. We need to be more conservative about doing the lazyParentPointer optimization, and we should only do it when all the frame's children are going to be pulled from its prev-in-flow's overflow. This fixes the bug.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-05 03:32:56 PST
Unfortunately, with that patch, we still assert in the testcase, although we don't leak. We assert at
        NS_ASSERTION(mFrames.ContainsFrame(nextInFlow), "unexpected flow");

The fundamental problem here is in the bidi resolver. Before bidi resolution the relevant frame subtree looks like
                  Inline(span)(0)@0x17e851c {0,96,1521,960} [state=00e00400] [content=0xc363ca0] [sc=0x78a06e4]<
                    Inline(span)(0)@0x17e85a4 next=0x17e87dc {0,720,0,0} [state=00e00000] [content=0xc3643b0] [sc=0x78a0760]<
                      Text(0)@0x17e862c[0,12,T]  {0,0,0,0} [state=08220000] [content=0xc3643e0] sc=0x78a07b0 pst=:-moz-non-element<
                        "\n      \n    "
                      >
                    >
                    Letter(span)(0)@0x17e87dc next=0x17e8ab0 next-continuation=0x17e8ab0 {0,0,641,960} [content=0xc363ca0] [sc=0x14bb1a4] pst=:first-letter<
                      Text(1)@0x17e879c[0,1,F]  next-continuation=0x17e8a6c {0,0,641,960} [state=80320200] SELECTED [content=0xc364430] sc=0x14bb154 pst=:-moz-non-element<
                        "A"
                      >
                    >
                    Letter(span)(0)@0x17e8ab0 prev-continuation=0x17e87dc {641,0,880,960} [state=00000004] [content=0xc363ca0] [sc=0x17e8554] pst=:-moz-non-element<
                      Text(1)@0x17e8a6c[1,2,T]  prev-continuation=0x17e879c {0,0,880,960} [state=90420204] SELECTED [content=0xc364430] sc=0x17e8554 pst=:-moz-non-element<
                        " B"
                      >
                    >
                  >
                >

After bidi resolution it looks like this:
                line 0x17ef818: count=3 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0xc040] {0,0,1521,1152} <
                  Inline(span)(0)@0x17ef51c next=0x14a6400 next-continuation=0x14a6400 {0,96,1521,960} [state=00e00401] [content=0xc363900] [sc=0x14a62e4]<
                    Inline(span)(0)@0x17ef5a4 {0,720,0,0} [state=00e00000] [content=0xc364760] [sc=0x14a6360]<
                      Text(0)@0x17ef62c[0,12,T]  {0,0,0,0} [state=08220000] [content=0xc364790] sc=0x14a63b0 pst=:-moz-non-element<
                        "\n      \n    "
                      >
                    >
                  >
                  Inline(span)(0)@0x14a6400 next=0x14a6438 prev-continuation=0x17ef51c next-continuation=0x14a6438 {0,0,0,0} [state=00000406] [content=0xc363900] [sc=0x14a62e4]<
                    Letter(span)(0)@0x17ef7dc next-continuation=0x17efab0 {0,0,641,960} [content=0xc363900] [sc=0x17f65a4] pst=:first-letter<
                      Text(1)@0x17ef79c[0,3,T]  next-continuation=0x17efa6c {0,0,641,960} [state=80320200] SELECTED [content=0xc3647e0] sc=0x17f6554 pst=:-moz-non-element<
                        "A B"
                      >
                    >
                  >
                  Inline(span)(0)@0x14a6438 prev-continuation=0x14a6400 {0,0,0,0} [state=00000402] [content=0xc363900] [sc=0x14a62e4]<
                    Letter(span)(0)@0x17efab0 prev-continuation=0x17ef7dc {641,0,880,960} [state=00000004] [content=0xc363900] [sc=0x17ef554] pst=:-moz-non-element<
                      Text(1)@0x17efa6c[3,0,T]  prev-continuation=0x17ef79c {0,0,880,960} [state=90420204] SELECTED [content=0xc3647e0] sc=0x17ef554 pst=:-moz-non-element<
                        ""
                      >
                    >
                  >
                >

The real problem is not shown in the dump, but it's that the two letter frames are fluid continuations, but the bidi resolver decides to split the parent span at the boundary between them anyway, but the boundary in the span is fixed, not fluid. So we have a situation where a fluid continuation chain spans a fixed continuation boundary in the parent, which is wrong. I think bidi should not be splitting the parent span in this case.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-05 03:34:25 PST
Created attachment 360692 [details] [diff] [review]
possible fix for the remaining assertion

This might work but I really don't understand this code so I'm not recommending it as a fix yet. I'll run reftests and crashtests at least first.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-02-05 08:33:34 PST
(In reply to comment #6)
> The real problem is not shown in the dump, but it's that the two letter frames
> are fluid continuations, but the bidi resolver decides to split the parent span
> at the boundary between them anyway, but the boundary in the span is fixed, not
> fluid. So we have a situation where a fluid continuation chain spans a fixed
> continuation boundary in the parent, which is wrong. I think bidi should not be
> splitting the parent span in this case.

It seems to me that in that case (in general), bidi should be overriding and turning that fluid continuation into a fixed one.

(That said, bug 460389 is also in this area; there are cases where bidi code is definitely splitting too much around letter frames.)
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-06 02:05:24 PST
(In reply to comment #8)
> (In reply to comment #6)
> > The real problem is not shown in the dump, but it's that the two letter frames
> > are fluid continuations, but the bidi resolver decides to split the parent span
> > at the boundary between them anyway, but the boundary in the span is fixed, not
> > fluid. So we have a situation where a fluid continuation chain spans a fixed
> > continuation boundary in the parent, which is wrong. I think bidi should not be
> > splitting the parent span in this case.
> 
> It seems to me that in that case (in general), bidi should be overriding and
> turning that fluid continuation into a fixed one.

Hmm ... I don't think we want first-letter or first-line continuations to become fixed.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-07 00:48:09 PST
Pushed http://hg.mozilla.org/mozilla-central/rev/b1358e48e6ca

I'm marking this fixed to get it off the radar. The remaining assertion doesn't lead to anything dangerous, as far as I can tell, and should probably just move to a new bug.
Comment 11 Jesse Ruderman 2009-02-07 16:29:27 PST
The assertion is probably the same issue as what's left in bug 429969.  The testcases use the same ingredients.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-26 12:56:45 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/05f6a829ca45
Comment 13 Simon Montagu :smontagu 2009-03-03 12:19:30 PST
Comment on attachment 360692 [details] [diff] [review]
possible fix for the remaining assertion

The remaining assertion is fixed by the patch in bug 332655
Comment 14 Daniel Veditz [:dveditz] 2009-05-15 08:22:41 PDT
Now that we've fixed this we ought to be able to move this out of the [sg:investigate] bucket -- was this a vulnerability or not? If it was, do we need this fix on the 1.9.0 branch which has the same assertions?
Comment 15 Daniel Veditz [:dveditz] 2009-05-22 10:18:42 PDT
OK, Assuming the worst based on roc making this block the 1.9.1 release.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-06-11 20:06:44 PDT
Checked into 1.9.0.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-06-11 20:07:38 PDT
(The patch applied cleanly.)
Comment 18 Martin Stránský 2009-07-07 05:38:54 PDT
Does not seem to affect 1.8.
Comment 19 Aakash Desai [:aakashd] 2009-08-11 08:57:07 PDT
verified FIXED on debug builds (using the attached testcases):

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a2pre) Gecko/20090811 Minefield/3.6a2pre ID:20090811082642

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.3pre) Gecko/20090811 Shiretoko/3.5.3pre ID:20090811074904

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