Closed Bug 472950 Opened 15 years ago Closed 15 years ago

"ASSERTION: child list is not empty for initial reflow" with :first-letter, rtl, pre

Categories

(Core :: Layout, defect, P2)

x86
macOS
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: roc)

References

Details

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

Attachments

(3 files)

Attached file 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.
Whiteboard: [sg:investigate]
Yeah, this looks a _lot_ like bug 429968, comeplete with white-space:pre newlines inside first-letter, etc...

roc, care to take a look?
Flags: blocking1.9.1?
yeah
Assignee: nobody → roc
This is NOT fixed by the patch in bug 460389, for what it's worth.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
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.
Attached patch fixSplinter Review
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.
Attachment #360691 - Flags: review?(dbaron)
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.
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.
Attachment #360692 - Flags: review?(smontagu)
Whiteboard: [sg:investigate] → [sg:investigate][needs review]
(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.)
(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.
Whiteboard: [sg:investigate][needs review] → [sg:investigate][needs review][needs landing]
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.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [sg:investigate][needs review][needs landing] → [sg:investigate][needs review][needs 191 landing]
Flags: in-testsuite+
The assertion is probably the same issue as what's left in bug 429969.  The testcases use the same ingredients.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/05f6a829ca45
Keywords: fixed1.9.1
Whiteboard: [sg:investigate][needs review][needs 191 landing] → [sg:investigate][needs review]
Comment on attachment 360692 [details] [diff] [review]
possible fix for the remaining assertion

The remaining assertion is fixed by the patch in bug 332655
Attachment #360692 - Flags: review?(smontagu)
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?
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.12?
OK, Assuming the worst based on roc making this block the 1.9.1 release.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.12?
Flags: blocking1.9.0.12+
Whiteboard: [sg:investigate][needs review] → [sg:critical?][needs review]
Checked into 1.9.0.
Keywords: fixed1.9.0
Whiteboard: [sg:critical?][needs review] → [sg:critical?]
(The patch applied cleanly.)
Does not seem to affect 1.8.
Flags: wanted1.8.1.x-
Group: core-security
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
Status: RESOLVED → VERIFIED
Blocks: 5588
You need to log in before you can comment on or make changes to this bug.