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

This implements flutter/flutter#132654 (comment), 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 flutter/flutter#132654

Author: yjbanov

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();
     }