Closed Bug 1351205 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: cbook, Assigned: TYLin)

References

(Blocks 1 open bug, )

Details

(Keywords: assertion, regression)

Attachments

(2 files)

Attached file 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 ?
Blocks: 1322570
Flags: needinfo?(tlin)
Flags: needinfo?(bzbarsky)
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Flags: needinfo?(tlin)
Flags: needinfo?(bzbarsky)
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 on attachment 8851929 [details]
Bug 1351205 - Skip debug check for Stylo in nsComputedDOMStyle::UpdateCurrentStyleSources().

https://reviewboard.mozilla.org/r/124182/#review128382
Attachment #8851929 - Flags: review?(cam)
needinfo for comment 2.
Flags: needinfo?(tlin)
Priority: -- → P2
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
Cameron, 

Per comment 5, do you think it worth finding another way to preserve the essence of this assertion?
Flags: needinfo?(tlin) → needinfo?(cam)
OK.  Well, in that case, I am fine with just skipping the assertion here.  Thanks for checking.
Flags: needinfo?(cam)
Comment on attachment 8851929 [details]
Bug 1351205 - Skip debug check for Stylo in nsComputedDOMStyle::UpdateCurrentStyleSources().

https://reviewboard.mozilla.org/r/124182/#review130794
Attachment #8851929 - Flags: review+
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4721f0c609d4
Skip debug check for Stylo in nsComputedDOMStyle::UpdateCurrentStyleSources(). r=heycam
https://hg.mozilla.org/mozilla-central/rev/4721f0c609d4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.