From 02419b742e39fd070faa613151db22e5b8c9f041 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20Andra=C5=A1ec?= Date: Thu, 13 Oct 2022 09:13:03 +0000 Subject: [PATCH] Fix: Use PlatformDispatcher.onError in Flutter 3.3 (#1039) Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> Co-authored-by: Manoel Aranda Neto --- CHANGELOG.md | 4 + dart/lib/src/sentry.dart | 42 ++- dart/test/sentry_test.dart | 25 ++ flutter/example/lib/main.dart | 14 - flutter/example/pubspec.yaml | 2 +- .../integrations/on_error_integration.dart | 4 - flutter/lib/src/sentry_flutter.dart | 18 +- flutter/test/mocks.mocks.dart | 2 +- flutter/test/sentry_flutter_test.dart | 336 +++++++++++++----- flutter/test/sentry_flutter_util.dart | 66 ++-- 10 files changed, 366 insertions(+), 147 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3634834d03..2dab80d96a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Features + +- Use PlatformDispatcher.onError in Flutter 3.3 ([#1039](https://github.com/getsentry/sentry-dart/pull/1039)) + ### Fixes - Bring protocol up to date with latest Sentry protocol ([#1038](https://github.com/getsentry/sentry-dart/pull/1038)) diff --git a/dart/lib/src/sentry.dart b/dart/lib/src/sentry.dart index 10bc8a6d2c..8314a37d3d 100644 --- a/dart/lib/src/sentry.dart +++ b/dart/lib/src/sentry.dart @@ -36,6 +36,7 @@ class Sentry { static Future init( OptionsConfiguration optionsConfiguration, { AppRunner? appRunner, + @internal bool callAppRunnerInRunZonedGuarded = true, @internal SentryOptions? options, }) async { final sentryOptions = options ?? SentryOptions(); @@ -56,7 +57,7 @@ class Sentry { throw ArgumentError('DSN is required.'); } - await _init(sentryOptions, appRunner); + await _init(sentryOptions, appRunner, callAppRunnerInRunZonedGuarded); } static Future _initDefaultValues( @@ -96,7 +97,11 @@ class Sentry { } /// Initializes the SDK - static Future _init(SentryOptions options, AppRunner? appRunner) async { + static Future _init( + SentryOptions options, + AppRunner? appRunner, + bool callAppRunnerInRunZonedGuarded, + ) async { if (isEnabled) { options.logger( SentryLevel.warning, @@ -113,21 +118,26 @@ class Sentry { // execute integrations after hub being enabled if (appRunner != null) { - var runIntegrationsAndAppRunner = () async { - final integrations = - options.integrations.where((i) => i is! RunZonedGuardedIntegration); - await _callIntegrations(integrations, options); + if (callAppRunnerInRunZonedGuarded) { + var runIntegrationsAndAppRunner = () async { + final integrations = options.integrations + .where((i) => i is! RunZonedGuardedIntegration); + await _callIntegrations(integrations, options); + await appRunner(); + }; + + final runZonedGuardedIntegration = + RunZonedGuardedIntegration(runIntegrationsAndAppRunner); + options.addIntegrationByIndex(0, runZonedGuardedIntegration); + + // RunZonedGuardedIntegration will run other integrations and appRunner + // runZonedGuarded so all exception caught in the error handler are + // handled + await runZonedGuardedIntegration(HubAdapter(), options); + } else { + await _callIntegrations(options.integrations, options); await appRunner(); - }; - - final runZonedGuardedIntegration = - RunZonedGuardedIntegration(runIntegrationsAndAppRunner); - options.addIntegrationByIndex(0, runZonedGuardedIntegration); - - // RunZonedGuardedIntegration will run other integrations and appRunner - // runZonedGuarded so all exception caught in the error handler are - // handled - await runZonedGuardedIntegration(HubAdapter(), options); + } } else { await _callIntegrations(options.integrations, options); } diff --git a/dart/test/sentry_test.dart b/dart/test/sentry_test.dart index ea07988980..0e949ffc2f 100644 --- a/dart/test/sentry_test.dart +++ b/dart/test/sentry_test.dart @@ -284,6 +284,31 @@ void main() { }); }); + test('should complete when appRunner is not called in runZonedGuarded', + () async { + final completer = Completer(); + var completed = false; + + final init = Sentry.init( + (options) { + options.dsn = fakeDsn; + }, + appRunner: () => completer.future, + callAppRunnerInRunZonedGuarded: false, + ).whenComplete(() => completed = true); + + await Future(() { + // We make the expectation only after all microtasks have completed, + // that Sentry.init might have scheduled. + expect(completed, false); + }); + + completer.complete(); + await init; + + expect(completed, true); + }); + test('options.environment debug', () async { final sentryOptions = SentryOptions(dsn: fakeDsn) ..platformChecker = FakePlatformChecker.debugMode(); diff --git a/flutter/example/lib/main.dart b/flutter/example/lib/main.dart index 7afdfe5f19..89b83d8bc2 100644 --- a/flutter/example/lib/main.dart +++ b/flutter/example/lib/main.dart @@ -187,20 +187,6 @@ class MainScaffold extends StatelessWidget { }, child: const Text('Capture from FlutterError.onError'), ), - ElevatedButton( - onPressed: () { - // Only usable on Flutter >= 3.3 - // and needs the following additional setup: - // options.addIntegration(OnErrorIntegration()); - (WidgetsBinding.instance.platformDispatcher as dynamic) - .onError - ?.call( - Exception('PlatformDispatcher.onError'), - StackTrace.current, - ); - }, - child: const Text('Capture from PlatformDispatcher.onError'), - ), ElevatedButton( onPressed: () => makeWebRequest(context), child: const Text('Dart: Web request'), diff --git a/flutter/example/pubspec.yaml b/flutter/example/pubspec.yaml index 9ea1146086..78d14ddb32 100644 --- a/flutter/example/pubspec.yaml +++ b/flutter/example/pubspec.yaml @@ -24,7 +24,7 @@ dependencies: dev_dependencies: pedantic: ^1.11.1 - sentry_dart_plugin: ^1.0.0-alpha.4 + sentry_dart_plugin: ^1.0.0-beta.1 dependency_overrides: sentry: diff --git a/flutter/lib/src/integrations/on_error_integration.dart b/flutter/lib/src/integrations/on_error_integration.dart index c6dad30761..440c197209 100644 --- a/flutter/lib/src/integrations/on_error_integration.dart +++ b/flutter/lib/src/integrations/on_error_integration.dart @@ -38,10 +38,6 @@ class OnErrorIntegration implements Integration { // WidgetsBinding works with WidgetsFlutterBinding and other custom bindings final wrapper = dispatchWrapper ?? PlatformDispatcherWrapper(binding.platformDispatcher); - - if (!wrapper.isOnErrorSupported(options)) { - return; - } _defaultOnError = wrapper.onError; _integrationOnError = (Object exception, StackTrace stackTrace) { diff --git a/flutter/lib/src/sentry_flutter.dart b/flutter/lib/src/sentry_flutter.dart index b169dd0ca5..517ad8aa5f 100644 --- a/flutter/lib/src/sentry_flutter.dart +++ b/flutter/lib/src/sentry_flutter.dart @@ -1,10 +1,11 @@ import 'dart:async'; +import 'dart:ui'; import 'package:flutter/scheduler.dart'; import 'package:flutter/services.dart'; import 'package:meta/meta.dart'; import 'package:package_info_plus/package_info_plus.dart'; -import 'package:sentry/sentry.dart'; +import '../sentry_flutter.dart'; import 'event_processor/android_platform_exception_event_processor.dart'; import 'native_scope_observer.dart'; import 'sentry_native.dart'; @@ -13,9 +14,7 @@ import 'sentry_native_channel.dart'; import 'event_processor/flutter_enricher_event_processor.dart'; import 'integrations/debug_print_integration.dart'; import 'integrations/native_app_start_integration.dart'; -import 'sentry_flutter_options.dart'; -import 'default_integrations.dart'; import 'file_system_transport.dart'; import 'version.dart'; @@ -45,12 +44,17 @@ mixin SentryFlutter { final native = SentryNative(); native.setNativeChannel(nativeChannel); + final platformDispatcher = PlatformDispatcher.instance; + final wrapper = PlatformDispatcherWrapper(platformDispatcher); + final isOnErrorSupported = wrapper.isOnErrorSupported(flutterOptions); + // first step is to install the native integration and set default values, // so we are able to capture future errors. final defaultIntegrations = _createDefaultIntegrations( packageLoader, channel, flutterOptions, + isOnErrorSupported, ); for (final defaultIntegration in defaultIntegrations) { flutterOptions.addIntegration(defaultIntegration); @@ -65,6 +69,8 @@ mixin SentryFlutter { appRunner: appRunner, // ignore: invalid_use_of_internal_member options: flutterOptions, + // ignore: invalid_use_of_internal_member + callAppRunnerInRunZonedGuarded: !isOnErrorSupported, ); } @@ -96,6 +102,7 @@ mixin SentryFlutter { PackageLoader packageLoader, MethodChannel channel, SentryFlutterOptions options, + bool isOnErrorSupported, ) { final integrations = []; final platformChecker = options.platformChecker; @@ -104,6 +111,11 @@ mixin SentryFlutter { // Will call WidgetsFlutterBinding.ensureInitialized() before all other integrations. integrations.add(WidgetsFlutterBindingIntegration()); + // Use PlatformDispatcher.onError instead of zones. + if (isOnErrorSupported) { + integrations.add(OnErrorIntegration()); + } + // Will catch any errors that may occur in the Flutter framework itself. integrations.add(FlutterErrorIntegration()); diff --git a/flutter/test/mocks.mocks.dart b/flutter/test/mocks.mocks.dart index 9bce2502a0..24f6161823 100644 --- a/flutter/test/mocks.mocks.dart +++ b/flutter/test/mocks.mocks.dart @@ -1,4 +1,4 @@ -// Mocks generated by Mockito 5.3.1 from annotations +// Mocks generated by Mockito 5.3.2 from annotations // in sentry_flutter/example/windows/flutter/ephemeral/.plugin_symlinks/sentry_flutter/example/linux/flutter/ephemeral/.plugin_symlinks/sentry_flutter/example/ios/.symlinks/plugins/sentry_flutter/test/mocks.dart. // Do not manually edit this file. diff --git a/flutter/test/sentry_flutter_test.dart b/flutter/test/sentry_flutter_test.dart index 1836fa0ed7..3905643048 100644 --- a/flutter/test/sentry_flutter_test.dart +++ b/flutter/test/sentry_flutter_test.dart @@ -4,11 +4,14 @@ import 'package:sentry_flutter/sentry_flutter.dart'; import 'package:sentry_flutter/src/integrations/debug_print_integration.dart'; import 'package:sentry_flutter/src/version.dart'; import 'mocks.dart'; +import 'mocks.mocks.dart'; import 'sentry_flutter_util.dart'; /// These are the integrations which should be added on every platform. /// They don't depend on the underlying platform. final platformAgnosticIntegrations = [ + WidgetsFlutterBindingIntegration, + OnErrorIntegration, FlutterErrorIntegration, LoadReleaseIntegration, DebugPrintIntegration, @@ -39,111 +42,207 @@ void main() { }); test('Android', () async { + List integrations = []; + Transport transport = MockTransport(); + await SentryFlutter.init( - getConfigurationTester( - hasFileSystemTransport: true, + (options) async { + options.dsn = fakeDsn; + integrations = options.integrations; + transport = options.transport; + }, + appRunner: appRunner, + packageLoader: loadTestPackage, + platformChecker: getPlatformChecker(platform: MockPlatform.android()), + ); + + testTransport( + transport: transport, + hasFileSystemTransport: true, + ); + + testConfiguration( + integrations: integrations, shouldHaveIntegrations: [ ...androidIntegrations, ...nativeIntegrations, ...platformAgnosticIntegrations, ], - shouldNotHaveIntegrations: iOsAndMacOsIntegrations, - ), - appRunner: appRunner, - packageLoader: loadTestPackage, - platformChecker: getPlatformChecker(platform: MockPlatform.android()), - ); + shouldNotHaveIntegrations: iOsAndMacOsIntegrations); + + integrations + .indexWhere((element) => element is WidgetsFlutterBindingIntegration); + + testBefore( + integrations: integrations, + beforeIntegration: WidgetsFlutterBindingIntegration, + afterIntegration: OnErrorIntegration); await Sentry.close(); }, testOn: 'vm'); test('iOS', () async { + List integrations = []; + Transport transport = MockTransport(); + await SentryFlutter.init( - getConfigurationTester( - hasFileSystemTransport: true, - shouldHaveIntegrations: [ - ...iOsAndMacOsIntegrations, - ...nativeIntegrations, - ...platformAgnosticIntegrations, - ], - shouldNotHaveIntegrations: androidIntegrations, - ), + (options) async { + options.dsn = fakeDsn; + integrations = options.integrations; + transport = options.transport; + }, appRunner: appRunner, packageLoader: loadTestPackage, platformChecker: getPlatformChecker(platform: MockPlatform.iOs()), ); + testTransport( + transport: transport, + hasFileSystemTransport: true, + ); + + testConfiguration( + integrations: integrations, + shouldHaveIntegrations: [ + ...iOsAndMacOsIntegrations, + ...nativeIntegrations, + ...platformAgnosticIntegrations, + ], + shouldNotHaveIntegrations: androidIntegrations, + ); + + testBefore( + integrations: integrations, + beforeIntegration: WidgetsFlutterBindingIntegration, + afterIntegration: OnErrorIntegration); + await Sentry.close(); }, testOn: 'vm'); test('macOS', () async { + List integrations = []; + Transport transport = MockTransport(); + await SentryFlutter.init( - getConfigurationTester( - hasFileSystemTransport: true, - shouldHaveIntegrations: [ - ...iOsAndMacOsIntegrations, - ...nativeIntegrations, - ...platformAgnosticIntegrations, - ], - shouldNotHaveIntegrations: androidIntegrations, - ), + (options) async { + options.dsn = fakeDsn; + integrations = options.integrations; + transport = options.transport; + }, appRunner: appRunner, packageLoader: loadTestPackage, platformChecker: getPlatformChecker(platform: MockPlatform.macOs()), ); + testTransport( + transport: transport, + hasFileSystemTransport: true, + ); + + testConfiguration( + integrations: integrations, + shouldHaveIntegrations: [ + ...iOsAndMacOsIntegrations, + ...nativeIntegrations, + ...platformAgnosticIntegrations, + ], + shouldNotHaveIntegrations: androidIntegrations, + ); + + testBefore( + integrations: integrations, + beforeIntegration: WidgetsFlutterBindingIntegration, + afterIntegration: OnErrorIntegration); + await Sentry.close(); }, testOn: 'vm'); test('Windows', () async { + List integrations = []; + Transport transport = MockTransport(); + await SentryFlutter.init( - getConfigurationTester( - hasFileSystemTransport: false, - shouldHaveIntegrations: platformAgnosticIntegrations, - shouldNotHaveIntegrations: [ - ...androidIntegrations, - ...iOsAndMacOsIntegrations, - ...nativeIntegrations, - ], - ), + (options) async { + options.dsn = fakeDsn; + integrations = options.integrations; + transport = options.transport; + }, appRunner: appRunner, packageLoader: loadTestPackage, platformChecker: getPlatformChecker(platform: MockPlatform.windows()), ); + testTransport( + transport: transport, + hasFileSystemTransport: false, + ); + + testConfiguration( + integrations: integrations, + shouldHaveIntegrations: platformAgnosticIntegrations, + shouldNotHaveIntegrations: [ + ...androidIntegrations, + ...iOsAndMacOsIntegrations, + ...nativeIntegrations, + ], + ); + + testBefore( + integrations: integrations, + beforeIntegration: WidgetsFlutterBindingIntegration, + afterIntegration: OnErrorIntegration); + await Sentry.close(); }, testOn: 'vm'); test('Linux', () async { + List integrations = []; + Transport transport = MockTransport(); + await SentryFlutter.init( - getConfigurationTester( - hasFileSystemTransport: false, - shouldHaveIntegrations: platformAgnosticIntegrations, - shouldNotHaveIntegrations: [ - ...androidIntegrations, - ...iOsAndMacOsIntegrations, - ...nativeIntegrations, - ], - ), + (options) async { + options.dsn = fakeDsn; + integrations = options.integrations; + transport = options.transport; + }, appRunner: appRunner, packageLoader: loadTestPackage, platformChecker: getPlatformChecker(platform: MockPlatform.linux()), ); + testTransport( + transport: transport, + hasFileSystemTransport: false, + ); + + testConfiguration( + integrations: integrations, + shouldHaveIntegrations: platformAgnosticIntegrations, + shouldNotHaveIntegrations: [ + ...androidIntegrations, + ...iOsAndMacOsIntegrations, + ...nativeIntegrations, + ], + ); + + testBefore( + integrations: integrations, + beforeIntegration: WidgetsFlutterBindingIntegration, + afterIntegration: OnErrorIntegration); + await Sentry.close(); }, testOn: 'vm'); test('Web', () async { + List integrations = []; + Transport transport = MockTransport(); + await SentryFlutter.init( - getConfigurationTester( - hasFileSystemTransport: false, - shouldHaveIntegrations: platformAgnosticIntegrations, - shouldNotHaveIntegrations: [ - ...androidIntegrations, - ...iOsAndMacOsIntegrations, - ...nativeIntegrations, - ], - ), + (options) async { + options.dsn = fakeDsn; + integrations = options.integrations; + transport = options.transport; + }, appRunner: appRunner, packageLoader: loadTestPackage, platformChecker: getPlatformChecker( @@ -152,22 +251,41 @@ void main() { ), ); + testTransport( + transport: transport, + hasFileSystemTransport: false, + ); + + testConfiguration( + integrations: integrations, + shouldHaveIntegrations: platformAgnosticIntegrations, + shouldNotHaveIntegrations: [ + ...androidIntegrations, + ...iOsAndMacOsIntegrations, + ...nativeIntegrations, + ], + ); + + testBefore( + integrations: integrations, + beforeIntegration: WidgetsFlutterBindingIntegration, + afterIntegration: OnErrorIntegration); + await Sentry.close(); }); test('Web && (iOS || macOS) ', () async { + List integrations = []; + Transport transport = MockTransport(); + // Tests that iOS || macOS integrations aren't added on a browser which // runs on iOS or macOS await SentryFlutter.init( - getConfigurationTester( - hasFileSystemTransport: false, - shouldHaveIntegrations: platformAgnosticIntegrations, - shouldNotHaveIntegrations: [ - ...androidIntegrations, - ...iOsAndMacOsIntegrations, - ...nativeIntegrations, - ], - ), + (options) async { + options.dsn = fakeDsn; + integrations = options.integrations; + transport = options.transport; + }, appRunner: appRunner, packageLoader: loadTestPackage, platformChecker: getPlatformChecker( @@ -176,22 +294,41 @@ void main() { ), ); + testTransport( + transport: transport, + hasFileSystemTransport: false, + ); + + testConfiguration( + integrations: integrations, + shouldHaveIntegrations: platformAgnosticIntegrations, + shouldNotHaveIntegrations: [ + ...androidIntegrations, + ...iOsAndMacOsIntegrations, + ...nativeIntegrations, + ], + ); + + testBefore( + integrations: integrations, + beforeIntegration: WidgetsFlutterBindingIntegration, + afterIntegration: OnErrorIntegration); + await Sentry.close(); }); test('Web && (macOS)', () async { + List integrations = []; + Transport transport = MockTransport(); + // Tests that iOS || macOS integrations aren't added on a browswer which // runs on iOS or macOS await SentryFlutter.init( - getConfigurationTester( - hasFileSystemTransport: false, - shouldHaveIntegrations: platformAgnosticIntegrations, - shouldNotHaveIntegrations: [ - ...androidIntegrations, - ...iOsAndMacOsIntegrations, - ...nativeIntegrations, - ], - ), + (options) async { + options.dsn = fakeDsn; + integrations = options.integrations; + transport = options.transport; + }, appRunner: appRunner, packageLoader: loadTestPackage, platformChecker: getPlatformChecker( @@ -200,21 +337,40 @@ void main() { ), ); + testTransport( + transport: transport, + hasFileSystemTransport: false, + ); + + testConfiguration( + integrations: integrations, + shouldHaveIntegrations: platformAgnosticIntegrations, + shouldNotHaveIntegrations: [ + ...androidIntegrations, + ...iOsAndMacOsIntegrations, + ...nativeIntegrations, + ], + ); + + testBefore( + integrations: integrations, + beforeIntegration: WidgetsFlutterBindingIntegration, + afterIntegration: OnErrorIntegration); + await Sentry.close(); }); test('Web && Android', () async { + List integrations = []; + Transport transport = MockTransport(); + // Tests that Android integrations aren't added on an Android browswer await SentryFlutter.init( - getConfigurationTester( - hasFileSystemTransport: false, - shouldHaveIntegrations: platformAgnosticIntegrations, - shouldNotHaveIntegrations: [ - ...androidIntegrations, - ...iOsAndMacOsIntegrations, - ...nativeIntegrations, - ], - ), + (options) async { + options.dsn = fakeDsn; + integrations = options.integrations; + transport = options.transport; + }, appRunner: appRunner, packageLoader: loadTestPackage, platformChecker: getPlatformChecker( @@ -223,6 +379,26 @@ void main() { ), ); + testTransport( + transport: transport, + hasFileSystemTransport: false, + ); + + testConfiguration( + integrations: integrations, + shouldHaveIntegrations: platformAgnosticIntegrations, + shouldNotHaveIntegrations: [ + ...androidIntegrations, + ...iOsAndMacOsIntegrations, + ...nativeIntegrations, + ], + ); + + testBefore( + integrations: integrations, + beforeIntegration: WidgetsFlutterBindingIntegration, + afterIntegration: OnErrorIntegration); + await Sentry.close(); }); }); diff --git a/flutter/test/sentry_flutter_util.dart b/flutter/test/sentry_flutter_util.dart index a0fb496c02..154ea359af 100644 --- a/flutter/test/sentry_flutter_util.dart +++ b/flutter/test/sentry_flutter_util.dart @@ -1,38 +1,48 @@ -import 'dart:async'; - import 'package:flutter_test/flutter_test.dart'; import 'package:sentry_flutter/sentry_flutter.dart'; import 'package:sentry_flutter/src/file_system_transport.dart'; -import 'package:sentry_flutter/src/sentry_flutter_options.dart'; -import 'mocks.dart'; +void testTransport({ + required Transport transport, + required hasFileSystemTransport, +}) { + expect( + transport is FileSystemTransport, + hasFileSystemTransport, + reason: '$FileSystemTransport was wrongly set', + ); +} -FutureOr Function(SentryFlutterOptions) getConfigurationTester({ +void testConfiguration({ + required Iterable integrations, required Iterable shouldHaveIntegrations, required Iterable shouldNotHaveIntegrations, - required bool hasFileSystemTransport, -}) => - (options) async { - options.dsn = fakeDsn; - - expect( - options.transport is FileSystemTransport, - hasFileSystemTransport, - reason: '$FileSystemTransport was wrongly set', - ); +}) { + final numberOfIntegrationsByType = {}; + for (var e in integrations) { + numberOfIntegrationsByType[e.runtimeType] = + numberOfIntegrationsByType[e.runtimeType] ?? 0 + 1; + } - final integrations = {}; - for (var e in options.integrations) { - integrations[e.runtimeType] = integrations[e.runtimeType] ?? 0 + 1; - } + for (final type in shouldHaveIntegrations) { + expect(numberOfIntegrationsByType, containsPair(type, 1)); + } - for (final type in shouldHaveIntegrations) { - expect(integrations, containsPair(type, 1)); - } + shouldNotHaveIntegrations = Set.of(shouldNotHaveIntegrations) + .difference(Set.of(shouldHaveIntegrations)); + for (final type in shouldNotHaveIntegrations) { + expect(integrations, isNot(contains(type))); + } +} - shouldNotHaveIntegrations = Set.of(shouldNotHaveIntegrations) - .difference(Set.of(shouldHaveIntegrations)); - for (final type in shouldNotHaveIntegrations) { - expect(integrations, isNot(contains(type))); - } - }; +void testBefore({ + required List integrations, + required Type beforeIntegration, + required Type afterIntegration, +}) { + final beforeIndex = integrations + .indexWhere((element) => element.runtimeType == beforeIntegration); + final afterIndex = integrations + .indexWhere((element) => element.runtimeType == afterIntegration); + expect(beforeIndex < afterIndex, true); +}