1417600 - stylo: browser/components/downloads/test/browser/browser_downloads_autohide.js times out with stylo-chrome

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+10]

The test checkStateDuringPrefFlips in browser/components/downloads/test/browser/browser_downloads_autohide.js times out when waiting for the third gCustomizeMode.addToPanel() in that test [1] after enabling stylo-chrome (via setting layout.css.servo.chrome.enabled to true and restarting the browser).

Looking at the code of gCustomizeMode.addToPanel, it seems that the only place it's waiting is a promise returned from _promiseWidgetAnimationOut [2], and the latter waits for animationend event or finishing customization mode [3].

So it seems that it is an animationend which is supposed to be fired isn't fired. But if I focus to another window, this test can continue running. Probably at that time, the animationend is eventually fired in a whole tree restyle.

birtles, hiro, or boris, could you have a look at this bug? I have no idea what's happening behind...

[1] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/browser/components/downloads/test/browser/browser_downloads_autohide.js#51
[2] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/browser/components/customizableui/CustomizeMode.jsm#659-662
[3] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/browser/components/customizableui/CustomizeMode.jsm#611-613

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I will look.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

I guess you would be able to reproduce this via |mach mochitest browser/components/downloads/test/browser/browser_downloads_autohide.js --setpref=layout.css.servo.chrome.enabled=true|.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Thank you, Xidorn. I can reproduce locally.  It seems the animation itself is not created at that time.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

I've almost forgotten how I had been debugging servo styling..  Anyway, after adding 'animate-out' class to the element, the element is not traversed for some reason, that's why the animation is not created.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Emilio helped me to understand what's going on there.  But I don't still understand it yet. :/
Anyway, now what I know is that after adding 'animate-out' class, class_added in collect_invalidations is empty. :/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> Emilio helped me to understand what's going on there.  But I don't still
> understand it yet. :/
> Anyway, now what I know is that after adding 'animate-out' class,
> class_added in collect_invalidations is empty. :/

That can happen if it was both removed and added, that is, if we had not flushed style before removing it. Is there any chance that that's what's happening?

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> That can happen if it was both removed and added, that is, if we had not
> flushed style before removing it. Is there any chance that that's what's
> happening?

That sounds plausible. If I add
> this.window.getComputedStyle(animationNode).animationName;
before
> animationNode.classList.add("animate-out");
then the test passes.

It feels like a more general issue related to CSS animation, then. Changing animation-name is supposed to always trigger a new animation, I believe.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

So, the spec seems to be somehow vague on behavior of this case. It says
> An animation specified by dynamically modifying the element's style
> will start when this style is resolved; that ... may be when the
> scripting engine returns control to the browser (in the case of style
> applied by script). 
so it gives browsers leeway for when to start the animation.

And it seems that the testcase indeed can potentially lead to something broken.

Basically, the "animate-out" class is removed in animationend, and re-added synchronously. There doesn't seem to be anywhere between them which hands control back to the browser.

So I guess we should probably fix this by always forcing a restyle before setting "animate-out", or forcing that after removing it? That does sound like something may harm the performance in UI, though.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

I'm not sure whether anyone would want to continuously animate the same node outside this test... If not, we can probably just add force restyle into the test so that it passes...

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1417600 - Wait for a style flush before setting animation out to ensure start of the animation.

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

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8929092 [details]
Bug 1417600 - Wait for a style flush before setting animation out to ensure start of the animation.

https://reviewboard.mozilla.org/r/200386/#review205544

::: browser/components/customizableui/CustomizeMode.jsm:611
(Diff revision 1)
> +      // Force a restyle before setting animate-out class to ensure we
> +      // do start the animation.
> +      this.window.getComputedStyle(animationNode).animationName;

We can use `BrowserUtils.promiseLayoutFlushed(document, "style", callback)` to return control back to the browser until style has been flushed and then move the remaining code in to the callback.

This would fix the bug without introducing a sync style flush.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8929092 [details]
Bug 1417600 - Wait for a style flush before setting animation out to ensure start of the animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200386/diff/1-2/

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8929092 [details]
Bug 1417600 - Wait for a style flush before setting animation out to ensure start of the animation.

https://reviewboard.mozilla.org/r/200386/#review205544

> We can use `BrowserUtils.promiseLayoutFlushed(document, "style", callback)` to return control back to the browser until style has been flushed and then move the remaining code in to the callback.
> 
> This would fix the bug without introducing a sync style flush.

Oh, didn't know that we have something like this. It looks useful. Thanks.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8929092 [details]
Bug 1417600 - Wait for a style flush before setting animation out to ensure start of the animation.

https://reviewboard.mozilla.org/r/200386/#review205580

Thanks! Yes it is very cool :)

Comment 15

[Hiroyuki Ikezoe (:hiro)]

yeah, flushing style fixes this timeout, but I think there is an underlying issue that the element is skipped to be traversed.  I will file bug 1418082.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04dac3301ecd
Wait for a style flush before setting animation out to ensure start of the animation. r=jaws

Comment 17

[Andreea Pavel [:apavel]]

Backed out for failing  browser/components/customizableui/test/browser_972267_customizationchange_events.js on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=04dac3301ecdea203603c8876f6bcb06cd6bca86&filter-searchStr=090557450b6ede7a78b5fceee956dc5d657d26e3

Error log: https://treeherder.mozilla.org/logviewer.html#?job_id=145486723&repo=autoland&lineNumber=2745

Back out: https://hg.mozilla.org/integration/autoland/rev/c9e9651562d3166e3d4f012b4dd8a8622cdbd4a2

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

It seems that somehow promiseLayoutFlushed is not guaranteed to be triggered...

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

jaws, any idea why the callback passed to promiseLayoutFlushed is never executed in test browser/components/customizableui/test/browser_972267_customizationchange_events.js?

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8929092 [details]
Bug 1417600 - Wait for a style flush before setting animation out to ensure start of the animation.

https://reviewboard.mozilla.org/r/200386/#review207124

::: browser/components/customizableui/CustomizeMode.jsm:613
(Diff revision 2)
>          resolve();
>        }
>  
> +      // Wait for a style flush to ensure we do start the animation.
> +      BrowserUtils.promiseLayoutFlushed(this.document, "style", () => {
> -      animationNode.classList.add("animate-out");
> +        animationNode.classList.add("animate-out");

I don't understand why the callback is not executed, but I do see an error here which might be related.

The callback must not trigger any reflows, and right now it is by setting the className.

You will need to move this within a requestAnimationFrame callback within this callback.

For example,
```
BrowserUtils.promiseLayoutFlushed(this.document, "style", () => {
  this.window.requestAnimationFrame(() => {
    animationNode.classList.add("animate-out");
    ...
  });
});
```

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Since the callback is not invoked at all... I think wrapping it with requestAnimationFrame wouldn't fix that failure...

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Please flag me for needinfo again if that doesn't help.

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

I wasn't sure how you were testing that the callback isn't invoked.

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> I wasn't sure how you were testing that the callback isn't invoked.

I put a dump before and inside promiseLayoutFlushed, and the one inside never shows up.

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

It is more weird that even if I request a synchronous style flush immediately before promiseLayoutFlushed, the callback still isn't invoked synchronously, and it still gets stuck on waiting for reflow.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

How about flushing the style after removing 'animated-out' class?  As far as I observed, we fail to snapshot for the removal (and thus animation is not triggered by adding the class).

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #26)
> How about flushing the style after removing 'animated-out' class?  As far as
> I observed, we fail to snapshot for the removal (and thus animation is not
> triggered by adding the class).

That doesn't work... because frontend people don't want to see more synchronous flush, otherwise we can just add a synchronous flush before adding 'animated-out' class. Also there are way more places where the class is removed than where it is added, so adding to those places means more duplicate code that may not have enough tests on.

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #25)
> It is more weird that even if I request a synchronous style flush
> immediately before promiseLayoutFlushed, the callback still isn't invoked
> synchronously, and it still gets stuck on waiting for reflow.

Somehow this works now... I don't think I changed anything, but it does work for some reason... But we don't want synchronous style flush so we still need to figure out some way without this.

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

So, looking at the code, I now suspect that promiseLayoutFlushed is somehow broken for at least this usage.

The callback would only be triggered if there is any actual reflow happens, which means, if there is no style / content change which needs to trigger reflow (e.g. some neutral style change or changes inside display:none tree), the callback would not be invoked until something else causes an actual reflow.

In the test, then, since nothing really triggers another reflow, it gets blocked.

I'm not sure what's the right approach to solve this. I guess we may need some kind of finer-grained notification to happen after style flush, rather than solely relying on reflow notification.

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8929092 [details]
Bug 1417600 - Wait for a style flush before setting animation out to ensure start of the animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200386/diff/2-3/

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1417600 - Wait for another frame before setting animation out to ensure start of the animation.

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

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8930970 [details]
Bug 1417600 - Wait for another frame before setting animation out to ensure start of the animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202064/diff/1-2/

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8930970 [details]
Bug 1417600 - Wait for another frame before setting animation out to ensure start of the animation.

https://reviewboard.mozilla.org/r/202064/#review207608

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d9c0a75a007
Wait for another frame before setting animation out to ensure start of the animation. r=jaws

Comment 35

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1d9c0a75a007