Skip to content

Commit b6afc16

Browse files
amirhamirh
authored
Make sure _handleAppFrame is only registered once per frame (flutter#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.
1 parent a83f6ea commit b6afc16

File tree

2 files changed

+119
-9
lines changed

2 files changed

+119
-9
lines changed

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

+18-6
Original file line numberDiff line numberDiff line change
@@ -508,9 +508,7 @@ class MultiFrameImageStreamCompleter extends ImageStreamCompleter {
508508
InformationCollector informationCollector,
509509
}) : assert(codec != null),
510510
_informationCollector = informationCollector,
511-
_scale = scale,
512-
_framesEmitted = 0,
513-
_timer = null {
511+
_scale = scale {
514512
codec.then<void>(_handleCodecReady, onError: (dynamic error, StackTrace stack) {
515513
reportError(
516514
context: 'resolving an image codec',
@@ -531,17 +529,23 @@ class MultiFrameImageStreamCompleter extends ImageStreamCompleter {
531529
// The requested duration for the current frame;
532530
Duration _frameDuration;
533531
// How many frames have been emitted so far.
534-
int _framesEmitted;
532+
int _framesEmitted = 0;
535533
Timer _timer;
536534

535+
// Used to guard against registering multiple _handleAppFrame callbacks for the same frame.
536+
bool _frameCallbackScheduled = false;
537+
537538
void _handleCodecReady(ui.Codec codec) {
538539
_codec = codec;
539540
assert(_codec != null);
540541

541-
_decodeNextFrameAndSchedule();
542+
if (hasListeners) {
543+
_decodeNextFrameAndSchedule();
544+
}
542545
}
543546

544547
void _handleAppFrame(Duration timestamp) {
548+
_frameCallbackScheduled = false;
545549
if (!hasListeners)
546550
return;
547551
if (_isFirstFrame() || _hasFrameDurationPassed(timestamp)) {
@@ -557,7 +561,7 @@ class MultiFrameImageStreamCompleter extends ImageStreamCompleter {
557561
}
558562
final Duration delay = _frameDuration - (timestamp - _shownTimestamp);
559563
_timer = Timer(delay * timeDilation, () {
560-
SchedulerBinding.instance.scheduleFrameCallback(_handleAppFrame);
564+
_scheduleAppFrame();
561565
});
562566
}
563567

@@ -589,6 +593,14 @@ class MultiFrameImageStreamCompleter extends ImageStreamCompleter {
589593
_emitFrame(ImageInfo(image: _nextFrame.image, scale: _scale));
590594
return;
591595
}
596+
_scheduleAppFrame();
597+
}
598+
599+
void _scheduleAppFrame() {
600+
if (_frameCallbackScheduled) {
601+
return;
602+
}
603+
_frameCallbackScheduled = true;
592604
SchedulerBinding.instance.scheduleFrameCallback(_handleAppFrame);
593605
}
594606

‎packages/flutter/test/painting/image_stream_test.dart

+101-3
Original file line numberDiff line numberDiff line change
@@ -89,15 +89,39 @@ void main() {
8989
expect(tester.takeException(), 'failure message');
9090
});
9191

92-
testWidgets('First frame decoding starts when codec is ready', (WidgetTester tester) async {
92+
testWidgets('Decoding starts when a listener is added after codec is ready', (WidgetTester tester) async {
9393
final Completer<Codec> completer = Completer<Codec>();
9494
final MockCodec mockCodec = MockCodec();
9595
mockCodec.frameCount = 1;
96-
MultiFrameImageStreamCompleter(
96+
final ImageStreamCompleter imageStream = MultiFrameImageStreamCompleter(
9797
codec: completer.future,
9898
scale: 1.0,
9999
);
100100

101+
completer.complete(mockCodec);
102+
await tester.idle();
103+
expect(mockCodec.numFramesAsked, 0);
104+
105+
final ImageListener listener = (ImageInfo image, bool synchronousCall) { };
106+
imageStream.addListener(listener);
107+
await tester.idle();
108+
expect(mockCodec.numFramesAsked, 1);
109+
});
110+
111+
testWidgets('Decoding starts when a codec is ready after a listener is added', (WidgetTester tester) async {
112+
final Completer<Codec> completer = Completer<Codec>();
113+
final MockCodec mockCodec = MockCodec();
114+
mockCodec.frameCount = 1;
115+
final ImageStreamCompleter imageStream = MultiFrameImageStreamCompleter(
116+
codec: completer.future,
117+
scale: 1.0,
118+
);
119+
120+
final ImageListener listener = (ImageInfo image, bool synchronousCall) { };
121+
imageStream.addListener(listener);
122+
await tester.idle();
123+
expect(mockCodec.numFramesAsked, 0);
124+
101125
completer.complete(mockCodec);
102126
await tester.idle();
103127
expect(mockCodec.numFramesAsked, 1);
@@ -108,11 +132,13 @@ void main() {
108132
mockCodec.frameCount = 1;
109133
final Completer<Codec> codecCompleter = Completer<Codec>();
110134

111-
MultiFrameImageStreamCompleter(
135+
final ImageStreamCompleter imageStream = MultiFrameImageStreamCompleter(
112136
codec: codecCompleter.future,
113137
scale: 1.0,
114138
);
115139

140+
final ImageListener listener = (ImageInfo image, bool synchronousCall) { };
141+
imageStream.addListener(listener);
116142
codecCompleter.complete(mockCodec);
117143
// MultiFrameImageStreamCompleter only sets an error handler for the next
118144
// frame future after the codec future has completed.
@@ -469,4 +495,76 @@ void main() {
469495
expect(tester.takeException(), isNull);
470496
expect(capturedException, 'frame completion error');
471497
});
498+
499+
testWidgets('remove and add listener ', (WidgetTester tester) async {
500+
final MockCodec mockCodec = MockCodec();
501+
mockCodec.frameCount = 3;
502+
mockCodec.repetitionCount = 0;
503+
final Completer<Codec> codecCompleter = Completer<Codec>();
504+
505+
final ImageStreamCompleter imageStream = MultiFrameImageStreamCompleter(
506+
codec: codecCompleter.future,
507+
scale: 1.0,
508+
);
509+
510+
final ImageListener listener = (ImageInfo image, bool synchronousCall) { };
511+
imageStream.addListener(listener);
512+
513+
codecCompleter.complete(mockCodec);
514+
515+
await tester.idle(); // let nextFrameFuture complete
516+
517+
imageStream.removeListener(listener);
518+
imageStream.addListener(listener);
519+
520+
521+
final FrameInfo frame1 = FakeFrameInfo(20, 10, const Duration(milliseconds: 200));
522+
523+
mockCodec.completeNextFrame(frame1);
524+
await tester.idle(); // let nextFrameFuture complete
525+
await tester.pump(); // first animation frame shows on first app frame.
526+
527+
await tester.pump(const Duration(milliseconds: 200)); // emit 2nd frame.
528+
});
529+
530+
// TODO(amirh): enable this once WidgetTester supports flushTimers.
531+
// https://github.com/flutter/flutter/issues/30344
532+
// testWidgets('remove and add listener before a delayed frame is scheduled', (WidgetTester tester) async {
533+
// final MockCodec mockCodec = MockCodec();
534+
// mockCodec.frameCount = 3;
535+
// mockCodec.repetitionCount = 0;
536+
// final Completer<Codec> codecCompleter = Completer<Codec>();
537+
//
538+
// final ImageStreamCompleter imageStream = MultiFrameImageStreamCompleter(
539+
// codec: codecCompleter.future,
540+
// scale: 1.0,
541+
// );
542+
//
543+
// final ImageListener listener = (ImageInfo image, bool synchronousCall) { };
544+
// imageStream.addListener(listener);
545+
//
546+
// codecCompleter.complete(mockCodec);
547+
// await tester.idle();
548+
//
549+
// final FrameInfo frame1 = FakeFrameInfo(20, 10, const Duration(milliseconds: 200));
550+
// final FrameInfo frame2 = FakeFrameInfo(200, 100, const Duration(milliseconds: 400));
551+
// final FrameInfo frame3 = FakeFrameInfo(200, 100, const Duration(milliseconds: 0));
552+
//
553+
// mockCodec.completeNextFrame(frame1);
554+
// await tester.idle(); // let nextFrameFuture complete
555+
// await tester.pump(); // first animation frame shows on first app frame.
556+
//
557+
// mockCodec.completeNextFrame(frame2);
558+
// await tester.pump(const Duration(milliseconds: 100)); // emit 2nd frame.
559+
//
560+
// tester.flushTimers();
561+
//
562+
// imageStream.removeListener(listener);
563+
// imageStream.addListener(listener);
564+
//
565+
// mockCodec.completeNextFrame(frame3);
566+
// await tester.idle(); // let nextFrameFuture complete
567+
//
568+
// await tester.pump();
569+
// });
472570
}

0 commit comments

Comments
 (0)