362901 - [FIX]nsCSSFrameConstructor::HaveFirstLetterStyle broken in the presence of asynchronous restyles or batching

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 [:bzbarsky]]

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-4, no longer working on Mozilla)]

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 [:bzbarsky]]

So I hate to ask this, but is there a free block-specific bit?  ;)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: 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 [:bzbarsky]]

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 [:bzbarsky]]

Attachment: 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-4, no longer working on Mozilla)]

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");
>     }
>+#endif

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?");
>+  }
>+#endif

>+  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. :-)


r+sr=dbaron

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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.

bit?

Comment 9

[Jesse Ruderman]

"bit?"?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ignore that part.  Shouldn't have been there.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> 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 [:bzbarsky]]

Attachment: Patch updated to comments

Comment 13

[Boris Zbarsky [:bzbarsky]]

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]]

Will wait on the branch nominations until dbaron responds to comment 13

Comment 15

[Boris Zbarsky [:bzbarsky]]

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-4, no longer working on Mozilla)]

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]]

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 1.8.1.5 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 1.8.1.5 code freeze (July 13).

Comment 18

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

er, I meant in bug 257868.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: 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]]

Comment on attachment 272139 [details] [diff] [review]
Branch patch

nsBlockFrame.cpp

Comment 22

[Daniel Veditz [:dveditz]]

> (From update of attachment 272139 [details] [diff] [review])
> nsBlockFrame.cpp

By which I mean "approved for 1.8.1.7, a=dveditz for release-drivers", of course

Comment 23

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8.1.7

Comment 24

[Alexander Sack]

Attachment: same for 1.8.0 (just fixed offsets)

a=asac for 1.8.0.15

(shipped by distro releases for some time)

Comment 25

[Alexander Sack]

please land on 1.8.0 branch.

Comment 26

[Reed Loden [:reed]]

MOZILLA_1_8_0_BRANCH:

Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1110.6.12.2.67; previous revision: 1.1110.6.12.2.66
done
Checking in layout/base/nsCSSFrameConstructor.h;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.h,v  <--  nsCSSFrameConstructor.h
new revision: 1.187.6.3.2.8; previous revision: 1.187.6.3.2.7
done
Checking in layout/base/nsStyleChangeList.cpp;
/cvsroot/mozilla/layout/base/nsStyleChangeList.cpp,v  <--  nsStyleChangeList.cpp
new revision: 1.16.38.1; previous revision: 1.16
done
Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.729.4.7.2.15; previous revision: 3.729.4.7.2.14
done
Checking in layout/generic/nsBlockFrame.h;
/cvsroot/mozilla/layout/generic/nsBlockFrame.h,v  <--  nsBlockFrame.h
new revision: 3.205.12.5; previous revision: 3.205.12.4
done
Checking in layout/generic/nsContainerFrame.cpp;
/cvsroot/mozilla/layout/generic/nsContainerFrame.cpp,v  <--  nsContainerFrame.cpp
new revision: 1.239.6.2.4.2; previous revision: 1.239.6.2.4.1
done
Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/Attic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.513.4.6.2.6; previous revision: 1.513.4.6.2.5
done