diff --git a/lib/src/layer/tile_layer/tile_image.dart b/lib/src/layer/tile_layer/tile_image.dart index 71fe06a54..0f8625b9c 100644 --- a/lib/src/layer/tile_layer/tile_image.dart +++ b/lib/src/layer/tile_layer/tile_image.dart @@ -55,7 +55,22 @@ class TileImage extends ChangeNotifier { DateTime? loadFinishedAt; /// Some meta data of the image. + /// + /// Ownership: this handle is ALWAYS the tile's own. `RawImage` clones the + /// image for its `RenderImage` (`createRenderObject`/`updateRenderObject` + /// both pass `image?.clone()`), so handing it to the widget tree never + /// transfers ownership — the render object frees its own clone, and this + /// one stays with the tile until the tile frees it: on frame replacement + /// and on [dispose]. + /// + /// The previous model ("the render object takes over at build time") was + /// wrong and leaked exactly one handle per painted frame: measured on an + /// iPhone with 768x768 tiles as ~0.3-0.9 leaked handles per tile, invisible + /// to `ImageCache`, with the process eventually killed by jetsam. GC + /// finalizers reclaim such handles EVENTUALLY, which is why small default + /// tiles get away with it — 2.25 MB tiles do not. ImageInfo? imageInfo; + ImageStream? _imageStream; late ImageStreamListener _listener; @@ -158,12 +173,28 @@ class TileImage extends ChangeNotifier { void _onImageLoadSuccess(ImageInfo imageInfo, bool synchronousCall) { loadError = false; - this.imageInfo = imageInfo; - if (!_disposed) { - _display(); - onLoadComplete(coordinates); + // After dispose() the owner that would free this handle is gone (dispose + // ran and nulled the field — see the ownership note on [imageInfo]), so + // the handler frees it on the spot. + // + // `dispose()` removes the listener, but `setImage` dispatches over a copy + // of the listener list, so a listener removed from inside that loop is + // still called. Tiles resolving equal keys share one completer, and + // `onLoadComplete` is where pruning happens (see the note in + // `TileImageManager.reloadImages`) — so a tile can be disposed + // mid-dispatch and handed an image anyway. See `tile_image_test.dart`. + if (_disposed) { + imageInfo.dispose(); + return; } + + // The previous frame's handle is ours to free — see the ownership note + // on [imageInfo]. + this.imageInfo?.dispose(); + this.imageInfo = imageInfo; + _display(); + onLoadComplete(coordinates); } void _onImageLoadError(Object exception, StackTrace? stackTrace) { @@ -242,6 +273,11 @@ class TileImage extends ChangeNotifier { _animationController?.dispose(); _imageStream?.removeListener(_listener); + // Same ownership rule as on frame replacement (see [imageInfo]). Nulling + // the field keeps a straggler build — dispose can race the layer rebuild — + // from cloning a disposed image; `RawImage` treats null as "paint nothing". + imageInfo?.dispose(); + imageInfo = null; super.dispose(); } diff --git a/test/layer/tile_layer/tile_image_test.dart b/test/layer/tile_layer/tile_image_test.dart new file mode 100644 index 000000000..dd13145d5 --- /dev/null +++ b/test/layer/tile_layer/tile_image_test.dart @@ -0,0 +1,143 @@ +import 'package:flutter/widgets.dart'; +import 'package:flutter_map/flutter_map.dart'; +import 'package:flutter_test/flutter_test.dart'; + +import '../../test_utils/test_frame_driver.dart'; + + + +void main() { + testWidgets( + 'disposes the image handed to a tile pruned during listener dispatch', + (tester) async { + // `runAsync`: decoding needs real async, which the fake clock inside + // `testWidgets` never advances — awaiting it directly hangs the test. + final image = + (await tester.runAsync(() => createTestImage(width: 8, height: 8)))!; + // Measured, not assumed: `createTestImage` may hand back an image that + // already has more than one handle open. + final baseline = image.debugGetOpenHandleStackTraces()!.length; + + final completer = DrivenCompleter(); + final provider = DrivenProvider(completer); + + late final TileImage second; + var secondDisposed = false; + + // Two tiles sharing one ImageStreamCompleter, which is what ImageCache + // does whenever two tiles resolve equal keys — e.g. a tile that leaves + // the viewport and comes back while its image is still in flight. + // + // `ImageStreamCompleter.setImage` dispatches over a COPY of its listener + // list ("Make a copy to allow for concurrent modification"), so removing + // a listener from inside that loop does not stop it from being called. + // The first tile's completion runs `onLoadComplete` — which is where + // flutter_map prunes tiles — disposing the second tile mid-dispatch. + final first = testTileImage( + provider: provider, + onLoadComplete: (_) { + if (secondDisposed) return; + secondDisposed = true; + second.dispose(); + }, + ); + second = testTileImage(x: 1, provider: provider); + + first.load(); + second.load(); + + completer.emit(ImageInfo(image: image)); + await tester.pump(); + + expect( + secondDisposed, + isTrue, + reason: 'the scenario under test never happened', + ); + expect( + second.imageInfo, + isNull, + reason: 'a disposed tile must not keep an image nobody will paint', + ); + expect( + image.debugGetOpenHandleStackTraces(), + hasLength(baseline + 1), + reason: 'only the live tile may still hold a handle; the one handed to ' + 'the disposed tile must be released', + ); + + first.dispose(); + }, + ); + + testWidgets( + 'replacing a frame frees the previous handle — painted or not', + (tester) async { + // Ownership truth (checked against Flutter sources, not assumed): + // `RawImage` CLONES the image for its `RenderImage` — both + // `createRenderObject` and `updateRenderObject` pass `image?.clone()`. + // So the render object only ever frees its own clone, and the tile's + // handle stays the tile's forever. The earlier model ("the render + // object takes over at build time") was wrong and leaked one handle + // per painted frame — measured on device as ~0.3/tile after the + // flag-based fix, because the flag exempted exactly the painted frames. + final first = + (await tester.runAsync(() => createTestImage(width: 8, height: 8, cache: false)))!; + final second = + (await tester.runAsync(() => createTestImage(width: 9, height: 9, cache: false)))!; + final base1 = first.debugGetOpenHandleStackTraces()!.length; + + final completer = DrivenCompleter(); + final tile = testTileImage(provider: DrivenProvider(completer)); + tile.load(); + + completer.emit(ImageInfo(image: first)); + expect( + first.debugGetOpenHandleStackTraces()!.length, + greaterThan(base1), + reason: 'the tile must hold the first frame — the scenario needs that', + ); + + completer.emit(ImageInfo(image: second)); + + expect( + first.debugGetOpenHandleStackTraces(), + hasLength(base1 - 1), + reason: 'on replacement the tile must free its own handle to the ' + 'previous frame; the completer freed the original at setImage, ' + 'hence one below baseline', + ); + + tile.dispose(); + }, + ); + + testWidgets( + 'dispose frees the current handle and nulls the field', + (tester) async { + final image = + (await tester.runAsync(() => createTestImage(width: 8, height: 8, cache: false)))!; + final baseline = image.debugGetOpenHandleStackTraces()!.length; + + final completer = DrivenCompleter(); + final tile = testTileImage(provider: DrivenProvider(completer)); + tile.load(); + completer.emit(ImageInfo(image: image)); + + tile.dispose(); + + expect( + image.debugGetOpenHandleStackTraces(), + hasLength(baseline), + reason: 'the tile owned its handle to the very end — dispose must ' + 'free it (the completer still holds the original it was given)', + ); + expect( + tile.imageInfo, + isNull, + reason: 'dispose can race the layer rebuild; a straggler build must ' + 'see null (paint nothing), not a disposed image it would clone', + ); + }, + ); +} diff --git a/test/layer/tile_layer/tile_widget_handles_test.dart b/test/layer/tile_layer/tile_widget_handles_test.dart new file mode 100644 index 000000000..06aec1add --- /dev/null +++ b/test/layer/tile_layer/tile_widget_handles_test.dart @@ -0,0 +1,164 @@ +import 'package:flutter/widgets.dart'; +import 'package:flutter_map/flutter_map.dart'; +import 'package:flutter_map/src/layer/tile_layer/tile.dart'; +import 'package:flutter_test/flutter_test.dart'; + +import '../../test_utils/test_frame_driver.dart'; + +/// Handle accounting through the FULL widget cycle: TileImage → Tile → +/// RawImage → RenderImage. +/// +/// This is the test that CAUGHT the second leak (0806): the flag-based fix +/// assumed `RawImage` hands ownership to `RenderImage` at build time. It does +/// not — `RawImage` CLONES for the render object, so the tile's own handle +/// stayed open forever on every painted frame (~0.3/tile on device after the +/// flag fix, because the flag exempted exactly the painted frames). +/// +/// Pure-TileImage tests can't see this: the bug lived in what the widget +/// integration does NOT do with the handle. Hence full cycle here, counting +/// OPEN HANDLES on the raw images after complete teardown — not code presence. +/// +/// ⚠️ `cache: false` and DIFFERENT sizes are load-bearing: `createTestImage` +/// caches by size and returns clones of one shared image, which makes two +/// "independent" counters move in lockstep and hides per-frame attribution. + + +Widget _host(TileImage tileImage) => Directionality( + textDirection: TextDirection.ltr, + child: Stack( + children: [ + Tile( + scaledTileDimension: 256, + currentPixelOrigin: Offset.zero, + tileImage: tileImage, + tileBuilder: null, + positionCoordinates: const TileCoordinates(0, 0, 0), + ), + ], + ), + ); + +void main() { + testWidgets( + 'two frames with a build in between: after unmount every handle is closed', + (tester) async { + final images = (await tester.runAsync(() async => [ + await createTestImage(width: 8, height: 8, cache: false), + await createTestImage(width: 9, height: 9, cache: false), + ]))!; + final base1 = images[0].debugGetOpenHandleStackTraces()!.length; + final base2 = images[1].debugGetOpenHandleStackTraces()!.length; + + final completer = DrivenCompleter(); + final tileImage = testTileImage(provider: DrivenProvider(completer)); + tileImage.load(); + + await tester.pumpWidget(_host(tileImage)); + + // Frame 1, then a real build (the widget hands the handle on), then + // frame 2, then another build. + completer.emit(ImageInfo(image: images[0])); + await tester.pump(); + completer.emit(ImageInfo(image: images[1])); + await tester.pump(); + + // Tile leaves the tree (prune) and the TileImage is disposed — the full + // real-life teardown. + await tester.pumpWidget(const SizedBox()); + tileImage.dispose(); + // The completer outlives the tile in reality only while ImageCache holds + // it; here nobody does, so its current image must go too. + + expect( + images[0].debugGetOpenHandleStackTraces(), + hasLength(base1 - 1), + reason: 'frame 1: the completer freed the original at setImage and ' + 'the tile freed its clone on replacement — after teardown NOTHING ' + 'may hold it (this exact assertion caught the flag-based leak)', + ); + expect( + images[1].debugGetOpenHandleStackTraces(), + hasLength(base2), + reason: 'frame 2: the only open handle is the ORIGINAL one, now owned ' + 'by the completer as its current image (ImageCache would own the ' + 'completer in production) — the tile\'s clone must be gone', + ); + }, + ); + + testWidgets( + 'two frames in the SAME frame budget: after unmount every handle is closed', + (tester) async { + final images = (await tester.runAsync(() async => [ + await createTestImage(width: 8, height: 8, cache: false), + await createTestImage(width: 9, height: 9, cache: false), + ]))!; + final base1 = images[0].debugGetOpenHandleStackTraces()!.length; + final base2 = images[1].debugGetOpenHandleStackTraces()!.length; + + final completer = DrivenCompleter(); + final tileImage = testTileImage(provider: DrivenProvider(completer)); + tileImage.load(); + await tester.pumpWidget(_host(tileImage)); + + // Both frames before any pump — the fast-device case that leaked. + completer.emit(ImageInfo(image: images[0])); + completer.emit(ImageInfo(image: images[1])); + await tester.pump(); + + await tester.pumpWidget(const SizedBox()); + tileImage.dispose(); + + expect( + images[0].debugGetOpenHandleStackTraces(), + hasLength(base1 - 1), + reason: 'frame 1: no build between frames — same rule, the tile frees ' + 'its own handle on replacement', + ); + expect( + images[1].debugGetOpenHandleStackTraces(), + hasLength(base2), + reason: 'frame 2: only the original handle (completer-owned) remains', + ); + }, + ); + + testWidgets( + 'tile pruned between the frames: after teardown every handle is closed', + (tester) async { + final images = (await tester.runAsync(() async => [ + await createTestImage(width: 8, height: 8, cache: false), + await createTestImage(width: 9, height: 9, cache: false), + ]))!; + final base1 = images[0].debugGetOpenHandleStackTraces()!.length; + final base2 = images[1].debugGetOpenHandleStackTraces()!.length; + + final completer = DrivenCompleter(); + final tileImage = testTileImage(provider: DrivenProvider(completer)); + tileImage.load(); + await tester.pumpWidget(_host(tileImage)); + + completer.emit(ImageInfo(image: images[0])); + await tester.pump(); + + // Prune happens NOW — then the (shared, cache-held) completer still + // delivers frame 2 to nobody. + await tester.pumpWidget(const SizedBox()); + tileImage.dispose(); + completer.emit(ImageInfo(image: images[1])); + + expect( + images[0].debugGetOpenHandleStackTraces(), + hasLength(base1 - 1), + reason: 'frame 1 went through a build; teardown must close every ' + 'handle to it', + ); + expect( + images[1].debugGetOpenHandleStackTraces(), + hasLength(base2), + reason: 'frame 2: only the original handle (completer-owned) remains ' + '— delivered to nobody, cloned by nobody', + ); + }, + ); +} diff --git a/test/test_utils/test_frame_driver.dart b/test/test_utils/test_frame_driver.dart new file mode 100644 index 000000000..bd6ef1760 --- /dev/null +++ b/test/test_utils/test_frame_driver.dart @@ -0,0 +1,54 @@ +import 'dart:async'; + +import 'package:flutter/foundation.dart'; +import 'package:flutter/widgets.dart'; +import 'package:flutter_map/flutter_map.dart'; +import 'package:flutter_test/flutter_test.dart'; + +/// A completer the test drives frame by frame — progressive tiles (a base +/// frame followed by a composed one) emit more than once, and handle-ownership +/// tests need to control exactly when each frame lands. +class DrivenCompleter extends ImageStreamCompleter { + /// Delivers [info] to every listener as the next frame. + void emit(ImageInfo info) => setImage(info); +} + +/// An [ImageProvider] whose key is itself, so every [TileImage] resolving it +/// shares ONE completer — exactly what [ImageCache] does for equal keys. +class DrivenProvider extends ImageProvider { + /// Creates a provider that exposes [completer] to all resolvers. + DrivenProvider(this.completer); + + /// The completer shared by every resolve of this provider. + final ImageStreamCompleter completer; + + @override + Future obtainKey(ImageConfiguration configuration) => + SynchronousFuture(this); + + @override + ImageStreamCompleter loadImage( + DrivenProvider key, + ImageDecoderCallback decode, + ) => + completer; +} + +/// One factory for the handle-ownership tests — the [TileImage] constructor +/// takes eight arguments and duplicating the boilerplate per test file meant a +/// constructor change touched every copy. +TileImage testTileImage({ + required ImageProvider provider, + int x = 0, + void Function(TileCoordinates)? onLoadComplete, +}) => + TileImage( + vsync: const TestVSync(), + coordinates: TileCoordinates(x, 0, 0), + imageProvider: provider, + onLoadComplete: onLoadComplete ?? (_) {}, + onLoadError: (_, __, ___) {}, + tileDisplay: const TileDisplay.instantaneous(), + errorImage: null, + cancelLoading: Completer(), + );