Last Comment Bug 362901 - [FIX]nsCSSFrameConstructor::HaveFirstLetterStyle broken in the presence of asynchronous restyles or batching
: [FIX]nsCSSFrameConstructor::HaveFirstLetterStyle broken in the presence of as...
[sg:critical?] One of the regression ...
: fixed1.8.0.15, fixed1.8.1.8
Product: Core
Classification: Components
Component: Layout: Misc Code (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.9alpha4
Assigned To: Boris Zbarsky [:bz] (TPAC)
Depends on: 257868 357531 367650 379383 379799
Blocks: 230611
  Show dependency treegraph
Reported: 2006-12-05 19:03 PST by David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
Modified: 2008-03-08 05:40 PST (History)
10 users (show)
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.14-
dveditz: wanted1.8.0.x+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Proposed patch (9.43 KB, patch)
2007-02-03 00:48 PST, Boris Zbarsky [:bz] (TPAC)
no flags Details | Diff | Splinter Review
Minor tweak (10.33 KB, patch)
2007-03-04 14:27 PST, Boris Zbarsky [:bz] (TPAC)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
Patch updated to comments (13.35 KB, patch)
2007-04-15 17:43 PDT, Boris Zbarsky [:bz] (TPAC)
no flags Details | Diff | Splinter Review
Branch patch (16.98 KB, patch)
2007-07-13 01:28 PDT, Boris Zbarsky [:bz] (TPAC)
dveditz: approval1.8.1.8+
Details | Diff | Splinter Review
same for 1.8.0 (just fixed offsets) (22.99 KB, patch)
2008-02-28 06:21 PST, Alexander Sack
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2006-12-05 19:03:55 PST
This is a bug on the second assertion described in bug 357531.  It could even also be related to the assertion we were seeing for a while going through the bloaturls on balsa -- maybe even still are.

nsCSSFrameConstructor::HaveFirstLetterStyle is quite broken in the presence of batching.  What seems to be happening in attachment 243039 [details] is the following:
 * the attachment, within one event:
   + changes the ID on the option such that the :first-letter style rule no longer applies, and
   + then removes the first child of the option

So when we get to nsCSSFrameConstructor::ContentRemoved, HaveFirstLetterStyle returns false because it uses ProbePseudoStyleContext based on the current style, not the style at the last time we processed restyle events.  So it concludes that there's no :first-letter frame to remove.

This means we end up with the non-first-letter frame still in the frame tree (we remove only the floating first letter frame), but pointing to content that's been removed from the content tree.

[ I think this problem is significantly worse due to asynchronous restyles, which I think are post-1.8 but not sure, but it may be present due to other batching mechanisms as well, or if we can trigger a failure of HaveFirstLetterStyle in other code, such as changing an attribute so that an element goes from having first letter style to having its first child with display:none. ]

The symptoms of this problem are the assertions loading attachment 243039 [details]:

WARNING: Content has no document.: file nsTextFrame.cpp, line 6143
WARNING: Reflow of frame failed in nsLineLayout: file nsLineLayout.cpp, line 890
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file nsBlockFrame.cpp, line 3399
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file nsBlockFrame.cpp, line 3230
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file nsBlockFrame.cpp, line 1770
###!!! ASSERTION: reflow dirty lines failed: 'NS_SUCCEEDED(rv)', file nsBlockFrame.cpp, line 884
Comment 1 Boris Zbarsky [:bz] (TPAC) 2006-12-05 20:49:46 PST
Hmm...  So async restyles are in fact in 1.8.

Should we just switch the test to actually testing whether the first frame is a first-letter frame?  That might be faster than probing the style context anyway...

Comment 2 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2006-12-05 23:12:23 PST
We'd half to walk the first-child chain, at the very least, and perhaps more if there are empty frames.
Comment 3 Boris Zbarsky [:bz] (TPAC) 2006-12-05 23:16:45 PST
So I hate to ask this, but is there a free block-specific bit?  ;)
Comment 4 Boris Zbarsky [:bz] (TPAC) 2007-02-03 00:48:35 PST
Created attachment 253848 [details] [diff] [review]
Proposed patch

Turns out we already have a flag for this.  I went ahead and made it a little better maintained and started using it in favor of style context probing.
Comment 5 Boris Zbarsky [:bz] (TPAC) 2007-02-03 00:52:04 PST
Incidentally, that patch fixes bug 230611.

It's a patch on top of the one for bug 367650, so that will need to land first.

Clearing deps, since I just realized this is security-sensitive...
Comment 6 Boris Zbarsky [:bz] (TPAC) 2007-03-04 14:27:49 PST
Created attachment 257272 [details] [diff] [review]
Minor tweak

Have to add the flag even if there's no actual letter frame in case someone adds text later to an empty block or whatnot.
Comment 7 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2007-03-04 17:54:46 PST
Comment on attachment 257272 [details] [diff] [review]
Minor tweak

>+  NS_ASSERTION(aFrame != (nsIFrame*)0xdddddddd && aFrame->GetContent() != (nsIContent*)0xdddddddd,
>+               "oops");

If you really want this, it's ok, but I'd rather not put these everywhere.  Why here in particular?

>+#ifdef DEBUG
>+    // Verify that our NS_BLOCK_HAS_FIRST_LETTER_STYLE bit is correct,
>+    // as needed.
>     if (nsnull == GetPrevInFlow()) {
>       nsRefPtr<nsStyleContext> firstLetterStyle = GetFirstLetterStyle(presContext);
>+      NS_ASSERTION((firstLetterStyle != nsnull) ==
>+                   ((mState & NS_BLOCK_HAS_FIRST_LETTER_STYLE) != 0),
>+                   "State mismatch");
>     }

This could be just:

NS_ASSERTION(GetPrevInFlow() || 
             (nsRefPtr<nsStyleContext>(GetFirstLetterStyle(presContext)) != nsnull) ==
             ((mState & NS_BLOCK_HAS_FIRST_LETTER_STYLE) != 0),
             "NS_BLOCK_HAS_FIRST_LETTER_STYLE state out of sync");

although maybe GetPrevContinuation makes more sense.

>+#ifdef DEBUG
>+  {
>+    nsBlockFrame* block;
>+    NS_ASSERTION(NS_SUCCEEDED(aBlockFrame->QueryInterface(kBlockFrameCID,
>+                                                          (void**)&block)),

 && block
just to be sure.

(multiple occurrences)

>+                 "Not a block frame?");
>+  }

>+  aBlockFrame->RemoveStateBits(NS_BLOCK_HAS_FIRST_LETTER_STYLE);

Do the callers of RemoveLetterFrame really match the semantics of removing this bit?  I haven't checked...

Have you tested :first-letter on table cells?  Does it apply to the anonymous block inside?  Is this at least not regressing things?  (Should it work at all?)  Please file bugs if needed. :-)

Comment 8 Boris Zbarsky [:bz] (TPAC) 2007-03-04 18:03:30 PST
> Why here in particular?

I was hitting it a lot with various mangled frame trees...  I didn't realize I'd left it in the diff; I'll take it out.

> This could be just:

Will do.

> although maybe GetPrevContinuation makes more sense.

Indeed, it does.  Will do.

> && block just to be sure.

Will do.

> Do the callers of RemoveLetterFrame really match the semantics of removing
> this

That's a good question.  Realistically, the only thing that the callers would need that bit for is knowing whether they need to remove letter frames, and they already know that...  I can probably take that line out.

> Have you tested :first-letter on table cells?

I haven't.  I'll do that.

Comment 9 Jesse Ruderman 2007-03-04 18:38:26 PST
Comment 10 Boris Zbarsky [:bz] (TPAC) 2007-03-04 18:52:30 PST
Ignore that part.  Shouldn't have been there.
Comment 11 Boris Zbarsky [:bz] (TPAC) 2007-03-05 00:02:26 PST
> Have you tested :first-letter on table cells?

Now I have, and it works.  Which makes sense -- when constructing the frame we use the style context check, so call WrapFramesInFirstLetterFrame, so set the bit on the block inside the table cell.

I need to write a bunch of tests, including ones with <td> and <fieldset> and probably with some scrollable blocks.

Also note to self:  the only caller of nsBlockFrame::GetFirstLetterStyle is now debug code, so the method should be debug too.
Comment 12 Boris Zbarsky [:bz] (TPAC) 2007-04-15 17:43:21 PDT
Created attachment 261622 [details] [diff] [review]
Patch updated to comments
Comment 13 Boris Zbarsky [:bz] (TPAC) 2007-04-15 17:45:32 PDT
Checked in on trunk.

David, do you think we should take this on branch?  We'd need to take the patch for bug 367650 as well, I think....
Comment 14 Daniel Veditz [:dveditz] 2007-04-18 16:04:47 PDT
Will wait on the branch nominations until dbaron responds to comment 13
Comment 15 Boris Zbarsky [:bz] (TPAC) 2007-06-14 22:11:49 PDT
This bug fixed bug 372550, but can't add a dep because with the regression-tracking via deps that gives us a dep loop.  :(
Comment 16 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2007-07-06 14:18:13 PDT
The idea of this patch isn't all that scary, and it's a relatively obscure feature, so I think we can take it on the branch if it's sufficiently tested.  I didn't look at bug 367650, though.
Comment 17 Daniel Veditz [:dveditz] 2007-07-09 10:59:39 PDT
Need a 1.8 branch patch that incorporates the regression fixes, at least the one mentioned in the whiteboard that appears not to have gotten a bug.

Given the remaining unfixed blockers we'll target this one for the next release for now, but will approve the above patch if it gets don't by the code freeze (July 13).
Comment 18 Boris Zbarsky [:bz] (TPAC) 2007-07-13 01:16:00 PDT
This also depends a bit on the changes in bug: we only want to clean up generated content in inline and block frames.
Comment 19 Boris Zbarsky [:bz] (TPAC) 2007-07-13 01:19:46 PDT
er, I meant in bug 257868.
Comment 20 Boris Zbarsky [:bz] (TPAC) 2007-07-13 01:28:00 PDT
Created attachment 272139 [details] [diff] [review]
Branch patch

This includes the relevant part of bug 257868, all of bug 367650, this bug, bug 379383, bug 379799, and the notation in this bug's status whiteboard.

I've tested this on the testcases in all these bugs, and on the reftests I checked in for bug 367650.
Comment 21 Daniel Veditz [:dveditz] 2007-08-21 15:05:50 PDT
Comment on attachment 272139 [details] [diff] [review]
Branch patch

Comment 22 Daniel Veditz [:dveditz] 2007-08-21 15:06:41 PDT
> (From update of attachment 272139 [details] [diff] [review])
> nsBlockFrame.cpp

By which I mean "approved for, a=dveditz for release-drivers", of course
Comment 23 Boris Zbarsky [:bz] (TPAC) 2007-08-22 10:32:12 PDT
Fixed for
Comment 24 Alexander Sack 2008-02-28 06:21:38 PST
Created attachment 306267 [details] [diff] [review]
same for 1.8.0 (just fixed offsets)

a=asac for

(shipped by distro releases for some time)
Comment 25 Alexander Sack 2008-02-28 07:51:00 PST
please land on 1.8.0 branch.
Comment 26 Reed Loden [:reed] (use needinfo?) 2008-03-08 05:40:20 PST

Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1110.; previous revision: 1.1110.
Checking in layout/base/nsCSSFrameConstructor.h;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.h,v  <--  nsCSSFrameConstructor.h
new revision:; previous revision:
Checking in layout/base/nsStyleChangeList.cpp;
/cvsroot/mozilla/layout/base/nsStyleChangeList.cpp,v  <--  nsStyleChangeList.cpp
new revision:; previous revision: 1.16
Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.729.; previous revision: 3.729.
Checking in layout/generic/nsBlockFrame.h;
/cvsroot/mozilla/layout/generic/nsBlockFrame.h,v  <--  nsBlockFrame.h
new revision:; previous revision:
Checking in layout/generic/nsContainerFrame.cpp;
/cvsroot/mozilla/layout/generic/nsContainerFrame.cpp,v  <--  nsContainerFrame.cpp
new revision:; previous revision:
Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/Attic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.513.; previous revision: 1.513.

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