Fix: Ensure Image.errorBuilder reliably prevents error reporting#166130
Fix: Ensure Image.errorBuilder reliably prevents error reporting#166130PerLycke wants to merge 3 commits into
Conversation
4cbc3c9 to
40eb238
Compare
40eb238 to
f1b49a3
Compare
|
Updating this PR with a revised approach. Instead of using an ephemeral listener, this fix now conditionally keeps the primary listener attached when TickerMode is disabled and an errorBuilder is present. This is simpler, covers the necessary scenarios, and integrates better with the existing _ImageState error handling. The description and commit have been updated accordingly. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
f1b49a3 to
e17fb63
Compare
When an Image widget included an errorBuilder, errors during image
loading could still be reported via FlutterError.reportError if the
widget was disposed before the asynchronous error completed. This occurred
because the disposed widget's listener was removed from the
ImageStreamCompleter, causing the completer to incorrectly treat the
error as unhandled.
This change implements a two-part fix:
1. ImageStreamCompleter now tracks if a listener with an `onError`
callback was ever attached (`_hadErrorHandlerListener`). Its final
error reporting logic skips the report if this flag is true and the
error wasn't handled by a currently attached listener, correctly
handling the disposal race condition.
2. _ImageState's listener now always attaches a non-null `onError`
callback. This callback always updates the state (`_lastException`)
to enable visual error feedback. It only calls FlutterError.reportError
directly if `errorBuilder` was null, shifting the reporting
responsibility for that case from the completer to the listener.
This ensures errors are correctly handled or suppressed according to the
presence of an `errorBuilder`, even across widget disposal, and preserves
the expected visual debug error placeholder for failed images when no
`errorBuilder` is present.
Added test: 'errorBuilder prevents FlutterError report even if widget is disposed'.
Fixes flutter#97077
e17fb63 to
4af29fc
Compare
|
Had to go a couple of rounds with this PR to find a comprehensive solution that reliably and properly fixes the problem. This new approach addresses a general disposal race condition more robustly. It also ensures correct debug visual feedback is preserved. Ready for review. |
|
We've been running this PR in production for a couple of weeks now and it successfully does what it claims, no more unwanted reports in Crashlytics, and no side effects. Question to the reviewer, do you see a better, or more proper, way of fixing the issue? Do you want me to refactor on your recent changes to image_stream, or what's the plan? |
There was a problem hiding this comment.
I doubt if this is the right approach.
To analyze this issue: the core problem is that Image's disposal will always want to dispose the image stream and remove all its listeners, at which point the error has to be reported due to having no listeners. This is an issue of the model, for which I can think of a few approaches. In general we can gracifully mark the stream "ended" to prevent its error reporting. On the other hand, the current PR kind of marks the image completer "forever tainted" by ever having one error reporter, which feels unnecessarily hacky.
Although the image stream is not built with a disposing method for now (which we can add one if necessary), I found another "workaround": addEphemeralErrorListener allows the first error to be handled without preventing the disposal. Do you think it's a good idea to instead make Image add an ephemeral error listener if the image widget has an onError handler?
| /// Tracks if any listener with an onError callback was ever added. | ||
| /// Used to prevent reporting errors even if the listener is removed | ||
| /// before the error occurs, if an errorBuilder was intended. | ||
| bool _hadErrorHandlerListener = false; |
There was a problem hiding this comment.
Private comments start with //, per Flutter's style guide.
| /// Tracks if any listener with an onError callback was ever added. | |
| /// Used to prevent reporting errors even if the listener is removed | |
| /// before the error occurs, if an errorBuilder was intended. | |
| bool _hadErrorHandlerListener = false; | |
| // Tracks if any listener with an onError callback was ever added. | |
| // Used to prevent reporting errors even if the listener is removed | |
| // before the error occurs, if an errorBuilder was intended. | |
| bool _hadErrorHandlerListener = false; |
There was a problem hiding this comment.
According to the Flutter style guide's Use /// for public-quality private documentation section, /// is appropriate for high-quality private docs aiming for the public standard, so the current format aligns with that guideline.
|
And sorry for the late review! I was in a vacation since mid last week. |
Code is self-explanatory (per review).
|
Hi @dkwingsmt, thank you for the review and feedback! I understand your concern about the This PR successfully fixes the primary issue where the errorBuilder was effectively ignored, as confirmed by the test case and our production testing. However, your feedback prompted a deeper look, and I've identified a potential side effect: For the case where While I agree a more explicit stream signalling mechanism might be cleaner architecturally, this PR offers a pragmatic solution for the existing bug within the current API. I'm happy to discuss further or explore other approaches if preferred. Thanks again for your guidance! |
|
My understanding is that your disagreement mostly comes from
Can you explain what is not covered? I've drafted a PR #167783 that implements the |
|
Hi again @dkwingsmt, thanks again for the feedback and for drafting the alternative fix #167783. I agree that using |
|
Still thank you very much for you analysis and contribution! They were really helpful :) |
…h `addEphemeralErrorListener`) (#167783) _An alternative to #166130 using `addEphemeralErrorListener`. The following text is adapted from #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 #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
Problem:
Currently, when using an
Imagewidget with anerrorBuilder, if the widget is removed from the widget tree (e.g., due to navigation orsetState) after the image loading process has started but before an asynchronous loading error is reported back, the error can still be reported viaFlutterError.reportError. This occurs because the_ImageStatelistener is removed upon disposal, and theImageStreamCompletersubsequently treats the error as unhandled, logging it despite the developer's intent to handle it via theerrorBuilder. This leads to unexpected noise in logs and crash reporting systems.Solution:
This PR implements a coordinated two-part fix in
widgets/image.dartandpainting/image_stream.dart:Modified
_ImageState._getListener(widgets/image.dart):onErrorcallback attached to theImageStreamListeneris now always non-null.setState({... _lastException = error ...})upon receiving an error. This ensures the state has the error information needed to render appropriate visual feedback.FlutterError.reportErrordirectly only ifwidget.errorBuilder == null. This ensures errors are reported when no user handler is provided.Modified
ImageStreamCompleter(painting/image_stream.dart):_hadErrorHandlerListener, is added.addListener, this flag is set totrueif the added listener has a non-nullonErrorcallback.reportErrorthat previously calledFlutterError.reportErrorif!handledis now changed toif (!handled && !_hadErrorHandlerListener).How it works:
This approach ensures the primary goal (respecting
errorBuildereven on disposal) is met while also preserving the correct visual error feedback in debug mode when noerrorBuilderis present (becausesetStateis always called upon error).errorBuilderis present, the completer's final report is suppressed if the widget is disposed, respecting the handler.errorBuilderis absent, the_ImageStatelistener reports the error and updates state, ensuring the error is logged and the debug visual is rendered as before.Related Issues:
Tests:
errorBuilder prevents FlutterError report even if widget is disposedtotest/widgets/image_test.dartto specifically verify the fix for the disposal race condition.test/widgets/image_test.dart(including golden tests like 'Failed image loads in debug mode') pass with these changes without requiring updates.Breaking Changes:
errorBuilderis present has shifted, but the user-facing outcome is consistent.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.