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

Handle change to pending state when resolving the ready promise #22441

Closed
wants to merge 1 commit into from

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Mar 25, 2020

The ready promise depends on the pending state, which in turn can be
impacted by a pending style change. Checking the pending state prior
to checking the ready promise fixes the problem. A few tweaks were
needed to the test itself. If we create a ready promise, we need to
wait for it to resolve. Otherwise, an error is potentially triggered
(race condition) during tear down of the test as it will cancel a
pending animation, which in turn, rejects the ready promise. Any
unexpected promise reject triggers a harness error.

Bug: 1064640
Change-Id: I25191dd26206368e497638ce9f73984b2e61cad4

Reviewed-on: https://chromium-review.googlesource.com/2118183
WPT-Export-Revision: e453aa387d3010fd8800a44a1ea509dac2121878

The ready promise depends on the pending state, which in turn can be
impacted by a pending style change. Checking the pending state prior
to checking the ready promise fixes the problem. A few tweaks were
needed to the test itself.  If we create a ready promise, we need to
wait for it to resolve. Otherwise, an error is potentially triggered
(race condition) during tear down of the test as it will cancel a
pending animation, which in turn, rejects the ready promise.  Any
unexpected promise reject triggers a harness error.

Bug: 1064640
Change-Id: I25191dd26206368e497638ce9f73984b2e61cad4
Copy link
Collaborator

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

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

@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Mar 27, 2020

@birtles can you take a look at the changes here? The bit I'm not sure about it:

If we create a ready promise, we need to
wait for it to resolve. Otherwise, an error is potentially triggered
(race condition) during tear down of the test as it will cancel a
pending animation, which in turn, rejects the ready promise. Any
unexpected promise reject triggers a harness error.

I believe I managed to create a minimal reproduction, which appears to reject during cleanup in Firefox Nightly:

// Modified css/css-animations/CSSAnimation-ready.tentative.html so this was the only test in it:
promise_test(async t => {
  const div = document.createElement('div');
  document.body.appendChild(div);
  t.add_cleanup(() => {
    div.remove();
  });

  div.style.animation = 'abc 100s';
  const animation = div.getAnimations()[0];
  const readyPromise = animation.ready;

  // Finish without waiting for ready.
}, 'Testing');
@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Mar 31, 2020

@birtles FYI Kevin realized that Chrome was missing the step to mark the promise as handled, hence the unexpected top-level rejections. So he has reverted the WPT test changes in the downstream Chromium CL and this PR is no longer valid (I will close it once the downstream change lands).

@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Mar 31, 2020

Upstream change landed with no changes to WPT, closing this.

@stephenmcgruer stephenmcgruer deleted the chromium-export-cl-2118183 branch Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.