1368617 - Assertion failure: aContent->IsElement() || (aFrame && aContent->GetParent() && aFrame->PresContext()->FrameManager()-> GetDisplayContentsStyleFor(aContent->GetParent())) || (aContent->IsNodeOfType(nsINode::eTEXT) && aContent->IsStyledByServo

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: crash stack

Assertion failure: !aContent || aContent->IsElement() || (aFrame && aContent->GetParent() && aFrame->PresContext()->FrameManager()-> GetDisplayContentsStyleFor(aContent->GetParent())) || (aContent->IsNodeOfType(nsINode::eTEXT) && aContent->IsStyledByServo() && aContent->HasFlag(NODE_NEEDS_FRAME) &&
aHint & nsChangeHint_ReconstructFrame) (Shouldn't be trying to restyle non-elements directly, except if it's a display:contents child or a text node doing lazy frame construction), at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/layout/base/nsStyleChangeList.cpp:39

Found by bughunter and reproduced on latest m-c windows trunk debug build on win7.

Steps to reproduce:
-> Load https://andreasgal.com/
--> Crash on Load

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Per my discussion with Tomcat on IRC, this assertion is unrelated to Stylo at all. It is an assertion caught on a normal build.

Comment 2

[Carsten Book [:Tomcat]]

Also did some bisecting and 

2:01.27 INFO: Narrowed inbound regression window from [7c9940c5, bd7af7e5] (3 revisions) to [620f5ed5, bd7af7e5] (2 revisions) (~1 steps left)
 2:01.27 INFO: No more inbound revisions, bisection finished.
 2:01.28 INFO: Last good revision: 620f5ed5c91ec42874c6b725d8caddb713bbe022
 2:01.28 INFO: First bad revision: bd7af7e530068aeebf1c357bfed8e8d4c43e2d05
 2:01.28 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=620f5ed5c91ec42874c6b725d8caddb713bbe022&tochange=bd7af7e530068aeebf1c357bfed8e8d4c43e2d05

From this its either  Bug 1302054 or  Bug 1301258 that cause the regression

Comment 3

[Emilio Cobos Álvarez (:emilio)]

This is on a text node within a first-line frame, FWIW.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 55. Cam, can you have a look?

Comment 5

[Cameron McCormack (:heycam)]

Attachment: reduced test

Comment 6

[Cameron McCormack (:heycam)]

This assertion is the one that fires when we restyle a text frame and produce a change hint, and we're not in some specific circumstances where we allow this to happen.  Generally it shouldn't happen, since the change should have been handled by the parent element.  It shouldn't be possible to generate changes-that-are-not-handled-for-descendants on the text node itself, since it matches no rules.

The reduced test involves an element with ::first-line and ::first-letter styles, and the ::first-letter is floating.  In Firefox and Edge, the floating ::first-letter style does not inherit from the ::first-line, while in Chrome and Safari it does.  As dbaron said 14 years ago in bug 13610 comment 30, it's not clear what the correct behaviour is per spec.

We hit the assertion because when we initially create the continuing text frame for the text content that goes in the floating ::first-letter, we give it the element's style context as its parent, and not the ::first-line's style context, but later when restyling, we choose the ::first-line's style context as the parent.  There is no change of style on the parent element itself, so the only style change we detect is the change between letter-spacing:normal and letter-spacing:1px on the nsContinuingTextFrame, and we hit the assertion when appending the reflow change hint for it.

I'm not sure if there are legitimate situations where the style of the continuing text frame in the floating ::first-letter can change where the parent element doesn't.  For this bug I'll just make us choose the right parent style context when restyling.

Comment 7

[Cameron McCormack (:heycam)]

I didn't look into why bug 1302054 or bug 1301258 revealed this bug, but I don't think it's relevant to the actual issue here.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #6)
> I'm not sure if there are legitimate situations where the style of the
> continuing text frame in the floating ::first-letter can change where the
> parent element doesn't.  For this bug I'll just make us choose the right
> parent style context when restyling.

That should be: I'll make us choose the right parent style context when initially creating the continuing text frame.  (We should be inheriting from the ::first-line.)

But, oddly, I can only reproduce this problem when the contents of the <p> is just &nbsp;, which causes the ::first-letter's text frame to be for "" and the continuing text frame to be for the "&nbsp;".

Comment 9

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #8)
> But, oddly, I can only reproduce this problem when the contents of the <p>
> is just &nbsp;, which causes the ::first-letter's text frame to be for ""
> and the continuing text frame to be for the "&nbsp;".

With "normal" content, like <p>ab</p>, we never call into nsFirstLetterFrame::CreateContinuationForFloatingParent, since nsCSSFrameConstructor::CreateFloatingLetterFrame's call to NeedFirstLetterContinuation returns true, and we call CreateContinuingFrame up front here.  (This means the continuation doesn't get a ::first-letter-continuation style, as nsFirstLetterFrame::CreateContinuationForFloatingParent would resolve, but maybe that doesn't really matter.)

In the <p>&nbsp;</p> case, NeedFirstLetterContinuation thinks we don't need a continuation, since that counts as a white space character to skip past when looking for the first "letter" character, which would mean that all of the text content goes in the ::first-letter and none should need to go in a continuation.  Later, when we reflow the nsTextFrame where it computes its own first letter length (in FindFirstLetterRange), it decides that the NBSP does not count as white space to skip when looking for the content that goes in the ::first-letter.

So this is ultimately due to nsCSSFrameConstructor.cpp's NeedFirstLetterContinuation doing different checks from nsTextFrame.cpp's FindFirstLetterRange.

FindFirstLetterRange is somewhat more complex than NeedFirstLetterContinuation, since it does a bunch of script/ligature-based decisions that FindFirstLetterRange doesn't, so I'm not sure whether we want to do that in NeedFirstLetterContinuation.  In any case, if we do fall back to having to reflow and create a new continuation frame in nsFirstLetterFrame::CreateContinuationForFloatingParent, we should be doing it with the correct parent style context when in the presence of ::first-line styles.

Comment 10

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=288990559c154ff57e1b65978c3bbdf74e128930

Comment 11

[Cameron McCormack (:heycam)]

Attachment: Bug 1368617 - Ensure continuing text frames for floating ::first-letters will inherit from a ::first-line.

Review commit: https://reviewboard.mozilla.org/r/145198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145198/

Comment 12

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

Comment on attachment 8873777 [details]
Bug 1368617 - Ensure continuing text frames for floating ::first-letters will inherit from a ::first-line.

https://reviewboard.mozilla.org/r/145198/#review149440

::: layout/generic/crashtests/crashtests.list:656
(Diff revision 1)
>  load 1304441.html
>  load 1316649.html
>  load 1349650.html
>  asserts-if(browserIsRemote,0-5) load 1349816-1.html # bug 1350352
>  load 1367413-1.html
> +load 1368617-1.html

Hopefully you tested that this asserts in the crashtest harness without the patch?

::: layout/generic/nsFirstLetterFrame.cpp:327
(Diff revision 1)
>      CreateContinuingFrame(aPresContext, aChild, parent, aIsFluid);
>  
>    // The continuation will have gotten the first letter style from its
>    // prev continuation, so we need to repair the style context so it
>    // doesn't have the first letter styling.
> -  nsStyleContext* parentSC = this->StyleContext()->GetParentAllowServo();
> +  nsStyleContext* parentSC = parent->StyleContext();

Maybe add a comment that parentSC is different from this->StyleContext()->GetParentAllowServo() in the presence of ::first-line?

Comment 13

[Cameron McCormack (:heycam)]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #12)
> Hopefully you tested that this asserts in the crashtest harness without the
> patch?

I didn't!  But I did just now, and it does assert without the patch.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8873777 [details]
Bug 1368617 - Ensure continuing text frames for floating ::first-letters will inherit from a ::first-line.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145198/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a83033b39544
Ensure continuing text frames for floating ::first-letters will inherit from a ::first-line. r=dbaron

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a83033b39544