Skip to content

F
Front-End
Unverified Commit 2830f073 authored by Jackson Gardner's avatar Jackson Gardner Committed by GitHub
Browse files
Options

Revert "Make inspector weakly referencing the inspected objects." (#128436) 

Reverts flutter/flutter#128095

We have a tree closure related to the `widget_inspector_test.dart`. I'd
like to see if reverting this resolves the issue.
parent 6848110f
Hide whitespace changes
Inline Side-by-side
Showing with 21 additions and 178 deletions
packages/flutter/lib/src/widgets/widget_inspector.dart
View file @ 2830f073
......@@ -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>(object);
}
WeakReference<Object>? _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<String, Set<InspectorReferenceData>> _groups = <String, Set<InspectorReferenceData>>{};
final Map<String, InspectorReferenceData> _idToReferenceData = <String, InspectorReferenceData>{};
final WeakMap<Object, String> _objectToId = WeakMap<Object, String>();
final Map<String, Set<_InspectorReferenceData>> _groups = <String, Set<_InspectorReferenceData>>{};
final Map<String, _InspectorReferenceData> _idToReferenceData = <String, _InspectorReferenceData>{};
final Map<Object, String> _objectToId = Map<Object, String>.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<InspectorReferenceData>? 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<InspectorReferenceData> group = _groups.putIfAbsent(groupName, () => Set<InspectorReferenceData>.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(<DiagnosticsNode>[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(<DiagnosticsNode>[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<K, V> {
Expando<Object> _objects = Expando<Object>();
/// Strings, numbers, booleans.
final Map<K, V> _primitives = <K, V>{};
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<Object>();
_primitives.clear();
}
}
packages/flutter/test/widgets/widget_inspector_test.dart
View file @ 2830f073
......@@ -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<void> _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<List<DateTime>> storage = <List<DateTime>>[];
void allocateMemory() {
storage.add(Iterable<DateTime>.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<void>.delayed(Duration.zero);
allocateMemory();
}
}
final List<Object> _weakValueTests = <Object>[1, 1.0, 'hello', true, false, Object(), <int>[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<ArgumentError>()));
});
});
group('$WeakMap', (){
for (final Object item in _weakValueTests) {
test('assigns and removes value, $item', () async {
final WeakMap<Object, Object> weakMap = WeakMap<Object, Object>();
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<Object, Object> weakMap = WeakMap<Object, Object>();
expect(weakMap[item], null);
});
}
});
_TestWidgetInspectorService.runTests();
}
......@@ -323,19 +261,6 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
}
});
test('WidgetInspector does not hold objects from GC', () async {
List<DateTime>? someObject = <DateTime>[DateTime.now(), DateTime.now()];
final String? id = service.toId(someObject, 'group_name');
expect(id, isNotNull);
final WeakReference<Object> ref = WeakReference<Object>(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.
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment

HIAST - GitLab