Renderer crash when setting animation.currentTIme twice |
||||||
Issue descriptionSee the CL at https://crrev.com/c/1037188, which reproduces this on the trybots. The layout test sets up a keyframe animation, triggers it in a script by setting a CSS class, and then attempts to manipulate the animation with the JS API. The last thing it does is to set currentTime to two different values. When DCHECKs are enabled, and experimental-web-platform-features are turned on, the layout test hits a DCHECK as follows: [1:1:0501/120306.953145:FATAL:css_animations.cc(391)] Check failed: !is_animation_style_change. #0 0x7f1e76ff915d base::debug::StackTrace::StackTrace() #1 0x7f1e76d236bc base::debug::StackTrace::StackTrace() #2 0x7f1e76d9517a logging::LogMessage::~LogMessage() #3 0x7f1e66b0b428 blink::CSSAnimations::CalculateAnimationUpdate() #4 0x7f1e66ea27a0 blink::StyleResolver::CalculateAnimationUpdate() #5 0x7f1e66e9f687 blink::StyleResolver::ApplyMatchedPropertiesAndCustomPropertyAnimations() #6 0x7f1e66e9f20a blink::StyleResolver::StyleForElement() #7 0x7f1e66fcfa48 blink::Element::OriginalStyleForLayoutObject() #8 0x7f1e66fcf609 blink::Element::StyleForLayoutObject() #9 0x7f1e66fd0461 blink::Element::RecalcOwnStyle() #10 0x7f1e66fcfe88 blink::Element::RecalcStyle() #11 0x7f1e66f441e3 blink::ContainerNode::RecalcDescendantStyles() #12 0x7f1e66fcffe6 blink::Element::RecalcStyle() #13 0x7f1e66f441e3 blink::ContainerNode::RecalcDescendantStyles() #14 0x7f1e66fcffe6 blink::Element::RecalcStyle() #15 0x7f1e66f655a9 blink::Document::UpdateStyle() #16 0x7f1e66f612f5 blink::Document::UpdateStyleAndLayoutTree() #17 0x7f1e66f78a83 blink::Document::FinishedParsing() #18 0x7f1e687c2f87 blink::HTMLConstructionSite::FinishedParsing() #19 0x7f1e6883867f blink::HTMLTreeBuilder::Finished() #20 0x7f1e687d706e blink::HTMLDocumentParser::end() In release mode, the condition which would trigger the DCHECK is actually handled, and the crash is avoided.
,
May 1 2018
Rather than setting twice, this appears to be triggered by a combination of pausing the animation and changing the currentTime during the same frame. It does not appear to matter whether animation.pause() happens before or after setting the time. Setting currentTime to its current value (not actually changing it) does not trigger a crash.
,
May 2 2018
,
May 2 2018
,
May 2 2018
,
May 9 2018
,
May 10 2018
This is an interesting case. I'm not familiar with CSSAnimations::CalculateAnimationUpdate, but here are a few notes: It has an optimization (only used when DCHECK is disabled) where we assume that if ElementAnimations::IsAnimationStyleChange is true, the animation will not have changed state (i.e. been paused, cancelled, started, etc) since the last update. Otherwise, when ElementAnimations::IsAnimationStyleChange is false, it: a. Iterates over the CSS property animation-name. b. For each name, retrieve the existing blink::Animation object if there is one, otherwise create one. Interestingly, it bases whether or not the animation is paused on the animation-play-state CSS property. It does two things with this: a. Marks the animation as paused in the animations_with_updates_ list (unsure of the effect of this yet) b. Compares animation-play-state to animation->Paused(); if they differ, it notes that the animation needs its pause state toggled later in the style update. The issue here is that the animation exists, but animation-play-state != animation->Paused(). What I can't determine yet is whether the code expects to double-flip the paused state (e.g. the animations_with_updates_ from (a) will un-pause it and then the toggle from (b) will re-pause it, or if we should be making sure that animation-play-state has been updated here. As a side note, the CSS animation/transition update logic during style recalc is horribly hard to understand :(.
,
May 10 2018
Oh, I've just immediately realized - the code does *not* expect to double-toggle. The |animations_with_updates_| list is *only* updated if the keyframes have changed. So the problem here is that this code assumes that in this case, animation-play-state would match animation->Paused(), but that's not true.
,
May 10 2018
Note: the currentTime update is required to cause CSSAnimations::CalculateAnimationUpdate to actually be called. Otherwise there is no update since the animation is paused at its start point.
,
May 10 2018
This is fascinating. In both Firefox and Chrome, pausing an animation via JS does *not* update the computed style value of 'animation-play-state'. CSS Animations 2 has some language about this, turns out it is by spec: https://drafts.csswg.org/css-animations-2/#animation-play-state . Whether we implement this or not fully I'm not entirely sure... In any case, the code here in CSSAnimations::CalculateAnimationUpdate is a disaster when CSS Animations and Web Animations mix, and the only reason this currently works in release mode is because we happen to think that this constitutes a 'AnimationStyleChange' and so optimize away the incorrect state change that would happen -_-.
,
May 10 2018
Ok, so I think I believe that this *is* an AnimationStyleChange. CalculateAnimationUpdate is only responsible for flipping state changes from CSS changes, e.g. if someone updates animation-play-state or the @keyframes rule. As such, I think we need to change CalculationAnimationUpdate to track a cached animation-play-state vector in RunningAnimation (https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/animation/css/css_animations.h?gsn=ToggleAnimationIndexPaused&l=108) and compare to that rather than to blink::Animation::Paused().
,
May 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2fb2fc1f681816489bb92cb94a128b3bf2f4f0bb commit 2fb2fc1f681816489bb92cb94a128b3bf2f4f0bb Author: Stephen McGruer <smcgruer@chromium.org> Date: Mon May 14 19:17:38 2018 CalculateAnimationUpdate - track previous/current css-animation-playstate Previously this code compared the current (possibly new) value of css-animation-playstate to the value of blink::Animation::Paused() to determine if it should toggle the paused state. This is incorrect; if blink::Animation::pause() is called directly from JS these values diverge by spec[0]. We only need to toggle the pause state from CalculateAnimationUpdate if css-animation-playstate itself has changed. Note that this incorrect behavior did not actually manifest in release builds because of a no-DCHECK-only optimization; if is_animation_style_change is set, we skip the entire state-update step and just calculate interpolations. [0]: https://drafts.csswg.org/css-animations-2/#animation-play-state Bug: 838594 Change-Id: I23a9eb0d338171c5a125b54ec4e29b916aceb3b0 Reviewed-on: https://chromium-review.googlesource.com/1053759 Reviewed-by: Robert Flack <flackr@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#558409} [add] https://crrev.com/2fb2fc1f681816489bb92cb94a128b3bf2f4f0bb/third_party/WebKit/LayoutTests/animations/stability/pause-set-current-time.html [modify] https://crrev.com/2fb2fc1f681816489bb92cb94a128b3bf2f4f0bb/third_party/blink/renderer/core/animation/css/css_animation_update.h [modify] https://crrev.com/2fb2fc1f681816489bb92cb94a128b3bf2f4f0bb/third_party/blink/renderer/core/animation/css/css_animations.cc [modify] https://crrev.com/2fb2fc1f681816489bb92cb94a128b3bf2f4f0bb/third_party/blink/renderer/core/animation/css/css_animations.h
,
May 14 2018
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by iclell...@chromium.org
, May 1 2018