[flutter_tools] Shut down Chromium cleanly using a command sent through the debug protocol (flutter#150645)

The previous approach of killing the Chromium parent process sometimes caused leaks of child processes on Windows.  The Browser.close command in the debug protocol will tell Chromium to shut down all of its processes.

Author: jason-simmons

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

@@ -415,7 +415,7 @@ class ChromiumLauncher {
     // connection is valid.
     if (!skipCheck) {
       try {
-        await _getFirstTab(chrome);
+        await chrome._validateChromeConnection();
       } on Exception catch (error, stackTrace) {
         _logger.printError('$error', stackTrace: stackTrace);
         await chrome.close();
@@ -427,40 +427,6 @@ class ChromiumLauncher {
     return chrome;
   }
 
-  /// Gets the first [chrome] tab.
-  ///
-  /// Retries getting tabs from Chrome for a few seconds and retries finding
-  /// the tab a few times. This reduces flakes caused by Chrome not returning
-  /// correct output if the call was too close to the start.
-  //
-  // TODO(ianh): remove the timeouts here, they violate our style guide.
-  // (We should just keep waiting forever, and print a warning when it's
-  // taking too long.)
-  Future<ChromeTab?> _getFirstTab(Chromium chrome) async {
-    const Duration retryFor = Duration(seconds: 2);
-    const int attempts = 5;
-
-    for (int i = 1; i <= attempts; i++) {
-      try {
-        final List<ChromeTab> tabs =
-          await chrome.chromeConnection.getTabs(retryFor: retryFor);
-
-        if (tabs.isNotEmpty) {
-          return tabs.first;
-        }
-        if (i == attempts) {
-          return null;
-        }
-      } on ConnectionException catch (_) {
-        if (i == attempts) {
-          rethrow;
-        }
-      }
-      await Future<void>.delayed(const Duration(milliseconds: 25));
-    }
-    return null;
-  }
-
   Future<Chromium> get connectedInstance => currentCompleter.future;
 }
 
@@ -483,6 +449,7 @@ class Chromium {
   final ChromeConnection chromeConnection;
   final ChromiumLauncher _chromiumLauncher;
   final Logger _logger;
+  bool _hasValidChromeConnection = false;
 
   /// Resolves to browser's main process' exit code, when the browser exits.
   Future<int> get onExit async => _process.exitCode;
@@ -493,6 +460,41 @@ class Chromium {
   @visibleForTesting
   Process get process => _process;
 
+  /// Gets the first [chrome] tab in order to verify that the connection to
+  /// the Chrome debug protocol is working properly.
+  ///
+  /// Retries getting tabs from Chrome for a few seconds and retries finding
+  /// the tab a few times. This reduces flakes caused by Chrome not returning
+  /// correct output if the call was too close to the start.
+  //
+  // TODO(ianh): remove the timeouts here, they violate our style guide.
+  // (We should just keep waiting forever, and print a warning when it's
+  // taking too long.)
+  Future<void> _validateChromeConnection() async {
+    const Duration retryFor = Duration(seconds: 2);
+    const int attempts = 5;
+
+    for (int i = 1; i <= attempts; i++) {
+      try {
+        final List<ChromeTab> tabs =
+          await chromeConnection.getTabs(retryFor: retryFor);
+
+        if (tabs.isNotEmpty) {
+          _hasValidChromeConnection = true;
+          return;
+        }
+        if (i == attempts) {
+          return;
+        }
+      } on ConnectionException catch (_) {
+        if (i == attempts) {
+          rethrow;
+        }
+      }
+      await Future<void>.delayed(const Duration(milliseconds: 25));
+    }
+  }
+
   /// Closes all connections to the browser and asks the browser to exit.
   Future<void> close() async {
     if (_logger.isVerbose) {
@@ -501,12 +503,31 @@ class Chromium {
     if (_chromiumLauncher.hasChromeInstance) {
       _chromiumLauncher.currentCompleter = Completer<Chromium>();
     }
+
+    // Send a command to shut down the browser cleanly.
+    Duration sigtermDelay = Duration.zero;
+    if (_hasValidChromeConnection) {
+      final ChromeTab? tab = await chromeConnection.getTab(
+          (_) => true, retryFor: const Duration(seconds: 1));
+      if (tab != null) {
+        final WipConnection wipConnection = await tab.connect();
+        await wipConnection.sendCommand('Browser.close');
+        await wipConnection.close();
+        sigtermDelay = const Duration(seconds: 1);
+      }
+    }
     chromeConnection.close();
+    _hasValidChromeConnection = false;
 
-    // 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);
+    // If the browser close command did not shut down the process, then try to
+    // exit Chromium using SIGTERM.
+    await _process.exitCode.timeout(sigtermDelay, onTimeout: () {
+      ProcessSignal.sigterm.kill(_process);
+      return 0;
+    });
+    // If the process still has not ended, then use SIGKILL. Wait up to 5
+    // seconds for Chromium to exit before falling back to SIGKILL and then to
+    // a warning message.
     await _process.exitCode.timeout(const Duration(seconds: 5), onTimeout: () {
       _logger.printWarning(
         'Failed to exit Chromium (pid: ${_process.pid}) using SIGTERM. Will try '

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

@@ -800,6 +800,26 @@ void main() {
           contains('<html> ...'),
         ));
   });
+
+  test('Chromium close sends browser close command', () async {
+    final BufferLogger logger = BufferLogger.test();
+    final List<String> commands = <String>[];
+    void onSendCommand(String cmd) { commands.add(cmd); }
+    final FakeChromeConnectionWithTab chromeConnection = FakeChromeConnectionWithTab(onSendCommand: onSendCommand);
+    final ChromiumLauncher chromiumLauncher = ChromiumLauncher(
+      fileSystem: fileSystem,
+      platform: platform,
+      processManager: processManager,
+      operatingSystemUtils: operatingSystemUtils,
+      browserFinder: findChromeExecutable,
+      logger: logger,
+    );
+    final FakeProcess process = FakeProcess();
+    final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
+    expect(await chromiumLauncher.connect(chrome, false), equals(chrome));
+    await chrome.close();
+    expect(commands, contains('Browser.close'));
+  });
 }
 
 /// Fake chrome connection that fails to get tabs a few times.
@@ -834,3 +854,52 @@ class FakeChromeConnection extends Fake implements ChromeConnection {
   @override
   void close() {}
 }
+
+typedef OnSendCommand = void Function(String);
+
+/// Fake chrome connection that returns a tab.
+class FakeChromeConnectionWithTab extends Fake implements ChromeConnection {
+  FakeChromeConnectionWithTab({OnSendCommand? onSendCommand})
+      : _tab = FakeChromeTab(onSendCommand);
+
+  final FakeChromeTab _tab;
+
+  @override
+  Future<ChromeTab?> getTab(bool Function(ChromeTab tab) accept, {Duration? retryFor}) async {
+    return _tab;
+  }
+
+  @override
+  Future<List<ChromeTab>> getTabs({Duration? retryFor}) async {
+    return <ChromeTab>[_tab];
+  }
+
+  @override
+  void close() {}
+}
+
+class FakeChromeTab extends Fake implements ChromeTab {
+  FakeChromeTab(this.onSendCommand);
+
+  OnSendCommand? onSendCommand;
+
+  @override
+  Future<WipConnection> connect({Function? onError}) async {
+    return FakeWipConnection(onSendCommand);
+  }
+}
+
+class FakeWipConnection extends Fake implements WipConnection {
+  FakeWipConnection(this.onSendCommand);
+
+  OnSendCommand? onSendCommand;
+
+  @override
+  Future<WipResponse> sendCommand(String method, [Map<String, dynamic>? params]) async {
+    onSendCommand?.call(method);
+    return WipResponse(<String, dynamic>{'id': 0, 'result': <String, dynamic>{}});
+  }
+
+  @override
+  Future<void> close() async {}
+}