From d84879d910e53209d8fd103aa27b5ed0ead08f42 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Thu, 3 Jan 2019 12:55:16 -0800 Subject: [PATCH] Ensure all errors thrown by image providers can be caught by developers. (#25980) * Ensure all errors thrown by image providers can be caught by developers. Add an `onError` parameter to the ImageCache.putIfAbsent method. In the event that an error is thrown when resolving an image, catch if this parameter is provided. Use the onError parameter to ensure that all errors thrown are forwarded to the ImageStream error channel instead of directly into the void. --- .../flutter/lib/src/painting/image_cache.dart | 18 +++++- .../lib/src/painting/image_provider.dart | 63 +++++++++++++------ .../test/painting/image_cache_test.dart | 13 ++++ .../test/painting/image_provider_test.dart | 17 +++++ .../test/painting/mocks_for_image_cache.dart | 12 ++++ 5 files changed, 102 insertions(+), 21 deletions(-) diff --git a/packages/flutter/lib/src/painting/image_cache.dart b/packages/flutter/lib/src/painting/image_cache.dart index b9f802e2179..53442d88776 100644 --- a/packages/flutter/lib/src/painting/image_cache.dart +++ b/packages/flutter/lib/src/painting/image_cache.dart @@ -126,7 +126,12 @@ class ImageCache { /// key is moved to the "most recently used" position. /// /// The arguments must not be null. The `loader` cannot return null. - ImageStreamCompleter putIfAbsent(Object key, ImageStreamCompleter loader()) { + /// + /// In the event that the loader throws an exception, it will be caught only if + /// `onError` is also provided. When an exception is caught resolving an image, + /// no completers are cached and `null` is returned instead of a new + /// completer. + ImageStreamCompleter putIfAbsent(Object key, ImageStreamCompleter loader(), { ImageErrorListener onError }) { assert(key != null); assert(loader != null); ImageStreamCompleter result = _pendingImages[key]; @@ -140,7 +145,16 @@ class ImageCache { _cache[key] = image; return image.completer; } - result = loader(); + try { + result = loader(); + } catch (error, stackTrace) { + if (onError != null) { + onError(error, stackTrace); + return null; + } else { + rethrow; + } + } void listener(ImageInfo info, bool syncCall) { // Images that fail to load don't contribute to cache size. final int imageSize = info?.image == null ? 0 : info.image.height * info.image.width * 4; diff --git a/packages/flutter/lib/src/painting/image_provider.dart b/packages/flutter/lib/src/painting/image_provider.dart index d8c42771478..2b563966907 100644 --- a/packages/flutter/lib/src/painting/image_provider.dart +++ b/packages/flutter/lib/src/painting/image_provider.dart @@ -262,27 +262,31 @@ abstract class ImageProvider { assert(configuration != null); final ImageStream stream = ImageStream(); T obtainedKey; + Future handleError(dynamic exception, StackTrace stack) async { + await null; // wait an event turn in case a listener has been added to the image stream. + final _ErrorImageCompleter imageCompleter = _ErrorImageCompleter(); + stream.setCompleter(imageCompleter); + imageCompleter.setError( + exception: exception, + stack: stack, + context: 'while resolving an image', + silent: true, // could be a network error or whatnot + informationCollector: (StringBuffer information) { + information.writeln('Image provider: $this'); + information.writeln('Image configuration: $configuration'); + if (obtainedKey != null) { + information.writeln('Image key: $obtainedKey'); + } + } + ); + } obtainKey(configuration).then((T key) { obtainedKey = key; - stream.setCompleter(PaintingBinding.instance.imageCache.putIfAbsent(key, () => load(key))); - }).catchError( - (dynamic exception, StackTrace stack) async { - FlutterError.reportError(FlutterErrorDetails( - exception: exception, - stack: stack, - library: 'services library', - context: 'while resolving an image', - silent: true, // could be a network error or whatnot - informationCollector: (StringBuffer information) { - information.writeln('Image provider: $this'); - information.writeln('Image configuration: $configuration'); - if (obtainedKey != null) - information.writeln('Image key: $obtainedKey'); - } - )); - return null; + final ImageStreamCompleter completer = PaintingBinding.instance.imageCache.putIfAbsent(key, () => load(key), onError: handleError); + if (completer != null) { + stream.setCompleter(completer); } - ); + }).catchError(handleError); return stream; } @@ -495,7 +499,7 @@ class NetworkImage extends ImageProvider { if (bytes.lengthInBytes == 0) throw Exception('NetworkImage is an empty file: $resolved'); - return await PaintingBinding.instance.instantiateImageCodec(bytes); + return PaintingBinding.instance.instantiateImageCodec(bytes); } @override @@ -773,3 +777,24 @@ class ExactAssetImage extends AssetBundleImageProvider { @override String toString() => '$runtimeType(name: "$keyName", scale: $scale, bundle: $bundle)'; } + +// A completer used when resolving an image fails sync. +class _ErrorImageCompleter extends ImageStreamCompleter { + _ErrorImageCompleter(); + + void setError({ + String context, + dynamic exception, + StackTrace stack, + InformationCollector informationCollector, + bool silent = false, + }) { + reportError( + context: context, + exception: exception, + stack: stack, + informationCollector: informationCollector, + silent: silent, + ); + } +} diff --git a/packages/flutter/test/painting/image_cache_test.dart b/packages/flutter/test/painting/image_cache_test.dart index fd25ad6d068..cd554edb901 100644 --- a/packages/flutter/test/painting/image_cache_test.dart +++ b/packages/flutter/test/painting/image_cache_test.dart @@ -128,5 +128,18 @@ void main() { expect(imageCache.currentSizeBytes, 256); expect(imageCache.maximumSizeBytes, 256 + 1000); }); + + test('Returns null if an error is caught resolving an image', () { + final ErrorImageProvider errorImage = ErrorImageProvider(); + expect(() => imageCache.putIfAbsent(errorImage, () => errorImage.load(errorImage)), throwsA(isInstanceOf())); + bool caughtError = false; + final ImageStreamCompleter result = imageCache.putIfAbsent(errorImage, () => errorImage.load(errorImage), onError: (dynamic error, StackTrace stackTrace) { + caughtError = true; + }); + expect(result, null); + expect(caughtError, true); + }); }); } + + diff --git a/packages/flutter/test/painting/image_provider_test.dart b/packages/flutter/test/painting/image_provider_test.dart index 67b8cfa9797..51e698e1ae2 100644 --- a/packages/flutter/test/painting/image_provider_test.dart +++ b/packages/flutter/test/painting/image_provider_test.dart @@ -5,11 +5,13 @@ import 'dart:async'; import 'dart:typed_data'; +import 'package:flutter/foundation.dart'; import 'package:flutter/painting.dart'; import 'package:flutter_test/flutter_test.dart'; import '../rendering/rendering_tester.dart'; import 'image_data.dart'; +import 'mocks_for_image_cache.dart'; void main() { TestRenderingFlutterBinding(); // initializes the imageCache @@ -53,5 +55,20 @@ void main() { expect(otherCache.currentSize, 0); expect(imageCache.currentSize, 1); }); + + test('ImageProvider errors can always be caught', () async { + final ErrorImageProvider imageProvider = ErrorImageProvider(); + final Completer caughtError = Completer(); + FlutterError.onError = (FlutterErrorDetails details) { + caughtError.complete(false); + }; + final ImageStream stream = imageProvider.resolve(ImageConfiguration.empty); + stream.addListener((ImageInfo info, bool syncCall) { + caughtError.complete(false); + }, onError: (dynamic error, StackTrace stackTrace) { + caughtError.complete(true); + }); + expect(await caughtError.future, true); + }); }); } diff --git a/packages/flutter/test/painting/mocks_for_image_cache.dart b/packages/flutter/test/painting/mocks_for_image_cache.dart index 0c9fd4cc9e1..05fa6daa56a 100644 --- a/packages/flutter/test/painting/mocks_for_image_cache.dart +++ b/packages/flutter/test/painting/mocks_for_image_cache.dart @@ -72,3 +72,15 @@ class TestImage implements ui.Image { throw UnimplementedError(); } } + +class ErrorImageProvider extends ImageProvider { + @override + ImageStreamCompleter load(ErrorImageProvider key) { + throw Error(); + } + + @override + Future obtainKey(ImageConfiguration configuration) { + return SynchronousFuture(this); + } +}