From 9afd397cd4b996b896cc49fa21d57b60aff6bf70 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Tue, 25 Jun 2024 14:43:02 -0700 Subject: [PATCH] [flutter_tools] Shut down Chromium cleanly using a command sent through the debug protocol (#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. --- .../flutter_tools/lib/src/web/chrome.dart | 101 +++++++++++------- .../test/web.shard/chrome_test.dart | 69 ++++++++++++ 2 files changed, 130 insertions(+), 40 deletions(-) diff --git a/packages/flutter_tools/lib/src/web/chrome.dart b/packages/flutter_tools/lib/src/web/chrome.dart index f35a6634a2d..90af6e5cf8c 100644 --- a/packages/flutter_tools/lib/src/web/chrome.dart +++ b/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 _getFirstTab(Chromium chrome) async { - const Duration retryFor = Duration(seconds: 2); - const int attempts = 5; - - for (int i = 1; i <= attempts; i++) { - try { - final List 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.delayed(const Duration(milliseconds: 25)); - } - return null; - } - Future 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 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 _validateChromeConnection() async { + const Duration retryFor = Duration(seconds: 2); + const int attempts = 5; + + for (int i = 1; i <= attempts; i++) { + try { + final List 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.delayed(const Duration(milliseconds: 25)); + } + } + /// Closes all connections to the browser and asks the browser to exit. Future close() async { if (_logger.isVerbose) { @@ -501,12 +503,31 @@ class Chromium { if (_chromiumLauncher.hasChromeInstance) { _chromiumLauncher.currentCompleter = Completer(); } - chromeConnection.close(); - // 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); + // 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; + + // 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 ' diff --git a/packages/flutter_tools/test/web.shard/chrome_test.dart b/packages/flutter_tools/test/web.shard/chrome_test.dart index f9611f4e7ee..08bc8a0502f 100644 --- a/packages/flutter_tools/test/web.shard/chrome_test.dart +++ b/packages/flutter_tools/test/web.shard/chrome_test.dart @@ -800,6 +800,26 @@ void main() { contains(' ...'), )); }); + + test('Chromium close sends browser close command', () async { + final BufferLogger logger = BufferLogger.test(); + final List commands = []; + 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 getTab(bool Function(ChromeTab tab) accept, {Duration? retryFor}) async { + return _tab; + } + + @override + Future> getTabs({Duration? retryFor}) async { + return [_tab]; + } + + @override + void close() {} +} + +class FakeChromeTab extends Fake implements ChromeTab { + FakeChromeTab(this.onSendCommand); + + OnSendCommand? onSendCommand; + + @override + Future connect({Function? onError}) async { + return FakeWipConnection(onSendCommand); + } +} + +class FakeWipConnection extends Fake implements WipConnection { + FakeWipConnection(this.onSendCommand); + + OnSendCommand? onSendCommand; + + @override + Future sendCommand(String method, [Map? params]) async { + onSendCommand?.call(method); + return WipResponse({'id': 0, 'result': {}}); + } + + @override + Future close() async {} +}