1405548 - stylo: getComputedValue might not flush style with stylo

Status: RESOLVED FIXED

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

Description

[Makoto Kato [:m_kato]]

When building stylo debug build for Fennec, the following mochitest is always failure.

https://treeherder.mozilla.org/logviewer.html#?job_id=134365330&repo=try&lineNumber=1880
TEST-UNEXPECTED-FAIL | layout/style/test/test_animations_async_tests.html | initial value of animation (force flush) - got "0px", expected "-1000px"

http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/layout/style/test/file_animations_async_tests.html#41

    gDisplay.appendChild(animdiv);
    var cs = getComputedStyle(animdiv, "");
    is(cs.marginLeft, "-1000px", "initial value of animation (force flush)");

When I add animdiv.clientHeight for force flush fefore calling getComputedStyle(), cs.marginLeft is expected "-1000px".

Also, this doesn't occur on release build even if Fennec.  Fennec's test infra is android emulator (qemu/arm), so this is only debug build on Fennec (may be due to too slow platform only)

Comment 1

[Makoto Kato [:m_kato]]

s/fefore calling getComputedStyle()/before calling getComputedStyle()/

Comment 2

[Cameron McCormack (:heycam)]

Could this be related to bug 1363805?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Yeah, this may be related to bug 1363805.
Oops, mid-air collision.

Comment 4

[Jet Villegas (inactive)]

Kato-san: please confirm comment 3 and if Firefox 57 (non-Fennec) is affected. Thx!

Comment 5

[Makoto Kato [:m_kato]]

(In reply to Cameron McCormack (:heycam) (away 4 Oct) from comment #2)
> Could this be related to bug 1363805?

No, when I back out bug 1363805, this still occurs.

(In reply to Jet Villegas (:jet) from comment #4)
> Kato-san: please confirm comment 3 and if Firefox 57 (non-Fennec) is
> affected. Thx!

This occurs on android/stylo-debug only due to slow platform (on arm emulator).  Since desktop build is too faster, I cannot reproduce this on our tests even if root cause is a bug.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Then this might be animation bug.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Note that getComputedStyle() should flush any pending restyles for the element, so it could also happen while machine is on heavy load even if it's on desktop.
One thing I am concerned is that the test runs on test refresh mode, if this bug does not happen on normal refresh mode, it's not a big problem.

Comment 8

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

AFAIK marginLeft needs layout flush, so the code path should be the same without bug 1363805.

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Taking this for now.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I finally managed to reproduce this locally and get logging to work too. I have confirmed is that we are successfully creating the CSS animation as part of the flush.

However, it seems that we don't compose the animation as part of that flush. In the next flush, however, we do.

That is, we have the following:

  var cs = getComputedStyle(animdiv, "");

When we do:

  cs.marginLeft;

we create the animation but *don't* compose it.

If we once more call:

  cs.marginLeft;

we sample the animation (and the test subsequently passes).

Furthermore, this appears to be true even if we when the refresh driver is not under test control.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

That's a bad news.
(In reply to Brian Birtles (:birtles) from comment #10)
> I finally managed to reproduce this locally and get logging to work too. I
> have confirmed is that we are successfully creating the CSS animation as
> part of the flush.
> 
> However, it seems that we don't compose the animation as part of that flush.
> In the next flush, however, we do.

So, we seems to fail the second animation-only restyle which should have processed after creating the CSS animation in a sequential task.

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

This is odd, but it seems like when we request a restyle from KeyframeEffectReadOnly::NotifyAnimationTimingUpdated as part of setting up a new CSS animation, we request a throttled restyle (i.e. CanThrottle is returning true -- perhaps because we don't have a frame yet?). As a result, EffectCompositor::PreTraverseInSubtree is returning false which is what would normally cause us to do a second animation-only restyle.

I would have expected something to trigger a *layer* restyle when we create a new CSS animation, but I guess that's not happening.

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #12)
> (i.e. CanThrottle is returning true -- perhaps because we don't have a frame yet?).

Confirmed that this is because we don't have a frame yet.

I also confirmed that in this test we never get a layer-type restyle.

I'll look into this again tomorrow but for a start I need to look into what causes us to update on desktop platforms.

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(Looks like bug 1332958 made us explicitly *not* call PostUpdate when creating a new CSS animation since with the Gecko style system we don't need that.)

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Birtles (:birtles) from comment #14)
> (Looks like bug 1332958 made us explicitly *not* call PostUpdate when
> creating a new CSS animation since with the Gecko style system we don't need
> that.)

That hasn't landed though, right?

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> (In reply to Brian Birtles (:birtles) from comment #14)
> > (Looks like bug 1332958 made us explicitly *not* call PostUpdate when
> > creating a new CSS animation since with the Gecko style system we don't need
> > that.)
> 
> That hasn't landed though, right?

Right. It turns out it doesn't matter in this case because the old and new animation indices are both zero so we don't post a restyle from here.

So far though this sounds like a pretty fundamental problem with our animation setup.

In Gecko-land we go to great lengths to avoid posting a restyle while generating animations. We have all sorts of XXXFromStyle, XXXNoUpdate methods that do work on Animation objects without posting restyles. That's because in Gecko we update CSS animation styles on the target elements as part of the same restyle (which introduces bugs when it comes to triggering transitions).

In Servo-land we update CSS animations as part of a subsequent traversal so we actually want those restyles to be posted.

What I don't understand yet, however, is how we got this far without noticing this. There must be something else that happens to trigger this restyle for us on desktop platforms. Perhaps something in the way do cascade updates?

Comment 17

[Hiroyuki Ikezoe (:hiro)]

FWIW, in gecko when we append an element to the document, we do style the element synchronously, so in the test case;

 gDisplay.appendChild(animdiv);

After this, the animdiv has been already styled, but in stylo we do lazily the element style, so I guess that's the different between stylo and gecko.  I haven't dug into detail though.

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

So it turns out this test failure reproduces for me locally on Ubuntu the first time I run it if I run the test in isolation.

The reason appears to be that the animation index assigned to the first Animation object created in the content process will be zero. When the CSS animation building work goes to assign the CSS-based animation index of zero it detects no change and so does NOT post an update.

If I reload the test, however, presumably we are using the same content process so we get a non-zero initial animation index and so we *do* post a restyle in SetAnimationIndex and *do* correctly run the second animation restyle. Hence the test passes the second time onwards.

Presumably when we run in automation on desktop platforms  this test runs after other tests that create Animations and we re-use the same content process and hence we never hit this condition.

For Android, however, this works differently although I don't recall how (I only recall that the setting that allows you to flip prefs in mochitest.ini doesn't work on Android, perhaps because of this).

Long-term once we drop the Gecko style system I think we want to just drop all the XXXFromStyle, XXXNoUpdate calls from CSSAnimationBuilder and make these things trigger restyles.

In terms of a short-term solution, one might be to unconditionally post an update for Stylo whenever the number of animations changes. However, I'm still really surprised this hasn't caused other bugs already and I need to look into it a bit more.

Comment 19

[Brian Birtles (:birtles, Feb 26~ parental leave)]

One note is that BuildAnimation is very careful to assemble KeyframeEffectReadOnly objects completely before attaching them to a CSSAnimation object. This prevents the effect from requesting restyles due to calls to, e.g. SetKeyframes (since we can't post a restyle without an animation telling us which cascade level to target).

On the other hand, when updating an animation, we update it in-place in UpdateOldAnimationPropertiesWithNew where the effect is already attached to an animation so in this case we *will* post restyles. That, I think, explains why we haven't seen bugs from *updates* to animations failing to trigger the second animation restyle.

So I think it's only the new / removed animation case we need to handle.

Given that this only reproduces when we are creating the first animation for a given content process I'm not sure how we'll be able to create an automated test for this.

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(If bug 1332958 had landed, we would have discovered this a lot sooner. However, now that we actually *do* want these changes to post restyle, we probably don't want to land that bug.)

Comment 21

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #18)
> For Android, however, this works differently although I don't recall how (I
> only recall that the setting that allows you to flip prefs in mochitest.ini
> doesn't work on Android, perhaps because of this).

Actually, I think this might just be due to the fact that we group tests differently on Android. For debug builds we have 48 batches of mochitests so there's a much higher chance that this test will be the first test in the batch to create an Animation. For release builds we have 36 batches hence why this doesn't reproduce in release builds in automation.

Comment 22

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35d3e692eb7e69de6eaa3e3caee1a80101ffab51

Comment 23

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1405548 - Post restyles when creating or removing new CSS animations when using the Servo backend;

As explained in the extended comment in this patch, for Servo we want to post
restyles when creating new animations so that we run a second animation
restyle and incorporate the result of new animations into style immediately.
(Gecko does everything in the one restyle, and although this causes other bugs
related to triggering transitions, at least it means it does not require
restyles to be posted here).

It turns out that we normally end up posting a restyle anyway in
CSSAnimation::SetAnimationIndex. Bug 1332958 was supposed to drop that but it
never landed.

However, CSSAnimation::SetAnimationIndex only posts a restyle when there is
a change to the animation index. It turns out that, by chance, there normally
*is* a change to a CSSAnimation's animation index when it is created. Initially
it takes its animation index from Animation::sNextAnimationIndex which is
incremented each time it is assigned to an animation.

If the first Animation we create for a given content process is a CSSAnimation
then sNextAnimationIndex will be zero and so we will initially assign an
animation index of zero. If that CSS animation is also the first in the list
of animations in animation-name, when we call SetAnimationIndex we will pass
zero as the index to use, and when we go to update the animation index we will
detect that there is no change, and will NOT post an animation restyle.
As a result the target element's style will NOT reflect the animated style.

To fix this we need to ensure that *new* CSS animations trigger a restyle.

For *changes* to animations, the corresponding calls to SetKeyframes and
SetSpecifiedTiming post restyles so the behavior should be correct in those
cases.

For *removed* animations I observed that in at least some cases we successfully
post a restyle. However, this appeared to be as much by chance as anything so
this patch also posts a restyle for removed animations. (Note that the
EffectCompositor will ignore redundant restyle requests so this is ok.)

This patch deliberately does not expose Animation::PostUpdate and call that
because the code introduced here is intended to be temporary. Long-term we
should remove the Gecko style backend and allow the calls to PlayFromStyle,
PauseFromStyle, CancelFromStyle etc. to post restyles just like calls to Play,
Pause, and Cancel do. At that point this code can also be removed.

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

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8917260 [details]
Bug 1405548 - Post restyles when creating or removing new CSS animations when using the Servo backend;

https://reviewboard.mozilla.org/r/188272/#review193502

This looks realy good to me.
One thing I am still concerned is the animation indices matching case.
Apart from this bug, it's possible that the animation index matches sNextAnimationIndex even if the animation index is not zero, and then we miss RequestRestyle for layer. I am guessing it can explain some intermittent failures.  What do you think?

Comment 25

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> One thing I am still concerned is the animation indices matching case.
> Apart from this bug, it's possible that the animation index matches
> sNextAnimationIndex even if the animation index is not zero, and then we
> miss RequestRestyle for layer. I am guessing it can explain some
> intermittent failures.  What do you think?

Right. However, that quickly becomes unlikely. For example, one Animation::sNextAnimationIndex reaches, say, 5, it's very unlikely we'll encounter this since it's unusual to have an animation-name property with as many as 5 elements.

I'm not even 100% certain we can hit it for the case when sNextAnimationIndex is 1 since we'd have to have an animation-name property with at least two elements and if we create the animations in reverse order (which I think we do?) then I think at least one of the created animations' indices would change so we wouldn't hit this bug.

So, yes, it might explain some intermittent failures (I hope it does!) but I'm not completely certain yet if it does.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17f1a2242a2c
Post restyles when creating or removing new CSS animations when using the Servo backend; r=hiro

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/17f1a2242a2c