mirror of
https://github.com/flutter/flutter.git
synced 2026-02-20 02:29:02 +08:00
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]. <!-- Links --> [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
This commit is contained in:
Tong Mu
GitHub
parent
e2b220b683
commit
e5e8c48753
@ -1299,6 +1299,26 @@ class _ImageState extends State<Image> 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;
|
||||
}
|
||||
|
||||
@ -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<FlutterErrorDetails> reportedErrors = <FlutterErrorDetails>[];
|
||||
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<FlutterErrorDetails> reportedErrors = <FlutterErrorDetails>[];
|
||||
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
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user