Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Gecko Bug 1622369] Await the async function to make sure any async steps inside the function will be blocked. #22502

Merged
merged 1 commit into from Mar 30, 2020
Merged

[Gecko Bug 1622369] Await the async function to make sure any async steps inside the function will be blocked. #22502

merged 1 commit into from Mar 30, 2020

Conversation

moz-wptsync-bot
Copy link
Collaborator

It's easy to get time out on mac if we don't await the async function.
The async functions return an implicit Promise. If the caller doesn't await
it, any async steps inside the function will not be blocked.

Differential Revision: https://phabricator.services.mozilla.com/D68448

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1622369
gecko-commit: 254999e1d7614bfcc52a572b1364c6be31a3b09b
gecko-integration-branch: autoland
gecko-reviewers: birtles

@BorisChiou @moz-wptsync-bot
Await the async function to make sure any async steps inside the func…
54a0f30 
…tion will be blocked.

It's easy to get time out on mac if we don't await the async function.
The async functions return an implicit Promise. If the caller doesn't await
it, any async steps inside the function will not be blocked.

Differential Revision: https://phabricator.services.mozilla.com/D68448

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1622369
gecko-commit: 254999e1d7614bfcc52a572b1364c6be31a3b09b
gecko-integration-branch: autoland
gecko-reviewers: birtles
wpt-pr-bot
wpt-pr-bot approved these changes Mar 27, 2020
View reviewed changes
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Firefox project.

@birtles
Copy link
Contributor

birtles commented Mar 29, 2020

@stephenmcgruer Any help interpreting the task failure here?

Seems like firefox-nightly-stability is failing due to (unrelated) AnimationWorklet errors.
And it seems like chrome-dev-stability is failing due to existing flakiness?

Not sure why the upstream/gecko task is marked failing either.

@stephenmcgruer
Copy link
Contributor

wpt-firefox-nightly-stability times out (likely due to number of tests affected, this is an ongoing problem and we have ~Q2 plans to make that not an error for folks):

[taskcluster:error] Task timeout after 7200 seconds. Force killing container.
[taskcluster 2020-03-27 21:04:40.372Z] === Task Finished ===

As you noted, chrome-dev-stability has flaky results:

9:12.42 INFO ## Unstable results ##
 9:12.42 INFO |                                        Test                                        |                             Subtest                             |          Results           |                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                         Messages                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                        |
 9:12.42 INFO |------------------------------------------------------------------------------------|-----------------------------------------------------------------|----------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 9:12.42 INFO | `/scroll-animations/two-animations-attach-to-same-scroll-timeline-cancel-one.html` |                                                                 | **FAIL: 8/10, PASS: 2/10** |                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                         |
 9:12.43 INFO | `/web-animations/interfaces/Animatable/getAnimations.html`                         | `Does not return an animation that has been removed`            | **FAIL: 6/10, PASS: 4/10** | `assert_array_equals: lengths differ, expected array [object "[object Animation]"] length 1, got [object "[object Animation]", object "[object Animation]"] length 2`                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                   |
 9:12.43 INFO | `/web-animations/interfaces/Animation/style-change-events.html`                    | `Animation.startTime produces expected style change events`     | **FAIL: 1/10, PASS: 9/10** | `assert_false: A transition should NOT have been triggered expected false got true`                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     |
 9:12.43 INFO | `/web-animations/timing-model/timelines/timelines.html`                            | `Timeline time increases once per animation frame`              | **FAIL: 8/10, PASS: 2/10** | `assert_greater_than: Timeline time increases between animation frames expected a number greater than 42.757 but got 42.757;assert_greater_than: Timeline time increases between animation frames expected a number greater than 51.292 but got 51.292;assert_greater_than: Timeline time increases between animation frames expected a number greater than 44.017 but got 44.017;assert_greater_than: Timeline time increases between animation frames expected a number greater than 41.41 but got 41.41;assert_greater_than: Timeline time increases between animation frames expected a number greater than 49.352 but got 49.352;assert_greater_than: Timeline time increases between animation frames expected a number greater than 37.914 but got 37.914;assert_greater_than: Timeline time increases between animation frames expected a number greater than 41.367 but got 41.367;assert_greater_than: Timeline time increases between animation frames expected a number greater than 45.491 but got 45.491` |
 9:12.43 INFO | `/web-animations/timing-model/timelines/timelines.html`                            | `Timeline time increases once per animation frame in an iframe` | **FAIL: 8/10, PASS: 2/10** | `assert_greater_than: Timeline time within an iframe increases between animation frames expected a number greater than 0 but got 0;assert_greater_than: Timeline time within an iframe increases between animation frames expected a number greater than 0.424 but got 0.424`                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                           |
 9:12.43 INFO

Based on wpt's flake view, the web-animations ones are definitely pre-existing (I believe kevers@ has a plan to look into these, but feel free to ping the Chromium Animations team about it). The scroll-timeline one has no flakes on wpt, but the original PR that landed it explicitly mentions that the test is flaky?! (cc @yi-gu and @majido as the owners of scroll timeline; are there plans to address this flaky test?)

I will admin merge this change since the diffs on browsers looks ok (the one 'regression' in Chrome is a flake).

@stephenmcgruer
Copy link
Contributor

Oh sorry, I didn't see the part about upstream/gecko failing. cc @jgraham for that specifically (I've checked the TC runs) - James, do you know how to investigate why the upstream/gecko check is red?

@jgraham
Copy link
Contributor

jgraham commented Mar 30, 2020

For reasons I haven't fuly understood that check sometimes doesn't get updated if there's a job failing. I looked at the code and it looks like it should work, but obviously I'm missing something.

@stephenmcgruer
Copy link
Contributor

Ok, admin-merging. Thanks :)

@stephenmcgruer stephenmcgruer merged commit dc5f049 into master Mar 30, 2020
@stephenmcgruer stephenmcgruer deleted the gecko/1622369 branch March 30, 2020 12:29
@majido
Copy link
Member

majido commented Mar 30, 2020

@yi-gu is looking to deflake the scroll timeline test (/scroll-animations/two-animations-attach-to-same-scroll-timeline-cancel-one.html) in https://bugs.chromium.org/p/chromium/issues/detail?id=1060974

@birtles
Copy link
Contributor

birtles commented Mar 30, 2020

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wpt-pr-bot wpt-pr-bot wpt-pr-bot approved these changes

Assignees
No one assigned
Labels
mozilla:gecko-sync web-animations wg-css wg-svg
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@moz-wptsync-bot @birtles @stephenmcgruer @jgraham @majido @wpt-pr-bot @BorisChiou