Closed Bug 1405548 Opened 2 years ago Closed 2 years ago

stylo: getComputedValue might not flush style with stylo

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: m_kato, Assigned: birtles)

References

Details

Attachments

(1 file)

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)
s/fefore calling getComputedStyle()/before calling getComputedStyle()/
Could this be related to bug 1363805?
Yeah, this may be related to bug 1363805.
Oops, mid-air collision.
Summary: getComputedValue might not flush style with stylo → sylo: getComputedValue might not flush style with stylo
Priority: -- → P3
Kato-san: please confirm comment 3 and if Firefox 57 (non-Fennec) is affected. Thx!
Flags: needinfo?(m_kato)
Summary: sylo: getComputedValue might not flush style with stylo → stylo: getComputedValue might not flush style with stylo
(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.
Flags: needinfo?(m_kato)
Then this might be animation bug.
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.
AFAIK marginLeft needs layout flush, so the code path should be the same without bug 1363805.
Taking this for now.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
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.
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.
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.
(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.
(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.)
(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?
(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?
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.
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.
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.
(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.)
(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.
See Also: → 1407527
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?
Attachment #8917260 - Flags: review?(hikezoe) → review+
(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.
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17f1a2242a2c
Post restyles when creating or removing new CSS animations when using the Servo backend; r=hiro
https://hg.mozilla.org/mozilla-central/rev/17f1a2242a2c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.