Fix: Multiple FlutterError.onError calls in FlutterErrorIntegration (#…

…345)
getsentry · Mar 10, 2021 · 15e1f53 · 15e1f53
1 parent 5eb036c
commit 15e1f53
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 22 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,7 @@
 # Unreleased
 
 * Bump: sentry-android to v4.3.0 (#343)
+* Fix: Multiple FlutterError.onError calls in FlutterErrorIntegration (#345)
 
 # 4.1.0-nullsafety.0
 

diff --git a/flutter/lib/src/default_integrations.dart b/flutter/lib/src/default_integrations.dart
@@ -34,11 +34,16 @@ class WidgetsFlutterBindingIntegration
 ///     and are stripped in release mode. See [Flutter build modes](https://flutter.dev/docs/testing/build-modes).
 ///     So they only get caught in debug mode.
 class FlutterErrorIntegration extends Integration<SentryFlutterOptions> {
+  /// Reference to the original handler.
+  FlutterExceptionHandler? _defaultOnError;
+
+  /// The error handler set by this integration.
+  FlutterExceptionHandler? _integrationOnError;
+
   @override
   void call(Hub hub, SentryFlutterOptions options) {
-    final defaultOnError = FlutterError.onError;
-
-    FlutterError.onError = (FlutterErrorDetails errorDetails) async {
+    _defaultOnError = FlutterError.onError;
+    _integrationOnError = (FlutterErrorDetails errorDetails) async {
       dynamic exception = errorDetails.exception;
 
       options.logger(
@@ -59,8 +64,8 @@ class FlutterErrorIntegration extends Integration<SentryFlutterOptions> {
         await hub.captureEvent(event, stackTrace: errorDetails.stack);
 
         // call original handler
-        if (defaultOnError != null) {
-          defaultOnError(errorDetails);
+        if (_defaultOnError != null) {
+          _defaultOnError!(errorDetails);
         }
 
         // we don't call Zone.current.handleUncaughtError because we'd like
@@ -73,9 +78,21 @@ class FlutterErrorIntegration extends Integration<SentryFlutterOptions> {
             'if you wish to capture silent errors');
       }
     };
+    FlutterError.onError = _integrationOnError;
 
     options.sdk.addIntegration('flutterErrorIntegration');
   }
+
+  @override
+  void close() {
+    /// Restore default if the integration error is still set.
+    if (FlutterError.onError == _integrationOnError) {
+      FlutterError.onError = _defaultOnError;
+      _defaultOnError = null;
+      _integrationOnError = null;
+    }
+    super.close();
+  }
 }
 
 /// Load Device's Contexts from the iOS SDK.

diff --git a/flutter/test/default_integrations_test.dart b/flutter/test/default_integrations_test.dart
@@ -48,18 +48,19 @@ void main() {
 
   test('FlutterError capture errors', () async {
     final exception = StateError('error');
+
     _reportError(exception: exception);
 
     final event = verify(
       await fixture.hub.captureEvent(captureAny),
     ).captured.first as SentryEvent;
 
-    expect(SentryLevel.fatal, event.level);
+    expect(event.level, SentryLevel.fatal);
 
     final throwableMechanism = event.throwableMechanism as ThrowableMechanism;
-    expect('FlutterError', throwableMechanism.mechanism.type);
-    expect(true, throwableMechanism.mechanism.handled);
-    expect(exception, throwableMechanism.throwable);
+    expect(throwableMechanism.mechanism.type, 'FlutterError');
+    expect(throwableMechanism.mechanism.handled, true);
+    expect(throwableMechanism.throwable, exception);
   });
 
   test('FlutterError calls default error', () async {
@@ -72,7 +73,61 @@ void main() {
 
     verify(await fixture.hub.captureEvent(captureAny));
 
-    expect(true, called);
+    expect(called, true);
+  });
+
+  test('FlutterErrorIntegration captureEvent only called once', () async {
+    var numberOfDefaultCalls = 0;
+    final defaultError = (FlutterErrorDetails errorDetails) async {
+      numberOfDefaultCalls++;
+    };
+    FlutterError.onError = defaultError;
+
+    when(fixture.hub.captureEvent(captureAny))
+        .thenAnswer((_) => Future.value(SentryId.empty()));
+
+    final details = FlutterErrorDetails(exception: StateError('error'));
+
+    final integrationA = FlutterErrorIntegration();
+    integrationA.call(fixture.hub, fixture.options);
+    integrationA.close();
+
+    final integrationB = FlutterErrorIntegration();
+    integrationB.call(fixture.hub, fixture.options);
+
+    FlutterError.reportError(details);
+
+    verify(await fixture.hub.captureEvent(captureAny)).called(1);
+
+    expect(numberOfDefaultCalls, 1);
+  });
+
+  test('FlutterErrorIntegration close restored default onError', () {
+    final defaultOnError = (FlutterErrorDetails errorDetails) async {};
+    FlutterError.onError = defaultOnError;
+
+    final integration = FlutterErrorIntegration();
+    integration.call(fixture.hub, fixture.options);
+    expect(false, defaultOnError == FlutterError.onError);
+
+    integration.close();
+    expect(FlutterError.onError, defaultOnError);
+  });
+
+  test('FlutterErrorIntegration default not restored if set after integration',
+      () {
+    final defaultOnError = (FlutterErrorDetails errorDetails) async {};
+    FlutterError.onError = defaultOnError;
+
+    final integration = FlutterErrorIntegration();
+    integration.call(fixture.hub, fixture.options);
+    expect(defaultOnError == FlutterError.onError, false);
+
+    final afterIntegrationOnError = (FlutterErrorDetails errorDetails) async {};
+    FlutterError.onError = afterIntegrationOnError;
+
+    integration.close();
+    expect(FlutterError.onError, afterIntegrationOnError);
   });
 
   test('FlutterError do not capture if silent error', () async {
@@ -89,11 +144,11 @@ void main() {
     verify(await fixture.hub.captureEvent(captureAny));
   });
 
-  test('FlutterError adds integration', () async {
+  test('FlutterError adds integration', () {
     FlutterErrorIntegration()(fixture.hub, fixture.options);
 
-    expect(true,
-        fixture.options.sdk.integrations.contains('flutterErrorIntegration'));
+    expect(fixture.options.sdk.integrations.contains('flutterErrorIntegration'),
+        true);
   });
 
   test('nativeSdkIntegration adds integration', () async {
@@ -103,8 +158,8 @@ void main() {
 
     await integration(fixture.hub, fixture.options);
 
-    expect(true,
-        fixture.options.sdk.integrations.contains('nativeSdkIntegration'));
+    expect(fixture.options.sdk.integrations.contains('nativeSdkIntegration'),
+        true);
   });
 
   test('nativeSdkIntegration do not throw', () async {
@@ -116,8 +171,8 @@ void main() {
 
     await integration(fixture.hub, fixture.options);
 
-    expect(false,
-        fixture.options.sdk.integrations.contains('nativeSdkIntegration'));
+    expect(fixture.options.sdk.integrations.contains('nativeSdkIntegration'),
+        false);
   });
 
   test('loadContextsIntegration adds integration', () async {
@@ -127,18 +182,18 @@ void main() {
 
     await integration(fixture.hub, fixture.options);
 
-    expect(true,
-        fixture.options.sdk.integrations.contains('loadContextsIntegration'));
+    expect(fixture.options.sdk.integrations.contains('loadContextsIntegration'),
+        true);
   });
 
   test('WidgetsFlutterBindingIntegration adds integration', () async {
     final integration = WidgetsFlutterBindingIntegration();
     await integration(fixture.hub, fixture.options);
 
     expect(
-        true,
         fixture.options.sdk.integrations
-            .contains('widgetsFlutterBindingIntegration'));
+            .contains('widgetsFlutterBindingIntegration'),
+        true);
   });
 
   test('WidgetsFlutterBindingIntegration calls ensureInitialized', () async {
@@ -150,7 +205,7 @@ void main() {
     final integration = WidgetsFlutterBindingIntegration(ensureInitialized);
     await integration(fixture.hub, fixture.options);
 
-    expect(true, called);
+    expect(called, true);
   });
 }