Make structuredErrors to not mess up with onError (#88253)

packages/flutter/lib/src/foundation/assertions.dart

@@ -867,7 +867,9 @@ class FlutterError extends Error with DiagnosticableTreeMixin implements Asserti
   /// The default behavior is to call [presentError].
   ///
   /// You can set this to your own function to override this default behavior.
-  /// For example, you could report all errors to your server.
+  /// For example, you could report all errors to your server. Consider calling
+  /// [presentError] from your custom error handler in order to see the logs in
+  /// the console as well.
   ///
   /// If the error handler throws an exception, it will not be caught by the
   /// Flutter framework.
@@ -877,6 +879,11 @@ class FlutterError extends Error with DiagnosticableTreeMixin implements Asserti
   ///
   /// Do not call [onError] directly, instead, call [reportError], which
   /// forwards to [onError] if it is not null.
+  ///
+  /// See also:
+  ///
+  ///  * <https://flutter.dev/docs/testing/errors>, more information about error
+  ///    handling in Flutter.
   static FlutterExceptionHandler? onError = presentError;
 
   /// Called by the Flutter framework before attempting to parse a [StackTrace].

packages/flutter/lib/src/widgets/widget_inspector.dart

@@ -746,8 +746,6 @@ mixin WidgetInspectorService {
   bool _trackRebuildDirtyWidgets = false;
   bool _trackRepaintWidgets = false;
 
-  FlutterExceptionHandler? _structuredExceptionHandler;
-
   late RegisterServiceExtensionCallback _registerServiceExtensionCallback;
   /// Registers a service extension method with the given name (full
   /// name "ext.flutter.inspector.name").
@@ -920,7 +918,7 @@ mixin WidgetInspectorService {
 
   int _errorsSinceReload = 0;
 
-  void _reportError(FlutterErrorDetails details) {
+  void _reportStructuredError(FlutterErrorDetails details) {
     final Map<String, Object?> errorJson = _nodeToJson(
       details.toDiagnosticsNode(),
       InspectorSerializationDelegate(
@@ -981,9 +979,10 @@ mixin WidgetInspectorService {
   ///  * [BindingBase.initServiceExtensions], which explains when service
   ///    extensions can be used.
   void initServiceExtensions(RegisterServiceExtensionCallback registerServiceExtensionCallback) {
-    _structuredExceptionHandler = _reportError;
+    final FlutterExceptionHandler defaultExceptionHandler = FlutterError.presentError;
+
     if (isStructuredErrorsEnabled()) {
-      FlutterError.onError = _structuredExceptionHandler;
+      FlutterError.presentError = _reportStructuredError;
     }
     _registerServiceExtensionCallback = registerServiceExtensionCallback;
     assert(!_debugServiceExtensionsRegistered);
@@ -994,13 +993,11 @@ mixin WidgetInspectorService {
 
     SchedulerBinding.instance!.addPersistentFrameCallback(_onFrameStart);
 
-    final FlutterExceptionHandler defaultExceptionHandler = FlutterError.presentError;
-
     _registerBoolServiceExtension(
       name: 'structuredErrors',
-      getter: () async => FlutterError.presentError == _structuredExceptionHandler,
+      getter: () async => FlutterError.presentError == _reportStructuredError,
       setter: (bool value) {
-        FlutterError.presentError = value ? _structuredExceptionHandler! : defaultExceptionHandler;
+        FlutterError.presentError = value ? _reportStructuredError : defaultExceptionHandler;
         return Future<void>.value();
       },
     );

packages/flutter/test/widgets/widget_inspector_init_with_structured_error_test.dart

-// Copyright 2014 The Flutter Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-import 'package:flutter/foundation.dart';
-import 'package:flutter/material.dart';
-import 'package:flutter/widgets.dart';
-import 'package:flutter_test/flutter_test.dart';
-
-import 'widget_inspector_test_utils.dart';
-
-void main() {
-  StructuredErrorTestService.runTests();
-}
-
-class StructuredErrorTestService extends TestWidgetInspectorService {
-  @override
-  bool isStructuredErrorsEnabled() {
-    return true;
-  }
-
-  static void runTests() {
-    final StructuredErrorTestService service = StructuredErrorTestService();
-    WidgetInspectorService.instance = service;
-    FlutterExceptionHandler? testHandler;
-    FlutterExceptionHandler? inspectorServiceErrorHandler;
-
-    setUpAll(() {
-      inspectorServiceErrorHandler = FlutterError.onError;
-    });
-
-    setUp(() {
-      testHandler = FlutterError.onError;
-    });
-
-    testWidgets('ext.flutter.inspector.setStructuredErrors', (WidgetTester tester) async {
-      // The test framework resets FlutterError.onError, so we set it back to
-      // what it was after WidgetInspectorService::initServiceExtensions ran.
-      FlutterError.onError = inspectorServiceErrorHandler;
-
-      List<Map<Object, Object?>> flutterErrorEvents =
-          service.getEventsDispatched('Flutter.Error');
-      expect(flutterErrorEvents, hasLength(0));
-
-      // Create an error.
-      FlutterError.reportError(FlutterErrorDetails(
-        library: 'rendering library',
-        context: ErrorDescription('during layout'),
-        exception: StackTrace.current,
-      ));
-
-      // Validate that we received an error.
-      flutterErrorEvents = service.getEventsDispatched('Flutter.Error');
-      expect(flutterErrorEvents, hasLength(1));
-    });
-
-    tearDown(() {
-      FlutterError.onError = testHandler;
-    });
-  }
-}

packages/flutter/test/widgets/widget_inspector_structure_error_test.dart

@@ -2,79 +2,44 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import 'dart:async';
-import 'dart:convert';
-
 import 'package:flutter/foundation.dart';
 import 'package:flutter/widgets.dart';
 import 'package:flutter_test/flutter_test.dart';
 
+import 'widget_inspector_test_utils.dart';
+
 void main() {
   StructureErrorTestWidgetInspectorService.runTests();
 }
 
-typedef InspectorServiceExtensionCallback = FutureOr<Map<String, Object?>> Function(Map<String, String> parameters);
-
-class StructureErrorTestWidgetInspectorService extends Object with WidgetInspectorService {
-  final Map<String, InspectorServiceExtensionCallback> extensions = <String, InspectorServiceExtensionCallback>{};
-
-  final Map<String, List<Map<Object, Object?>>> eventsDispatched = <String, List<Map<Object, Object?>>>{};
-
-  @override
-  void registerServiceExtension({
-    required String name,
-    required FutureOr<Map<String, Object?>> Function(Map<String, String> parameters) callback,
-  }) {
-    assert(!extensions.containsKey(name));
-    extensions[name] = callback;
-  }
-
-  @override
-  void postEvent(String eventKind, Map<Object, Object?> eventData) {
-    getEventsDispatched(eventKind).add(eventData);
-  }
-
-  List<Map<Object, Object?>> getEventsDispatched(String eventKind) {
-    return eventsDispatched.putIfAbsent(eventKind, () => <Map<Object, Object?>>[]);
-  }
-
-  Iterable<Map<Object, Object?>> getServiceExtensionStateChangedEvents(String extensionName) {
-    return getEventsDispatched('Flutter.ServiceExtensionStateChanged')
-      .where((Map<Object, Object?> event) => event['extension'] == extensionName);
-  }
-
-  Future<String> testBoolExtension(String name, Map<String, String> arguments) async {
-    expect(extensions, contains(name));
-    // Encode and decode to JSON to match behavior using a real service
-    // extension where only JSON is allowed.
-    // ignore: avoid_dynamic_calls
-    return json.decode(json.encode(await extensions[name]!(arguments)))['enabled'] as String;
-  }
-
-
+class StructureErrorTestWidgetInspectorService extends TestWidgetInspectorService {
   static void runTests() {
     final StructureErrorTestWidgetInspectorService service = StructureErrorTestWidgetInspectorService();
     WidgetInspectorService.instance = service;
 
-    test('ext.flutter.inspector.structuredErrors reports error to _structuredExceptionHandler on error', () async {
+    test('ext.flutter.inspector.structuredErrors - custom FlutterError.onError', () async {
+      // Regression test for https://github.com/flutter/flutter/issues/41540
+
+      // Ensures that
+      //  * structured errors are enabled by default
+      //  * FlutterError.onError without FlutterError.presentError doesn't present structured errors
+      //  * FlutterError.onError with FlutterError.presentError does present structured errors
+      //  * disabling structured errors sets the default FlutterError.presentError
+
       final FlutterExceptionHandler? oldHandler = FlutterError.onError;
 
-      bool usingNewHandler = false;
-      // Creates a spy onError. This spy needs to be set before widgets binding
-      // initializes.
+      try {
+        expect(service.getEventsDispatched('Flutter.Error'), isEmpty);
+
+        // Set callback that doesn't call presentError.
+        bool onErrorCalled = false;
         FlutterError.onError = (FlutterErrorDetails details) {
-        usingNewHandler = true;
+          onErrorCalled = true;
         };
 
+        // Get the service registered.
         WidgetsFlutterBinding.ensureInitialized();
-      try {
-        // Enables structured errors.
-        expect(
-          await service.testBoolExtension('structuredErrors', <String, String>{'enabled': 'true'}),
-          equals('true'),
-        );
 
-        // Creates an error.
         final FlutterErrorDetails expectedError = FlutterErrorDetails(
           library: 'rendering library',
           context: ErrorDescription('during layout'),
@@ -82,13 +47,40 @@ class StructureErrorTestWidgetInspectorService extends Object with WidgetInspect
         );
         FlutterError.reportError(expectedError);
 
-        // For non-web apps, this validates the new handler did not receive an
-        // error because `FlutterError.onError` was set to
-        // `WidgetInspectorService._structuredExceptionHandler` when service
-        // extensions were initialized. For web apps, the new handler should
-        // have received an error because structured errors are disabled by
-        // default on the web.
-        expect(usingNewHandler, equals(kIsWeb));
+        // Verify structured errors are not shown.
+        expect(onErrorCalled, true);
+        expect(service.getEventsDispatched('Flutter.Error'), isEmpty);
+
+        // Set callback that calls presentError.
+        onErrorCalled = false;
+        FlutterError.onError = (FlutterErrorDetails details) {
+          FlutterError.presentError(details);
+          onErrorCalled = true;
+        };
+
+        FlutterError.reportError(expectedError);
+
+        // Verify structured errors are shown.
+        expect(onErrorCalled, true);
+        // Structured errors are not supported on web.
+        if (!kIsWeb) {
+          expect(service.getEventsDispatched('Flutter.Error'), hasLength(1));
+        } else {
+          expect(service.getEventsDispatched('Flutter.Error'), isEmpty);
+        }
+
+        // Verify disabling structured errors sets the default FlutterError.presentError
+        expect(
+          await service.testBoolExtension('structuredErrors', <String, String>{'enabled': 'true'}),
+          equals('true'),
+        );
+        expect(FlutterError.presentError, isNot(equals(oldHandler)));
+
+        expect(
+          await service.testBoolExtension('structuredErrors', <String, String>{'enabled': 'false'}),
+          equals('false'),
+        );
+        expect(FlutterError.presentError, equals(oldHandler));
       } finally {
         FlutterError.onError = oldHandler;
       }