From 371aadd822a258100f7481be84ddd6969ffba82c Mon Sep 17 00:00:00 2001 From: Yegor Date: Thu, 28 Sep 2023 15:28:31 -0700 Subject: [PATCH] [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 | 18 ++- .../lib/src/test/flutter_web_platform.dart | 20 ++- .../flutter_tools/lib/src/web/chrome.dart | 52 ++++++-- .../ios/ios_device_start_prebuilt_test.dart | 8 +- .../resident_web_runner_test.dart | 10 +- .../test/src/fake_process_manager.dart | 10 +- .../test/web.shard/chrome_test.dart | 120 +++++++++++++++++- 7 files changed, 212 insertions(+), 26 deletions(-) diff --git a/packages/flutter_tools/lib/src/base/io.dart b/packages/flutter_tools/lib/src/base/io.dart index 2a6352737b1..83473499a91 100644 --- a/packages/flutter_tools/lib/src/base/io.dart +++ b/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(); } diff --git a/packages/flutter_tools/lib/src/test/flutter_web_platform.dart b/packages/flutter_tools/lib/src/test/flutter_web_platform.dart index ec99e517b06..6400b7a0593 100644 --- a/packages/flutter_tools/lib/src/test/flutter_web_platform.dart +++ b/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; } diff --git a/packages/flutter_tools/lib/src/web/chrome.dart b/packages/flutter_tools/lib/src/web/chrome.dart index 3d2ed0be9b5..f35a6634a2d 100644 --- a/packages/flutter_tools/lib/src/web/chrome.dart +++ b/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 _spawnChromiumProcess(List args, String chromeExecutable) async { + Future _spawnChromiumProcess(List args, String chromeExecutable) async { if (_operatingSystemUtils.hostPlatform == HostPlatform.darwin_arm64) { final ProcessResult result = _processManager.runSync(['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 get onExit async => _process?.exitCode; + /// Resolves to browser's main process' exit code, when the browser exits. + Future 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 close() async { + if (_logger.isVerbose) { + _logger.printTrace('Shutting down Chromium.'); + } if (_chromiumLauncher.hasChromeInstance) { _chromiumLauncher.currentCompleter = Completer(); } 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; + }); + }); } } diff --git a/packages/flutter_tools/test/general.shard/ios/ios_device_start_prebuilt_test.dart b/packages/flutter_tools/test/general.shard/ios/ios_device_start_prebuilt_test.dart index 37edf3ebe38..90f834081b2 100644 --- a/packages/flutter_tools/test/general.shard/ios/ios_device_start_prebuilt_test.dart +++ b/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 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( diff --git a/packages/flutter_tools/test/src/fake_process_manager.dart b/packages/flutter_tools/test/src/fake_process_manager.dart index 427f19a5233..305a37cbf96 100644 --- a/packages/flutter_tools/test/src/fake_process_manager.dart +++ b/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 _stdout; + /// The list of [kill] signals this process received so far. + @visibleForTesting + List get signals => _signals; + final List _signals = []; + @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(); } diff --git a/packages/flutter_tools/test/web.shard/chrome_test.dart b/packages/flutter_tools/test/web.shard/chrome_test.dart index 0c6c1ea4f0b..f9611f4e7ee 100644 --- a/packages/flutter_tools/test/web.shard/chrome_test.dart +++ b/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 kChromeArgs = [ '--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 closeFuture = chrome.close(); + fakeAsync.elapse(const Duration(seconds: 4)); + await closeFuture; + + expect(process.signals, [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 closeFuture = chrome.close(); + fakeAsync.elapse(const Duration(seconds: 7)); + await closeFuture; + + expect(process.signals, [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 closeFuture = chrome.close(); + fakeAsync.elapse(const Duration(seconds: 30)); + await closeFuture; + expect(process.signals, [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(