Closed Bug 1308848 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 - ---
firefox51 - fixed
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: cbook, Assigned: xidorn)

References

()

Details

(Keywords: assertion)

Attachments

(2 files)

Attached file 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
[Tracking Requested - why for this release]:

affects beta to trunk on debug builds on a live site
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.
Component: General → DOM: Core & HTML
Ah, nope, there is no mismatch between those two. 0x8 in NodeInfo means Comment, and the class is Comment.
Component: DOM: Core & HTML → Layout
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.
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?
Flags: needinfo?(xidorn+moz)
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.
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
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?
Flags: needinfo?(bzbarsky)
Duplicate of this bug: 1279235
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?
Attachment #8801626 - Flags: review+
Flags: needinfo?(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

> 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.
> It doesn't seem to me text could be a valid sibling there either.

Text is a valid sibling for rowgroup/headergroup/footergroup, no?
(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 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"
Thanks for review.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c63279dd3ec4
Not request style of comment node when checking valid sibling. r=bz
https://hg.mozilla.org/mozilla-central/rev/c63279dd3ec4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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 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
Attachment #8801626 - Flags: approval-mozilla-beta?
Attachment #8801626 - Flags: approval-mozilla-aurora?
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
Attachment #8801626 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8801626 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
needs rebasing for beta
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(xidorn+moz)
Thanks, Ryan.
You need to log in before you can comment on or make changes to this bug.