Commit 0697b335 authored by Rahul Arakeri's avatar Rahul Arakeri Committed by Chromium LUCI CQ

ScrollOffsetAnimationCurve::UpdateTarget crash fix.

This CL fixes a CHECK failure with a base::Optional type. After the CL
crrev.com/c/2553869 was landed, it exposed a pre-existing product bug
(crbug.com/1164008) and that led to an increase in crashes in
cc::ScrollOffsetAnimationCurve::UpdateTarget.

Problem:
In ScrollOffsetAnimationCurve::EaseInOutBoundedSegmentDuration the
variable duration_behavior_ gets referenced. This variable did not
have any value assigned to it and hence causes the crash.

Why it did not fail before:
Before the CL 2553869 landed, the logical flow was different. As in, if
the animation_type_ (in UpdateTarget) was kEaseInOut, the new_duration
would've been calculated by calling EaseInOutBoundedSegmentDuration.
This is the only function that references the base::Optional value
(duration_behavior_). All other animation types (kImpulse and kLinear)
would've led to the function ImpulseSegmentDuration being called where
duration_behavior_ was not referenced. In the crash dump, I observed
that the animation_type_ was kLinear. This type does *not* have
duration_behavior_ defined and UpdateTarget does *not* support kLinear
animation_type_ and hence the crash. So in fact, UpdateTarget being
called for a linear animation is the real issue here (and this issue is
a pre-existing bug in the product). The CL 2553869 merely exposed it.

Fix:
I've added a bandaid solution where UpdateTarget simply returns if the
animation_type_ is kLinear. Even after multiple tries, I was never able
to repro the original bug (where UpdateTarget is called on linear
animations). The original bug is tracked via crbug.com/1164008.

Bug: 1158655
Change-Id: I0aca4870e8578de7690a6c4f8ee91f6786da1f15
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2613934Reviewed-by: default avatarXianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: Rahul Arakeri <arakeri@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#842295}
parent 20e51e75
......@@ -361,6 +361,11 @@ void ScrollOffsetAnimationCurve::UpdateTarget(
DCHECK_NE(animation_type_, AnimationType::kLinear)
<< "UpdateTarget is not supported on linear scroll animations.";
// UpdateTarget is still called for linear animations occasionally. This is
// tracked via crbug.com/1164008.
if (animation_type_ == AnimationType::kLinear)
return;
// If the new UpdateTarget actually happened before the previous one, keep
// |t| as the most recent, but reduce the duration of any generated
// animation.
......
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