fix: Guard against double-dispose of TextBoxComponent cached image#3909
fix: Guard against double-dispose of TextBoxComponent cached image#3909Barba2k2 wants to merge 2 commits intoflame-engine:mainfrom
Conversation
TextBoxComponent.redraw() schedules the previous cached image for disposal after a 100 ms delay (to let pending rendering pipeline frames finish using it). The delayed callback unconditionally called Image.dispose() with only an isMounted gate. Two problems: 1. If the underlying native peer was already collected (e.g. low memory or the engine dropped the image), Image.dispose() throws and crashes the app — issue flame-engine#3724 reports this surfacing in production via Crashlytics, with 86 events / 39 users in 24 h. 2. The isMounted gate leaked the cached image whenever the component was removed within the 100 ms window, since dispose was simply skipped without an alternative cleanup path. Replaces the gate with a best-effort _safeDispose helper that swallows errors raised by an already-collected peer, and uses the same helper in onRemove so removal at any point is safe. Adds a regression test that disposes the cached image manually before removing the component, reproducing the original crash without the fix. Fixes flame-engine#3724
Address review feedback on flame-engine#3909: use explicit `on StateError` / `on AssertionError` clauses in place of the bare catch. - StateError: production crash from issue flame-engine#3724 — Flutter's Image throws "Bad state: native peer has been collected" when dispose accesses an engine-released SkImage. - AssertionError: debug-mode `assert(!_disposed)` guard fires when the image is disposed twice; test environments run in debug, so catching this keeps the safety net consistent across modes.
|
Pushed
Yes — there are two flavours and I've split the bare catch into explicit
Agreed that "next tick" is a cleaner mental model than the magic 100 ms — would let the GPU pipeline complete its current frame before we free the image. I left the existing scheduling alone in this PR to keep the change minimal and focused on the crash, but happy to follow up with a separate PR that swaps the |
|
The PR description needs to follow the PR template. |
|
@luanpotter both review threads addressed and resolved — |
3 participants
Description
TextBoxComponent.redraw()schedules the previous cachedImagefor disposal after a 100 ms delay (to let pending rendering-pipeline frames finish using it). The delayed callback used to callImage.dispose()directly, gated only byisMounted. Two problems:Image.dispose()throws and crashes the app — issue TextBoxComponent: A Dart object attempted to access a native peer... #3724 reports this surfacing in production via Crashlytics with 86 events / 39 users in 24 h.isMountedgate leaked the cached image whenever the component was removed within the 100 ms window, sincedisposewas simply skipped with no alternative cleanup path.This PR replaces the gate with a best-effort
_safeDisposehelper that catchesStateError(release-mode: native peer already collected) andAssertionError(debug-mode: already disposed by a parallel path), and uses the same helper fromonRemoveso removal at any point is crash-safe.A regression test was added in
text_box_component_test.dartthat disposes the cached image manually before removing the component, which without the fix throws and crashes the test runner.Checklist
docsand added dartdoc comments with///.examplesordocs.Breaking Change?
Related Issues
Fixes #3724