1308848 - Assertion failure: aTextNode && aTextNode->IsNodeOfType(nsINode::eTEXT), at nsStyleSet.cpp:1784

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: stack

Found via bughunter and reproduced on a win 7 with the recent tinderbox trunk debug build 

Assertion failure: aTextNode && aTextNode->IsNodeOfType(nsINode::eTEXT), at nsStyleSet.cpp:1784

Steps to reproduce:
-> Load https://northwestern.rivals.com/commitments/football/
---> Assertion on load

according to bughunter affects trunk -> beta

Comment 1

[Carsten Book [:Tomcat]]

[Tracking Requested - why for this release]:

affects beta to trunk on debug builds on a live site

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

This looks like a mismatch between NodeType recorded in mNodeInfo (which is Text) and the class it uses (which is Comment).

I guess it is a bug from DOM or HTML Parser.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Ah, nope, there is no mismatch between those two. 0x8 in NodeInfo means Comment, and the class is Comment.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

It doesn't seem to be a serious issue, and should be as easy as just add

The reason seems to be that nsCSSFrameConstructor::GetInsertionPrevSibling tries to figure out the proper prev sibling for an inserted comment node, and the comment node's prev sibling is a table part element, which triggers computing the style context of the comment node in nsCSSFrameConstructor::IsValidSibling.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1308848 - Not request style of comment node when checking valid sibling.

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

Comment 6

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

Xidorn, are you taking this bug, or can you suggest someone who might work on it?
Do you think we should aim a fix at 52, or to more recent versions?

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

I can take this. I'm just waiting for bz to be available to review it. It isn't critical. It doesn't seem to me breaking this assertion this way can cause any serious breakage, so I don't think we need to aim landing it at 52.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Hi, bz, this is a small patch to fix an assertion. I suppose it shouldn't take much time to review, but it has a low priority as well. Could you review it when you have time?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8801626 [details]
Bug 1308848 - Not request style of comment node when checking valid sibling.

https://reviewboard.mozilla.org/r/86306/#review94686

::: layout/base/nsCSSFrameConstructor.cpp:6578
(Diff revision 1)
>        }
>        if (!styleParent) {
>          NS_NOTREACHED("Shouldn't happen");
>          return false;
>        }
> +      if (aContent->IsNodeOfType(nsINode::eCOMMENT)) {

I assume aContent can't be a processing instruction here?  Or should we test for that too?

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8801626 [details]
Bug 1308848 - Not request style of comment node when checking valid sibling.

https://reviewboard.mozilla.org/r/86306/#review94686

> I assume aContent can't be a processing instruction here?  Or should we test for that too?

Good catch. Progressing instruction has the same issue here. I guess we should just return false for all non-element node directly? It doesn't seem to me text could be a valid sibling there either.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8801626 [details]
Bug 1308848 - Not request style of comment node when checking valid sibling.

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

> It doesn't seem to me text could be a valid sibling there either.

Text is a valid sibling for rowgroup/headergroup/footergroup, no?

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8801626 [details]
Bug 1308848 - Not request style of comment node when checking valid sibling.

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

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> > It doesn't seem to me text could be a valid sibling there either.
> 
> Text is a valid sibling for rowgroup/headergroup/footergroup, no?

I see. Updated the patch to exclude comment and processing instruction.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8801626 [details]
Bug 1308848 - Not request style of comment node when checking valid sibling.

https://reviewboard.mozilla.org/r/86306/#review94814

::: layout/base/nsCSSFrameConstructor.cpp:6569
(Diff revision 3)
>          NS_NOTREACHED("Shouldn't happen");
>          return false;
>        }
> +      if (aContent->IsNodeOfType(nsINode::eCOMMENT) ||
> +          aContent->IsNodeOfType(nsINode::ePROCESSING_INSTRUCTION)) {
> +        // Comments and processing instructions never have frame, so we

"never have frames"

::: layout/base/nsCSSFrameConstructor.cpp:6570
(Diff revision 3)
>          return false;
>        }
> +      if (aContent->IsNodeOfType(nsINode::eCOMMENT) ||
> +          aContent->IsNodeOfType(nsINode::ePROCESSING_INSTRUCTION)) {
> +        // Comments and processing instructions never have frame, so we
> +        // should not try to generate style context for them.

"style contexts"

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Thanks for review.

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8801626 [details]
Bug 1308848 - Not request style of comment node when checking valid sibling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86306/diff/3-4/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c63279dd3ec4
Not request style of comment node when checking valid sibling. r=bz

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Carsten Book [:Tomcat]]

bughunter run into this on aurora on into on https://www.barchart.com -> Assertion failure: aTextNode && aTextNode->IsNodeOfType(nsINode::eTEXT) so would it be possible to uplift this to aurora ?

Comment 22

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8801626 [details]
Bug 1308848 - Not request style of comment node when checking valid sibling.

Approval Request Comment
[Feature/Bug causing the regression]: not sure
[User impact if declined]: n/a
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: has landed for a while
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: fixing an assertion on some edge cases
[String changes made/needed]: n/a

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8801626 [details]
Bug 1308848 - Not request style of comment node when checking valid sibling.

fix assertion failure in layout, let's take this in aurora52

Comment 24

[Gerry Chang [:gchang]]

Comment on attachment 8801626 [details]
Bug 1308848 - Not request style of comment node when checking valid sibling.

Fix an assertion related to layout. Beta51+. Should be in 51 beta 8.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/40a2d2693600

Comment 26

[Carsten Book [:Tomcat]]

needs rebasing for beta

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bd7b72ee1c08

Comment 28

[Xidorn Quan [:xidorn] UTC+11]

Thanks, Ryan.