diff --git a/packages/flutter/lib/src/widgets/widget_inspector.dart b/packages/flutter/lib/src/widgets/widget_inspector.dart index 34ad78bc35..3eb08f8ac0 100644 --- a/packages/flutter/lib/src/widgets/widget_inspector.dart +++ b/packages/flutter/lib/src/widgets/widget_inspector.dart @@ -680,11 +680,39 @@ typedef InspectorSelectionChangedCallback = void Function(); /// Structure to help reference count Dart objects referenced by a GUI tool /// using [WidgetInspectorService]. -class _InspectorReferenceData { - _InspectorReferenceData(this.object); +/// +/// Does not hold the object from garbage collection. +@visibleForTesting +class InspectorReferenceData { + /// Creates an instance of [InspectorReferenceData]. + InspectorReferenceData(Object object, this.id) { + // These types are not supported by [WeakReference]. + // See https://api.dart.dev/stable/3.0.2/dart-core/WeakReference-class.html + if (object is String || object is num || object is bool) { + _value = object; + return; + } - final Object object; + _ref = WeakReference(object); + } + + WeakReference? _ref; + + Object? _value; + + /// The id of the object in the widget inspector records. + final String id; + + /// The number of times the object has been referenced. int count = 1; + + /// The value. + Object? get value { + if (_ref != null) { + return _ref!.target; + } + return _value; + } } // Production implementation of [WidgetInspectorService]. @@ -742,9 +770,9 @@ mixin WidgetInspectorService { /// The VM service protocol does not keep alive object references so this /// class needs to manually manage groups of objects that should be kept /// alive. - final Map> _groups = >{}; - final Map _idToReferenceData = {}; - final Map _objectToId = Map.identity(); + final Map> _groups = >{}; + final Map _idToReferenceData = {}; + final WeakMap _objectToId = WeakMap(); int _nextId = 0; /// The pubRootDirectories that are currently configured for the widget inspector. @@ -1270,20 +1298,22 @@ mixin WidgetInspectorService { /// references from a different group. @protected void disposeGroup(String name) { - final Set<_InspectorReferenceData>? references = _groups.remove(name); + final Set? references = _groups.remove(name); if (references == null) { return; } references.forEach(_decrementReferenceCount); } - void _decrementReferenceCount(_InspectorReferenceData reference) { + void _decrementReferenceCount(InspectorReferenceData reference) { reference.count -= 1; assert(reference.count >= 0); if (reference.count == 0) { - final String? id = _objectToId.remove(reference.object); - assert(id != null); - _idToReferenceData.remove(id); + final Object? value = reference.value; + if (value != null) { + _objectToId.remove(value); + } + _idToReferenceData.remove(reference.id); } } @@ -1295,14 +1325,15 @@ mixin WidgetInspectorService { return null; } - final Set<_InspectorReferenceData> group = _groups.putIfAbsent(groupName, () => Set<_InspectorReferenceData>.identity()); + final Set group = _groups.putIfAbsent(groupName, () => Set.identity()); String? id = _objectToId[object]; - _InspectorReferenceData referenceData; + InspectorReferenceData referenceData; if (id == null) { + // TODO(polina-c): comment here why we increase memory footprint by the prefix 'inspector-'. id = 'inspector-$_nextId'; _nextId += 1; _objectToId[object] = id; - referenceData = _InspectorReferenceData(object); + referenceData = InspectorReferenceData(object, id); _idToReferenceData[id] = referenceData; group.add(referenceData); } else { @@ -1332,11 +1363,11 @@ mixin WidgetInspectorService { return null; } - final _InspectorReferenceData? data = _idToReferenceData[id]; + final InspectorReferenceData? data = _idToReferenceData[id]; if (data == null) { throw FlutterError.fromParts([ErrorSummary('Id does not exist.')]); } - return data.object; + return data.value; } /// Returns the object to introspect to determine the source location of an @@ -1368,7 +1399,7 @@ mixin WidgetInspectorService { return; } - final _InspectorReferenceData? referenceData = _idToReferenceData[id]; + final InspectorReferenceData? referenceData = _idToReferenceData[id]; if (referenceData == null) { throw FlutterError.fromParts([ErrorSummary('Id does not exist')]); } @@ -3713,3 +3744,54 @@ class _WidgetFactory { // recognize the annotation. // ignore: library_private_types_in_public_api const _WidgetFactory widgetFactory = _WidgetFactory(); + +/// Does not hold keys from garbage collection. +@visibleForTesting +class WeakMap { + Expando _objects = Expando(); + + /// Strings, numbers, booleans. + final Map _primitives = {}; + + bool _isPrimitive(Object? key) { + return key == null || key is String || key is num || key is bool; + } + + /// Returns the value for the given [key] or null if [key] is not in the map + /// or garbage collected. + /// + /// Does not support records to act as keys. + V? operator [](K key){ + if (_isPrimitive(key)) { + return _primitives[key]; + } else { + return _objects[key!] as V?; + } + } + + /// Associates the [key] with the given [value]. + void operator []=(K key, V value){ + if (_isPrimitive(key)) { + _primitives[key] = value; + } else { + _objects[key!] = value; + } + } + + /// Removes the value for the given [key] from the map. + V? remove(K key) { + if (_isPrimitive(key)) { + return _primitives.remove(key); + } else { + final V? result = _objects[key!] as V?; + _objects[key] = null; + return result; + } + } + + /// Removes all pairs from the map. + void clear() { + _objects = Expando(); + _primitives.clear(); + } +} diff --git a/packages/flutter/test/widgets/widget_inspector_test.dart b/packages/flutter/test/widgets/widget_inspector_test.dart index aa64ca4789..4e0d2378a0 100644 --- a/packages/flutter/test/widgets/widget_inspector_test.dart +++ b/packages/flutter/test/widgets/widget_inspector_test.dart @@ -9,7 +9,9 @@ @TestOn('!chrome') library; +import 'dart:async'; import 'dart:convert'; +import 'dart:developer'; import 'dart:math'; import 'dart:ui' as ui; @@ -240,7 +242,67 @@ extension TextFromString on String { } } +/// Forces garbage collection by aggressive memory allocation. +Future _forceGC() async { + const Duration timeout = Duration(seconds: 5); + const int gcCycles = 3; // 1 should be enough, but we do 3 to make sure test is not flaky. + final Stopwatch stopwatch = Stopwatch()..start(); + final int barrier = reachabilityBarrier; + + final List> storage = >[]; + + void allocateMemory() { + storage.add(Iterable.generate(10000, (_) => DateTime.now()).toList()); + if (storage.length > 100) { + storage.removeAt(0); + } + } + + while (reachabilityBarrier < barrier + gcCycles) { + if (stopwatch.elapsed > timeout) { + throw TimeoutException('forceGC timed out', timeout); + } + await Future.delayed(Duration.zero); + allocateMemory(); + } +} + + +final List _weakValueTests = [1, 1.0, 'hello', true, false, Object(), [3, 4], DateTime(2023)]; + void main() { + group('$InspectorReferenceData', (){ + for (final Object item in _weakValueTests) { + test('can be created for any type but $Record, $item', () async { + final InspectorReferenceData weakValue = InspectorReferenceData(item, 'id'); + expect(weakValue.value, item); + }); + } + + test('throws for $Record', () async { + expect(()=> InspectorReferenceData((1, 2), 'id'), throwsA(isA())); + }); + }); + + group('$WeakMap', (){ + for (final Object item in _weakValueTests) { + test('assigns and removes value, $item', () async { + final WeakMap weakMap = WeakMap(); + weakMap[item] = 1; + expect(weakMap[item], 1); + expect(weakMap.remove(item), 1); + expect(weakMap[item], null); + }); + } + + for (final Object item in _weakValueTests) { + test('returns null for absent value, $item', () async { + final WeakMap weakMap = WeakMap(); + expect(weakMap[item], null); + }); + } + }); + _TestWidgetInspectorService.runTests(); } @@ -261,6 +323,19 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { } }); + test('WidgetInspector does not hold objects from GC', () async { + List? someObject = [DateTime.now(), DateTime.now()]; + final String? id = service.toId(someObject, 'group_name'); + + expect(id, isNotNull); + + final WeakReference ref = WeakReference(someObject); + someObject = null; + await _forceGC(); + + expect(ref.target, null); + }); + testWidgets('WidgetInspector smoke test', (WidgetTester tester) async { // This is a smoke test to verify that adding the inspector doesn't crash. await tester.pumpWidget( @@ -3745,7 +3820,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { _CreationLocation location = knownLocations[id]!; expect(location.file, equals(file)); // ClockText widget. - expect(location.line, equals(55)); + expect(location.line, equals(57)); expect(location.column, equals(9)); expect(location.name, equals('ClockText')); expect(count, equals(1)); @@ -3755,7 +3830,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { location = knownLocations[id]!; expect(location.file, equals(file)); // Text widget in _ClockTextState build method. - expect(location.line, equals(93)); + expect(location.line, equals(95)); expect(location.column, equals(12)); expect(location.name, equals('Text')); expect(count, equals(1)); @@ -3782,7 +3857,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { location = knownLocations[id]!; expect(location.file, equals(file)); // ClockText widget. - expect(location.line, equals(55)); + expect(location.line, equals(57)); expect(location.column, equals(9)); expect(location.name, equals('ClockText')); expect(count, equals(3)); // 3 clock widget instances rebuilt. @@ -3792,7 +3867,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { location = knownLocations[id]!; expect(location.file, equals(file)); // Text widget in _ClockTextState build method. - expect(location.line, equals(93)); + expect(location.line, equals(95)); expect(location.column, equals(12)); expect(location.name, equals('Text')); expect(count, equals(3)); // 3 clock widget instances rebuilt.