Closed Bug 429968 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jruderman, Assigned: roc)

References

Details

(6 keywords, Whiteboard: [sg:investigate] fixes 429969)

Attachments

(6 files)

testcase
396 bytes, text/html
Details
simpler testcase
190 bytes, text/html
Details
harder testcase
217 bytes, text/html
Details
work in progress
7.67 KB, patch
Details | Diff | Splinter Review
fix
11.31 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
1.9.0 patch
12.34 KB, patch
dveditz
: approval1.9.0.11+
Details | Diff | Splinter Review
Attached file testcase — 
Loading the testcase triggers:

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

Closing it triggers:

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

Reduced from a testcase Gary Kwong sent me.

Security-sensitive because the second assertion often indicates the presense of a security hole.
See also bug 429969, which triggers different assertions and a crash with a slightly different testcase.
This is a bug in nsBidiPresUtils::RemoveBidiContinuation, as far as I can tell.  We have a frametree like this:

                      Block(div)(0)@0x13bae44 {0,0,55440,1152} [state=a0000401] sc=0x13bacf8(i=1,b=0)<
                        line 0x13bb344: count=2 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x8141] {0,0,1333,1152} <
                          Inline(span)(0)@0x13baf6c next=0x13b2b90 next-continuation=0x13b2b90 {0,96,1333,960} [content=0xc8754f0] [sc=0x13baca8]<
                            Inline(span)(0)@0x13bb018 {0,0,0,960} [content=0xd990330] [sc=0x13bb170]<
                              Text(0)@0x13bb0a0[0,2,T]  {0,720,0,0} [state=08220000] [content=0x6a0e7b0] sc=0x13b2ca8 pst=:-moz-non-element<
                                "\n\n"
                              >
                            >
                          >
                          Inline(span)(0)@0x13b2b90 prev-continuation=0x13baf6c {0,0,0,0} [state=00000402] [content=0xc8754f0] [sc=0x13baca8]<
                            Letter(span)(0)@0x13bb2ac next=0x13bb6b0 next-continuation=0x13bb6b0 {0,0,693,960} [content=0xc8754f0] [sc=0x13b2b40] pst=:first-letter<
                              Text(1)@0x13bb26c[0,1,F]  next-continuation=0x13bb638 {0,0,693,960} [state=02300000] [content=0xca39ed0] sc=0x13b2af0 pst=:-moz-non-element<
                                "A"
                              >
                            >
                            Letter(span)(0)@0x13bb6b0 prev-continuation=0x13bb2ac {693,0,640,960} [state=00000004] [content=0xc8754f0] [sc=0x13bafc8] pst=:-moz-non-element<
                              Text(1)@0x13bb638[1,1,T]  prev-continuation=0x13bb26c {0,0,640,960} [state=02420004] [content=0xca39ed0] sc=0x13bafc8 pst=:-moz-non-element<
                                "B"
                              >
                            >
                          >
                        >
                      >
 
where the two inlines that are immediate kids of the block's line were split apart by bidi.  Now we're doing RemoveBidiContinuation, and making them in-flows of each other (fluid continuations)... but we've never reflown this stuff, and both frames have kids.  Just switching them to be fluid continuations is no good here.

Then later in nsInlineFrame::Reflow we assert and lose part of the framelist, hence the second assert.

Looks like a regression from bug 319930.
Blocks: 319930
Flags: wanted1.9.1?
Maybe we should just fix nsInlineFrame to not assume that it has no kids if it's an in-flow on its first reflow...
Depends on: 429969
Now I get:

###!!! ASSERTION: First-in-flow first-letter should have first-letter style enabled in nsLineLayout!: 'll->GetFirstLetterStyleOK() || GetPrevInFlow()', file /Users/jruderman/central/layout/generic/nsFirstLetterFrame.cpp, line 240
Flags: wanted1.9.1? → blocking1.9.1+
Priority: -- → P3
I still get the assertion from comment 4 with this testcase even after the change to it I made in bug 448610.

The frame tree I'm seeing is:

                  Block(div)(0)@0xaee95850 {0,0,55860,1140} [state=a0100401] sc=0xaee95728(i=3,b=0)<
                    line 0xaee95df8: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x5100] {55860,0,0,1140} <
                      Inline(span)(0)@0xaee959b0 next=0xb014a168 next-continuation=0xb014a168 {55860,0,0,1140} [content=0xaee6eae0] [sc=0xaee956d8]
<
                        Inline(span)(0)@0xaee95a5c next-continuation=0xaee7ff60 {0,0,0,1140} [content=0xaee6eb20] [sc=0xaee95bb4]
<
                          Text(0)@0xaee95ae4[0,0,F]  next=0xb014a0e0 next-continuation=0xb014a0e0 {0,900,0,0} [state=00200000] [content=0xaee8f5e0] sc=0xb014a090 pst=:-moz-non-element<
                            ""
                          >
                          Text(0)@0xb014a0e0[0,1,F]  prev-continuation=0xaee95ae4 next-continuation=0xb014a124 {0,0,0,1140} [state=80600004] [content=0xaee8f5e0] sc=0xb014a090 pst=:-moz-non-element<
                            "\n"
                          >
                        >
                      >
                    >
                    line 0xaee7f070: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x5100] {55860,1140,0,1140} <
                      Inline(span)(0)@0xb014a168 next=0xb014a1a0 prev-continuation=0xaee959b0 next-continuation=0xb014a1a0 {55860,1140,0,1140} [state=00000004] [content=0xaee6eae0] [sc=0xaee956d8]
<
                        Inline(span)(0)@0xaee7ff60 prev-continuation=0xaee95a5c {0,0,0,1140} [state=00000004] [content=0xaee6eb20] [sc=0xaee95bb4]
<
                          Text(0)@0xb014a124[1,1,T]  prev-continuation=0xb014a0e0 {0,0,0,1140} [state=80600004] [content=0xaee8f5e0] sc=0xb014a090 pst=:-moz-non-element<
                            "\n"
                          >
                        >
                      >
                    >
                    line 0xb014a1d8: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4000] {0,0,0,0} <
                      Inline(span)(0)@0xb014a1a0 prev-continuation=0xb014a168 {0,0,0,0} [state=00000407] [content=0xaee6eae0] [sc=0xaee956d8]
<
                        Letter(span)(0)@0xaee95d28 next=0xaee7f238 next-continuation=0xaee7f238 {0,0,720,1140} [state=00000401] [content=0xaee6eae0] [sc=0xaee7feb4] pst=:first-letter
<
                          Text(1)@0xaee95ce8[0,1,F]  next-continuation=0xaee7f1c4 {0,0,720,1140} [state=80300000] [content=0xaee8f610] sc=0xaee7fe64 pst=:-moz-non-element<
                            "A"
                          >
                        >
                        Letter(span)(0)@0xaee7f238 prev-continuation=0xaee95d28 {720,0,720,1140} [state=00000004] [content=0xaee6eae0] [sc=0xaee95a0c] pst=:-moz-non-element
<
                          Text(1)@0xaee7f1c4[1,1,T]  prev-continuation=0xaee95ce8 {0,0,720,1140} [state=80400004] [content=0xaee8f610] sc=0xaee95a0c pst=:-moz-non-element<
                            "B"
                          >
                        >
                      >
                    >
                  >

with this=0xaee95d28

Note that the testcase doesn't now trigger bidi resolution at all (at least in my local build).
The bug described in comment 2 is still present, though.  Perhaps we need a different testcase for it, but it's alive and kicking.
Indeed, and also bug 429969 doesn't seem to have changed its behaviour.
Whiteboard: [sg:investigate]
Attached file simpler testcase — 
This testcase causes the assertion to fire and also displays "AB" in red. In fact, both letters should be black.
Assignee: nobody → roc
The problem is that we don't wrap the newline in the span in a letter-frame, because it's all whitespace. We carry on and wrap "AB" in a letter-frame. But because it's white-space:pre, the newline does turn into a line break. So we turn off the first-letter flag in nsLineLayout when we move to the second line, and then we reflow the letter-frame and it asserts. Since we've turned off the first-letter flag in nsLineLayout, we don't break the text after the first letter and it all gets the first-letter style.

I'm not sure how to fix this. Our design for first-letter isn't really able to handle it. If we wrap the newline in a letter-frame we'll get all confused because we expect all the first-letter frames for a block to be continuations of one another and that definitely isn't possible here.

The only thing I can think of is to remove the NS_ASSERTION(ll->GetFirstLetterStyleOK() || GetPrevContinuation()) and handle the case where the first-in-flow letter frame does not have first-letter style. I think we can get away with restyling the text child frame before we reflow it --- nsFirstLetterFrame::DrainOverflowFrames already does restyle the next-in-flow text children during reflow.
Attached file harder testcase — 
This testcase is somewhat harder. Here, we need to restyle the first-in-flow nsFirstLetterFrame so that it loses its text-decoration. We could hack around the text-decoration by modifying its BuildDisplayList to not do the decorations, but we'd still lose with a testcase that, e.g., used font-size and added the wrong ascents to line-heights.
Attached patch work in progress — — Splinter Review 
This patch fixes many testcases but it doesn't fix the problem I mentioned above.

I'm not sure what havoc would be wreaked if we tried to have nsFirstLetterFrame::Reflow modify its own style context.

Another option would be to let it complete without reflowing its kid, pushing the kid to the overflow list instead, so we end up with an empty nsFirstLetterFrame representing the non-existent first letter.
That patch modifies ReResolveStyleContext to always reflow when re-resolving style on a first-letter. This ensures that the code that mangles style contexts on reflow happens again to fix up those style contexts.
Attached patch fix — — Splinter Review 
OK, this patch is by far my best attempt to fix this. The idea is to have the text frame child of the first-in-flow nsFirstLetterFrame always execute the first-letter path during reflow. However, if it detects that the first-letter style should not actually apply, we just make the first-letter text be zero characters. This is very simple and works very well. It passes the first-letter reftests and some additional hard testcases I created.

The patch removes a redundant call to SetFirstLetterStyleOK(PR_FALSE) in nsTextFrameThebes::Reflow.
Attachment #353181 - Flags: superreview?(dbaron)
Attachment #353181 - Flags: review?(dbaron)
Note that we could already create zero-length first-letter text --- e.g., when the first character is not a valid 'letter' --- so the risk here is actually very low.
Whiteboard: [sg:investigate] → [sg:investigate][needs review]
Comment on attachment 353181 [details] [diff] [review]
fix

>-    NS_ASSERTION(ll->GetFirstLetterStyleOK() || GetPrevContinuation(),
>-                 "First-continuation first-letter should have first-letter style enabled in nsLineLayout!");
>+    ll->SetInFirstLetter(GetPrevContinuation() == nsnull);

I think GetPrevContinuation() is the wrong test... I can imagine the actual first-letter part having multiple continuations in some cases (even if we may never do that right now).

I would prefer:

  // Set based on whether we're really the :first-letter or we're
  // the unstyled continuation of it.
  ll->SetInFirstLetter(mStyleContext->GetPseudoType() ==
                       nsCSSPseudoElements::firstLetter);

I still need to look at nsTextFrameThebes.cpp and the tests.
Comment on attachment 353181 [details] [diff] [review]
fix

OK, r+sr=dbaron with the fix in the previous comment.

(I'm also wondering if you really meant to remove the assertion that I quoted in the previous comment.)
Attachment #353181 - Flags: superreview?(dbaron)
Attachment #353181 - Flags: superreview+
Attachment #353181 - Flags: review?(dbaron)
Attachment #353181 - Flags: review+
Whiteboard: [sg:investigate][needs review] → [sg:investigate][needs updated patch and landing]
(In reply to comment #16)
> (I'm also wondering if you really meant to remove the assertion that I quoted
> in the previous comment.)

I did. That assertion would fire in these testcases even with my patch. The point of this patch is to correctly handle situations where the first-letter text is not on the first line so ll->GetFirstLetterStyleOK() is false.
Pushed d170052b6a6b
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [sg:investigate][needs updated patch and landing] → [sg:investigate][needs 191 landing]
Pushed 3b9272c15bfe to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [sg:investigate][needs 191 landing] → [sg:investigate]
Blocks: 429969
No longer depends on: 429969
Blocking 1.9.0.10 because this fixes bug 429969
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.10+
Whiteboard: [sg:investigate] → [sg:investigate] fixes 429969
Comment on attachment 353181 [details] [diff] [review]
fix

Will this patch apply to the 1.9.0 branch or will we need a backport?
I've backported the patch to 1.9.0, which wasn't entirely nontrivial, and it fixes the bugs in comment #3 and later, but it doesn't fix the issues in comments #0 to #2, and it's not really a security bug.

So I suspect Boris was right in comment #6. Something changed on trunk to make the underlying problem in this bug go away, and I only fixed some first-letter rendering issues.

Dan, should we take this off blocking 1.9.0.10 since we don't have a branch fix here?

Simon, can you dig into comment #2? Finding the changeset that fixed trunk would be a great start.
Attached patch 1.9.0 patch — — Splinter Review 
For the record, here's the backported rendering fix. I don't expect it to fix any security issues so it shouldn't land on branch.
(In reply to comment #22)

What about the crash in bug 429969? Is that included in the things not fixed on 1.9.0 branch?

> Simon, can you dig into comment #2? Finding the changeset that fixed trunk
> would be a great start.

On it.
Hmm, actually that patch *does* seem to fix 429969 in 1.9.0, so I guess it is wanted for branch, but not for this bug!
Whiteboard: [sg:investigate] fixes 429969 → [sg:investigate] fixes 429969 [needs landing]
http://hg.mozilla.org/mozilla-central/rev/08bf39f0754a from bug 441367 is the changeset that stopped the assertions in comment 0
Er really? I thought that was just cleanup!
It certainly *shouldn't* have changed any behavior...
If you still have the tree set up in which you did that bisect, could you figure out which changes within nsRuleNode caused the change (the changes within nsRuleNode.cpp other than the introduction of SetFactor and SetDiscrete can all be backed out independently), or whether it was somehow one of the other changes?
The change was caused by the change to "direction" in nsRuleNode::ComputeVisibilityData, specifically removing:

-    if (NS_STYLE_DIRECTION_RTL == visibility->mDirection)
-      mPresContext->SetBidiEnabled();

Adding a similar line in nsCSSFrameConstructor presumably prevented this causing a regression in static pages, but it doesn't help when changing direction dynamically.

I filed bug 489517 on this. Putting back that line brings back the assertions from comment 0 in a build from right after the checkin of bug 441367; but the assertions don't appear in trunk, even with the checkin from this bug backed out.
Bisecting again while applying the patch from bug 489517, it turns out that the assertions were finally fixed by bug 472950, so I suppose we need the patch from bug 472950 on 1.9.0.x to fix this bug, and the patch from here to fix bug 429969 ;-)
Please request branch approval on the 1.9.0 patches you want to take.
Whiteboard: [sg:investigate] fixes 429969 [needs landing] → [sg:investigate] fixes 429969 [needs 1.9.0 approval request(s) -- comment 31]
Comment on attachment 373597 [details] [diff] [review]
1.9.0 patch

Approved for 1.9.0.11, a=dveditz for release-drivers 

This patch fixes security bug 429969 on the 1.9.0 branch. It does not fix the assertions from comment 0 in this bug so I'm not sure if marking it fixed1.9.0.11 after landing is appropriate. Probably it is OK, we can decide whether the branch needs those fixed in bug 472950.
Attachment #373597 - Flags: approval1.9.0.11+
Checked patch into 1.9.0.11
Keywords: fixed1.9.0.11
On the 1.9.0 branch there were three reftest that failed. 429968-1a and 429968-1b make sense, this patch makes the 1.9.0 branch better but not entirely up to snuff and so the tests fail.

23604 is a surprise. 23604-1.html actually looks fine, but 23604-1-ref.html has the first-line style applied to the first two lines. Removing the first-letter style fixes it, or making a dynamic style change as in 23604-1 itself. We'll have to fix this regression in 1.9.0.12
Whiteboard: [sg:investigate] fixes 429969 [needs 1.9.0 approval request(s) -- comment 31] → [sg:investigate] fixes 429969
(In reply to comment #0)

> Loading the testcase triggers:
> 
> ###!!! ASSERTION: child list is not empty for initial reflow:
> 'mFrames.IsEmpty()', file
> /Users/jruderman/trunk/mozilla/layout/generic/nsInlineFrame.cpp, line 326
> 
> Closing it triggers:
> 
> ###!!! ASSERTION: Some objects allocated with AllocateFrame were not freed:
> 'mFrameCount == 0', file
> /Users/jruderman/trunk/mozilla/layout/base/nsPresShell.cpp, line 673

I still see this in the build I built today (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11pre) Gecko/2009051812 GranParadiso/3.0.11pre).

For the test case in comment #8:

> This testcase causes the assertion to fire and also displays "AB" in red. In
> fact, both letters should be black.

I do not see this assert with my build from today and the "AB is black.

Should I be concerned about the assertions from comment #0?
Depends on: 493652
Filed bug 493652 on the 1.9.0-branch regression on the 23604-1-ref test.
Flags: wanted1.8.1.x-
Blocks: 412543
Is there more to do for this bug or can it be unhidden now?
Keywords: regression
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: