Commit 1d915632 authored by Majid Valipour's avatar Majid Valipour Committed by Commit Bot

[animation-worklet] Fix crash in tests when running on main thread

Worklet animation supports invalidation of its compositing state.

This patch makes this invalidation process a no-op for animations that are
running on main (i.e., |compositor_animation_| is null). This means we no longer
try to read/modify the non-existent |compositor_animation_| which was causing
the crash.

Note that in some situations, even for currently main thread animaiton, we may
actually want to create and start the |compositor_animation_| but this is a
separate issue that can be fixed as a follow up.

The added DCHECK uncovered a bug where we were attaching the worklet animation
to early even if play was failing. This is also fixed.

Bug: 887659, 889330
Change-Id: I4a89de6632e053f68beafb810355c25e4bfba0e3
Reviewed-on: https://chromium-review.googlesource.com/c/1249387
Commit-Queue: Majid Valipour <majidvp@chromium.org>
Reviewed-by: default avatarRobert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597199}
parent 0f552288
...@@ -5236,13 +5236,9 @@ crbug.com/887659 animations/animationworklet/animation-worklet-inside-iframe.htm ...@@ -5236,13 +5236,9 @@ crbug.com/887659 animations/animationworklet/animation-worklet-inside-iframe.htm
crbug.com/887659 animations/animationworklet/animator-animate.html [ Failure ] crbug.com/887659 animations/animationworklet/animator-animate.html [ Failure ]
crbug.com/887659 animations/animationworklet/animator-with-options.html [ Failure ] crbug.com/887659 animations/animationworklet/animator-with-options.html [ Failure ]
crbug.com/887659 animations/animationworklet/peek-updated-composited-property-on-main.html [ Failure ] crbug.com/887659 animations/animationworklet/peek-updated-composited-property-on-main.html [ Failure ]
crbug.com/887659 animations/animationworklet/scroll-timeline-display-none.html [ Crash ]
crbug.com/887659 animations/animationworklet/scroll-timeline-dynamic-update.html [ Crash ]
crbug.com/887659 animations/animationworklet/worklet-animation-currentTime.html [ Failure ] crbug.com/887659 animations/animationworklet/worklet-animation-currentTime.html [ Failure ]
crbug.com/887659 animations/animationworklet/worklet-animation-local-time-after-duration.html [ Failure ] crbug.com/887659 animations/animationworklet/worklet-animation-local-time-after-duration.html [ Failure ]
crbug.com/887659 animations/animationworklet/worklet-animation-local-time-undefined.html [ Failure ] crbug.com/887659 animations/animationworklet/worklet-animation-local-time-undefined.html [ Failure ]
crbug.com/887659 animations/animationworklet/worklet-animation-set-keyframes.html [ Crash ]
crbug.com/887659 animations/animationworklet/worklet-animation-set-timing.html [ Crash ]
crbug.com/887659 animations/animationworklet/worklet-animation-style-update.html [ Failure ] crbug.com/887659 animations/animationworklet/worklet-animation-style-update.html [ Failure ]
crbug.com/887659 animations/animationworklet/worklet-animation-responsive-to-zoom.html [ Crash ] crbug.com/887659 animations/animationworklet/worklet-animation-responsive-to-zoom.html [ Crash ]
......
...@@ -306,7 +306,6 @@ void WorkletAnimation::play(ExceptionState& exception_state) { ...@@ -306,7 +306,6 @@ void WorkletAnimation::play(ExceptionState& exception_state) {
DCHECK(IsMainThread()); DCHECK(IsMainThread());
if (play_state_ == Animation::kPending) if (play_state_ == Animation::kPending)
return; return;
document_->GetWorkletAnimationController().AttachAnimation(*this);
String failure_message; String failure_message;
if (!CheckCanStart(&failure_message)) { if (!CheckCanStart(&failure_message)) {
...@@ -315,6 +314,7 @@ void WorkletAnimation::play(ExceptionState& exception_state) { ...@@ -315,6 +314,7 @@ void WorkletAnimation::play(ExceptionState& exception_state) {
return; return;
} }
document_->GetWorkletAnimationController().AttachAnimation(*this);
SetPlayState(Animation::kPending); SetPlayState(Animation::kPending);
for (auto& effect : effects_) { for (auto& effect : effects_) {
...@@ -416,6 +416,8 @@ void WorkletAnimation::SetStartTimeToNow() { ...@@ -416,6 +416,8 @@ void WorkletAnimation::SetStartTimeToNow() {
} }
void WorkletAnimation::UpdateCompositingState() { void WorkletAnimation::UpdateCompositingState() {
DCHECK(play_state_ != Animation::kIdle && play_state_ != Animation::kUnset);
if (play_state_ == Animation::kPending) { if (play_state_ == Animation::kPending) {
String warning_message; String warning_message;
DCHECK(CheckCanStart(&warning_message)); DCHECK(CheckCanStart(&warning_message));
...@@ -429,8 +431,14 @@ void WorkletAnimation::UpdateCompositingState() { ...@@ -429,8 +431,14 @@ void WorkletAnimation::UpdateCompositingState() {
kOtherMessageSource, kWarningMessageLevel, message)); kOtherMessageSource, kWarningMessageLevel, message));
StartOnMain(); StartOnMain();
} else if (play_state_ == Animation::kRunning) { } else if (play_state_ == Animation::kRunning) {
UpdateOnCompositor(); // TODO(majidvp): If keyframes have changed then it may be possible to now
// run the animation on compositor. The current logic does not allow this
// switch from main to compositor to happen.
if (!running_on_main_thread_)
UpdateOnCompositor();
} }
DCHECK(running_on_main_thread_ != !!compositor_animation_)
<< "Active worklet animation should either run on main or compositor";
} }
void WorkletAnimation::InvalidateCompositingState() { void WorkletAnimation::InvalidateCompositingState() {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment