1222226 - CSS class applied to wrong elements

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Tom S [:evilpie]]

Attachment: test case

Reported here: https://www.reddit.com/r/firefox/comments/3roday/firefox_42_dom_ready_issue/

Boris thinks this might be caused by bug 1180118.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=993384a9dc2ea2af1c86d841ed78698ebc028dea&tochange=25b33fb7dc51

Regressed by: Bug 1180120

Comment 2

[Cameron McCormack (:heycam)]

The conditions we're checking to see if it's safe to return eRestyleResult_StopWithStyleChange are insufficient.

We check that CalcStyleDifference() told us that only reset structs changed, and assume that means inherited style can't have changed, and that if we know that no descendants have explicitly inherited reset properties, then it is safe not to restyle them.

The frames that we're restyling are two of the <span>s.  We don't have a Color struct cached on their style contexts (we're restyling before we build display lists, so we won't have looked up Color structs yet, and in any case the <span>s themselves doesn't need a Color, their child nsTextFrames do).

In this case, all four <span>s frames share the same style context, which means that when we move the child style context (there's only one, as the nsTextFrames also share the same style context), we'll resolve the same red Color struct for all four of them.

So I think we could either (a) prevent eRestyleResult_StopWithStyleChange from being returned when oldContext is shared, or (b) allow eRestyleResult_StopWithStyleChange for shared style contexts but only if we know that there is no inherited style difference between the old and new style contexts.

(a) is easy and we should maybe do that to begin with, but we should consider whether (b) is feasible.  We could compare the old and new style contexts' rule nodes -- if we find their nearest common ancestor, and didn't find any inherited properties on the rule nodes on those two branches (excluding the common ancestor itself), then we know we're safe.

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #2)
> We could compare the old and new style
> contexts' rule nodes -- if we find their nearest common ancestor, and didn't
> find any inherited properties on the rule nodes on those two branches
> (excluding the common ancestor itself), then we know we're safe.

We have mStyleBits on nsCSSCompressedDataBlock, so for StyleRules it would be easy to look that up, but we'd need to add something to other types of nsIStyleRules to store which structs' properties they can have...

Comment 4

[Cameron McCormack (:heycam)]

Attachment: patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5537781be9ca

Let's fault out when we find the old context is shared, and work out how to do the better thing in another bug.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So is the idea here that the tradeoff we're making is that in order to be able to reparent *children* that are shared, we can't use stopWithStyleChange on a parent that's shared?  (We can reparent children that are shared, right?)

How does this affect the effectiveness of the original optimization?

Comment 6

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #5)
> So is the idea here that the tradeoff we're making is that in order to be
> able to reparent *children* that are shared, we can't use
> stopWithStyleChange on a parent that's shared?  (We can reparent children
> that are shared, right?)

I'm not exactly sure what you mean about the tradeoff -- what's the other choice?  But yes if the parent is unshared and the children are shared, you will be able to reparent them all (because you're sure that it is correct for all of them to move).

> How does this affect the effectiveness of the original optimization?

It does weaken it; shared style contexts are pretty common.  So I think it does make sense as a followup to try and restore it with a stronger check for whether we have changed inherited data.  I figured it'd be better to get things correct first, though, since it wasn't immediately straightforward to do option (b).

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #6)
> (In reply to David Baron [:dbaron] ⌚UTC+8 from comment #5)
> > So is the idea here that the tradeoff we're making is that in order to be
> > able to reparent *children* that are shared, we can't use
> > stopWithStyleChange on a parent that's shared?  (We can reparent children
> > that are shared, right?)
> 
> I'm not exactly sure what you mean about the tradeoff -- what's the other
> choice?  But yes if the parent is unshared and the children are shared, you
> will be able to reparent them all (because you're sure that it is correct
> for all of them to move).

I think this would be ok if we forbade reparenting of shared children -- but I think reparenting of shared children is even more important than this, so it feels like the right tradeoff to me.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #7)
> I think this would be ok if we forbade reparenting of shared children -- but
> I think reparenting of shared children is even more important than this, so
> it feels like the right tradeoff to me.

I see, and I agree.

Comment 9

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/css-classes-applied-to-wrong-elements/

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0733342d8c56

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8684049 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1180120
[User impact if declined]: incorrect styling of elements if restyled after frame construction but before display list construction
[Describe test coverage new/current, TreeHerder]: try run passed; just landed on inbound
[Risks and why]: low, this just disables an optimisation in certain situations
[String/UUID change made/needed]: N/A

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0733342d8c56

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8684049 [details] [diff] [review]
patch

Patch is a one-liner fix that has been in Nightly for 2 days and includes automated reftests. Let's uplift to Aurora44.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Recent regression in 42, since 41 shipped. Tracked for 44 to make sure we don't re-regress.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8684049 [details] [diff] [review]
patch

Low risk, has tests, let's take this for beta also.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e7cd2596aeb

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5ff2e264e7ff

Comment 18

[Mahendra Potharaju [:mahe]]

Comment on attachment 8684049 [details] [diff] [review]
patch

Approved for 2.5 uplift. Seems to have landed in Aurora and Beta too.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Belatedly adding tracking flags since this was a regression - in case it reopens.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5e7cd2596aeb

Comment 22

[Sylvestre Ledru [:Sylvestre]]

42 is gone.