From e5e8c487531728b0b684b106952d66a67e59bdab Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 1 May 2025 10:33:58 -0700 Subject: [PATCH] Fix: Ensure Image.errorBuilder reliably prevents error reporting (with `addEphemeralErrorListener`) (#167783) _An alternative to https://github.com/flutter/flutter/pull/166130 using `addEphemeralErrorListener`. The following text is adapted from https://github.com/flutter/flutter/pull/166130._ **Problem:** Currently, when using an `Image` widget with an `errorBuilder`, if the widget is removed from the widget tree (e.g., due to navigation or `setState`) *after* the image loading process has started but *before* an asynchronous loading error is reported back, the error can still be reported via `FlutterError.reportError`. This occurs because the `_ImageState` listener is removed upon disposal, and the `ImageStreamCompleter` subsequently treats the error as unhandled, logging it despite the developer's intent to handle it via the `errorBuilder`. This leads to unexpected noise in logs and crash reporting systems. **Solution:** This PR utilizes `addEphemeralErrorListener`, which allows the image stream to be disposed while having an error reporter. The error will not be reported as long as there is an error reporter. The `Image` widget adds an empty error reporter if `errorBuilder` is not null. **Related Issues:** * Fixes #97077 * Related: #107416, #69125, #34451, Baseflow/flutter_cached_network_image#780 **Tests:** * Added a new test case `errorBuilder prevents FlutterError report even if widget is disposed` to `test/widgets/image_test.dart` to specifically verify the fix for the disposal race condition. * This test was written by @/perlycke in https://github.com/flutter/flutter/pull/166130. * Existing tests in `test/widgets/image_test.dart` (including golden tests like 'Failed image loads in debug mode') pass with these changes without requiring updates. **Breaking Changes:** * None. This change fixes incorrect behavior and preserves expected debug visuals. The internal mechanism for reporting errors when no `errorBuilder` is present has shifted, but the user-facing outcome is consistent. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- packages/flutter/lib/src/widgets/image.dart | 20 +++ packages/flutter/test/widgets/image_test.dart | 120 ++++++++++++++++++ 2 files changed, 140 insertions(+) diff --git a/packages/flutter/lib/src/widgets/image.dart b/packages/flutter/lib/src/widgets/image.dart index 54d978f09a3..07bdf2b0492 100644 --- a/packages/flutter/lib/src/widgets/image.dart +++ b/packages/flutter/lib/src/widgets/image.dart @@ -1299,6 +1299,26 @@ class _ImageState extends State with WidgetsBindingObserver { _completerHandle = _imageStream!.completer!.keepAlive(); } + // It's almost time to remove the last listener, which triggers the + // disposal. But before that, add an ephemeral listener to potentially + // suppress errors. + // + // Reason: When an app provides an `Image` widget with an `errorBuilder`, it + // expects the widget to never report errors through `FlutterError` in any + // cases. This is hard if the stream fails after the disposal, because an + // image stream must have no listeners to be disposed, which then has + // nothing to suppress the errors. This is solve with the help of an + // ephemeral listener, which also suppresses the error but does not hinder + // disposal. For more details, see + // https://github.com/flutter/flutter/issues/97077 . + if (_imageStream!.completer != null && widget.errorBuilder != null) { + _imageStream!.completer!.addEphemeralErrorListener(( + Object exception, + StackTrace? stackTrace, + ) { + // Intentionally blank. + }); + } _imageStream!.removeListener(_getListener()); _isListeningToStream = false; } diff --git a/packages/flutter/test/widgets/image_test.dart b/packages/flutter/test/widgets/image_test.dart index 0c721f15cc6..8629578555c 100644 --- a/packages/flutter/test/widgets/image_test.dart +++ b/packages/flutter/test/widgets/image_test.dart @@ -2137,6 +2137,126 @@ void main() { codec.dispose(); }, ); + + testWidgets('errorBuilder prevents FlutterError report even if widget is disposed', ( + WidgetTester tester, + ) async { + // This test verifies that if an errorBuilder is provided, FlutterError.reportError + // is NOT called, even if the Image widget is removed from the tree before the + // image load fails. Regression test for https://github.com/flutter/flutter/issues/97077. + + // 1. Setup: Capture FlutterError reports + final List reportedErrors = []; + final FlutterExceptionHandler? oldHandler = FlutterError.onError; + FlutterError.onError = reportedErrors.add; + addTearDown(() { + FlutterError.onError = oldHandler; + }); // Ensure handler is restored + + final _TestImageProvider provider = _TestImageProvider(); + final Exception testException = Exception('Network failed'); + final StackTrace testStack = StackTrace.current; + + Widget buildImage() { + return Directionality( + textDirection: TextDirection.ltr, + child: Image( + image: provider, + errorBuilder: (_, _, _) => const SizedBox(width: 10, height: 10), + ), + ); + } + + // 2. Pump the widget with the Image. + await tester.pumpWidget(buildImage()); + expect(find.byType(Image), findsOneWidget); + expect(reportedErrors, isEmpty); // No errors yet + + // 3. Remove the Image widget from the tree. + await tester.pumpWidget(const SizedBox.shrink()); + expect(find.byType(Image), findsNothing); + + // 4. Now, make the image provider fail *after* the widget state is disposed. + provider.fail(testException, testStack); + + // 5. Allow asynchronous error propagation to complete robustly. + await tester.pumpAndSettle(); + // Restore the handler now in case `expect`s in step 6 fail. + FlutterError.onError = oldHandler; + + // 6. CRITICAL ASSERTION: Verify that no FlutterError was reported via the onError handler + expect( + reportedErrors, + isEmpty, + reason: 'FlutterError.onError should not be called when an errorBuilder was provided.', + ); + // Also check takeException as a standard backup. + expect(tester.takeException(), isNull); + }); + + testWidgets( + 'errorBuilder prevents FlutterError report only if errorBuilder is non-null when widget is disposed', + (WidgetTester tester) async { + // This test verifies that if an errorBuilder is provided, FlutterError.reportError + // is called, only if the errorBuilder stays present when the widget is unmounted. + + // 1. Setup: Capture FlutterError reports + final List reportedErrors = []; + final FlutterExceptionHandler? oldHandler = FlutterError.onError; + FlutterError.onError = reportedErrors.add; + addTearDown(() { + FlutterError.onError = oldHandler; + }); // Ensure handler is restored + + final _TestImageProvider provider = _TestImageProvider(); + final Exception testException = Exception('Network failed'); + final StackTrace testStack = StackTrace.current; + + // Function to build the widget with the Image + Widget buildImage({required bool hasErrorBuilder}) { + return Directionality( + textDirection: TextDirection.ltr, + child: Image( + image: provider, + errorBuilder: + hasErrorBuilder ? (_, _, _) => const SizedBox(width: 10, height: 10) : null, + ), + ); + } + + // 2. Pump the widget with an errorBuilder + await tester.pumpWidget(buildImage(hasErrorBuilder: true)); + expect(find.byType(Image), findsOneWidget); + expect(reportedErrors, isEmpty); // No errors yet + + // 3. Update the widget with no errorBuilder + await tester.pumpWidget(buildImage(hasErrorBuilder: false)); + expect(find.byType(Image), findsOneWidget); + expect(reportedErrors, isEmpty); // No errors yet + + // 4. Remove the Image widget from the tree. + await tester.pumpWidget(const SizedBox.shrink()); + expect(find.byType(Image), findsNothing); + + // 5. Now, make the image provider fail *after* the widget state is disposed. + provider.fail(testException, testStack); + + // 5. Allow asynchronous error propagation to complete robustly. + await tester.pumpAndSettle(); + // Restore the handler now in case `expect`s in step 6 fail. + FlutterError.onError = oldHandler; + + // 6. Verify that a FlutterError was reported via the onError handler + expect( + reportedErrors, + isNotEmpty, + reason: + 'FlutterError.onError should be called when an errorBuilder was not provided eventually.', + ); + // Also check takeException as a standard backup. + expect(tester.takeException(), isNull); + }, + ); } @immutable