From ea7fb0a8879d3dcd206cd99b7f6da44124246bd3 Mon Sep 17 00:00:00 2001 From: flutteractionsbot <154381524+flutteractionsbot@users.noreply.github.com> Date: Thu, 27 Feb 2025 13:34:02 -0800 Subject: [PATCH] [CP-stable][web] robustify safaridriver launch sequence (#164191) This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request) Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request. ### Issue Link: What is the link to the issue this cherry-pick is addressing? https://github.com/flutter/flutter/issues/150023 ### Changelog Description: Explain this cherry pick in one line that is accessible to most Flutter developers. See [best practices](https://github.com/flutter/flutter/blob/main/docs/releases/Hotfix-Documentation-Best-Practices.md) for examples Improve safaridriver launch process in Flutter's CI testing. ### Impact Description: What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping production app (the app crashes on launch) Improve stability of Flutter's macOS web tests in CI. No impact to customers, only release team. ### Workaround: Is there a workaround for this issue? Rerun. ### Risk: What is the risk level of this cherry-pick? ### Test Coverage: Are you confident that your fix is well-tested by automated tests? ### Validation Steps: What are the steps to validate that this fix works? N/A --- .../src/flutter/lib/web_ui/dev/pipeline.dart | 9 +- .../flutter/lib/web_ui/dev/safari_macos.dart | 189 ++++++++++++++---- .../lib/web_ui/dev/webdriver_browser.dart | 77 +------ 3 files changed, 169 insertions(+), 106 deletions(-) diff --git a/engine/src/flutter/lib/web_ui/dev/pipeline.dart b/engine/src/flutter/lib/web_ui/dev/pipeline.dart index 54cb5aa3b5b..471b48bcf8e 100644 --- a/engine/src/flutter/lib/web_ui/dev/pipeline.dart +++ b/engine/src/flutter/lib/web_ui/dev/pipeline.dart @@ -89,10 +89,11 @@ abstract class ProcessStep implements PipelineStep { } class _PipelineStepFailure { - _PipelineStepFailure(this.step, this.error); + _PipelineStepFailure(this.step, this.error, this.stackTrace); final PipelineStep step; final Object error; + final StackTrace stackTrace; } /// Executes a sequence of asynchronous tasks, typically as part of a build/test @@ -133,8 +134,8 @@ class Pipeline { _currentStepFuture = step.run(); try { await _currentStepFuture; - } catch (e) { - failures.add(_PipelineStepFailure(step, e)); + } catch (error, stackTrace) { + failures.add(_PipelineStepFailure(step, error, stackTrace)); } finally { _currentStep = null; } @@ -145,7 +146,7 @@ class Pipeline { _status = PipelineStatus.error; print('Pipeline experienced the following failures:'); for (final _PipelineStepFailure failure in failures) { - print(' "${failure.step.description}": ${failure.error}'); + print(' "${failure.step.description}": ${failure.error}\n${failure.stackTrace}'); } throw ToolExit('Test pipeline failed.'); } diff --git a/engine/src/flutter/lib/web_ui/dev/safari_macos.dart b/engine/src/flutter/lib/web_ui/dev/safari_macos.dart index a7405f54b70..73d0e6c86c8 100644 --- a/engine/src/flutter/lib/web_ui/dev/safari_macos.dart +++ b/engine/src/flutter/lib/web_ui/dev/safari_macos.dart @@ -7,11 +7,21 @@ import 'dart:convert'; import 'dart:io'; import 'package:test_api/backend.dart'; +import 'package:webdriver/async_io.dart' show WebDriver, createDriver; +import 'browser.dart'; import 'webdriver_browser.dart'; /// Provides an environment for the desktop variant of Safari running on macOS. -class SafariMacOsEnvironment extends WebDriverBrowserEnvironment { +class SafariMacOsEnvironment extends BrowserEnvironment { + static const Duration _waitBetweenRetries = Duration(seconds: 1); + static const int _maxRetryCount = 5; + + late int _portNumber; + late Process _driverProcess; + Uri get _driverUri => Uri(scheme: 'http', host: 'localhost', port: _portNumber); + WebDriver? webDriver; + @override final String name = 'Safari macOS'; @@ -21,21 +31,34 @@ class SafariMacOsEnvironment extends WebDriverBrowserEnvironment { @override String get packageTestConfigurationYamlFile => 'dart_test_safari.yaml'; - @override - Uri get driverUri => Uri(scheme: 'http', host: 'localhost', port: portNumber); - - late Process _driverProcess; - int _retryCount = 0; - static const int _waitBetweenRetryInSeconds = 1; - static const int _maxRetryCount = 10; - - @override - Future spawnDriverProcess() => - Process.start('safaridriver', ['-p', portNumber.toString()]); - @override Future prepare() async { - await _startDriverProcess(); + int retryCount = 0; + + while (true) { + try { + if (retryCount > 0) { + print('Retry #$retryCount'); + } + retryCount += 1; + await _startDriverProcess(); + return; + } catch (error, stackTrace) { + if (retryCount < _maxRetryCount) { + print(''' +Failed to start safaridriver: + +Error: $error +$stackTrace +'''); + print('Will try again.'); + await Future.delayed(_waitBetweenRetries); + } else { + print('Too many retries. Giving up.'); + rethrow; + } + } + } } /// Pick an unused port and start `safaridriver` using that port. @@ -45,36 +68,130 @@ class SafariMacOsEnvironment extends WebDriverBrowserEnvironment { /// again with a different port. Wait [_waitBetweenRetryInSeconds] seconds /// between retries. Try up to [_maxRetryCount] times. Future _startDriverProcess() async { - _retryCount += 1; - if (_retryCount > 1) { - await Future.delayed(const Duration(seconds: _waitBetweenRetryInSeconds)); - } - portNumber = await pickUnusedPort(); + _portNumber = await pickUnusedPort(); + print('Starting safaridriver on port $_portNumber'); - print('Attempt $_retryCount to start safaridriver on port $portNumber'); + try { + _driverProcess = await Process.start('safaridriver', ['-p', _portNumber.toString()]); - _driverProcess = await spawnDriverProcess(); + _driverProcess.stdout.transform(utf8.decoder).transform(const LineSplitter()).listen(( + String log, + ) { + print('[safaridriver] $log'); + }); - _driverProcess.stderr.transform(utf8.decoder).transform(const LineSplitter()).listen(( - String error, - ) { - print('[Webdriver][Error] $error'); - if (_retryCount > _maxRetryCount) { - print('[Webdriver][Error] Failed to start after $_maxRetryCount tries.'); - } else if (error.contains('Operation not permitted')) { - _driverProcess.kill(); - _startDriverProcess(); + _driverProcess.stderr.transform(utf8.decoder).transform(const LineSplitter()).listen(( + String error, + ) { + print('[safaridriver][error] $error'); + }); + + await _waitForSafariDriverServerReady(); + + // Smoke-test the web driver process by connecting to it and asking for a + // list of windows. It doesn't matter how many windows there are. + webDriver = await createDriver( + uri: _driverUri, + desired: {'browserName': packageTestRuntime.identifier}, + ); + + await webDriver!.windows.toList(); + } catch (_) { + print('safaridriver failed to start.'); + + final badDriver = webDriver; + webDriver = null; // let's not keep faulty driver around + + if (badDriver != null) { + // This means the launch process got to a point where a WebDriver + // instance was created, but it failed the smoke test. To make sure no + // stray driver sessions are left hanging, try to close the session. + try { + // The method is called "quit" but all it does is close the session. + // + // See: https://www.w3.org/TR/webdriver2/#delete-session + await badDriver.quit(); + } catch (error, stackTrace) { + // Just print. Do not rethrow. The attempt to close the session is + // only a best-effort thing. + print(''' +Failed to close driver session. Will try to kill the safaridriver process. + +Error: $error +$stackTrace +'''); + } } - }); - _driverProcess.stdout.transform(utf8.decoder).transform(const LineSplitter()).listen(( - String log, - ) { - print('[Webdriver] $log'); - }); + + // Try to kill gracefully using SIGTERM first. + _driverProcess.kill(); + await _driverProcess.exitCode.timeout( + const Duration(seconds: 2), + onTimeout: () async { + // If the process fails to exit gracefully in a reasonable amount of + // time, kill it forcefully. + print('safaridriver failed to exit normally. Killing with SIGKILL.'); + _driverProcess.kill(ProcessSignal.sigkill); + return 0; + }, + ); + + // Rethrow the error to allow the caller to retry, if need be. + rethrow; + } + } + + /// The Safari Driver process cannot instantly spawn a server, so this function + /// attempts to connect to the server in a loop until it succeeds. + /// + /// A healthy driver process is expected to respond to a `GET /status` HTTP + /// request with `{value: {ready: true}}` JSON response. + /// + /// See also: https://www.w3.org/TR/webdriver2/#status + Future _waitForSafariDriverServerReady() async { + // Wait just a tiny bit before connecting for the very first time because + // frequently safaridriver isn't quick enough to bring up the server. + // + // 100ms seems enough in most cases, but feel free to revisit this. + await Future.delayed(const Duration(milliseconds: 100)); + + int retryCount = 0; + while (true) { + retryCount += 1; + final httpClient = HttpClient(); + try { + final request = await httpClient.get('localhost', _portNumber, '/status'); + final response = await request.close(); + final stringData = await response.transform(utf8.decoder).join(); + final jsonResponse = json.decode(stringData) as Map; + final value = jsonResponse['value']! as Map; + final ready = value['ready']! as bool; + if (ready) { + break; + } + } catch (_) { + if (retryCount < 10) { + print('safaridriver not ready yet. Waiting...'); + await Future.delayed(const Duration(milliseconds: 100)); + } else { + print( + 'safaridriver failed to reach ready state in a reasonable amount of time. Giving up.', + ); + rethrow; + } + } + } + } + + @override + Future launchBrowserInstance(Uri url, {bool debug = false}) async { + return WebDriverBrowser(webDriver!, url); } @override Future cleanup() async { + // WebDriver.quit() is not called here, because that's done in + // WebDriverBrowser.close(). _driverProcess.kill(); } } diff --git a/engine/src/flutter/lib/web_ui/dev/webdriver_browser.dart b/engine/src/flutter/lib/web_ui/dev/webdriver_browser.dart index efe0e023f6f..ab4d32e3314 100644 --- a/engine/src/flutter/lib/web_ui/dev/webdriver_browser.dart +++ b/engine/src/flutter/lib/web_ui/dev/webdriver_browser.dart @@ -3,80 +3,25 @@ // found in the LICENSE file. import 'dart:async'; -import 'dart:convert'; import 'dart:io'; import 'dart:math'; import 'package:image/image.dart'; -import 'package:webdriver/async_io.dart' show WebDriver, createDriver; +import 'package:webdriver/async_io.dart' show WebDriver; import 'browser.dart'; -abstract class WebDriverBrowserEnvironment extends BrowserEnvironment { - late int portNumber; - late final Process _driverProcess; - - Future spawnDriverProcess(); - Uri get driverUri; - - /// Finds and returns an unused port on the test host in the local port range. - Future pickUnusedPort() async { - // Use bind to allocate an unused port, then unbind from that port to - // make it available for use. - final ServerSocket socket = await ServerSocket.bind('localhost', 0); - final int port = socket.port; - await socket.close(); - - return port; - } - - @override - Future prepare() async { - portNumber = await pickUnusedPort(); - - _driverProcess = await spawnDriverProcess(); - - _driverProcess.stderr.transform(utf8.decoder).transform(const LineSplitter()).listen(( - String error, - ) { - print('[Webdriver][Error] $error'); - }); - - _driverProcess.stdout.transform(utf8.decoder).transform(const LineSplitter()).listen(( - String log, - ) { - print('[Webdriver] $log'); - }); - } - - @override - Future cleanup() async { - _driverProcess.kill(); - } - - @override - Future launchBrowserInstance(Uri url, {bool debug = false}) async { - while (true) { - try { - final WebDriver driver = await createDriver( - uri: driverUri, - desired: {'browserName': packageTestRuntime.identifier}, - ); - return WebDriverBrowser(driver, url); - } on SocketException { - // Sometimes we may try to connect before the web driver port is ready. - // So we should retry here. Note that if there was some issue with the - // webdriver process, we may loop infinitely here, so we're relying on - // the test timeout to kill us if it takes too long to connect. - print('Failed to connect to webdriver process. Retrying in 100 ms'); - await Future.delayed(const Duration(milliseconds: 100)); - } catch (exception) { - rethrow; - } - } - } +/// Finds and returns an unused port on the test host in the local port range. +Future pickUnusedPort() async { + // Use bind to allocate an unused port, then unbind from that port to + // make it available for use. + final ServerSocket socket = await ServerSocket.bind('localhost', 0); + final int port = socket.port; + await socket.close(); + return port; } +/// A browser managed through a WebDriver connection. class WebDriverBrowser extends Browser { WebDriverBrowser(this._driver, this._url) { _driver.get(_url); @@ -104,7 +49,7 @@ class WebDriverBrowser extends Browser { _shouldStopActivating = true; await _activateLoopFuture; - await (await _driver.window).close(); + await _driver.quit(); if (!_onExitCompleter.isCompleted) { _onExitCompleter.complete(); }