[tool] fallback to sigkill when closing Chromium (#135521)

This implements https://github.com/flutter/flutter/issues/132654#issuecomment-1738221257, namely:

Make `Chromium.close` more robust:

* Send `SIGTERM` and wait up to 5 seconds, if the process exited, great! Return from the function.
* If the process has not exited, then send a `SIGKILL`, which is a much firmer way to exit a process. Same as before, wait up to 5 seconds, if the process exited, great! Return from the function.
* If it still hasn't exited then give up trying to exit Chromium, just print a warning to the console and return from the function.

Bonus: a few nullability fixes and extra `-v` logging.

Fixes https://github.com/flutter/flutter/issues/132654

packages/flutter_tools/lib/src/base/io.dart

@@ -191,15 +191,27 @@ class ProcessSignal {
   ///
   /// Returns true if the signal was delivered, false otherwise.
   ///
-  /// On Windows, this can only be used with [ProcessSignal.sigterm], which
-  /// terminates the process.
+  /// On Windows, this can only be used with [sigterm], which terminates the
+  /// process.
   ///
-  /// This is implemented by sending the signal using [Process.killPid].
+  /// This is implemented by sending the signal using [io.Process.killPid] and
+  /// therefore cannot be faked in tests. To fake sending signals in tests, use
+  /// [kill] instead.
   bool send(int pid) {
     assert(!_platform.isWindows || this == ProcessSignal.sigterm);
     return io.Process.killPid(pid, _delegate);
   }
 
+  /// A more testable variant of [send].
+  ///
+  /// Sends this signal to the given `process` by invoking [io.Process.kill].
+  ///
+  /// In tests this method can be faked by passing a fake implementation of the
+  /// [io.Process] interface.
+  bool kill(io.Process process) {
+    return process.kill(_delegate);
+  }
+
   @override
   String toString() => _delegate.toString();
 }

packages/flutter_tools/lib/src/test/flutter_web_platform.dart

@@ -441,6 +441,14 @@ class FlutterWebPlatform extends PlatformPlugin {
     if (_closed) {
       throw StateError('Load called on a closed FlutterWebPlatform');
     }
+
+    final String pathFromTest = _fileSystem.path.relative(path, from: _fileSystem.path.join(_root, 'test'));
+    final Uri suiteUrl = url.resolveUri(_fileSystem.path.toUri('${_fileSystem.path.withoutExtension(pathFromTest)}.html'));
+    final String relativePath = _fileSystem.path.relative(_fileSystem.path.normalize(path), from: _fileSystem.currentDirectory.path);
+    if (_logger.isVerbose) {
+      _logger.printTrace('Loading test suite $relativePath.');
+    }
+
     final PoolResource lockResource = await _suiteLock.request();
 
     final Runtime browser = platform.runtime;
@@ -455,17 +463,23 @@ class FlutterWebPlatform extends PlatformPlugin {
       throw StateError('Load called on a closed FlutterWebPlatform');
     }
 
-    final String pathFromTest = _fileSystem.path.relative(path, from: _fileSystem.path.join(_root, 'test'));
-    final Uri suiteUrl = url.resolveUri(_fileSystem.path.toUri('${_fileSystem.path.withoutExtension(pathFromTest)}.html'));
-    final String relativePath = _fileSystem.path.relative(_fileSystem.path.normalize(path), from: _fileSystem.currentDirectory.path);
+    if (_logger.isVerbose) {
+      _logger.printTrace('Running test suite $relativePath.');
+    }
+
     final RunnerSuite suite = await _browserManager!.load(relativePath, suiteUrl, suiteConfig, message, onDone: () async {
       await _browserManager!.close();
       _browserManager = null;
       lockResource.release();
+      if (_logger.isVerbose) {
+        _logger.printTrace('Test suite $relativePath finished.');
+      }
     });
+
     if (_closed) {
       throw StateError('Load called on a closed FlutterWebPlatform');
     }
+
     return suite;
   }
 

packages/flutter_tools/lib/src/web/chrome.dart

@@ -225,10 +225,10 @@ class ChromiumLauncher {
       url,
     ];
 
-    final Process? process = await _spawnChromiumProcess(args, chromeExecutable);
+    final Process process = await _spawnChromiumProcess(args, chromeExecutable);
 
     // When the process exits, copy the user settings back to the provided data-dir.
-    if (process != null && cacheDir != null) {
+    if (cacheDir != null) {
       unawaited(process.exitCode.whenComplete(() {
         _cacheUserSessionInformation(userDataDir, cacheDir);
         // cleanup temp dir
@@ -245,10 +245,11 @@ class ChromiumLauncher {
       url: url,
       process: process,
       chromiumLauncher: this,
+      logger: _logger,
     ), skipCheck);
   }
 
-  Future<Process?> _spawnChromiumProcess(List<String> args, String chromeExecutable) async {
+  Future<Process> _spawnChromiumProcess(List<String> args, String chromeExecutable) async {
     if (_operatingSystemUtils.hostPlatform == HostPlatform.darwin_arm64) {
       final ProcessResult result = _processManager.runSync(<String>['file', chromeExecutable]);
       // Check if ARM Chrome is installed.
@@ -469,25 +470,58 @@ class Chromium {
     this.debugPort,
     this.chromeConnection, {
     this.url,
-    Process? process,
+    required Process process,
     required ChromiumLauncher chromiumLauncher,
+    required Logger logger,
   })  : _process = process,
-        _chromiumLauncher = chromiumLauncher;
+        _chromiumLauncher = chromiumLauncher,
+        _logger = logger;
 
   final String? url;
   final int debugPort;
-  final Process? _process;
+  final Process _process;
   final ChromeConnection chromeConnection;
   final ChromiumLauncher _chromiumLauncher;
+  final Logger _logger;
 
-  Future<int?> get onExit async => _process?.exitCode;
+  /// Resolves to browser's main process' exit code, when the browser exits.
+  Future<int> get onExit async => _process.exitCode;
+
+  /// The main Chromium process that represents this instance of Chromium.
+  ///
+  /// Killing this process should result in the browser exiting.
+  @visibleForTesting
+  Process get process => _process;
 
+  /// Closes all connections to the browser and asks the browser to exit.
   Future<void> close() async {
+    if (_logger.isVerbose) {
+      _logger.printTrace('Shutting down Chromium.');
+    }
     if (_chromiumLauncher.hasChromeInstance) {
       _chromiumLauncher.currentCompleter = Completer<Chromium>();
     }
     chromeConnection.close();
-    _process?.kill();
-    await _process?.exitCode;
+
+    // Try to exit Chromium nicely using SIGTERM, before exiting it rudely using
+    // SIGKILL. Wait no longer than 5 seconds for Chromium to exit before
+    // falling back to SIGKILL, and then to a warning message.
+    ProcessSignal.sigterm.kill(_process);
+    await _process.exitCode.timeout(const Duration(seconds: 5), onTimeout: () {
+      _logger.printWarning(
+        'Failed to exit Chromium (pid: ${_process.pid}) using SIGTERM. Will try '
+        'sending SIGKILL instead.'
+      );
+      ProcessSignal.sigkill.kill(_process);
+      return _process.exitCode.timeout(const Duration(seconds: 5), onTimeout: () async {
+        _logger.printWarning(
+          'Failed to exit Chromium (pid: ${_process.pid}) using SIGKILL. Giving '
+          'up. Will continue, assuming Chromium has exited successfully, but '
+          'it is possible that this left a dangling Chromium process running '
+          'on the system.'
+        );
+        return 0;
+      });
+    });
   }
 }

packages/flutter_tools/test/general.shard/ios/ios_device_start_prebuilt_test.dart

@@ -213,7 +213,7 @@ void main() {
     completer.complete();
     expect(secondLaunchResult.started, true);
     expect(secondLaunchResult.hasVmService, true);
-    expect(await device.stopApp(iosApp), false);
+    expect(await device.stopApp(iosApp), true);
   });
 
   testWithoutContext('IOSDevice.startApp launches in debug mode via log reading on <iOS 13', () async {
@@ -291,7 +291,7 @@ void main() {
 
     expect(launchResult.started, true);
     expect(launchResult.hasVmService, true);
-    expect(await device.stopApp(iosApp), false);
+    expect(await device.stopApp(iosApp), true);
     expect(logger.errorText, contains('The Dart VM Service was not discovered after 30 seconds. This is taking much longer than expected...'));
     expect(utf8.decoder.convert(stdin.writes.first), contains('process interrupt'));
     completer.complete();
@@ -336,7 +336,7 @@ void main() {
 
     expect(launchResult.started, true);
     expect(launchResult.hasVmService, true);
-    expect(await device.stopApp(iosApp), false);
+    expect(await device.stopApp(iosApp), true);
     expect(logger.errorText, contains('The Dart VM Service was not discovered after 45 seconds. This is taking much longer than expected...'));
     expect(logger.errorText, contains('Click "Allow" to the prompt asking if you would like to find and connect devices on your local network.'));
     completer.complete();
@@ -388,7 +388,7 @@ void main() {
     expect(processManager, hasNoRemainingExpectations);
     expect(launchResult.started, true);
     expect(launchResult.hasVmService, true);
-    expect(await device.stopApp(iosApp), false);
+    expect(await device.stopApp(iosApp), true);
   });
 
   testWithoutContext('IOSDevice.startApp does not retry when ios-deploy loses connection if not in CI', () async {

packages/flutter_tools/test/general.shard/resident_web_runner_test.dart

@@ -39,6 +39,7 @@ import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';
 
 import '../src/common.dart';
 import '../src/context.dart';
+import '../src/fake_process_manager.dart';
 import '../src/fake_vm_services.dart';
 
 const List<VmServiceExpectation> kAttachLogExpectations =
@@ -620,8 +621,9 @@ void main() {
     ]);
     setupMocks();
     final TestChromiumLauncher chromiumLauncher = TestChromiumLauncher();
+    final FakeProcess process = FakeProcess();
     final Chromium chrome =
-        Chromium(1, chromeConnection, chromiumLauncher: chromiumLauncher);
+        Chromium(1, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
     chromiumLauncher.setInstance(chrome);
 
     flutterDevice.device = GoogleChromeDevice(
@@ -687,8 +689,9 @@ void main() {
     ]);
     setupMocks();
     final TestChromiumLauncher chromiumLauncher = TestChromiumLauncher();
+    final FakeProcess process = FakeProcess();
     final Chromium chrome =
-        Chromium(1, chromeConnection, chromiumLauncher: chromiumLauncher);
+        Chromium(1, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
     chromiumLauncher.setInstance(chrome);
 
     flutterDevice.device = GoogleChromeDevice(
@@ -1025,8 +1028,9 @@ void main() {
     setupMocks();
     final FakeChromeConnection chromeConnection = FakeChromeConnection();
     final TestChromiumLauncher chromiumLauncher = TestChromiumLauncher();
+    final FakeProcess process = FakeProcess();
     final Chromium chrome =
-        Chromium(1, chromeConnection, chromiumLauncher: chromiumLauncher);
+        Chromium(1, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
     chromiumLauncher.setInstance(chrome);
 
     flutterDevice.device = GoogleChromeDevice(

packages/flutter_tools/test/src/fake_process_manager.dart

@@ -213,10 +213,17 @@ class FakeProcess implements io.Process {
   /// The raw byte content of stdout.
   final List<int> _stdout;
 
+  /// The list of [kill] signals this process received so far.
+  @visibleForTesting
+  List<io.ProcessSignal> get signals => _signals;
+  final List<io.ProcessSignal> _signals = <io.ProcessSignal>[];
+
   @override
   bool kill([io.ProcessSignal signal = io.ProcessSignal.sigterm]) {
+    _signals.add(signal);
+
     // Killing a fake process has no effect.
-    return false;
+    return true;
   }
 }
 
@@ -400,6 +407,7 @@ abstract class FakeProcessManager implements ProcessManager {
     if (fakeProcess == null) {
       return false;
     }
+    fakeProcess.kill(signal);
     if (fakeProcess._completer != null) {
       fakeProcess._completer.complete();
     }

packages/flutter_tools/test/web.shard/chrome_test.dart

@@ -3,7 +3,9 @@
 // found in the LICENSE file.
 
 import 'dart:async';
+import 'dart:io' as io;
 
+import 'package:fake_async/fake_async.dart';
 import 'package:file/memory.dart';
 import 'package:file_testing/file_testing.dart';
 import 'package:flutter_tools/src/base/file_system.dart';
@@ -16,7 +18,7 @@ import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';
 
 import '../src/common.dart';
 import '../src/fake_process_manager.dart';
-import '../src/fakes.dart';
+import '../src/fakes.dart' hide FakeProcess;
 
 const List<String> kChromeArgs = <String>[
   '--disable-background-timer-throttling',
@@ -165,6 +167,116 @@ void main() {
     );
   });
 
+  testWithoutContext('exits normally using SIGTERM', () async {
+    final BufferLogger logger = BufferLogger.test();
+    final FakeAsync fakeAsync = FakeAsync();
+
+    fakeAsync.run((_) {
+      () async {
+        final FakeChromeConnection chromeConnection = FakeChromeConnection(maxRetries: 4);
+        final ChromiumLauncher chromiumLauncher = ChromiumLauncher(
+          fileSystem: fileSystem,
+          platform: platform,
+          processManager: processManager,
+          operatingSystemUtils: operatingSystemUtils,
+          browserFinder: findChromeExecutable,
+          logger: logger,
+        );
+
+        final FakeProcess process = FakeProcess(
+          duration: const Duration(seconds: 3),
+        );
+
+        final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
+
+        final Future<void> closeFuture = chrome.close();
+        fakeAsync.elapse(const Duration(seconds: 4));
+        await closeFuture;
+
+        expect(process.signals, <io.ProcessSignal>[io.ProcessSignal.sigterm]);
+      }();
+    });
+
+    fakeAsync.flushTimers();
+    expect(logger.warningText, isEmpty);
+  });
+
+  testWithoutContext('falls back to SIGKILL if SIGTERM did not work', () async {
+    final BufferLogger logger = BufferLogger.test();
+    final FakeAsync fakeAsync = FakeAsync();
+
+    fakeAsync.run((_) {
+      () async {
+        final FakeChromeConnection chromeConnection = FakeChromeConnection(maxRetries: 4);
+        final ChromiumLauncher chromiumLauncher = ChromiumLauncher(
+          fileSystem: fileSystem,
+          platform: platform,
+          processManager: processManager,
+          operatingSystemUtils: operatingSystemUtils,
+          browserFinder: findChromeExecutable,
+          logger: logger,
+        );
+
+        final FakeProcess process = FakeProcess(
+          duration: const Duration(seconds: 6),
+        );
+
+        final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
+
+        final Future<void> closeFuture = chrome.close();
+        fakeAsync.elapse(const Duration(seconds: 7));
+        await closeFuture;
+
+        expect(process.signals, <io.ProcessSignal>[io.ProcessSignal.sigterm, io.ProcessSignal.sigkill]);
+      }();
+    });
+
+    fakeAsync.flushTimers();
+    expect(
+      logger.warningText,
+      'Failed to exit Chromium (pid: 1234) using SIGTERM. Will try sending SIGKILL instead.\n',
+    );
+  });
+
+  testWithoutContext('falls back to a warning if SIGKILL did not work', () async {
+    final BufferLogger logger = BufferLogger.test();
+    final FakeAsync fakeAsync = FakeAsync();
+
+    fakeAsync.run((_) {
+      () async {
+        final FakeChromeConnection chromeConnection = FakeChromeConnection(maxRetries: 4);
+        final ChromiumLauncher chromiumLauncher = ChromiumLauncher(
+          fileSystem: fileSystem,
+          platform: platform,
+          processManager: processManager,
+          operatingSystemUtils: operatingSystemUtils,
+          browserFinder: findChromeExecutable,
+          logger: logger,
+        );
+
+        final FakeProcess process = FakeProcess(
+          duration: const Duration(seconds: 20),
+        );
+
+        final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
+
+        final Future<void> closeFuture = chrome.close();
+        fakeAsync.elapse(const Duration(seconds: 30));
+        await closeFuture;
+        expect(process.signals, <io.ProcessSignal>[io.ProcessSignal.sigterm, io.ProcessSignal.sigkill]);
+      }();
+    });
+
+    fakeAsync.flushTimers();
+    expect(
+      logger.warningText,
+      'Failed to exit Chromium (pid: 1234) using SIGTERM. Will try sending SIGKILL instead.\n'
+      'Failed to exit Chromium (pid: 1234) using SIGKILL. Giving up. Will continue, assuming '
+      'Chromium has exited successfully, but it is possible that this left a dangling Chromium '
+      'process running on the system.\n',
+    );
+  });
+
   testWithoutContext('does not crash if saving profile information fails due to a file system exception.', () async {
     final BufferLogger logger = BufferLogger.test();
     chromeLauncher = ChromiumLauncher(
@@ -656,7 +768,8 @@ void main() {
       browserFinder: findChromeExecutable,
       logger: logger,
     );
-    final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher);
+    final FakeProcess process = FakeProcess();
+    final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
     expect(await chromiumLauncher.connect(chrome, false), equals(chrome));
     expect(logger.errorText, isEmpty);
   });
@@ -672,7 +785,8 @@ void main() {
       browserFinder: findChromeExecutable,
       logger: logger,
     );
-    final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher);
+    final FakeProcess process = FakeProcess();
+    final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
     await expectToolExitLater(
       chromiumLauncher.connect(chrome, false),
         allOf(