Fixing crash caused by Scrollable.animateTo interrupting HoldActivity before proper disposal is possible by Piinks · Pull Request #37267 · flutter/flutter

Piinks · 2019-07-30T22:41:32Z

Description

This PR fixes a crash caused by an animating ScrollPositionWithSingleContext interrupting an active HoldActivity before the handle has been returned to the Scrollable. When this happens, the HoldActivity cannot be disposed of properly. This change keeps track of the _currentHold from within the ScrollPositionWithSingleConext so that is may be disposed of in this case, effectively resetting the ScrollActivity.

Related Issues

Fixes #14452
Fixes #11433

Tests

I added the following tests:

Will need update for new implementation

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

No, this is not a breaking change.

…llCrash

packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart

packages/flutter/test/widgets/scrollable_animations_test.dart

chunhtai

LGTM

packages/flutter/test/widgets/scrollable_animations_test.dart

packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart

packages/flutter/test/widgets/scrollable_animations_test.dart

goderbauer · 2019-08-02T12:32:00Z

I just tried to reproduce this with the code given in #14452 and I am seeing a slightly different chain of events to get to the crash: It doesn't seem to be the user gesture that's interrupted by the automated animation that's causing the crash. Instead, it seem to be the other way around: The animation is interrupting an "in-progress" user gesture and that causes the crash. (Maybe that's what was meant by the PR description and I misunderstood?) Here is the sequence of events I am seeing:

Fling the scrollable so that it continues to move after the finger is lifted (generates Hold, Drag, Ballistic scroll activities in that order)
while the scrollable is still moving, put the finger down (triggers a hold activity, and will trigger another drag activity)
However, before the drag activity starts, the animation is triggered, interrupting the user gesture (this creates a Driven scroll activity)
When now the previously triggered drag activity starts, things crash

Is this another sequence that leads to a crash? Or is that what you meant?

goderbauer · 2019-08-02T12:33:41Z

Also, I am not convinced that this actually fixes the root cause of the crash. It removes a notification, that is used in user-defined code to trigger the animation. The animation could also be triggered via other means from user code (e.g. a timer that just happens to fire in the right moment) and that would still crash...

chunhtai · 2019-08-02T16:20:52Z

Also, I am not convinced that this actually fixes the root cause of the crash. It removes a notification, that is used in user-defined code to trigger the animation. The animation could also be triggered via other means from user code (e.g. a timer that just happens to fire in the right moment) and that would still crash...

No, the timer will not cause the crash, it has to happen at the exact moment at this line.

flutter/packages/flutter/lib/src/widgets/scrollable.dart

Line 482 in 0cf4033

_hold = position.hold(_disposeHold);

After calling position.hold(_disposeHold) before returning the hold handle. This is a sync function except it trigger a a notification in the middle which allow other action to interrupt before the hold handle returns. I think the other way to fix this is not letting automatic action interrupt user action, but we still want to fix hold action trigger an errant notification. That is why I think this is the right fix since i cannot think of any other way automatic action can interrupt user action if we fix the errant notification.

chunhtai · 2019-08-02T16:22:23Z

I just tried to reproduce this with the code given in #14452 and I am seeing a slightly different chain of events to get to the crash: It doesn't seem to be the user gesture that's interrupted by the automated animation that's causing the crash. Instead, it seem to be the other way around: The animation is interrupting an "in-progress" user gesture and that causes the crash. (Maybe that's what was meant by the PR description and I misunderstood?) Here is the sequence of events I am seeing:

Fling the scrollable so that it continues to move after the finger is lifted (generates Hold, Drag, Ballistic scroll activities in that order)

while the scrollable is still moving, put the finger down (triggers a hold activity, and will trigger another drag activity)

However, before the drag activity starts, the animation is triggered, interrupting the user gesture (this creates a Driven scroll activity)

When now the previously triggered drag activity starts, things crash

Is this another sequence that leads to a crash? Or is that what you meant?

Yes that is what i observed as well.

Piinks · 2019-08-02T16:40:41Z

I just tried to reproduce this with the code given in #14452 and I am seeing a slightly different chain of events to get to the crash: It doesn't seem to be the user gesture that's interrupted by the automated animation that's causing the crash. Instead, it seem to be the other way around: The animation is interrupting an "in-progress" user gesture and that causes the crash. (Maybe that's what was meant by the PR description and I misunderstood?) Here is the sequence of events I am seeing:

Fling the scrollable so that it continues to move after the finger is lifted (generates Hold, Drag, Ballistic scroll activities in that order)

while the scrollable is still moving, put the finger down (triggers a hold activity, and will trigger another drag activity)

However, before the drag activity starts, the animation is triggered, interrupting the user gesture (this creates a Driven scroll activity)

When now the previously triggered drag activity starts, things crash

Is this another sequence that leads to a crash? Or is that what you meant?

Yes. I also agree. Very well explained. :)

I'm going to see about addressing the notification error, since I think we all agree that a HoldActivity does not constitute that the ScrollDirection is idle.

As for other cases where an animation may interrupt a user gesture, I will see about creating a reproduction of the timer case you recommended @goderbauer and see if it causes a crash.

…llCrash

Piinks · 2019-08-02T17:43:14Z

I created a gist that triggers the animation after a time delay instead of after a notification.

Available here: https://gist.github.com/Piinks/955c449d2764e4591b574859c7a0395f

If you hold (don't drag) after triggering the future that delays the animation, the HoldActivity is properly disposed of before the DrivenActivity begins. In this case, it does not crash.
Is this similar to the concern you mentioned earlier @goderbauer?

packages/flutter/lib/src/widgets/scroll_activity.dart

packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart

goderbauer · 2019-08-05T19:57:55Z

@chunhtai Ah, I see. Looking at the line you pointed out:

flutter/packages/flutter/lib/src/widgets/scrollable.dart

Line 482 in 0cf4033

_hold = position.hold(_disposeHold);

The underlying problem is that _disposeHold is executed (as a result of the dispatched scroll notification) before hold returns and therefore before _hold is assigned. Because of this, _hold is never cleared properly.

We also dispatch a scrollendnotification when the hold starts:

flutter/packages/flutter/lib/src/widgets/scroll_position.dart

Lines 638 to 639 in 4ccd811

    
           if (wasScrolling && !newActivity.isScrolling) 
        
             didEndScroll(); // notifies and then saves the scroll offset

What if you trigger the animation from that one?

codecov · 2019-08-05T20:14:27Z

Codecov Report

Merging #37267 into master will increase coverage by 0.82%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #37267      +/-   ##
==========================================
+ Coverage   55.22%   56.04%   +0.82%     
==========================================
  Files         193      193              
  Lines       18172    18141      -31     
==========================================
+ Hits        10036    10168     +132     
+ Misses       8136     7973     -163

Flag	Coverage Δ
#flutter_tool	`56.04% <0%> (+0.82%)`	⬆️

Impacted Files	Coverage Δ
...es/flutter_tools/lib/src/macos/cocoapod_utils.dart	`0% <0%> (-25%)`	⬇️
packages/flutter_tools/lib/src/desktop.dart	`65.21% <0%> (-21.74%)`	⬇️
packages/flutter_tools/lib/src/project.dart	`74.75% <0%> (-7.12%)`	⬇️
packages/flutter_tools/lib/src/features.dart	`80.85% <0%> (-4.26%)`	⬇️
packages/flutter_tools/lib/src/build_info.dart	`69.61% <0%> (-1.11%)`	⬇️
packages/flutter_tools/lib/src/android/gradle.dart	`44.17% <0%> (-0.88%)`	⬇️
packages/flutter_tools/lib/src/web/web_fs.dart	`23.66% <0%> (-0.58%)`	⬇️
...utter_tools/lib/src/build_system/targets/dart.dart	`74.19% <0%> (-0.41%)`	⬇️
...ckages/flutter_tools/lib/src/flutter_manifest.dart	`82.63% <0%> (-0.19%)`	⬇️
packages/flutter_tools/lib/src/ios/devices.dart	`36.66% <0%> (+0.15%)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cf1a35...5f8e4de. Read the comment docs.

chunhtai · 2019-08-05T20:24:41Z

What if you trigger the animation from that one?

Yes, I think that is another corner case. it seems like we should somehow know how to reset _hold when it gets interrupted before 'hold' returns.

Piinks · 2019-08-05T20:51:13Z

What if you trigger the animation from that one?

Yes, I think that is another corner case. it seems like we should somehow know how to reset _hold when it gets interrupted before 'hold' returns.

Thank you for the feedback. I will look into this corner case as well and see if adding a reset _hold type method would be a better approach to fixing this crash. I will close this for now while I investigate further.

chunhtai · 2019-08-05T21:09:14Z

That being said, I am still thinking this pr is worth fixing as hold activity should not be consider as idle according to the documentation

flutter/packages/flutter/lib/src/widgets/scroll_activity.dart

Line 173 in 9bb7142

/// A scroll activity that does nothing but can be released to resume

…llCrash

… scrollActivity

Piinks · 2019-08-07T20:45:18Z

I've added a reset-like path for when an animation is fired before the hold can be disposed of properly. A similar functionality existed in the class for keeping track of _currentDrag status, so I tried to mirror this through _currentHold. I will add more tests if we agree this is a good direction to move in to solve this problem. 😉

Outdated

goderbauer · 2019-08-15T19:43:10Z

packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart

      delegate: this,
      onHoldCanceled: holdCancelCallback,
    );
+    _currentHold = holdActivity;


Can we assert that _currentHold is null to ensure we are not overwriting a hold?

goderbauer · 2019-08-15T20:04:24Z

packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart

+    // the [ScrollableState] has not yet received the handle to the
+    // [HoldActivity], it will not be properly disposed of in [beginActivity].
+    // Therefore, we dispose of any active hold here.
+    if (_currentHold != null) {


Would jumpTo have the same issue?

goderbauer · 2019-08-15T20:13:03Z

packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart

+    // the [ScrollableState] has not yet received the handle to the
+    // [HoldActivity], it will not be properly disposed of in [beginActivity].
+    // Therefore, we dispose of any active hold here.
+    if (_currentHold != null) {


And wouldn't we also have a similar problem when there is an ongoing drag? (e.g. when an animation triggers based on the onScrollStartNotification before we had a chance to get the handle for the drag)

Piinks · 2019-08-19T23:30:49Z

After offline chat with @goderbauer, the problem here lies in the potential gap that is created when a scroll notification is used to initiate an activity. It leaves open a small window where the activity can interrupt a currently executing one since scroll notifications are dispatched at different points in an activity's execution.
This leads us to believe that the use of scroll notifiers for triggering these events in not a sound use of the API. I'm looking currently for a good alternative to recommend and document, and will close this since I do not believe we can try to stifle each possible scroll notification creating an undesirable state.

Piinks added 2 commits July 30, 2019 10:13

Fixing idle scroll update for hold.

3a3c621

Added test, cleaned up test file

31f15ef

googlebot added the cla: yes label Jul 30, 2019

Merge branch 'master' of github.com:flutter/flutter into animatedScro…

74bc3da

…llCrash

Piinks requested review from chunhtai and goderbauer July 31, 2019 22:04

chunhtai reviewed Aug 1, 2019

View reviewed changes

packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart Outdated Show resolved Hide resolved

packages/flutter/test/widgets/scrollable_animations_test.dart Outdated Show resolved Hide resolved

chunhtai previously approved these changes Aug 1, 2019

View reviewed changes

packages/flutter/test/widgets/scrollable_animations_test.dart Outdated Show resolved Hide resolved

goderbauer reviewed Aug 2, 2019

View reviewed changes

packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart Outdated Show resolved Hide resolved

packages/flutter/test/widgets/scrollable_animations_test.dart Outdated Show resolved Hide resolved

Piinks changed the title ~~Fixing crash caused by interrupted scrolling animation~~ WIP Fixing crash caused by animateTo interrupting user gesture Aug 2, 2019

Piinks added the work in progress; do not review label Aug 2, 2019

Merge branch 'master' of github.com:flutter/flutter into animatedScro…

466e64d

…llCrash

Review

a5d1abb

Piinks commented Aug 2, 2019

View reviewed changes

packages/flutter/lib/src/widgets/scroll_activity.dart Show resolved Hide resolved

packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart Outdated Show resolved Hide resolved

Piinks removed the work in progress; do not review label Aug 2, 2019

Piinks changed the title ~~WIP Fixing crash caused by animateTo interrupting user gesture~~ Fixing crash caused by animateTo interrupting user gesture Aug 2, 2019

Piinks changed the title ~~Fixing crash caused by animateTo interrupting user gesture~~ Fixing crash caused by Scrollable.animateTo Interrupting User Gesture Aug 2, 2019

Piinks closed this Aug 5, 2019

Piinks added 2 commits August 7, 2019 09:53

Merge branch 'master' of github.com:flutter/flutter into animatedScro…

5bbf03d

…llCrash

Adding a hold reset to prevent exceptions when interrupted by another…

0f287f3

… scrollActivity

Piinks reopened this Aug 7, 2019

Piinks changed the title ~~Fixing crash caused by Scrollable.animateTo Interrupting User Gesture~~ Fixing crash caused by Scrollable.animateTo interrupting HoldActivity before proper disposal is possible. Aug 7, 2019

Nit

b572bca

Piinks added the work in progress; do not review label Aug 9, 2019

Piinks changed the title ~~Fixing crash caused by Scrollable.animateTo interrupting HoldActivity before proper disposal is possible.~~ WIP Fixing crash caused by Scrollable.animateTo interrupting HoldActivity before proper disposal is possible. Aug 9, 2019

Piinks changed the title ~~WIP Fixing crash caused by Scrollable.animateTo interrupting HoldActivity before proper disposal is possible.~~ WIP Fixing crash caused by Scrollable.animateTo interrupting HoldActivity before proper disposal is possible Aug 12, 2019

Piinks added 2 commits August 12, 2019 14:07

Updated for currentHold test

26a45fb

Removing unused import

5f8e4de

Piinks changed the title ~~WIP Fixing crash caused by Scrollable.animateTo interrupting HoldActivity before proper disposal is possible~~ Fixing crash caused by Scrollable.animateTo interrupting HoldActivity before proper disposal is possible Aug 12, 2019

Piinks removed the work in progress; do not review label Aug 12, 2019

goderbauer reviewed Aug 15, 2019

View reviewed changes

Piinks closed this Aug 19, 2019

Piinks mentioned this pull request Aug 19, 2019

Dragging a list that is currently handling an animateTo animation throws exception #14452

Closed

Piinks deleted the animatedScrollCrash branch November 22, 2019 00:54

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Conversation

Piinks commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Uh oh!

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goderbauer commented Aug 2, 2019

Uh oh!

goderbauer commented Aug 2, 2019

Uh oh!

chunhtai commented Aug 2, 2019

Uh oh!

chunhtai commented Aug 2, 2019

Uh oh!

Piinks commented Aug 2, 2019

Uh oh!

Piinks commented Aug 2, 2019

Uh oh!

Uh oh!

Uh oh!

goderbauer commented Aug 5, 2019

Uh oh!

codecov bot commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chunhtai commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks commented Aug 5, 2019

Uh oh!

chunhtai commented Aug 5, 2019

Uh oh!

Piinks commented Aug 7, 2019

Uh oh!

goderbauer Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

goderbauer Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

goderbauer Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

Piinks commented Aug 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Piinks commented Jul 30, 2019 •

edited

Loading

codecov bot commented Aug 5, 2019 •

edited

Loading

chunhtai commented Aug 5, 2019 •

edited

Loading