diff --git a/mobile/lib/presentation/widgets/images/animated_image_stream_completer.dart b/mobile/lib/presentation/widgets/images/animated_image_stream_completer.dart index be4fbff8cf334..796d30e9920d1 100644 --- a/mobile/lib/presentation/widgets/images/animated_image_stream_completer.dart +++ b/mobile/lib/presentation/widgets/images/animated_image_stream_completer.dart @@ -3,24 +3,21 @@ import 'dart:ui' as ui; import 'package:flutter/foundation.dart' show InformationCollector; import 'package:flutter/painting.dart'; +import 'package:immich_mobile/presentation/widgets/images/cache_aware_listener_tracker.mixin.dart'; /// A [MultiFrameImageStreamCompleter] with support for listener tracking /// which makes resource cleanup possible when no longer needed. /// Codec is disposed through the MultiFrameImageStreamCompleter's internals onDispose method -class AnimatedImageStreamCompleter extends MultiFrameImageStreamCompleter { - void Function()? _onLastListenerRemoved; - int _listenerCount = 0; - // True once any image or the codec has been provided. - // Until then the image cache holds one listener, so "last real listener gone" - // is _listenerCount == 1, not 0. - bool didProvideImage = false; - +class AnimatedImageStreamCompleter extends MultiFrameImageStreamCompleter with CacheAwareListenerTrackerMixin { AnimatedImageStreamCompleter._({ required super.codec, required super.scale, + required bool hadInitialImage, super.informationCollector, void Function()? onLastListenerRemoved, - }) : _onLastListenerRemoved = onLastListenerRemoved; + }) { + setupListenerTracking(hadInitialImage: hadInitialImage, onLastListenerRemoved: onLastListenerRemoved); + } factory AnimatedImageStreamCompleter({ required Stream stream, @@ -33,23 +30,21 @@ class AnimatedImageStreamCompleter extends MultiFrameImageStreamCompleter { final self = AnimatedImageStreamCompleter._( codec: codecCompleter.future, scale: scale, + hadInitialImage: initialImage != null, informationCollector: informationCollector, onLastListenerRemoved: onLastListenerRemoved, ); if (initialImage != null) { - self.didProvideImage = true; self.setImage(initialImage); } stream.listen( (item) { if (item is ImageInfo) { - self.didProvideImage = true; self.setImage(item); } else if (item is ui.Codec) { if (!codecCompleter.isCompleted) { - self.didProvideImage = true; codecCompleter.complete(item); } } @@ -70,27 +65,4 @@ class AnimatedImageStreamCompleter extends MultiFrameImageStreamCompleter { return self; } - - @override - void addListener(ImageStreamListener listener) { - super.addListener(listener); - _listenerCount++; - } - - @override - void removeListener(ImageStreamListener listener) { - super.removeListener(listener); - _listenerCount--; - - final bool onlyCacheListenerLeft = _listenerCount == 1 && !didProvideImage; - final bool noListenersAfterCodec = _listenerCount == 0 && didProvideImage; - - if (onlyCacheListenerLeft || noListenersAfterCodec) { - final onLastListenerRemoved = _onLastListenerRemoved; - if (onLastListenerRemoved != null) { - _onLastListenerRemoved = null; - onLastListenerRemoved(); - } - } - } } diff --git a/mobile/lib/presentation/widgets/images/cache_aware_listener_tracker.mixin.dart b/mobile/lib/presentation/widgets/images/cache_aware_listener_tracker.mixin.dart new file mode 100644 index 0000000000000..e63d5c4cfca80 --- /dev/null +++ b/mobile/lib/presentation/widgets/images/cache_aware_listener_tracker.mixin.dart @@ -0,0 +1,84 @@ +import 'package:flutter/painting.dart'; + +/// Tracks listeners on an [ImageStreamCompleter] to safely cancel in-flight +/// network requests without interfering with [ImageCache] internals. +/// +/// ### Problem +/// Cancelling fetches when the listener count drops to 1 (cache only) or 0 +/// is unsafe due to three framework behaviours: +/// +/// 1. **Memory-pressure eviction** — `ImageCache.clear()` removes the cache +/// listener while UI widgets still need the image. A count-based check +/// would cancel the active fetch, leaving the UI with no image. +/// 2. **Synchronous detach during `putIfAbsent`** — When an `initialImage` +/// is provided, the cache attaches, receives the frame, and detaches +/// synchronously *before* the UI widget can attach. Count reaches 0 and +/// would trigger a false cancel. +/// 3. **Listener misidentification** — After the cache detaches (via 1 or 2), +/// the next UI listener could be mistaken for the cache listener, causing +/// incorrect cancellations when that widget is disposed. +/// +/// ### Solution: First-Listener Heuristic +/// The cache is always the first listener attached (via `putIfAbsent`). This +/// mixin records that identity once and uses it for all subsequent decisions: +/// +/// * **Identity locking** — The first listener is assumed to be the cache. +/// Once identified, `_hasIdentifiedCacheListener` prevents reassignment. +/// * **Targeted cancellation** — Cancel only when the identified cache +/// listener is the sole remaining listener and no image has been delivered. +/// * **Sync-removal bypass** — When `hadInitialImage` is set, the first +/// synchronous removal of the cache listener is ignored so the fetch +/// survives until the UI attaches. +mixin CacheAwareListenerTrackerMixin on ImageStreamCompleter { + void Function()? _onLastListenerRemoved; + int _listenerCount = 0; + bool _hadInitialImage = false; + bool _hasIgnoredFirstSyncRemoval = false; + ImageStreamListener? _cacheListener; + bool _hasIdentifiedCacheListener = false; + + /// Initializes the tracking state. Must be called in the subclass constructor. + void setupListenerTracking({required bool hadInitialImage, void Function()? onLastListenerRemoved}) { + _hadInitialImage = hadInitialImage; + _onLastListenerRemoved = onLastListenerRemoved; + } + + @override + void addListener(ImageStreamListener listener) { + if (!_hasIdentifiedCacheListener) { + _hasIdentifiedCacheListener = true; + _cacheListener = listener; + } + + _listenerCount++; + super.addListener(listener); + } + + @override + void removeListener(ImageStreamListener listener) { + super.removeListener(listener); + _listenerCount--; + + final bool isCacheListener = listener == _cacheListener; + if (isCacheListener) { + _cacheListener = null; + } + + if (_hadInitialImage && !_hasIgnoredFirstSyncRemoval && isCacheListener) { + _hasIgnoredFirstSyncRemoval = true; + return; + } + + final bool onlyCacheListenerLeft = _listenerCount == 1 && _cacheListener != null; + + final bool completelyAbandoned = _listenerCount == 0; + + if (onlyCacheListenerLeft || completelyAbandoned) { + final onLastListenerRemoved = _onLastListenerRemoved; + if (onLastListenerRemoved != null) { + _onLastListenerRemoved = null; + onLastListenerRemoved(); + } + } + } +} diff --git a/mobile/lib/presentation/widgets/images/one_frame_multi_image_stream_completer.dart b/mobile/lib/presentation/widgets/images/one_frame_multi_image_stream_completer.dart index 302deca4a7a1f..e80d191ac9bf9 100644 --- a/mobile/lib/presentation/widgets/images/one_frame_multi_image_stream_completer.dart +++ b/mobile/lib/presentation/widgets/images/one_frame_multi_image_stream_completer.dart @@ -6,14 +6,10 @@ import 'dart:async'; import 'package:flutter/foundation.dart'; import 'package:flutter/painting.dart'; +import 'package:immich_mobile/presentation/widgets/images/cache_aware_listener_tracker.mixin.dart'; /// An ImageStreamCompleter with support for loading multiple images. -class OneFramePlaceholderImageStreamCompleter extends ImageStreamCompleter { - void Function()? _onLastListenerRemoved; - int _listenerCount = 0; - // True once setImage() has been called at least once. - bool didProvideImage = false; - +class OneFramePlaceholderImageStreamCompleter extends ImageStreamCompleter with CacheAwareListenerTrackerMixin { /// The constructor to create an OneFramePlaceholderImageStreamCompleter. The [images] /// should be the primary images to display (typically asynchronously as they load). /// The [initialImage] is an optional image that will be emitted synchronously @@ -24,14 +20,14 @@ class OneFramePlaceholderImageStreamCompleter extends ImageStreamCompleter { InformationCollector? informationCollector, void Function()? onLastListenerRemoved, }) { + setupListenerTracking(hadInitialImage: initialImage != null, onLastListenerRemoved: onLastListenerRemoved); + if (initialImage != null) { - didProvideImage = true; setImage(initialImage); } - _onLastListenerRemoved = onLastListenerRemoved; + images.listen( (image) { - didProvideImage = true; setImage(image); }, onError: (Object error, StackTrace stack) { @@ -45,26 +41,4 @@ class OneFramePlaceholderImageStreamCompleter extends ImageStreamCompleter { }, ); } - - @override - void addListener(ImageStreamListener listener) { - super.addListener(listener); - _listenerCount = _listenerCount + 1; - } - - @override - void removeListener(ImageStreamListener listener) { - super.removeListener(listener); - _listenerCount = _listenerCount - 1; - - final bool onlyCacheListenerLeft = _listenerCount == 1 && !didProvideImage; - final bool noListenersAfterImage = _listenerCount == 0 && didProvideImage; - - final onLastListenerRemoved = _onLastListenerRemoved; - - if (onLastListenerRemoved != null && (noListenersAfterImage || onlyCacheListenerLeft)) { - _onLastListenerRemoved = null; - onLastListenerRemoved(); - } - } } diff --git a/mobile/test/presentation/widgets/images/cache_aware_listener_tracker_mixin_test.dart b/mobile/test/presentation/widgets/images/cache_aware_listener_tracker_mixin_test.dart new file mode 100644 index 0000000000000..02bb0c1053d89 --- /dev/null +++ b/mobile/test/presentation/widgets/images/cache_aware_listener_tracker_mixin_test.dart @@ -0,0 +1,183 @@ +import 'dart:ui' as ui; + +import 'package:flutter/painting.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:immich_mobile/presentation/widgets/images/cache_aware_listener_tracker.mixin.dart'; + +class TestImageCompleter extends ImageStreamCompleter with CacheAwareListenerTrackerMixin { + bool wasCancelled = false; + + TestImageCompleter({required bool hadInitialImage}) { + setupListenerTracking( + hadInitialImage: hadInitialImage, + onLastListenerRemoved: () { + wasCancelled = true; + }, + ); + } + + @override + void setImage(ImageInfo image) { + super.setImage(image); + } +} + +void main() { + late ImageCache cache; + late ImageStreamListener uiListener; + + setUp(() { + // Create a fresh, real Flutter ImageCache for every test + cache = ImageCache(); + uiListener = ImageStreamListener((_, __) {}); + }); + + group('CacheAwareListenerTrackerMixin with Real ImageCache', () { + + testWidgets('cancels fetch when UI detaches before completion', (WidgetTester tester) async { + final completer = TestImageCompleter(hadInitialImage: false); + final key = Object(); + + // 1. Request image from the real cache (simulating the provider) + final stream = cache.putIfAbsent(key, () => completer)!; + + // 2. UI attaches + stream.addListener(uiListener); + expect(completer.wasCancelled, isFalse); + + // 3. Simulate asynchronous network delay... + await tester.pump(const Duration(milliseconds: 150)); + + // 4. User scrolls away before network finishes. UI detaches. + stream.removeListener(uiListener); + + expect(completer.wasCancelled, isTrue); + }); + + testWidgets('survives cache eviction while UI listener is still attached', (WidgetTester tester) async { + final completer = TestImageCompleter(hadInitialImage: false); + final key = Object(); + + // 1. Request image and attach UI + final stream = cache.putIfAbsent(key, () => completer)!; + stream.addListener(uiListener); + + // 2. Simulate app going to background -> OS Memory Warning -> Cache clears + cache.clear(); + + // Even though the real cache just aggressively detached its listener, + // the stream MUST survive because the UI widget is still on screen! + expect(completer.wasCancelled, isFalse); + + // 3. UI widget finally detaches + stream.removeListener(uiListener); + expect(completer.wasCancelled, isTrue); + }); + + testWidgets('survives synchronous cache detach during putIfAbsent with initialImage', (WidgetTester tester) async { + final completer = TestImageCompleter(hadInitialImage: true); + final key = Object(); + + // Run image creation outside FakeAsync zone to avoid hang + late ui.Image dummyImage; + await tester.runAsync(() async { + dummyImage = await createTestImage(width: 1, height: 1); + }); + + final initialImageInfo = ImageInfo(image: dummyImage); + + final stream = cache.putIfAbsent(key, () { + completer.setImage(initialImageInfo); + return completer; + })!; + + expect(completer.wasCancelled, isFalse); + + stream.addListener(uiListener); + expect(completer.wasCancelled, isFalse); + + stream.removeListener(uiListener); + expect(completer.wasCancelled, isTrue); + }); + + testWidgets('fires cleanup on full abandonment even after successful fetch', (WidgetTester tester) async { + final completer = TestImageCompleter(hadInitialImage: false); + final key = Object(); + + final stream = cache.putIfAbsent(key, () => completer)!; + stream.addListener(uiListener); + + await tester.pump(const Duration(milliseconds: 100)); + + // Run image creation outside FakeAsync zone to avoid hang + late ui.Image dummyImage; + await tester.runAsync(() async { + dummyImage = await createTestImage(width: 1, height: 1); + }); + + completer.setImage(ImageInfo(image: dummyImage)); + + stream.removeListener(uiListener); + + // The stream is completely abandoned (0 listeners), so it fires the cleanup hook. + // Since the image is already downloaded, canceling the network token is a safe no-op. + expect(completer.wasCancelled, isTrue); + }); + + testWidgets('Multiple UI listeners — only all detached, should cancel', (WidgetTester tester) async { + final completer = TestImageCompleter(hadInitialImage: false); + final key = Object(); + + final stream = cache.putIfAbsent(key, () => completer)!; + + final uiListener2 = ImageStreamListener((_, __) {}); + stream.addListener(uiListener); + stream.addListener(uiListener2); + + // First UI detach leaves cache + one UI → no cancel + stream.removeListener(uiListener); + expect(completer.wasCancelled, isFalse); + + // Second UI detach leaves only cache → cancel + stream.removeListener(uiListener2); + expect(completer.wasCancelled, isTrue); + }); + + testWidgets('Listener misidentification: new listener after cache eviction is not treated as cache', (WidgetTester tester) async { + final completer = TestImageCompleter(hadInitialImage: false); + final key = Object(); + + final stream = cache.putIfAbsent(key, () => completer)!; + + // UI attaches + stream.addListener(uiListener); + + // Cache eviction removes the cache listener + cache.clear(); + expect(completer.wasCancelled, isFalse); + + // A second UI listener attaches — must NOT be treated as cache + final uiListener2 = ImageStreamListener((_, __) {}); + stream.addListener(uiListener2); + + // Remove first UI listener; second UI still active → no cancel + stream.removeListener(uiListener); + expect(completer.wasCancelled, isFalse); + + // Remove second UI listener; completely abandoned → cancel + stream.removeListener(uiListener2); + expect(completer.wasCancelled, isTrue); + }); + + testWidgets('No UI listener ever attaches (cache-only) — cache detaches should cancel', (WidgetTester tester) async { + final completer = TestImageCompleter(hadInitialImage: false); + final key = Object(); + + cache.putIfAbsent(key, () => completer); + + // Cache eviction removes the only listener + cache.clear(); + expect(completer.wasCancelled, isTrue); + }); + }); +}