fix: dispose the image handed to a tile pruned during dispatch - #2239
Open
dinin92-del wants to merge 5 commits into
Open
dinin92-del wants to merge 5 commits into
dinin92-del wants to merge 5 commits into
Conversation
added 4 commits
August 5, 2026 12:02
…patch
`TileImage._onImageLoadSuccess` stored the `ImageInfo` even when the tile was
already disposed. `ImageStreamCompleter.setImage` gives every listener its own
handle and the listener owns it: a displayed tile passes ownership to
`RenderImage`, which disposes it, but a disposed tile never builds one — so the
decoded image stays alive for the lifetime of the process.
`dispose()` does remove the listener, which is why this is not reachable by
simply disposing a tile and then completing its image. It is reachable because
`setImage` dispatches over a copy of the listener list ("Make a copy to allow
for concurrent modification"): a listener removed from inside that loop is
still called. Two tiles resolving equal keys share one completer, and
`onLoadComplete` is where tiles get pruned — as already noted in
`TileImageManager.reloadImages` — so a tile can be disposed mid-dispatch and
handed an image regardless.
Unnoticeable with 256x256 tiles (256 KB). With 768x768 RGBA tiles (2.25 MB
each) it killed an app on iOS while browsing the map.
Adds a regression test that reproduces the dispatch race; it fails on the
current code with `Expected: null / Actual: ImageInfo:<[8x8] @ 1.0x>`.
Ownership of a tile's decoded image transfers at BUILD time: `RawImage` passes the raw `ui.Image` to `RenderImage`, which disposes it when it is replaced or unmounted. That is why `TileImage.dispose()` never freed `imageInfo` — by then the render object owns it. The model assumes ONE frame per tile. That holds for static tiles, but a completer emitting a second frame (progressive tiles: a base frame, then a composed one) overwrites `imageInfo` before any build has to happen. When both frames land within the same frame budget — the common case on a fast device — the first handle never reaches a `RenderImage` and nothing frees it. Measured on an iPhone with 768x768 composed tiles: ~0.9 leaked handles per tile, growing linearly with tiles browsed, and invisible to `ImageCache`, which reported ~25 live images while 3336 `ui.Image` objects were alive. Two app kills within four minutes. The tile now tracks whether its handle was passed on (`Tile` marks it while building `RawImage`) and frees it only while it is still its own — on frame replacement and on dispose. Freeing a handle the render object owns would be a double free, so the flag is what makes this safe rather than lucky. Two tests, both counting real open handles rather than asserting on code: one for the leaking path, one for the mirror case where the widget already took the handle and the tile must keep its hands off.
…ever takes over The flag-based fix (823e8a8) rested on a false premise. It assumed ownership of the decoded image transfers to `RenderImage` at build time, so it freed the handle only when NO build had taken it. Verified against Flutter sources: `RawImage` CLONES the image for its render object — both `createRenderObject` and `updateRenderObject` pass `image?.clone()`. The render object frees its own clone; the tile's handle never stops being the tile's. Consequence: the flag exempted exactly the PAINTED frames, so every painted frame still leaked one handle. Measured on device after 823e8a8: ~0.3 leaked handles per tile — matching the fraction of tiles that emit two frames — linear, no plateau, invisible to ImageCache. GC finalizers reclaim such handles eventually, which is why 256 KB default tiles get away with the upstream model and 2.25 MB tiles do not. The correct model is simpler than the flag: the tile frees its own handle, always — on frame replacement and on dispose. Dispose also nulls the field, because dispose can race the layer rebuild and a straggler build must see null (paint nothing) rather than clone a disposed image. Caught by a new full-cycle test (TileImage → Tile → RawImage → RenderImage) counting open handles after complete teardown — the pure-TileImage tests could not see it, because the bug lived in what the widget integration does NOT do with the handle. Sensitivity: reverting both files to 823e8a8 fails all three cases of that test.⚠️ Test hygiene that mattered: `createTestImage` caches by size and returns clones of ONE shared image, making two "independent" handle counters move in lockstep. `cache: false` + distinct sizes are load-bearing in these tests.
…ed once
Simplify pass over the leak fix, four angles (reuse / simplification /
efficiency / altitude), behavior untouched — the sensitivity check (both lib
files reverted to the flag model) still fails all three full-cycle cases.
- test_utils/test_frame_driver.dart: ONE DrivenCompleter/DrivenProvider pair
and one testTileImage factory replace three byte-identical class pairs and
two TileImage factories spread over two files. A constructor change now
touches one place.
- _SharedManualImageProvider deleted: OneFrame-wrapping a Completer was a
strict subset of the driven provider.
- The ownership doctrine lives ONCE, on the imageInfo field doc; the
replacement and dispose sites state the rule in one line and point there.
The story already changed once (the flag model) and needed every copy
edited — that class of drift is what this fork patch exists to kill.
- The _disposed branch comment no longer speaks the dead model's language
("never hands its image to a RenderImage") — it argues from ownership:
after dispose() the owner that would free the handle is gone.
- Whitespace orphans of the removed flag mechanism dropped (tile.dart is now
untouched by this branch, as it should be — the fix lives entirely in the
owner).
JaffaKetchup
changed the title
Member
|
Thanks for the contribution! I'll try to take a look at this soon. |
Member
|
Hey, could you please open a bug report and move the bug report from the PR description into it, and create an MRE so I can manually confirm that this fix fixes the memory leak? Thanks! |
bandy87
added a commit
to salonichu/flutter_map
that referenced
this pull request
Sep 14, 2026
Fixes two issues on top of v8.3.2. Both were reproduced on a physical device (Samsung SM S938B, Android 16, release build) with rapid pinch in/out zooming, and both fixes are verified by measurement. 1. TileImage never released its decoded image (lib/src/layer/tile_layer/tile_image.dart) `ImageStreamCompleter.setImage` hands every listener its own `image.clone()`, and `RawImage` clones again for its `RenderImage`. Handing the image to the widget tree therefore never transfers ownership: the handle belongs to the TileImage. It was released neither on frame replacement nor in `dispose()`, so every tile that was loaded and later pruned leaked its decoded bytes (~4 MiB for a 512px tile at @2x) for the lifetime of the process. Three cases are handled: an image delivered to an already-disposed tile is released in place (`setImage` iterates over a copy of the listener list, so a listener removed in `dispose()` can still receive a frame), the previous frame is released on replacement, and the handle is released in `dispose()`. Measured on the device, during continuous rapid zooming: Private Other memory peaked at 6025 MB before the fix and the app was killed after ~50 s; after the fix it stays at 43-65 MB and the app is stable for 150 s with no growth trend. See fleaflet#2239 2. NaN propagating from the pinch scale into the camera (lib/src/gestures/map_interactive_viewer.dart, lib/src/map/controller/map_controller_impl.dart) Lifting one finger mid-gesture while zoomed in makes `_scaleCorrector` drive the corrected scale to zero or below. `math.log()` then returns NaN, which `clamp()` does not filter out, and the tile layer throws on every frame. The pinch guard now checks the corrected scale, and `moveRaw()` rejects non-finite centre and zoom values as defence in depth. See fleaflet#2244
|
Opened #2265 with a full reproduction and before/after measurements on a physical device (Samsung SM S938B, Android 16, release build), as requested. I can confirm this PR's fix works: with these changes applied on top of v8.3.2, memory stays flat through 130+ seconds of continuous rapid pinch zooming (Private Other 34-77 MB), where the unpatched build was killed by the OS after ~50 seconds. The issue also lists five hypotheses I ruled out by measurement before arriving here (smaller tiles, disabling the built-in disk cache, forcing a layer reset, upgrading to 8.3.2, and reducing tile creation), in case that saves anyone repeating them. One note on CI: the only red check is "Analyse Code", which fails at |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TileImage._onImageLoadSuccessstores theImageInfoeven when the tile is already disposed.ImageStreamCompleter.setImagegives every listener its own handle and the listener owns it: a displayed tile passes ownership toRenderImage, which disposes it — but a disposed tile never builds one, so the decoded image stays alive for the lifetime of the process.Why
dispose()removing the listener isn't enoughThis is not reachable by simply disposing a tile and then completing its image —
dispose()removes the listener andsetImageearly-returns on an empty listener list.It is reachable because
setImagedispatches over a copy of the listener list:A listener removed from inside that loop is still called. Two tiles resolving equal keys share one completer, and
onLoadCompleteis where tiles get pruned — asTileImageManager.reloadImagesalready notes:So the first tile's completion can dispose the second tile mid-dispatch, and the second tile is handed an image anyway.
Impact
Unnoticeable with 256×256 tiles (256 KB). With 768×768 RGBA tiles (2.25 MB each) it killed an app on iOS while browsing the map — the process was terminated at its memory limit after a few minutes of panning.
Test
Adds
test/layer/tile_layer/tile_image_test.dart, which reproduces the dispatch race with two tiles sharing one completer and asserts both that the disposed tile keeps noImageInfoand that the handle it was given is released (debugGetOpenHandleStackTraces).On the current code it fails with:
Full suite passes with the change (118/118).
AI usage disclosure (per CONTRIBUTING): this patch and its test were written with AI assistance (Claude). I reviewed the change and the reasoning, ran the test suite, and verified that the added test fails on unpatched
masterand passes with the fix. I take responsibility for the code.