interfaces/Animation/cancel.html test trips on DCHECK |
||||
Issue descriptionThe third test in third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/Animation/cancel.html crashes in debug mode. This line: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/Animation/cancel.html?l=49&rcl=885c4a8194dba985d53315443b2c9eff761c55c3 causes the code to trip over this DCHECK: DCHECK_NE(m_animation->m_readyPromise->getState(), AnimationPromise::Pending); which is located here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/animation/Animation.cpp?l=1021&rcl=5fa4cb0d7a1a784fec7991eaeb1a80e43f9156e5 The test cancels and then plays the animation and checks that the promise is correctly being reused. If I understand correctly, the chain of events is: - animation cancelled, causing play state to become Idle - ready promise rejected - ready promise reset, which makes the promise state Pending - resolvePromiseAsync called, which should take the promise state out of Pending - animation.play() called, which transitions the play state from Idle to Pending - DCHECK that promise state is not Pending It seems like the async resolve of the promise is not causing the expected promise state change soon enough for the check in transitioning to play. If I modify the test to wait for the ready promise to be rejected after cancelling before calling play, the test does not crash (but fails on the later assertion, which may or may not be an independent problem). adithyas, as author of https://codereview.chromium.org/2615253002 which adds the call to resolvePromiseAsync, do you have any recommendations or pointers that would help us work out how to fix this?
,
Apr 6 2017
The suggestion in #1 would break compat with Firefox I think.
In Firefox and Chrome (experimental) this logs true:
var animation = document.body.animate(null);
var ready = animation.ready;
ready.then(() => {
console.log(ready === animation.ready);
});
If you clear m_readyPromise during resolvePromiseAsync() I think that would cause the object instance to change between resolving and accessing animation.ready.
,
Apr 7 2017
In a local patch I undid the async part of https://codereview.chromium.org/2615253002 and recreated the problem in issue 678706 . The resulting crash logs showed that the forbidden scope cases occurred from Animation::notifyCompositorStartTime, Animation::preCommit and Animation::update. These three functions create the PlayStateUpdateScope with the argument DoNotSetCompositorPending, and the only other place where the PlayStateUpdateScope is created with that argument is Animation::postCommit. Could we restrict resolvePromiseAsync to when the PlayStateUpdateScope is created with DoNotSetCompositorPending?
,
Apr 7 2017
I think the suggestion in #3 sounds reasonable, but I'm not sure how I would explain the link between creating PlayStateUpdateScope in a forbidden scope and not setting DoNotSetCompositorPending. I was thinking of just restricting resolvePromiseAsync() to when ScriptForbiddenScope::isScriptForbidden(). So when calling cancel() (which definitely does not happen in a forbidden scope), the ready promise is resolved synchronously, and calling play() directly after will satisfy the DCHECK (the promise will be in a resolved state).
,
Apr 10 2017
#4: Uh, yeah, good point, we could just check isScriptForbidden rather than DoNotSetCompositorPending. I was guessing that the lifecycle stages where we call this function and set compositor pending were necessarily not in a forbidden scope, but your suggestion would be much more readable and less fragile. I'll make a patch and send it your way for review.
,
Apr 11 2017
,
Apr 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/220578cfcb90370db3b268c6e59c1c4417a60708 commit 220578cfcb90370db3b268c6e59c1c4417a60708 Author: suzyh <suzyh@chromium.org> Date: Wed Apr 12 02:40:58 2017 Resolve promises synchronously sometimes https://codereview.chromium.org/2615253002 changed ~PlayStateUpdateScope() to resolve promises asynchronously in order to avoid script execution inside a forbidden scope. However, this causes the promises to behave differently from expected in some cases, demonstrated in part by a crash in the interfaces/Animation/cancel.html test due to failing a DCHECK. This patch modifies the promise resolution code to first check whether we are in a forbidden scope, and only resolve the promise asynchronously if so. BUG= 708887 Review-Url: https://codereview.chromium.org/2808013002 Cr-Commit-Position: refs/heads/master@{#463907} [modify] https://crrev.com/220578cfcb90370db3b268c6e59c1c4417a60708/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/220578cfcb90370db3b268c6e59c1c4417a60708/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/Animation/finish-expected.txt [modify] https://crrev.com/220578cfcb90370db3b268c6e59c1c4417a60708/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/Animation/finished-expected.txt [modify] https://crrev.com/220578cfcb90370db3b268c6e59c1c4417a60708/third_party/WebKit/Source/core/animation/Animation.cpp [modify] https://crrev.com/220578cfcb90370db3b268c6e59c1c4417a60708/third_party/WebKit/Source/core/animation/Animation.h
,
Apr 12 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by suzyh@chromium.org
, Apr 6 2017