Make sure _handleAppFrame is only registered once per frame by amirh · Pull Request #30346 · flutter/flutter

amirh · 2019-04-01T23:36:49Z

Description

There were 2 possible scenarios in which _handleAppFrame is added more than once as a frame callback. When this happens it is possible that the second invocation will try to access _nextFrame.image when _nextFrame is null and crash. The 2 scenarios are:

Scenario 1

A GIF frame is decoded and a Flutter frame is executed before it's time to show the next GIF frame.
The timer that's waiting for enough time to elapse is invoked, and schedules a callback for the next Flutter frame(here).
Before the next Flutter frame is executed, MultiFrameImageStreamCompleter#removeListener is called followed by ``MultiFrameImageStreamCompleter#addListenerthat is invoking_decodeNextFrameAndSchedule` which is adding `_handleAppFrame` again as a next frame callback.

Scenario 2
removeListener and addListener are called multiple times in succession, every call to addListener can result in another registration of _handleAppFrame to the next Flutter frame callbacks list.

This patch fixes the issue by guarding against a second registration of _handleAppFrame.

Related Issues

fixes #30340

Tests

I added the following tests:

A test that follows scenario 2 above.
[temporarily disabled] A test that follows scenario 1 above.

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?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

amirh · 2019-04-01T23:36:57Z

cc @xster

dnfield · 2019-04-02T00:30:42Z

@@ -557,7 +562,7 @@ class MultiFrameImageStreamCompleter extends ImageStreamCompleter {
    }
    final Duration delay = _frameDuration - (timestamp - _shownTimestamp);
    _timer = Timer(delay * timeDilation, () {


Would it make sense guarding the timer with this check too, so that you don't even register the timer if it's already been registered?

Could you possibly achieve this same effect by just doing something like _timer ??=?

I don't see how guarding just against double timer registration fixes the described issue (e.g in the addListener,removeListener,addListener scenario).

I'm not sure I see a scenario where we will have double timer registration with the current guard given that removeListener cancels the timer after removing all listeners.

Can you describe the scenario you're thinking about in which there will be double timer registration?

I may be misreading something here, but it looks like this timer can get created multiple times without getting cancelled, which could be leading to multiple registration.

I'm probably missing what you see, can you please describe the sequence of events that leads to multiple timer registration?

It looks like the constructor of this class calls _handleAppFrame when the codec is ready (

flutter/packages/flutter/lib/src/painting/image_stream.dart

Line 515 in 44c1270

codec.then<void>(_handleCodecReady, onError: (dynamic error, StackTrace stack) {

), and when the first listener is added it's also called (

flutter/packages/flutter/lib/src/painting/image_stream.dart

Line 616 in 44c1270

_decodeNextFrameAndSchedule();

). It seems entirely possible that the Image widget ends up making a new image stream which ends up creating a timer and then adding a listener which creates a second timer before the first has finished.

Good catch!
The scenario you describe can actually result in the first frame being skipped(as _nextFrame can be set to the second codec frame before the next flutter frame is executed).

Fixed it by only starting to decode after both the codec is ready and a frame listener is added.
In practice I'd imagine that this was very unlikely to happen as the first frame listener was likely to be added synchronously right after creating the stream completer at which point _codec is still null (as the async call had no chance to complete) thus the second _decodeNextFrameAndSchedule will not be invoked (it's guarded by a check that _codec is not null).

Anyway with this fixed I don't see a scenario where the timer is registered multiple times simultaneously.

goderbauer

LGTM

Thanks for debugging and fixing!

goderbauer · 2019-04-02T17:00:33Z

       _framesEmitted = 0,
-       _timer = null {
+       _timer = null,
+       _frameCallbackScheduled = false {


nit: It reads a little cleaner to me if you would just initialize the member with false in line 539 instead of doing it here.

make sure _handleAppFrame is only registered once per frame

e091271

amirh requested a review from goderbauer April 1, 2019 23:36

googlebot added the cla: yes label Apr 1, 2019

remove extra blank line

44c1270

dnfield reviewed Apr 2, 2019

View reviewed changes

dnfield added the framework flutter/packages/flutter repository. See also f: labels. label Apr 2, 2019

only start decoding after the codec is ready and a listener was added

9085983

goderbauer approved these changes Apr 2, 2019

View reviewed changes

review comment followup

bec0589

amirh force-pushed the image_stream_crash branch from bab89ea to bec0589 Compare April 2, 2019 18:15

amirh merged commit b6afc16 into flutter:master Apr 2, 2019

amirh deleted the image_stream_crash branch April 2, 2019 19:14

github-actions Bot locked as resolved and limited conversation to collaborators Aug 7, 2021

Navigation Menu

Repository navigation

Make sure _handleAppFrame is only registered once per frame - #30346

Make sure _handleAppFrame is only registered once per frame#30346
amirh merged 4 commits into
flutter:masterfrom
amirh:image_stream_crash

amirh commented Apr 1, 2019

Uh oh!

amirh commented Apr 1, 2019

Uh oh!

dnfield Apr 2, 2019

Uh oh!

amirh Apr 2, 2019

Uh oh!

dnfield Apr 2, 2019

Uh oh!

amirh Apr 2, 2019

Uh oh!

dnfield Apr 2, 2019

Uh oh!

amirh Apr 2, 2019

Uh oh!

goderbauer left a comment

Uh oh!

goderbauer Apr 2, 2019

Uh oh!

amirh Apr 2, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

amirh commented Apr 1, 2019

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

amirh commented Apr 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants