Unverified Commit ac8e1f8f authored by Michael Goderbauer's avatar Michael Goderbauer Committed by GitHub

Reduce severity of memory leak when BuildContext/Element is retained (#79957)

parent 6db88907
...@@ -3085,8 +3085,21 @@ abstract class Element extends DiagnosticableTree implements BuildContext { ...@@ -3085,8 +3085,21 @@ abstract class Element extends DiagnosticableTree implements BuildContext {
/// The configuration for this element. /// The configuration for this element.
@override @override
Widget get widget => _widget; Widget get widget => _widget!;
Widget _widget; Widget? _widget;
/// Returns true if the Element is defunct.
///
/// This getter always returns false in profile and release builds.
/// See the lifecycle documentation for [Element] for additional information.
bool get debugIsDefunct {
bool isDefunct = false;
assert(() {
isDefunct = _lifecycleState == _ElementLifecycle.defunct;
return true;
}());
return isDefunct;
}
/// The object that manages the lifecycle of this element. /// The object that manages the lifecycle of this element.
@override @override
...@@ -3801,10 +3814,14 @@ abstract class Element extends DiagnosticableTree implements BuildContext { ...@@ -3801,10 +3814,14 @@ abstract class Element extends DiagnosticableTree implements BuildContext {
assert(depth != null); assert(depth != null);
assert(owner != null); assert(owner != null);
// Use the private property to avoid a CastError during hot reload. // Use the private property to avoid a CastError during hot reload.
final Key? key = _widget.key; final Key? key = _widget!.key;
if (key is GlobalKey) { if (key is GlobalKey) {
owner!._unregisterGlobalKey(key, this); owner!._unregisterGlobalKey(key, this);
} }
// Release resources to reduce the severity of memory leaks caused by
// defunct, but accidentally retained Elements.
_widget = null;
_dependencies = null;
_lifecycleState = _ElementLifecycle.defunct; _lifecycleState = _ElementLifecycle.defunct;
} }
...@@ -4116,7 +4133,7 @@ abstract class Element extends DiagnosticableTree implements BuildContext { ...@@ -4116,7 +4133,7 @@ abstract class Element extends DiagnosticableTree implements BuildContext {
/// A short, textual description of this element. /// A short, textual description of this element.
@override @override
String toStringShort() => widget.toStringShort(); String toStringShort() => _widget?.toStringShort() ?? '${describeIdentity(this)}(DEFUNCT)';
@override @override
DiagnosticsNode toDiagnosticsNode({ String? name, DiagnosticsTreeStyle? style }) { DiagnosticsNode toDiagnosticsNode({ String? name, DiagnosticsTreeStyle? style }) {
...@@ -4134,9 +4151,9 @@ abstract class Element extends DiagnosticableTree implements BuildContext { ...@@ -4134,9 +4151,9 @@ abstract class Element extends DiagnosticableTree implements BuildContext {
if (_lifecycleState != _ElementLifecycle.initial) { if (_lifecycleState != _ElementLifecycle.initial) {
properties.add(ObjectFlagProperty<int>('depth', depth, ifNull: 'no depth')); properties.add(ObjectFlagProperty<int>('depth', depth, ifNull: 'no depth'));
} }
properties.add(ObjectFlagProperty<Widget>('widget', widget, ifNull: 'no widget')); properties.add(ObjectFlagProperty<Widget>('widget', _widget, ifNull: 'no widget'));
properties.add(DiagnosticsProperty<Key>('key', widget.key, showName: false, defaultValue: null, level: DiagnosticLevel.hidden)); properties.add(DiagnosticsProperty<Key>('key', _widget?.key, showName: false, defaultValue: null, level: DiagnosticLevel.hidden));
widget.debugFillProperties(properties); _widget?.debugFillProperties(properties);
properties.add(FlagProperty('dirty', value: dirty, ifTrue: 'dirty')); properties.add(FlagProperty('dirty', value: dirty, ifTrue: 'dirty'));
if (_dependencies != null && _dependencies!.isNotEmpty) { if (_dependencies != null && _dependencies!.isNotEmpty) {
final List<DiagnosticsNode> diagnosticsDependencies = _dependencies! final List<DiagnosticsNode> diagnosticsDependencies = _dependencies!
...@@ -4296,7 +4313,9 @@ class _ElementDiagnosticableTreeNode extends DiagnosticableTreeNode { ...@@ -4296,7 +4313,9 @@ class _ElementDiagnosticableTreeNode extends DiagnosticableTreeNode {
Map<String, Object?> toJsonMap(DiagnosticsSerializationDelegate delegate) { Map<String, Object?> toJsonMap(DiagnosticsSerializationDelegate delegate) {
final Map<String, Object?> json = super.toJsonMap(delegate); final Map<String, Object?> json = super.toJsonMap(delegate);
final Element element = value as Element; final Element element = value as Element;
json['widgetRuntimeType'] = element.widget.runtimeType.toString(); if (!element.debugIsDefunct) {
json['widgetRuntimeType'] = element.widget.runtimeType.toString();
}
json['stateful'] = stateful; json['stateful'] = stateful;
return json; return json;
} }
...@@ -4660,7 +4679,7 @@ class StatelessElement extends ComponentElement { ...@@ -4660,7 +4679,7 @@ class StatelessElement extends ComponentElement {
class StatefulElement extends ComponentElement { class StatefulElement extends ComponentElement {
/// Creates an element that uses the given widget as its configuration. /// Creates an element that uses the given widget as its configuration.
StatefulElement(StatefulWidget widget) StatefulElement(StatefulWidget widget)
: state = widget.createState(), : _state = widget.createState(),
super(widget) { super(widget) {
assert(() { assert(() {
if (!state._debugTypesAreRight(widget)) { if (!state._debugTypesAreRight(widget)) {
...@@ -4695,7 +4714,8 @@ class StatefulElement extends ComponentElement { ...@@ -4695,7 +4714,8 @@ class StatefulElement extends ComponentElement {
/// There is a one-to-one relationship between [State] objects and the /// There is a one-to-one relationship between [State] objects and the
/// [StatefulElement] objects that hold them. The [State] objects are created /// [StatefulElement] objects that hold them. The [State] objects are created
/// by [StatefulElement] in [mount]. /// by [StatefulElement] in [mount].
final State<StatefulWidget> state; State<StatefulWidget> get state => _state!;
State<StatefulWidget>? _state;
@override @override
void reassemble() { void reassemble() {
...@@ -4810,6 +4830,9 @@ class StatefulElement extends ComponentElement { ...@@ -4810,6 +4830,9 @@ class StatefulElement extends ComponentElement {
]); ]);
}()); }());
state._element = null; state._element = null;
// Release resources to reduce the severity of memory leaks caused by
// defunct, but accidentally retained Elements.
_state = null;
} }
@override @override
...@@ -4895,7 +4918,7 @@ class StatefulElement extends ComponentElement { ...@@ -4895,7 +4918,7 @@ class StatefulElement extends ComponentElement {
@override @override
void debugFillProperties(DiagnosticPropertiesBuilder properties) { void debugFillProperties(DiagnosticPropertiesBuilder properties) {
super.debugFillProperties(properties); super.debugFillProperties(properties);
properties.add(DiagnosticsProperty<State<StatefulWidget>>('state', state, defaultValue: null)); properties.add(DiagnosticsProperty<State<StatefulWidget>>('state', _state, defaultValue: null));
} }
} }
...@@ -5705,13 +5728,14 @@ abstract class RenderObjectElement extends Element { ...@@ -5705,13 +5728,14 @@ abstract class RenderObjectElement extends Element {
@override @override
void unmount() { void unmount() {
final RenderObjectWidget oldWidget = widget;
super.unmount(); super.unmount();
assert( assert(
!renderObject.attached, !renderObject.attached,
'A RenderObject was still attached when attempting to unmount its ' 'A RenderObject was still attached when attempting to unmount its '
'RenderObjectElement: $renderObject', 'RenderObjectElement: $renderObject',
); );
widget.didUnmountRenderObject(renderObject); oldWidget.didUnmountRenderObject(renderObject);
} }
void _updateParentData(ParentDataWidget<ParentData> parentDataWidget) { void _updateParentData(ParentDataWidget<ParentData> parentDataWidget) {
......
...@@ -1205,6 +1205,7 @@ mixin WidgetInspectorService { ...@@ -1205,6 +1205,7 @@ mixin WidgetInspectorService {
/// ///
/// Use this method only for testing to ensure that object references from one /// Use this method only for testing to ensure that object references from one
/// test case do not impact other test cases. /// test case do not impact other test cases.
@visibleForTesting
@protected @protected
void disposeAllGroups() { void disposeAllGroups() {
_groups.clear(); _groups.clear();
...@@ -1213,6 +1214,19 @@ mixin WidgetInspectorService { ...@@ -1213,6 +1214,19 @@ mixin WidgetInspectorService {
_nextId = 0; _nextId = 0;
} }
/// Reset all InspectorService state.
///
/// Use this method only for testing to write hermetic tests for
/// WidgetInspectorService.
@visibleForTesting
@protected
@mustCallSuper
void resetAllState() {
disposeAllGroups();
selection.clear();
setPubRootDirectories(<String>[]);
}
/// Free all references to objects in a group. /// Free all references to objects in a group.
/// ///
/// Objects and their associated ids in the group may be kept alive by /// Objects and their associated ids in the group may be kept alive by
...@@ -2413,7 +2427,8 @@ class InspectorSelection { ...@@ -2413,7 +2427,8 @@ class InspectorSelection {
/// Setting [candidates] or calling [clear] resets the selection. /// Setting [candidates] or calling [clear] resets the selection.
/// ///
/// Returns null if the selection is invalid. /// Returns null if the selection is invalid.
RenderObject? get current => _current; RenderObject? get current => active ? _current : null;
RenderObject? _current; RenderObject? _current;
set current(RenderObject? value) { set current(RenderObject? value) {
if (_current != value) { if (_current != value) {
...@@ -2427,7 +2442,10 @@ class InspectorSelection { ...@@ -2427,7 +2442,10 @@ class InspectorSelection {
/// Setting [candidates] or calling [clear] resets the selection. /// Setting [candidates] or calling [clear] resets the selection.
/// ///
/// Returns null if the selection is invalid. /// Returns null if the selection is invalid.
Element? get currentElement => _currentElement; Element? get currentElement {
return _currentElement?.debugIsDefunct ?? true ? null : _currentElement;
}
Element? _currentElement; Element? _currentElement;
set currentElement(Element? element) { set currentElement(Element? element) {
if (currentElement != element) { if (currentElement != element) {
...@@ -3039,7 +3057,7 @@ String? _describeCreationLocation(Object object) { ...@@ -3039,7 +3057,7 @@ String? _describeCreationLocation(Object object) {
/// ///
/// Currently creation locations are only available for [Widget] and [Element]. /// Currently creation locations are only available for [Widget] and [Element].
_Location? _getCreationLocation(Object? object) { _Location? _getCreationLocation(Object? object) {
final Object? candidate = object is Element ? object.widget : object; final Object? candidate = object is Element && !object.debugIsDefunct ? object.widget : object;
return candidate is _HasCreationLocation ? candidate._location : null; return candidate is _HasCreationLocation ? candidate._location : null;
} }
......
...@@ -1572,6 +1572,22 @@ void main() { ...@@ -1572,6 +1572,22 @@ void main() {
await tester.pumpWidget(Container()); await tester.pumpWidget(Container());
expect(tester.binding.buildOwner!.globalKeyCount, initialCount + 0); expect(tester.binding.buildOwner!.globalKeyCount, initialCount + 0);
}); });
testWidgets('Widget and State properties are nulled out when unmounted', (WidgetTester tester) async {
await tester.pumpWidget(const _StatefulLeaf());
final StatefulElement element = tester.element<StatefulElement>(find.byType(_StatefulLeaf));
expect(element.state, isA<State<_StatefulLeaf>>());
expect(element.widget, isA<_StatefulLeaf>());
// Replace the widget tree to unmount the element.
await tester.pumpWidget(Container());
// Accessing state/widget now throws a CastError because they have been
// nulled out to reduce severity of memory leaks when an Element (e.g. in
// the form of a BuildContext) is retained past its useful life. See also
// https://github.com/flutter/flutter/issues/79605 for examples why this may
// occur.
expect(() => element.state, throwsA(isA<TypeError>()));
expect(() => element.widget, throwsA(isA<TypeError>()));
}, skip: kIsWeb);
} }
class _WidgetWithNoVisitChildren extends StatelessWidget { class _WidgetWithNoVisitChildren extends StatelessWidget {
......
...@@ -233,6 +233,10 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { ...@@ -233,6 +233,10 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
final TestWidgetInspectorService service = TestWidgetInspectorService(); final TestWidgetInspectorService service = TestWidgetInspectorService();
WidgetInspectorService.instance = service; WidgetInspectorService.instance = service;
tearDown(() {
service.resetAllState();
});
testWidgets('WidgetInspector smoke test', (WidgetTester tester) async { testWidgets('WidgetInspector smoke test', (WidgetTester tester) async {
// This is a smoke test to verify that adding the inspector doesn't crash. // This is a smoke test to verify that adding the inspector doesn't crash.
await tester.pumpWidget( await tester.pumpWidget(
...@@ -759,6 +763,61 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { ...@@ -759,6 +763,61 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
expect(service.selection.currentElement, equals(elementA)); expect(service.selection.currentElement, equals(elementA));
}); });
testWidgets('WidgetInspectorService defunct selection regression test', (WidgetTester tester) async {
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: Stack(
children: const <Widget>[
Text('a', textDirection: TextDirection.ltr),
],
),
),
);
final Element elementA = find.text('a').evaluate().first;
service.setSelection(elementA);
expect(service.selection.currentElement, equals(elementA));
expect(service.selection.current, equals(elementA.renderObject));
await tester.pumpWidget(
const SizedBox(
child: Text('b', textDirection: TextDirection.ltr),
),
);
// Selection is now empty as the element is defunct.
expect(service.selection.currentElement, equals(null));
expect(service.selection.current, equals(null));
// Verify that getting the debug creation location of the defunct element
// does not crash.
expect(debugIsLocalCreationLocation(elementA), isFalse);
// Verify that generating json for a defunct element does not crash.
expect(
elementA.toDiagnosticsNode().toJsonMap(
InspectorSerializationDelegate(
service: service,
summaryTree: false,
includeProperties: true,
addAdditionalPropertiesCallback: null,
),
),
isNotNull,
);
final Element elementB = find.text('b').evaluate().first;
service.setSelection(elementB);
expect(service.selection.currentElement, equals(elementB));
expect(service.selection.current, equals(elementB.renderObject));
// Set selection back to a defunct element.
service.setSelection(elementA);
expect(service.selection.currentElement, equals(null));
expect(service.selection.current, equals(null));
});
testWidgets('WidgetInspectorService getParentChain', (WidgetTester tester) async { testWidgets('WidgetInspectorService getParentChain', (WidgetTester tester) async {
const String group = 'test-group'; const String group = 'test-group';
...@@ -934,6 +993,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { ...@@ -934,6 +993,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
), ),
); );
final Element elementA = find.text('a').evaluate().first; final Element elementA = find.text('a').evaluate().first;
service.setSelection(elementA, 'my-group');
late String pubRootTest; late String pubRootTest;
if (widgetTracked) { if (widgetTracked) {
final Map<String, Object?> jsonObject = json.decode( final Map<String, Object?> jsonObject = json.decode(
...@@ -1047,6 +1107,8 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { ...@@ -1047,6 +1107,8 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
activeDevToolsServerAddress = 'http://127.0.0.1:9100'; activeDevToolsServerAddress = 'http://127.0.0.1:9100';
connectedVmServiceUri = 'http://127.0.0.1:55269/798ay5al_FM=/'; connectedVmServiceUri = 'http://127.0.0.1:55269/798ay5al_FM=/';
setupDefaultPubRootDirectory(service);
await tester.pumpWidget( await tester.pumpWidget(
Directionality( Directionality(
textDirection: TextDirection.ltr, textDirection: TextDirection.ltr,
...@@ -1079,6 +1141,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { ...@@ -1079,6 +1141,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
testWidgets('test transformDebugCreator will not add DevToolsDeepLinkProperty for non-overflow errors', (WidgetTester tester) async { testWidgets('test transformDebugCreator will not add DevToolsDeepLinkProperty for non-overflow errors', (WidgetTester tester) async {
activeDevToolsServerAddress = 'http://127.0.0.1:9100'; activeDevToolsServerAddress = 'http://127.0.0.1:9100';
connectedVmServiceUri = 'http://127.0.0.1:55269/798ay5al_FM=/'; connectedVmServiceUri = 'http://127.0.0.1:55269/798ay5al_FM=/';
setupDefaultPubRootDirectory(service);
await tester.pumpWidget( await tester.pumpWidget(
Directionality( Directionality(
...@@ -1110,6 +1173,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { ...@@ -1110,6 +1173,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
testWidgets('test transformDebugCreator will not add DevToolsDeepLinkProperty if devtoolsServerAddress is unavailable', (WidgetTester tester) async { testWidgets('test transformDebugCreator will not add DevToolsDeepLinkProperty if devtoolsServerAddress is unavailable', (WidgetTester tester) async {
activeDevToolsServerAddress = null; activeDevToolsServerAddress = null;
connectedVmServiceUri = 'http://127.0.0.1:55269/798ay5al_FM=/'; connectedVmServiceUri = 'http://127.0.0.1:55269/798ay5al_FM=/';
setupDefaultPubRootDirectory(service);
await tester.pumpWidget( await tester.pumpWidget(
Directionality( Directionality(
...@@ -2782,6 +2846,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { ...@@ -2782,6 +2846,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
), ),
), ),
); );
service.setSelection(find.text('Hello, World!').evaluate().first, 'my-group');
// Figure out the pubRootDirectory // Figure out the pubRootDirectory
final Map<String, Object?> jsonObject = (await service.testExtension( final Map<String, Object?> jsonObject = (await service.testExtension(
...@@ -2855,10 +2920,13 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { ...@@ -2855,10 +2920,13 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
final Map<String, Object> additionalJson = <String, Object>{}; final Map<String, Object> additionalJson = <String, Object>{};
final Object? value = node.value; final Object? value = node.value;
if (value is Element) { if (value is Element) {
additionalJson['renderObject'] = final RenderObject? renderObject = value.renderObject;
value.renderObject!.toDiagnosticsNode().toJsonMap( if (renderObject != null) {
delegate.copyWith(subtreeDepth: 0), additionalJson['renderObject'] =
); renderObject.toDiagnosticsNode().toJsonMap(
delegate.copyWith(subtreeDepth: 0),
);
}
} }
additionalJson['callbackExecuted'] = true; additionalJson['callbackExecuted'] = true;
return additionalJson; return additionalJson;
...@@ -2892,6 +2960,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { ...@@ -2892,6 +2960,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
}); });
testWidgets('debugIsLocalCreationLocation test', (WidgetTester tester) async { testWidgets('debugIsLocalCreationLocation test', (WidgetTester tester) async {
setupDefaultPubRootDirectory(service);
final GlobalKey key = GlobalKey(); final GlobalKey key = GlobalKey();
...@@ -2953,6 +3022,23 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { ...@@ -2953,6 +3022,23 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
); );
}); });
} }
static void setupDefaultPubRootDirectory(TestWidgetInspectorService service) {
final Map<String, Object?> jsonObject = const SizedBox().toDiagnosticsNode().toJsonMap(InspectorSerializationDelegate(service: service));
final Map<String, Object?> creationLocation = jsonObject['creationLocation']! as Map<String, Object?>;
expect(creationLocation, isNotNull);
final String file = creationLocation['file']! as String;
expect(file, endsWith('widget_inspector_test.dart'));
final List<String> segments = Uri
.parse(file)
.pathSegments;
final String pubRootTest = '/' +
segments.take(segments.length - 2).join('/');
// Strip a couple subdirectories away to generate a plausible pub root
// directory.
service.setPubRootDirectories(<String>[pubRootTest]);
}
} }
void addToKnownLocationsMap({ void addToKnownLocationsMap({
......
...@@ -62,4 +62,11 @@ class TestWidgetInspectorService extends Object with WidgetInspectorService { ...@@ -62,4 +62,11 @@ class TestWidgetInspectorService extends Object with WidgetInspectorService {
binding.buildOwner!.reassemble(binding.renderViewElement!); binding.buildOwner!.reassemble(binding.renderViewElement!);
} }
} }
@override
void resetAllState() {
super.resetAllState();
eventsDispatched.clear();
rebuildCount = 0;
}
} }
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