From 948150ee54e52201fc1a80b3cefc21309a891d6e Mon Sep 17 00:00:00 2001 From: denrase Date: Tue, 9 Mar 2021 13:20:24 +0100 Subject: [PATCH 1/9] Fix issue where errors were reported multiple times When doing multiple close/init on the SDK, multiple FlutterErrorIntegration instances were intantiated during application lifetime. Every one of them would replace FlutterError.onError function and reference the previouse one. This resulted in a chained call, where errors were reported multiple times. --- flutter/lib/src/default_integrations.dart | 17 ++++++++-- flutter/test/default_integrations_test.dart | 35 +++++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/flutter/lib/src/default_integrations.dart b/flutter/lib/src/default_integrations.dart index b63a4f1c1c..bbbb3fd5c9 100644 --- a/flutter/lib/src/default_integrations.dart +++ b/flutter/lib/src/default_integrations.dart @@ -34,9 +34,14 @@ 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 { + /// Keep a reference to the original handler. + static FlutterExceptionHandler? defaultOnError; + @override void call(Hub hub, SentryFlutterOptions options) { - final defaultOnError = FlutterError.onError; + if (defaultOnError == null && FlutterError.onError != null) { + defaultOnError = FlutterError.onError; + } FlutterError.onError = (FlutterErrorDetails errorDetails) async { dynamic exception = errorDetails.exception; @@ -60,7 +65,7 @@ class FlutterErrorIntegration extends Integration { // call original handler if (defaultOnError != null) { - defaultOnError(errorDetails); + defaultOnError!(errorDetails); } // we don't call Zone.current.handleUncaughtError because we'd like @@ -76,6 +81,14 @@ class FlutterErrorIntegration extends Integration { options.sdk.addIntegration('flutterErrorIntegration'); } + + @override + void close() { + /// Restore default + FlutterError.onError = defaultOnError; + defaultOnError = 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 index 44c2468586..bfb78d19fd 100644 --- a/flutter/test/default_integrations_test.dart +++ b/flutter/test/default_integrations_test.dart @@ -75,6 +75,41 @@ void main() { expect(true, called); }); + 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 errorIntegration = FlutterErrorIntegration(); + errorIntegration.call(fixture.hub, fixture.options); + errorIntegration.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', () async { + final defaultOnError = (FlutterErrorDetails errorDetails) async {}; + FlutterError.onError = defaultOnError; + + final integrtion = FlutterErrorIntegration(); + integrtion.call(fixture.hub, fixture.options); + expect(false, defaultOnError == FlutterError.onError); + + integrtion.close(); + + expect(true, defaultOnError == FlutterError.onError); + }); + test('FlutterError do not capture if silent error', () async { _reportError(silent: true); From aaef9dc834bab43b4a2c8e2d6c94138f67a81fa8 Mon Sep 17 00:00:00 2001 From: denrase Date: Tue, 9 Mar 2021 13:26:46 +0100 Subject: [PATCH 2/9] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58b780c9cf..6eaee12b70 100644 --- 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 # 4.1.0-nullsafety.0 From b3d41119aa2cf52512fdf1af83b7e919b8ebd0a9 Mon Sep 17 00:00:00 2001 From: denrase Date: Tue, 9 Mar 2021 13:35:26 +0100 Subject: [PATCH 3/9] Don't null defaultOnError in close --- flutter/lib/src/default_integrations.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/flutter/lib/src/default_integrations.dart b/flutter/lib/src/default_integrations.dart index bbbb3fd5c9..b3906b0a64 100644 --- a/flutter/lib/src/default_integrations.dart +++ b/flutter/lib/src/default_integrations.dart @@ -86,7 +86,6 @@ class FlutterErrorIntegration extends Integration { void close() { /// Restore default FlutterError.onError = defaultOnError; - defaultOnError = null; super.close(); } } From f67a890bdb0587fd1e2a31985c7886477df46c2b Mon Sep 17 00:00:00 2001 From: denrase Date: Tue, 9 Mar 2021 13:51:11 +0100 Subject: [PATCH 4/9] Reset FlutterErrorIntegration.defaultOnError in teardown --- flutter/lib/src/default_integrations.dart | 1 + flutter/test/default_integrations_test.dart | 1 + 2 files changed, 2 insertions(+) diff --git a/flutter/lib/src/default_integrations.dart b/flutter/lib/src/default_integrations.dart index b3906b0a64..bbbb3fd5c9 100644 --- a/flutter/lib/src/default_integrations.dart +++ b/flutter/lib/src/default_integrations.dart @@ -86,6 +86,7 @@ class FlutterErrorIntegration extends Integration { void close() { /// Restore default FlutterError.onError = defaultOnError; + defaultOnError = null; super.close(); } } diff --git a/flutter/test/default_integrations_test.dart b/flutter/test/default_integrations_test.dart index bfb78d19fd..945004bb96 100644 --- a/flutter/test/default_integrations_test.dart +++ b/flutter/test/default_integrations_test.dart @@ -22,6 +22,7 @@ void main() { tearDown(() { _channel.setMockMethodCallHandler(null); + FlutterErrorIntegration.defaultOnError = null; }); void _reportError({ From aad741ff234c5ecac84b24172bad8f3a9924ac8d Mon Sep 17 00:00:00 2001 From: denrase Date: Tue, 9 Mar 2021 13:52:28 +0100 Subject: [PATCH 5/9] Add link to the pr --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6eaee12b70..d4f64c8c5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,7 @@ # Unreleased * Bump: sentry-android to v4.3.0 (#343) -* Fix: Multiple FlutterError.onError calls in FlutterErrorIntegration +* Fix: Multiple FlutterError.onError calls in FlutterErrorIntegration (#345) # 4.1.0-nullsafety.0 From 139078a1f73cf621d897212e6c74fc3e0567c45e Mon Sep 17 00:00:00 2001 From: denrase Date: Tue, 9 Mar 2021 17:24:42 +0100 Subject: [PATCH 6/9] Hold reference of integration onError and don't overwrite, Fix tests and expect param order --- flutter/lib/src/default_integrations.dart | 28 +++++---- flutter/test/default_integrations_test.dart | 70 +++++++++++++-------- 2 files changed, 60 insertions(+), 38 deletions(-) diff --git a/flutter/lib/src/default_integrations.dart b/flutter/lib/src/default_integrations.dart index bbbb3fd5c9..ce6c76e1d4 100644 --- a/flutter/lib/src/default_integrations.dart +++ b/flutter/lib/src/default_integrations.dart @@ -34,16 +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 { - /// Keep a reference to the original handler. - static FlutterExceptionHandler? defaultOnError; + /// Reference to the original handler. + FlutterExceptionHandler? _defaultOnError; + + /// The error handler set by this integration. + FlutterExceptionHandler? _integrationOnError; @override void call(Hub hub, SentryFlutterOptions options) { - if (defaultOnError == null && FlutterError.onError != null) { - defaultOnError = FlutterError.onError; - } - - FlutterError.onError = (FlutterErrorDetails errorDetails) async { + _defaultOnError = FlutterError.onError; + _integrationOnError = (FlutterErrorDetails errorDetails) async { dynamic exception = errorDetails.exception; options.logger( @@ -64,8 +64,8 @@ class FlutterErrorIntegration extends Integration { 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 @@ -78,15 +78,19 @@ class FlutterErrorIntegration extends Integration { 'if you wish to capture silent errors'); } }; + FlutterError.onError = _integrationOnError; options.sdk.addIntegration('flutterErrorIntegration'); } @override void close() { - /// Restore default - FlutterError.onError = defaultOnError; - defaultOnError = null; + /// Restore default if the integration error is still set. + if (FlutterError.onError == _integrationOnError) { + FlutterError.onError = _defaultOnError; + _defaultOnError = null; + _integrationOnError = null; + } super.close(); } } diff --git a/flutter/test/default_integrations_test.dart b/flutter/test/default_integrations_test.dart index 945004bb96..dbaa43a036 100644 --- a/flutter/test/default_integrations_test.dart +++ b/flutter/test/default_integrations_test.dart @@ -22,7 +22,6 @@ void main() { tearDown(() { _channel.setMockMethodCallHandler(null); - FlutterErrorIntegration.defaultOnError = null; }); void _reportError({ @@ -49,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 { @@ -73,7 +73,7 @@ void main() { verify(await fixture.hub.captureEvent(captureAny)); - expect(true, called); + expect(called, true); }); test('FlutterErrorIntegration captureEvent only called once', () async { @@ -88,13 +88,17 @@ void main() { final details = FlutterErrorDetails(exception: StateError('error')); - final errorIntegration = FlutterErrorIntegration(); - errorIntegration.call(fixture.hub, fixture.options); - errorIntegration.call(fixture.hub, fixture.options); + 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); }); @@ -102,13 +106,28 @@ void main() { final defaultOnError = (FlutterErrorDetails errorDetails) async {}; FlutterError.onError = defaultOnError; - final integrtion = FlutterErrorIntegration(); - integrtion.call(fixture.hub, fixture.options); + final integration = FlutterErrorIntegration(); + integration.call(fixture.hub, fixture.options); expect(false, defaultOnError == FlutterError.onError); - integrtion.close(); + integration.close(); + expect(defaultOnError == FlutterError.onError, true); + }); + + test('FlutterErrorIntegration default not restored if set after integration', + () async { + 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; - expect(true, defaultOnError == FlutterError.onError); + integration.close(); + expect(afterIntegrationOnError == FlutterError.onError, true); }); test('FlutterError do not capture if silent error', () async { @@ -128,8 +147,8 @@ void main() { test('FlutterError adds integration', () async { 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 { @@ -139,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 { @@ -152,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 { @@ -163,18 +182,17 @@ 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')); + expect(fixture.options.sdk.integrations + .contains('widgetsFlutterBindingIntegration'), + true); }); test('WidgetsFlutterBindingIntegration calls ensureInitialized', () async { @@ -186,7 +204,7 @@ void main() { final integration = WidgetsFlutterBindingIntegration(ensureInitialized); await integration(fixture.hub, fixture.options); - expect(true, called); + expect(called, true); }); } From c0d9e879a4231c60340e7e4f5b2196a7e4f5a79e Mon Sep 17 00:00:00 2001 From: denrase Date: Tue, 9 Mar 2021 17:28:44 +0100 Subject: [PATCH 7/9] Remove unneccessary async --- flutter/test/default_integrations_test.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/flutter/test/default_integrations_test.dart b/flutter/test/default_integrations_test.dart index dbaa43a036..9ca4923a36 100644 --- a/flutter/test/default_integrations_test.dart +++ b/flutter/test/default_integrations_test.dart @@ -102,7 +102,7 @@ void main() { expect(numberOfDefaultCalls, 1); }); - test('FlutterErrorIntegration close restored default onError', () async { + test('FlutterErrorIntegration close restored default onError', () { final defaultOnError = (FlutterErrorDetails errorDetails) async {}; FlutterError.onError = defaultOnError; @@ -115,7 +115,7 @@ void main() { }); test('FlutterErrorIntegration default not restored if set after integration', - () async { + () { final defaultOnError = (FlutterErrorDetails errorDetails) async {}; FlutterError.onError = defaultOnError; @@ -144,7 +144,7 @@ void main() { verify(await fixture.hub.captureEvent(captureAny)); }); - test('FlutterError adds integration', () async { + test('FlutterError adds integration', () { FlutterErrorIntegration()(fixture.hub, fixture.options); expect(fixture.options.sdk.integrations.contains('flutterErrorIntegration'), From fba6bfb09fc281666ea7d732a908a1c1bd93a33d Mon Sep 17 00:00:00 2001 From: denrase Date: Tue, 9 Mar 2021 17:33:11 +0100 Subject: [PATCH 8/9] Run flutter format --- flutter/test/default_integrations_test.dart | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/flutter/test/default_integrations_test.dart b/flutter/test/default_integrations_test.dart index 9ca4923a36..8c9a8c30bb 100644 --- a/flutter/test/default_integrations_test.dart +++ b/flutter/test/default_integrations_test.dart @@ -148,7 +148,7 @@ void main() { FlutterErrorIntegration()(fixture.hub, fixture.options); expect(fixture.options.sdk.integrations.contains('flutterErrorIntegration'), - true); + true); }); test('nativeSdkIntegration adds integration', () async { @@ -159,7 +159,7 @@ void main() { await integration(fixture.hub, fixture.options); expect(fixture.options.sdk.integrations.contains('nativeSdkIntegration'), - true); + true); }); test('nativeSdkIntegration do not throw', () async { @@ -172,7 +172,7 @@ void main() { await integration(fixture.hub, fixture.options); expect(fixture.options.sdk.integrations.contains('nativeSdkIntegration'), - false); + false); }); test('loadContextsIntegration adds integration', () async { @@ -183,16 +183,17 @@ void main() { await integration(fixture.hub, fixture.options); expect(fixture.options.sdk.integrations.contains('loadContextsIntegration'), - true); + true); }); test('WidgetsFlutterBindingIntegration adds integration', () async { final integration = WidgetsFlutterBindingIntegration(); await integration(fixture.hub, fixture.options); - expect(fixture.options.sdk.integrations - .contains('widgetsFlutterBindingIntegration'), - true); + expect( + fixture.options.sdk.integrations + .contains('widgetsFlutterBindingIntegration'), + true); }); test('WidgetsFlutterBindingIntegration calls ensureInitialized', () async { From ae67dd359d3a98791337ab1fe240a552ba42aa43 Mon Sep 17 00:00:00 2001 From: denrase Date: Wed, 10 Mar 2021 16:56:03 +0100 Subject: [PATCH 9/9] Use expect directly --- flutter/test/default_integrations_test.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/flutter/test/default_integrations_test.dart b/flutter/test/default_integrations_test.dart index 8c9a8c30bb..df6b143e6d 100644 --- a/flutter/test/default_integrations_test.dart +++ b/flutter/test/default_integrations_test.dart @@ -111,7 +111,7 @@ void main() { expect(false, defaultOnError == FlutterError.onError); integration.close(); - expect(defaultOnError == FlutterError.onError, true); + expect(FlutterError.onError, defaultOnError); }); test('FlutterErrorIntegration default not restored if set after integration', @@ -127,7 +127,7 @@ void main() { FlutterError.onError = afterIntegrationOnError; integration.close(); - expect(afterIntegrationOnError == FlutterError.onError, true); + expect(FlutterError.onError, afterIntegrationOnError); }); test('FlutterError do not capture if silent error', () async {