[web] robustify safaridriver launch sequence (flutter#162919)

Improve safaridriver launch sequence as follows:

- Fix retry: the previous version would call `_startDriverProcess`
recursively from within a listener to the stderr output. But by then the
outer `_startDriverProcess` call would have completed its future, so the
retry mechanism would kick in while tests are already running.
- Wait for `safaridriver` server process to start listening and
responding to WebDriver commands (using `/status` as the smoke test).
- Smoke-test the driver session by attempting to list all windows
(`WebDriver.windows`).
- When `safaridriver` fails to pass all of the above checks, both close
the session and kill the `safaridriver` process. Killing the process
alone leaves Safari windows open. Closing the session alone leaves
`safaridriver` processes open.
- When tests are finished use `quit()` instead of `window.close()`,
because the latter does not close the session.

Author: yjbanov

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.');
     }

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';
 
@@ -22,20 +32,33 @@ class SafariMacOsEnvironment extends WebDriverBrowserEnvironment {
   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;
+  Future<void> prepare() async {
+    int retryCount = 0;
 
-  @override
-  Future<Process> spawnDriverProcess() =>
-      Process.start('safaridriver', <String>['-p', portNumber.toString()]);
+    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:
 
-  @override
-  Future<void> prepare() async {
-    await _startDriverProcess();
+Error: $error
+$stackTrace
+''');
+          print('Will try again.');
+          await Future<void>.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<void> _startDriverProcess() async {
-    _retryCount += 1;
-    if (_retryCount > 1) {
-      await Future<void>.delayed(const Duration(seconds: _waitBetweenRetryInSeconds));
-    }
-    portNumber = await pickUnusedPort();
+    _portNumber = await pickUnusedPort();
+    print('Starting safaridriver on port $_portNumber');
+
+    try {
+      _driverProcess = await Process.start('safaridriver', <String>['-p', _portNumber.toString()]);
+
+      _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('[safaridriver][error] $error');
+      });
+
+      await _waitForSafariDriverServerReady();
 
-    print('Attempt $_retryCount to start safaridriver on port $portNumber');
+      // 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: <String, dynamic>{'browserName': packageTestRuntime.identifier},
+      );
 
-    _driverProcess = await spawnDriverProcess();
+      await webDriver!.windows.toList();
+    } catch (_) {
+      print('safaridriver failed to start.');
 
-    _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();
+      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<void> _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<void>.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<String, Object?>;
+        final value = jsonResponse['value']! as Map<String, Object?>;
+        final ready = value['ready']! as bool;
+        if (ready) {
+          break;
+        }
+      } catch (_) {
+        if (retryCount < 10) {
+          print('safaridriver not ready yet. Waiting...');
+          await Future<void>.delayed(const Duration(milliseconds: 100));
+        } else {
+          print(
+            'safaridriver failed to reach ready state in a reasonable amount of time. Giving up.',
+          );
+          rethrow;
+        }
+      }
+    }
+  }
+
+  @override
+  Future<Browser> launchBrowserInstance(Uri url, {bool debug = false}) async {
+    return WebDriverBrowser(webDriver!, url);
   }
 
   @override
   Future<void> cleanup() async {
+    // WebDriver.quit() is not called here, because that's done in
+    // WebDriverBrowser.close().
     _driverProcess.kill();
   }
 }

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<Process> spawnDriverProcess();
-  Uri get driverUri;
-
-  /// Finds and returns an unused port on the test host in the local port range.
-  Future<int> 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<void> 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<void> cleanup() async {
-    _driverProcess.kill();
-  }
-
-  @override
-  Future<Browser> launchBrowserInstance(Uri url, {bool debug = false}) async {
-    while (true) {
-      try {
-        final WebDriver driver = await createDriver(
-          uri: driverUri,
-          desired: <String, dynamic>{'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<void>.delayed(const Duration(milliseconds: 100));
-      } catch (exception) {
-        rethrow;
-      }
-    }
-  }
+/// Finds and returns an unused port on the test host in the local port range.
+Future<int> 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();
     }