Closed Bug 1437774 Opened 6 years ago Closed 6 years ago

stylo-chrome: Assertion failure: StylistNeedsUpdate() for test_bug514660.xul

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(1 file)

It asserts from
* ServoStyleSet::InvalidateStyleForCSSRuleChanges()
* nsIPresShell::RestyleForCSSRuleChanges()
* nsIPresShell::SetAuthorStyleDisabled()

ServoStyleSet::SetAuthorStyleDisabled() simply changes stylist.author_styles_enabled, and post a restyle subtree from root, without marking the stylist dirty, while generally CSSRuleChange requires a stylist rebuild.

nsIPresShell::SetAuthorStyleDisabled() actually should invoke RestyleForCSSRuleChanges() because we may need to rebuild font set, counter style manager, and font feature values. This is buggy in stylo, though, that we don't disable them when author rules are disabled.

Actually, as far as we only skip them during cascading, there are more things we need to do to respect author style disabling, e.g. @page rules? Probably we should just make iter_origins() aware of that...

Anyway, we still need to solve the conflict between InvalidateStyleForCSSRuleChanges() asserting StylistNeedsUpdate() and SetAuthorStyleDisabled() doesn't invalidate the stylist.
It probably doesn't matter that we just mark stylist needs update to satisfy the assertion... It doesn't hurt anyway.
Assignee: nobody → xidorn+moz
Filed the behavior bug in bug 1437785.
Comment on attachment 8950502 [details]
Bug 1437774 - Mark stylist dirty when author style disabled state changes.

https://reviewboard.mozilla.org/r/219788/#review225490

::: layout/style/ServoStyleSet.cpp:376
(Diff revision 2)
>    Servo_StyleSet_SetAuthorStyleDisabled(mRawSet.get(), mAuthorStyleDisabled);
> +  // XXX This is not exactly necessary, because we don't need to rebuild stylist
> +  // for this change currently. But we have bug due to not doing that, which we
> +  // should fix eventually. See bug 1437785. This is currently a workaround for
> +  // assertion in InvalidateStyleForCSSRuleChanges.
> +  SetStylistStyleSheetsDirty();

So this is intentional, to avoid having to rebuild styleset in every shadow root on the document...

This feels like a workaround, maybe we shouldn't be calling RestyleForCssRuleChanges from PresShell for Servo?

Anyway, either this patch or avoiding calling RestyleForCssRuleChanges look fine to me.

Also, please note in the commit message that I regressed this in bug 1436798?
Attachment #8950502 - Flags: review?(emilio) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7e2a7833180
Mark stylist dirty when author style disabled state changes. r=emilio
https://hg.mozilla.org/mozilla-central/rev/f7e2a7833180
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: