From 21bea32f6620e0f08e9e16fc31617fd88c20a902 Mon Sep 17 00:00:00 2001 From: Elliott Brooks <21270878+elliette@users.noreply.github.com> Date: Tue, 26 Nov 2024 13:22:23 -0800 Subject: [PATCH] [Widget Inspector] Fix stack overflow error for Flutter web when requesting a large widget tree (#159454) Fixes https://github.com/flutter/devtools/issues/8553 Context: A Flutter web customer with a large widget tree was getting a stack overflow error when they toggled on "show implementation widgets" in the Flutter DevTools Inspector. This is because building the JSON tree recursively was hitting Chrome's stack limit. This PR creates the JSON tree **iteratively** if the `getRootWidgetTree` service extension is called with `fullDetails = false` (which is what DevTools uses to fetch the widget tree). For all other instances of creating a widget JSON map (for example, when fetching widget properties) the recursive implementation is used. This allows properties provided by subclasses implementing `toJsonMap` to be included in the response. Note: Because with this change `toJsonMap` is only called when `fullDetails = true` and `toJsonMapIterative` is only called when `fullDetails = false`, this PR partially reverts the changes in https://github.com/flutter/flutter/pull/157309. --- .../lib/src/foundation/diagnostics.dart | 243 ++++++++++-------- packages/flutter/lib/src/painting/colors.dart | 13 +- .../flutter/lib/src/widgets/framework.dart | 11 +- .../flutter/lib/src/widgets/icon_data.dart | 13 +- .../lib/src/widgets/widget_inspector.dart | 18 +- .../foundation/diagnostics_json_test.dart | 22 +- .../test/widgets/widget_inspector_test.dart | 4 +- 7 files changed, 170 insertions(+), 154 deletions(-) diff --git a/packages/flutter/lib/src/foundation/diagnostics.dart b/packages/flutter/lib/src/foundation/diagnostics.dart index 5d50c0ef22..1cdb4c7fd4 100644 --- a/packages/flutter/lib/src/foundation/diagnostics.dart +++ b/packages/flutter/lib/src/foundation/diagnostics.dart @@ -8,6 +8,7 @@ /// @docImport 'package:flutter/widgets.dart'; library; +import 'dart:collection'; import 'dart:math' as math; import 'dart:ui' show clampDouble; @@ -1421,6 +1422,17 @@ class TextTreeRenderer { } } +/// The JSON representation of a [DiagnosticsNode]. +typedef _JsonDiagnosticsNode = Map; + +/// Stack containing [DiagnosticNode]s to convert to JSON and the callback to +/// call with the JSON. +/// +/// Using a stack is required to process the widget tree iteratively instead of +/// recursively. +typedef _NodesToJsonifyStack + = ListQueue<(DiagnosticsNode, void Function(_JsonDiagnosticsNode))>; + /// Defines diagnostics data for a [value]. /// /// For debug and profile modes, [DiagnosticsNode] provides a high quality @@ -1605,29 +1617,12 @@ abstract class DiagnosticsNode { /// by this method and interactive tree views in the Flutter IntelliJ /// plugin. @mustCallSuper - Map toJsonMap( - DiagnosticsSerializationDelegate delegate, { - bool fullDetails = true, - }) { + Map toJsonMap(DiagnosticsSerializationDelegate delegate) { Map result = {}; assert(() { final bool hasChildren = getChildren().isNotEmpty; - final Map essentialDetails = { + result = { 'description': toDescription(), - 'shouldIndent': style != DiagnosticsTreeStyle.flat && - style != DiagnosticsTreeStyle.error, - ...delegate.additionalNodeProperties(this, fullDetails: fullDetails), - if (delegate.subtreeDepth > 0) - 'children': toJsonList( - delegate.filterChildren(getChildren(), this), - this, - delegate, - fullDetails: fullDetails, - ), - }; - - result = !fullDetails ? essentialDetails : { - ...essentialDetails, 'type': runtimeType.toString(), if (name != null) 'name': name, @@ -1651,12 +1646,18 @@ abstract class DiagnosticsNode { 'allowWrap': allowWrap, if (allowNameWrap) 'allowNameWrap': allowNameWrap, + ...delegate.additionalNodeProperties(this), if (delegate.includeProperties) 'properties': toJsonList( delegate.filterProperties(getProperties(), this), this, delegate, - fullDetails: fullDetails, + ), + if (delegate.subtreeDepth > 0) + 'children': toJsonList( + delegate.filterChildren(getChildren(), this), + this, + delegate, ), }; return true; @@ -1664,6 +1665,35 @@ abstract class DiagnosticsNode { return result; } + /// Iteratively serialize the node to a JSON map according to the + /// configuration provided in the [DiagnosticsSerializationDelegate]. + /// + /// This is only used when [WidgetInspectorServiceExtensions.getRootWidgetTree] + /// is called with fullDetails=false. To get the full widget details, including + /// any details provided by subclasses, [toJsonMap] should be used instead. + /// + /// See https://github.com/flutter/devtools/issues/8553 for details about this + /// iterative approach. + Map toJsonMapIterative( + DiagnosticsSerializationDelegate delegate, + ) { + final _NodesToJsonifyStack childrenToJsonify = + ListQueue<(DiagnosticsNode, void Function(_JsonDiagnosticsNode))>(); + _JsonDiagnosticsNode result = {}; + assert(() { + result = _toJson( + delegate, + childrenToJsonify: childrenToJsonify, + ); + _jsonifyNextNodesInStack( + childrenToJsonify, + delegate: delegate, + ); + return true; + }()); + return result; + } + /// Serializes a [List] of [DiagnosticsNode]s to a JSON list according to /// the configuration provided by the [DiagnosticsSerializationDelegate]. /// @@ -1672,9 +1702,8 @@ abstract class DiagnosticsNode { static List> toJsonList( List? nodes, DiagnosticsNode? parent, - DiagnosticsSerializationDelegate delegate, { - bool fullDetails = true, - }) { + DiagnosticsSerializationDelegate delegate, + ) { bool truncated = false; if (nodes == null) { return const >[]; @@ -1685,11 +1714,9 @@ abstract class DiagnosticsNode { nodes.add(DiagnosticsNode.message('...')); truncated = true; } - final List> json = nodes.map>((DiagnosticsNode node) { - return node.toJsonMap( - delegate.delegateForNode(node), - fullDetails: fullDetails, - ); + final List<_JsonDiagnosticsNode> json = + nodes.map<_JsonDiagnosticsNode>((DiagnosticsNode node) { + return node.toJsonMap(delegate.delegateForNode(node)); }).toList(); if (truncated) { json.last['truncated'] = true; @@ -1803,6 +1830,73 @@ abstract class DiagnosticsNode { }()); return result; } + + void _jsonifyNextNodesInStack( + _NodesToJsonifyStack toJsonify, { + required DiagnosticsSerializationDelegate delegate, + }) { + while (toJsonify.isNotEmpty) { + final ( + DiagnosticsNode nextNode, + void Function(_JsonDiagnosticsNode) callback + ) = toJsonify.removeFirst(); + final _JsonDiagnosticsNode nodeAsJson = nextNode._toJson( + delegate, + childrenToJsonify: toJsonify, + ); + callback(nodeAsJson); + } + } + + Map _toJson( + DiagnosticsSerializationDelegate delegate, { + required _NodesToJsonifyStack childrenToJsonify, + }) { + final List<_JsonDiagnosticsNode> childrenJsonList = + <_JsonDiagnosticsNode>[]; + final bool includeChildren = + getChildren().isNotEmpty && delegate.subtreeDepth > 0; + + // Collect the children nodes to convert to JSON later. + bool truncated = false; + if (includeChildren) { + List childrenNodes = + delegate.filterChildren(getChildren(), this); + final int originalNodeCount = childrenNodes.length; + childrenNodes = delegate.truncateNodesList(childrenNodes, this); + if (childrenNodes.length != originalNodeCount) { + childrenNodes.add(DiagnosticsNode.message('...')); + truncated = true; + } + for (final DiagnosticsNode child in childrenNodes) { + childrenToJsonify.add(( + child, + (_JsonDiagnosticsNode jsonChild) { + childrenJsonList.add(jsonChild); + } + )); + } + } + + final String description = toDescription(); + final String widgetRuntimeType = + description == '[root]' ? 'RootWidget' : description.split('-').first; + final bool shouldIndent = style != DiagnosticsTreeStyle.flat && + style != DiagnosticsTreeStyle.error; + + return { + 'description': description, + 'shouldIndent': shouldIndent, + // TODO(elliette): This can be removed to reduce the JSON response even + // further once DevTools computes the widget runtime type from the + // description instead, see: + // https://github.com/flutter/devtools/issues/8556 + 'widgetRuntimeType': widgetRuntimeType, + 'truncated': truncated, + ...delegate.additionalNodeProperties(this, fullDetails: false), + if (includeChildren) 'children': childrenJsonList, + }; + } } /// Debugging message displayed like a property. @@ -1872,17 +1966,8 @@ class StringProperty extends DiagnosticsProperty { final bool quoted; @override - Map toJsonMap( - DiagnosticsSerializationDelegate delegate, { - bool fullDetails = true, - }) { - final Map json = super.toJsonMap( - delegate, - fullDetails: fullDetails, - ); - if (!fullDetails) { - return json; - } + Map toJsonMap(DiagnosticsSerializationDelegate delegate) { + final Map json = super.toJsonMap(delegate); json['quoted'] = quoted; return json; } @@ -1937,18 +2022,8 @@ abstract class _NumProperty extends DiagnosticsProperty { }) : super.lazy(); @override - Map toJsonMap( - DiagnosticsSerializationDelegate delegate, { - bool fullDetails = true, - }) { - final Map json = super.toJsonMap( - delegate, - fullDetails: fullDetails, - ); - if (!fullDetails) { - return json; - } - + Map toJsonMap(DiagnosticsSerializationDelegate delegate) { + final Map json = super.toJsonMap(delegate); if (unit != null) { json['unit'] = unit; } @@ -2131,17 +2206,8 @@ class FlagProperty extends DiagnosticsProperty { ); @override - Map toJsonMap( - DiagnosticsSerializationDelegate delegate, { - bool fullDetails = true, - }) { - final Map json = super.toJsonMap( - delegate, - fullDetails: fullDetails, - ); - if (!fullDetails) { - return json; - } + Map toJsonMap(DiagnosticsSerializationDelegate delegate) { + final Map json = super.toJsonMap(delegate); if (ifTrue != null) { json['ifTrue'] = ifTrue; } @@ -2262,17 +2328,8 @@ class IterableProperty extends DiagnosticsProperty> { } @override - Map toJsonMap( - DiagnosticsSerializationDelegate delegate, { - bool fullDetails = true, - }) { - final Map json = super.toJsonMap( - delegate, - fullDetails: fullDetails, - ); - if (!fullDetails) { - return json; - } + Map toJsonMap(DiagnosticsSerializationDelegate delegate) { + final Map json = super.toJsonMap(delegate); if (value != null) { json['values'] = value!.map((T value) => value.toString()).toList(); } @@ -2409,17 +2466,8 @@ class ObjectFlagProperty extends DiagnosticsProperty { } @override - Map toJsonMap( - DiagnosticsSerializationDelegate delegate, { - bool fullDetails = true, - }) { - final Map json = super.toJsonMap( - delegate, - fullDetails: fullDetails, - ); - if (!fullDetails) { - return json; - } + Map toJsonMap(DiagnosticsSerializationDelegate delegate) { + final Map json = super.toJsonMap(delegate); if (ifPresent != null) { json['ifPresent'] = ifPresent; } @@ -2496,17 +2544,8 @@ class FlagsSummary extends DiagnosticsProperty> { } @override - Map toJsonMap( - DiagnosticsSerializationDelegate delegate, { - bool fullDetails = true, - }) { - final Map json = super.toJsonMap( - delegate, - fullDetails: fullDetails, - ); - if (!fullDetails) { - return json; - } + Map toJsonMap(DiagnosticsSerializationDelegate delegate) { + final Map json = super.toJsonMap(delegate); if (value.isNotEmpty) { json['values'] = _formattedValues().toList(); } @@ -2625,10 +2664,7 @@ class DiagnosticsProperty extends DiagnosticsNode { final bool allowNameWrap; @override - Map toJsonMap( - DiagnosticsSerializationDelegate delegate, { - bool fullDetails = true, - }) { + Map toJsonMap(DiagnosticsSerializationDelegate delegate) { final T? v = value; List>? properties; if (delegate.expandPropertyValues && delegate.includeProperties && v is Diagnosticable && getProperties().isEmpty) { @@ -2638,16 +2674,9 @@ class DiagnosticsProperty extends DiagnosticsNode { delegate.filterProperties(v.toDiagnosticsNode().getProperties(), this), this, delegate, - fullDetails: fullDetails, ); } - final Map json = super.toJsonMap( - delegate, - fullDetails: fullDetails, - ); - if (!fullDetails) { - return json; - } + final Map json = super.toJsonMap(delegate); if (properties != null) { json['properties'] = properties; } diff --git a/packages/flutter/lib/src/painting/colors.dart b/packages/flutter/lib/src/painting/colors.dart index b748626576..a897c0b33d 100644 --- a/packages/flutter/lib/src/painting/colors.dart +++ b/packages/flutter/lib/src/painting/colors.dart @@ -495,17 +495,8 @@ class ColorProperty extends DiagnosticsProperty { }); @override - Map toJsonMap( - DiagnosticsSerializationDelegate delegate, { - bool fullDetails = true, - }) { - final Map json = super.toJsonMap( - delegate, - fullDetails: fullDetails, - ); - if (!fullDetails) { - return json; - } + Map toJsonMap(DiagnosticsSerializationDelegate delegate) { + final Map json = super.toJsonMap(delegate); if (value != null) { json['valueProperties'] = { 'red': value!.red, diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index e0d5d9377f..8867da3503 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -5382,18 +5382,13 @@ class _ElementDiagnosticableTreeNode extends DiagnosticableTreeNode { final bool stateful; @override - Map toJsonMap( - DiagnosticsSerializationDelegate delegate, { - bool fullDetails = true, - }) { - final Map json = super.toJsonMap(delegate, fullDetails: fullDetails,); + Map toJsonMap(DiagnosticsSerializationDelegate delegate) { + final Map json = super.toJsonMap(delegate); final Element element = value as Element; if (!element.debugIsDefunct) { json['widgetRuntimeType'] = element.widget.runtimeType.toString(); } - if (fullDetails) { - json['stateful'] = stateful; - } + json['stateful'] = stateful; return json; } } diff --git a/packages/flutter/lib/src/widgets/icon_data.dart b/packages/flutter/lib/src/widgets/icon_data.dart index d13ce97863..ca118126d1 100644 --- a/packages/flutter/lib/src/widgets/icon_data.dart +++ b/packages/flutter/lib/src/widgets/icon_data.dart @@ -121,17 +121,8 @@ class IconDataProperty extends DiagnosticsProperty { }); @override - Map toJsonMap( - DiagnosticsSerializationDelegate delegate, { - bool fullDetails = true, - }) { - final Map json = super.toJsonMap( - delegate, - fullDetails: fullDetails, - ); - if (!fullDetails) { - return json; - } + Map toJsonMap(DiagnosticsSerializationDelegate delegate) { + final Map json = super.toJsonMap(delegate); if (value != null) { json['valueProperties'] = { 'codePoint': value!.codePoint, diff --git a/packages/flutter/lib/src/widgets/widget_inspector.dart b/packages/flutter/lib/src/widgets/widget_inspector.dart index 6e1bae2f9c..8d80e50efc 100644 --- a/packages/flutter/lib/src/widgets/widget_inspector.dart +++ b/packages/flutter/lib/src/widgets/widget_inspector.dart @@ -1754,7 +1754,15 @@ mixin WidgetInspectorService { bool fullDetails = true, } ) { - return node?.toJsonMap(delegate, fullDetails: fullDetails); + if (fullDetails) { + return node?.toJsonMap(delegate); + } else { + // If we don't need the full details fetched from all the subclasses, we + // can iteratively build the JSON map. This prevents a stack overflow + // exception for particularly large widget trees. For details, see: + // https://github.com/flutter/devtools/issues/8553 + return node?.toJsonMapIterative(delegate); + } } bool _isValueCreatedByLocalProject(Object? value) { @@ -1824,14 +1832,8 @@ mixin WidgetInspectorService { List nodes, InspectorSerializationDelegate delegate, { required DiagnosticsNode? parent, - bool fullDetails = true, }) { - return DiagnosticsNode.toJsonList( - nodes, - parent, - delegate, - fullDetails: fullDetails, - ); + return DiagnosticsNode.toJsonList(nodes, parent, delegate); } /// Returns a JSON representation of the properties of the [DiagnosticsNode] diff --git a/packages/flutter/test/foundation/diagnostics_json_test.dart b/packages/flutter/test/foundation/diagnostics_json_test.dart index 867605d62c..67a517e3d7 100644 --- a/packages/flutter/test/foundation/diagnostics_json_test.dart +++ b/packages/flutter/test/foundation/diagnostics_json_test.dart @@ -24,10 +24,15 @@ void main() { }); group('Serialization', () { - const List essentialDiagnosticKeys = [ + // These are always included. + const List defaultDiagnosticKeys = [ 'description', + ]; + // These are only included when fullDetails = false. + const List essentialDiagnosticKeys = [ 'shouldIndent', ]; + // These are only included with fullDetails = true. const List detailedDiagnosticKeys = [ 'type', 'hasChildren', @@ -81,7 +86,7 @@ void main() { expect(result.containsKey('properties'), isFalse); expect(result.containsKey('children'), isFalse); - for (final String keyName in essentialDiagnosticKeys) { + for (final String keyName in defaultDiagnosticKeys) { expect( result.containsKey(keyName), isTrue, @@ -97,15 +102,20 @@ void main() { } }); - test('without full details', () { + test('iterative implementation (without full details)', () { final Map result = testTree .toDiagnosticsNode() - .toJsonMap( - const DiagnosticsSerializationDelegate(), fullDetails: false + .toJsonMapIterative(const DiagnosticsSerializationDelegate() ); expect(result.containsKey('properties'), isFalse); expect(result.containsKey('children'), isFalse); - + for (final String keyName in defaultDiagnosticKeys) { + expect( + result.containsKey(keyName), + isTrue, + reason: '$keyName is included.', + ); + } for (final String keyName in essentialDiagnosticKeys) { expect( result.containsKey(keyName), diff --git a/packages/flutter/test/widgets/widget_inspector_test.dart b/packages/flutter/test/widgets/widget_inspector_test.dart index d1f2978181..55a84ecad5 100644 --- a/packages/flutter/test/widgets/widget_inspector_test.dart +++ b/packages/flutter/test/widgets/widget_inspector_test.dart @@ -2177,7 +2177,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { /// Gets the children nodes from the JSON response. List childrenFromJsonResponse(Map json) { - return json['children']! as List; + return (json['children'] as List?) ?? []; } /// Gets the children nodes using a call to @@ -2571,7 +2571,6 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { ), isTrue, ); - expect( allChildrenSatisfyCondition(rootJson, condition: wasCreatedByLocalProject, @@ -5758,7 +5757,6 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { node.toJsonMap(const DiagnosticsSerializationDelegate()), equals({ 'description': 'description of the deep link', - 'shouldIndent': true, 'type': 'DevToolsDeepLinkProperty', 'name': '', 'style': 'singleLine',