Make sure _handleAppFrame is only registered once per frame (#30346)

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.

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

@@ -508,9 +508,7 @@ class MultiFrameImageStreamCompleter extends ImageStreamCompleter {
     InformationCollector informationCollector,
   }) : assert(codec != null),
        _informationCollector = informationCollector,
-       _scale = scale,
-       _framesEmitted = 0,
-       _timer = null {
+       _scale = scale {
     codec.then<void>(_handleCodecReady, onError: (dynamic error, StackTrace stack) {
       reportError(
         context: 'resolving an image codec',
@@ -531,17 +529,23 @@ class MultiFrameImageStreamCompleter extends ImageStreamCompleter {
   // The requested duration for the current frame;
   Duration _frameDuration;
   // How many frames have been emitted so far.
-  int _framesEmitted;
+  int _framesEmitted = 0;
   Timer _timer;
 
+  // Used to guard against registering multiple _handleAppFrame callbacks for the same frame.
+  bool _frameCallbackScheduled = false;
+
   void _handleCodecReady(ui.Codec codec) {
     _codec = codec;
     assert(_codec != null);
 
+    if (hasListeners) {
       _decodeNextFrameAndSchedule();
     }
+  }
 
   void _handleAppFrame(Duration timestamp) {
+    _frameCallbackScheduled = false;
     if (!hasListeners)
       return;
     if (_isFirstFrame() || _hasFrameDurationPassed(timestamp)) {
@@ -557,7 +561,7 @@ class MultiFrameImageStreamCompleter extends ImageStreamCompleter {
     }
     final Duration delay = _frameDuration - (timestamp - _shownTimestamp);
     _timer = Timer(delay * timeDilation, () {
-      SchedulerBinding.instance.scheduleFrameCallback(_handleAppFrame);
+      _scheduleAppFrame();
     });
   }
 
@@ -589,6 +593,14 @@ class MultiFrameImageStreamCompleter extends ImageStreamCompleter {
       _emitFrame(ImageInfo(image: _nextFrame.image, scale: _scale));
       return;
     }
+    _scheduleAppFrame();
+  }
+
+  void _scheduleAppFrame() {
+    if (_frameCallbackScheduled) {
+      return;
+    }
+    _frameCallbackScheduled = true;
     SchedulerBinding.instance.scheduleFrameCallback(_handleAppFrame);
   }
 

packages/flutter/test/painting/image_stream_test.dart

@@ -89,15 +89,39 @@ void main() {
     expect(tester.takeException(), 'failure message');
   });
 
-  testWidgets('First frame decoding starts when codec is ready', (WidgetTester tester) async {
+  testWidgets('Decoding starts when a listener is added after codec is ready', (WidgetTester tester) async {
     final Completer<Codec> completer = Completer<Codec>();
     final MockCodec mockCodec = MockCodec();
     mockCodec.frameCount = 1;
-    MultiFrameImageStreamCompleter(
+    final ImageStreamCompleter imageStream = MultiFrameImageStreamCompleter(
       codec: completer.future,
       scale: 1.0,
     );
 
+    completer.complete(mockCodec);
+    await tester.idle();
+    expect(mockCodec.numFramesAsked, 0);
+
+    final ImageListener listener = (ImageInfo image, bool synchronousCall) { };
+    imageStream.addListener(listener);
+    await tester.idle();
+    expect(mockCodec.numFramesAsked, 1);
+  });
+
+  testWidgets('Decoding starts when a codec is ready after a listener is added', (WidgetTester tester) async {
+    final Completer<Codec> completer = Completer<Codec>();
+    final MockCodec mockCodec = MockCodec();
+    mockCodec.frameCount = 1;
+    final ImageStreamCompleter imageStream = MultiFrameImageStreamCompleter(
+      codec: completer.future,
+      scale: 1.0,
+    );
+
+    final ImageListener listener = (ImageInfo image, bool synchronousCall) { };
+    imageStream.addListener(listener);
+    await tester.idle();
+    expect(mockCodec.numFramesAsked, 0);
+
     completer.complete(mockCodec);
     await tester.idle();
     expect(mockCodec.numFramesAsked, 1);
@@ -108,11 +132,13 @@ void main() {
     mockCodec.frameCount = 1;
     final Completer<Codec> codecCompleter = Completer<Codec>();
 
-    MultiFrameImageStreamCompleter(
+    final ImageStreamCompleter imageStream = MultiFrameImageStreamCompleter(
       codec: codecCompleter.future,
       scale: 1.0,
     );
 
+    final ImageListener listener = (ImageInfo image, bool synchronousCall) { };
+    imageStream.addListener(listener);
     codecCompleter.complete(mockCodec);
     // MultiFrameImageStreamCompleter only sets an error handler for the next
     // frame future after the codec future has completed.
@@ -469,4 +495,76 @@ void main() {
     expect(tester.takeException(), isNull);
     expect(capturedException, 'frame completion error');
   });
+
+  testWidgets('remove and add listener ', (WidgetTester tester) async {
+    final MockCodec mockCodec = MockCodec();
+    mockCodec.frameCount = 3;
+    mockCodec.repetitionCount = 0;
+    final Completer<Codec> codecCompleter = Completer<Codec>();
+
+    final ImageStreamCompleter imageStream = MultiFrameImageStreamCompleter(
+      codec: codecCompleter.future,
+      scale: 1.0,
+    );
+
+    final ImageListener listener = (ImageInfo image, bool synchronousCall) { };
+    imageStream.addListener(listener);
+
+    codecCompleter.complete(mockCodec);
+
+    await tester.idle(); // let nextFrameFuture complete
+
+    imageStream.removeListener(listener);
+    imageStream.addListener(listener);
+
+
+    final FrameInfo frame1 = FakeFrameInfo(20, 10, const Duration(milliseconds: 200));
+
+    mockCodec.completeNextFrame(frame1);
+    await tester.idle(); // let nextFrameFuture complete
+    await tester.pump(); // first animation frame shows on first app frame.
+
+    await tester.pump(const Duration(milliseconds: 200)); // emit 2nd frame.
+  });
+
+  // TODO(amirh): enable this once WidgetTester supports flushTimers.
+  // https://github.com/flutter/flutter/issues/30344
+  // testWidgets('remove and add listener before a delayed frame is scheduled', (WidgetTester tester) async {
+  //   final MockCodec mockCodec = MockCodec();
+  //   mockCodec.frameCount = 3;
+  //   mockCodec.repetitionCount = 0;
+  //   final Completer<Codec> codecCompleter = Completer<Codec>();
+  //
+  //   final ImageStreamCompleter imageStream = MultiFrameImageStreamCompleter(
+  //     codec: codecCompleter.future,
+  //     scale: 1.0,
+  //   );
+  //
+  //   final ImageListener listener = (ImageInfo image, bool synchronousCall) { };
+  //   imageStream.addListener(listener);
+  //
+  //   codecCompleter.complete(mockCodec);
+  //   await tester.idle();
+  //
+  //   final FrameInfo frame1 = FakeFrameInfo(20, 10, const Duration(milliseconds: 200));
+  //   final FrameInfo frame2 = FakeFrameInfo(200, 100, const Duration(milliseconds: 400));
+  //   final FrameInfo frame3 = FakeFrameInfo(200, 100, const Duration(milliseconds: 0));
+  //
+  //   mockCodec.completeNextFrame(frame1);
+  //   await tester.idle(); // let nextFrameFuture complete
+  //   await tester.pump(); // first animation frame shows on first app frame.
+  //
+  //   mockCodec.completeNextFrame(frame2);
+  //   await tester.pump(const Duration(milliseconds: 100)); // emit 2nd frame.
+  //
+  //   tester.flushTimers();
+  //
+  //   imageStream.removeListener(listener);
+  //   imageStream.addListener(listener);
+  //
+  //   mockCodec.completeNextFrame(frame3);
+  //   await tester.idle(); // let nextFrameFuture complete
+  //
+  //   await tester.pump();
+  // });
 }