[ Tool ] Only process a single unhandled tool exception (#178335)
Without this change, if multiple asynchronous exceptions are thrown
while processing an exception, multiple exception analytics events can
be sent for a single process crash, skewing crashlytics data.
Fixes https://github.com/flutter/flutter/issues/178318
diff --git a/packages/flutter_tools/lib/runner.dart b/packages/flutter_tools/lib/runner.dart
index 75c1090..4e89497 100644
--- a/packages/flutter_tools/lib/runner.dart
+++ b/packages/flutter_tools/lib/runner.dart
@@ -46,6 +46,9 @@
args.removeWhere((String option) => option == '-vv' || option == '-v' || option == '--verbose');
}
+ // Reset this on each run to ensure we don't leak state across tests.
+ _alreadyHandlingToolError = null;
+
final bool usingLocalEngine = args.any((a) => a.startsWith('--local-engine'));
return runInContext<int>(() async {
@@ -149,6 +152,13 @@
}, overrides: overrides);
}
+/// Track if we're actively processing an error so we don't try and process
+/// additional asynchronous exceptions while we're trying to shut down.
+///
+/// NOTE: This state is cleared at the beginning of [run] to ensure state
+/// doesn't leak when running tests.
+Future<int>? _alreadyHandlingToolError;
+
Future<int> _handleToolError(
Object error,
StackTrace? stackTrace,
@@ -160,6 +170,30 @@
required bool usingLocalEngine,
required FeatureFlags featureFlags,
}) async {
+ return _alreadyHandlingToolError ??= _handleToolErrorImpl(
+ error,
+ stackTrace,
+ verbose,
+ args,
+ reportCrashes,
+ getFlutterVersion,
+ shutdownHooks,
+ usingLocalEngine: usingLocalEngine,
+ featureFlags: featureFlags,
+ );
+}
+
+Future<int> _handleToolErrorImpl(
+ Object error,
+ StackTrace? stackTrace,
+ bool verbose,
+ List<String> args,
+ bool reportCrashes,
+ String Function() getFlutterVersion,
+ ShutdownHooks shutdownHooks, {
+ required bool usingLocalEngine,
+ required FeatureFlags featureFlags,
+}) async {
if (error is UsageException) {
globals.printError('${error.message}\n');
globals.printError(
diff --git a/packages/flutter_tools/test/general.shard/runner/runner_test.dart b/packages/flutter_tools/test/general.shard/runner/runner_test.dart
index dfe6f03..7e4a7e1 100644
--- a/packages/flutter_tools/test/general.shard/runner/runner_test.dart
+++ b/packages/flutter_tools/test/general.shard/runner/runner_test.dart
@@ -23,6 +23,7 @@
import 'package:flutter_tools/src/reporting/crash_reporting.dart';
import 'package:flutter_tools/src/runner/flutter_command.dart';
import 'package:test/fake.dart';
+import 'package:unified_analytics/testing.dart';
import 'package:unified_analytics/unified_analytics.dart';
import '../../src/common.dart';
@@ -290,6 +291,67 @@
);
testUsingContext(
+ "doesn't send multiple events for additional asynchronous exceptions "
+ 'thrown during shutdown',
+ () async {
+ // Regression test for https://github.com/flutter/flutter/issues/178318.
+ final command = MultipleExceptionCrashingFlutterCommand();
+ var exceptionCount = 0;
+ unawaited(
+ runZonedGuarded<Future<void>?>(
+ () {
+ unawaited(
+ runner.run(
+ <String>['crash'],
+ () => <FlutterCommand>[command],
+ // This flutterVersion disables crash reporting.
+ flutterVersion: '[user-branch]/',
+ reportCrashes: true,
+ shutdownHooks: ShutdownHooks(),
+ ),
+ );
+ return null;
+ },
+ (Object error, StackTrace stack) {
+ // Keep track of the number of exceptions thrown to ensure that
+ // the count matches the number of exceptions we expect.
+ exceptionCount++;
+ },
+ ),
+ );
+ await command.doneThrowing;
+
+ // This is the main check of this test.
+ //
+ // We are checking that, even though multiple asynchronous errors were
+ // thrown, only a single crash report is sent. This ensures that a
+ // single process crash can't result in multiple crash events.
+
+ // This test only makes sense if we've thrown more than one exception.
+ expect(exceptionCount, greaterThan(1));
+ expect(exceptionCount, command.exceptionCount);
+
+ // Ensure only a single exception analytics event was sent.
+ final List<Event> exceptionEvents = fakeAnalytics.sentEvents
+ .where((e) => e.eventName == DashEvent.exception)
+ .toList();
+ expect(exceptionEvents, hasLength(1));
+ },
+ overrides: <Type, Generator>{
+ Analytics: () => fakeAnalytics,
+ Platform: () => FakePlatform(
+ environment: <String, String>{'FLUTTER_ANALYTICS_LOG_FILE': 'test', 'FLUTTER_ROOT': '/'},
+ ),
+ FileSystem: () => fileSystem,
+ ProcessManager: () => FakeProcessManager.any(),
+ CrashReporter: () => WaitingCrashReporter(Future<void>.value()),
+ Artifacts: () => Artifacts.test(),
+ HttpClientFactory: () =>
+ () => FakeHttpClient.any(),
+ },
+ );
+
+ testUsingContext(
'create local report',
() async {
// Since crash reporting calls the doctor, which checks for the devtools
@@ -819,6 +881,34 @@
}
}
+class MultipleExceptionCrashingFlutterCommand extends FlutterCommand {
+ final _completer = Completer<void>();
+
+ @override
+ String get description => '';
+
+ @override
+ String get name => 'crash';
+
+ Future<void> get doneThrowing => _completer.future;
+
+ var exceptionCount = 0;
+
+ @override
+ Future<FlutterCommandResult> runCommand() async {
+ Timer.periodic(const Duration(milliseconds: 10), (timer) {
+ exceptionCount++;
+ if (exceptionCount < 5) {
+ throw Exception('ERROR: $exceptionCount');
+ }
+ timer.cancel();
+ _completer.complete();
+ });
+
+ return FlutterCommandResult.success();
+ }
+}
+
class _GitNotFoundFlutterCommand extends FlutterCommand {
@override
String get description => '';
