1351205 - Assertion failure: mSource.IsGeckoRuleNode() (This should be used only in Gecko-backed style system!), at nsStyleContext.h:168

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Carsten Book [:Tomcat]]

Attachment: crash stack

Assertion failure: mSource.IsGeckoRuleNode() (This should be used only in Gecko-backed style system!), at /home/worker/workspace/build/src/layout/style/nsStyleContext.h:168

Found via bughunter on topsite tests and reproduced on latest stylo debug build from tinderbox 

Steps to reproduce:
-> Load http://www.coronadonewsca.com
---> Assertion failure into some seconds

Assertion failure: mSource.IsGeckoRuleNode() (This should be used only in Gecko-backed style system!), at /home/worker/workspace/build/src/layout/style/nsStyleContext.h:168

and that assertion was added in Bug 1322570 

Bz, Ting-Yu can you take a look ?

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1351205 - Skip debug check for Stylo in nsComputedDOMStyle::UpdateCurrentStyleSources().

The debug code calls nsStyleContext::GetParent(), which is invalid in stylo.
Let's skip it.

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

Comment 2

[Cameron McCormack (:heycam)]

Comment on attachment 8851929 [details]
Bug 1351205 - Skip debug check for Stylo in nsComputedDOMStyle::UpdateCurrentStyleSources().

https://reviewboard.mozilla.org/r/124182/#review126714

::: layout/style/nsComputedDOMStyle.cpp:872
(Diff revision 1)
>        nsStyleContext* topWithPseudoElementData = mStyleContext;
>        while (topWithPseudoElementData->GetParent()->HasPseudoElementData()) {
>          topWithPseudoElementData = topWithPseudoElementData->GetParent();
>        }

It seems like we should be able to preserve the essence of this assertion, without using nsStyleContext::GetParent.  (I'm not sure how important the assertion is in practice, although with stylo changes maybe there is a higher chance of accidentally falling down into this slower path.)

At the point where we check mStyleContext->HasPseudoElementData() before the #ifdef DEBUG, the only place mStyleContext is set to a non-null value is the SetFrameStyleContext call earlier in the function.  That means mInnerFrame is valid here.  So we can look up the frame tree instead to find the top frame whose style context is HasPseudoElementData().

Can you see if that works?

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8851929 [details]
Bug 1351205 - Skip debug check for Stylo in nsComputedDOMStyle::UpdateCurrentStyleSources().

https://reviewboard.mozilla.org/r/124182/#review128382

Comment 4

[Cameron McCormack (:heycam)]

needinfo for comment 2.

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8851929 [details]
Bug 1351205 - Skip debug check for Stylo in nsComputedDOMStyle::UpdateCurrentStyleSources().

https://reviewboard.mozilla.org/r/124182/#review126714

> It seems like we should be able to preserve the essence of this assertion, without using nsStyleContext::GetParent.  (I'm not sure how important the assertion is in practice, although with stylo changes maybe there is a higher chance of accidentally falling down into this slower path.)
> 
> At the point where we check mStyleContext->HasPseudoElementData() before the #ifdef DEBUG, the only place mStyleContext is set to a non-null value is the SetFrameStyleContext call earlier in the function.  That means mInnerFrame is valid here.  So we can look up the frame tree instead to find the top frame whose style context is HasPseudoElementData().
> 
> Can you see if that works?

In general, walking up on the frame tree is different from on the context tree. 

I've tried to travese the frame tree up from mInnerFrame, but the result is different. It seems some of frame's style context in the middle of the walk has `HasPseudoElementData() == false`. Also, `NS_STYLE_HAS_PSEUDO_ELEMENT_DATA` bit was set by looking at the style context's parent [1], I'm not sure whether it's reliable set in stylo or not.

Another observation is that gecko doesn't go this code path by loading `http://www.coronadonewsca.com` in comment 0.

[1] http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/layout/style/nsStyleContext.cpp#724-725

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Cameron, 

Per comment 5, do you think it worth finding another way to preserve the essence of this assertion?

Comment 7

[Cameron McCormack (:heycam)]

OK.  Well, in that case, I am fine with just skipping the assertion here.  Thanks for checking.

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8851929 [details]
Bug 1351205 - Skip debug check for Stylo in nsComputedDOMStyle::UpdateCurrentStyleSources().

https://reviewboard.mozilla.org/r/124182/#review130794

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4721f0c609d4
Skip debug check for Stylo in nsComputedDOMStyle::UpdateCurrentStyleSources(). r=heycam

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4721f0c609d4