fix(tile_layer): dispose the image handed to a tile pruned during dispatch - #2239
Open
dinin92-del wants to merge 1 commit into
Open
Conversation
…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>`.
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
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.