From 2251bc5e3d3858d40564851418073dd97b30547f Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Wed, 7 Jun 2023 09:00:45 -0700 Subject: [PATCH] Revert "Make inspector weakly referencing the inspected objects. (#128095)" This reverts commit a5accb779b749c0677a38aef454c28c5acac355c. --- .../lib/src/widgets/widget_inspector.dart | 116 +++--------------- .../test/widgets/widget_inspector_test.dart | 83 +------------ 2 files changed, 21 insertions(+), 178 deletions(-) diff --git a/packages/flutter/lib/src/widgets/widget_inspector.dart b/packages/flutter/lib/src/widgets/widget_inspector.dart index 3eb08f8ac06ce..34ad78bc35a88 100644 --- a/packages/flutter/lib/src/widgets/widget_inspector.dart +++ b/packages/flutter/lib/src/widgets/widget_inspector.dart @@ -680,39 +680,11 @@ typedef InspectorSelectionChangedCallback = void Function(); /// Structure to help reference count Dart objects referenced by a GUI tool /// using [WidgetInspectorService]. -/// -/// 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; - } - - _ref = WeakReference(object); - } - - WeakReference? _ref; - - Object? _value; - - /// The id of the object in the widget inspector records. - final String id; +class _InspectorReferenceData { + _InspectorReferenceData(this.object); - /// The number of times the object has been referenced. + final Object object; int count = 1; - - /// The value. - Object? get value { - if (_ref != null) { - return _ref!.target; - } - return _value; - } } // Production implementation of [WidgetInspectorService]. @@ -770,9 +742,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 WeakMap _objectToId = WeakMap(); + final Map> _groups = >{}; + final Map _idToReferenceData = {}; + final Map _objectToId = Map.identity(); int _nextId = 0; /// The pubRootDirectories that are currently configured for the widget inspector. @@ -1298,22 +1270,20 @@ mixin WidgetInspectorService { /// references from a different group. @protected void disposeGroup(String name) { - final Set? references = _groups.remove(name); + final Set<_InspectorReferenceData>? 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 Object? value = reference.value; - if (value != null) { - _objectToId.remove(value); - } - _idToReferenceData.remove(reference.id); + final String? id = _objectToId.remove(reference.object); + assert(id != null); + _idToReferenceData.remove(id); } } @@ -1325,15 +1295,14 @@ mixin WidgetInspectorService { return null; } - final Set group = _groups.putIfAbsent(groupName, () => Set.identity()); + final Set<_InspectorReferenceData> group = _groups.putIfAbsent(groupName, () => Set<_InspectorReferenceData>.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, id); + referenceData = _InspectorReferenceData(object); _idToReferenceData[id] = referenceData; group.add(referenceData); } else { @@ -1363,11 +1332,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.value; + return data.object; } /// Returns the object to introspect to determine the source location of an @@ -1399,7 +1368,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')]); } @@ -3744,54 +3713,3 @@ 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 4e0d2378a0105..aa64ca4789d4b 100644 --- a/packages/flutter/test/widgets/widget_inspector_test.dart +++ b/packages/flutter/test/widgets/widget_inspector_test.dart @@ -9,9 +9,7 @@ @TestOn('!chrome') library; -import 'dart:async'; import 'dart:convert'; -import 'dart:developer'; import 'dart:math'; import 'dart:ui' as ui; @@ -242,67 +240,7 @@ 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(); } @@ -323,19 +261,6 @@ 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( @@ -3820,7 +3745,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { _CreationLocation location = knownLocations[id]!; expect(location.file, equals(file)); // ClockText widget. - expect(location.line, equals(57)); + expect(location.line, equals(55)); expect(location.column, equals(9)); expect(location.name, equals('ClockText')); expect(count, equals(1)); @@ -3830,7 +3755,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { location = knownLocations[id]!; expect(location.file, equals(file)); // Text widget in _ClockTextState build method. - expect(location.line, equals(95)); + expect(location.line, equals(93)); expect(location.column, equals(12)); expect(location.name, equals('Text')); expect(count, equals(1)); @@ -3857,7 +3782,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { location = knownLocations[id]!; expect(location.file, equals(file)); // ClockText widget. - expect(location.line, equals(57)); + expect(location.line, equals(55)); expect(location.column, equals(9)); expect(location.name, equals('ClockText')); expect(count, equals(3)); // 3 clock widget instances rebuilt. @@ -3867,7 +3792,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { location = knownLocations[id]!; expect(location.file, equals(file)); // Text widget in _ClockTextState build method. - expect(location.line, equals(95)); + expect(location.line, equals(93)); expect(location.column, equals(12)); expect(location.name, equals('Text')); expect(count, equals(3)); // 3 clock widget instances rebuilt.