1387913 - stylo: Rules are not cloned properly before mutation on shared stylesheet

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Xidorn Quan [:xidorn] UTC+10]

Test layout/inspector/tests/test_bug536379.html fails because of some weird behavior. I cannot tell exactly what happens there, but it seems to me that the removal by
> firstPRule.style.removeProperty("color");
removes color property from both stylesheet, which is clearly undesired.

Other failures may be side effect of this.

Comment 1

[Brad Werth [:bradwerth]]

Could be a failure in the stylesheet cloning code I wrote. I'll take a look.

Comment 2

[Brad Werth [:bradwerth]]

This doesn't fail for me, either with or without e10s. Can you please confirm it's still failing for you?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

This is an enabled test marked with fail-if [1] so it must still be failing at least on infra.

I can try next Monday and see if it fails for me locally, so leave the ni? for now.


[1] https://searchfox.org/mozilla-central/rev/33295c6f4d57d16fe76476ac131cadd4cd3d05ad/layout/inspector/tests/mochitest.ini#12

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

So, it does fail, but if you run it with "mach mochitest" directly, it would show as if it passes, because the failure is expected.

To see this failure explicitly, you need to remove the "fail-if = stylo" from mochitest.ini.

Alternatively, you can apply whatever fix in your mind and then observe the following message for that test:
> failed | fail-if condition in manifest - We expected at least one failure
which indicates that we can remove the "fail-if" then.

Comment 5

[Brad Werth [:bradwerth]]

Got it. I can indeed reproduce, thank you.

Comment 6

[Brad Werth [:bradwerth]]

I need some help on this. My understanding of the Stylo styling algorithm is lacking. My analysis so far...

The test page has a <p> tag with matching rules in two identical stylesheets. The call to nsIDOMUtils.getCSSStyleRules() triggers cloning of the inners, and triggers restyling. I've verified that the stylesheet is cloned, and that the ServoStyleContexts are regenerated and applied to the frames. The ServoStyleContext for the <p> element then has two rules that should point to different declaration blocks. Those rules are typed Arc<Locked<StyleRule>> pointers. The two rules have different Arc objects, but those objects both have the same ArcInner object inside them (and therefore the same Locked object inside them). I assume this indicates that the same Arc object has been cloned, when what should have happened is that the StyleRules from the two cloned stylesheets should have been added instead.

I've done a separate walk of the stylesheet cloning methods and it appears that rules are deep cloned correctly. They just don't get used in the ServoStyleContext rules.

a) How and where is the matching rule list built? At the point where I see ServoComputedValues / ServoStyleContext objects being applied, the rules member object has already been built.
b) Is there a simple problem in a function somewhere, where an Arc is cloned and it should be a deep clone? Perhaps a function that duplicates a rule in the ServoStyleContext as an optimization instead of re-fetching it?

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Hacky fix.

This is a hack, because relies on what RecordShadowStyleChange does (which is a hack on its own).

But the basic problem is that the stylist isn't getting updated. The reason for that is simple, and it's because we don't want to, nor usually need to.

I removed the MarkStylistDirty back in the day from EnsureUniqueInner on purpose, and the reason is that there's no need for it for styling correctness.

I'd prefer it to be kept that way, since most of the time (in fact, all the time expect that specific caller) it's correct and does the right thing. However, the caller of EnsureUniqueInnerOnCSSSheets, expects the stylist to be refreshed, because it's going to grab the rules from a new style context.

I can write a proper patch in a bit.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1387913: Make sure to update the stylist if needed on EnsureUniqueInnerOnCSSSheets.

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

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8899943 [details]
Bug 1387913: Make sure to update the stylist if needed on EnsureUniqueInnerOnCSSSheets.

https://reviewboard.mozilla.org/r/171274/#review177784

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/774d5c954828
Make sure to update the stylist if needed on EnsureUniqueInnerOnCSSSheets. r=heycam

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/774d5c954828