From e8e6b06c3c0eba251cafcd1892a7d183bb1ac481 Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinkle@fastmail.com>
Date: Sun, 5 Jan 2025 02:52:01 +0000
Subject: [PATCH] Add safari browser, using AppleScript to start/stop

---
 .github/workflows/CI.yaml |   4 +
 ARCHITECTURE.md           |  27 ++++--
 eslint.config.js          |   2 +
 src/browsers.js           | 173 ++++++++++++++++++++++++++++++++++++--
 src/server.js             |  17 ++--
 src/util.js               |   3 +
 6 files changed, 207 insertions(+), 19 deletions(-)

diff --git a/.github/workflows/CI.yaml b/.github/workflows/CI.yaml
index bd9ac82..a5f478a 100644
--- a/.github/workflows/CI.yaml
+++ b/.github/workflows/CI.yaml
@@ -57,3 +57,7 @@ jobs:
 
       - name: Check system browsers
         run: node bin/qtap.js -v -b firefox -b chrome -b chromium -b edge test/pass.html
+
+      - name: Check system browsers (Safari)
+        if: ${{ runner.os == 'macOS' }}
+        run: node bin/qtap.js -v -b safari test/pass.html
diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md
index ff0678d..cc95b38 100644
--- a/ARCHITECTURE.md
+++ b/ARCHITECTURE.md
@@ -69,12 +69,13 @@ One of the passed parameters is a standard [`AbortSignal` object](https://develo
 
 ```js
 // Using our utility
-async function myBrowser(url, signal, logger) {
+async function myBrowser (url, signal, logger) {
   await LocalBrowser.spawn(['/bin/mybrowser'], ['-headless', url], signal, logger);
 }
 
-// Minimal custom implementation on native Node.js
-async function myBrowser(url, signal, logger) {
+// Minimal sub process
+import child_process from 'node:child_process';
+async function myBrowser (url, signal, logger) {
   logger.debug('Spawning /bin/mybrowser');
   const spawned = child_process.spawn('/bin/mybrowser', ['-headless', url], { signal });
   await new Promise((resolve, reject) => {
@@ -82,6 +83,19 @@ async function myBrowser(url, signal, logger) {
     spawned.on('exit', (code) => reject(new Error(`Process exited ${code}`)));
   });
 }
+
+// Minimal custom
+async function myBrowser (url, signal, logger) {
+  // * start browser and navigate to `url`
+  // * if you encounter problems, throw
+  await new Promise((resolve, reject) => {
+    // * once browser has stopped, call resolve()
+    // * if you encounter problems, call reject()
+    signal.addEventListener('abort', () => {
+      // stop browser
+    });
+  });
+}
 ```
 
 Alternatives considered:
@@ -150,14 +164,13 @@ Alternatives considered:
   // Using our utility
   import qtap from 'qtap';
 
-  function myBrowser (url, signal, logger) {
+  async function myBrowser (url, signal, logger) {
     await qtap.LocalBrowser.spawn(['/bin/mybrowser'], ['-headless', url], signal, logger );
   }
 
-  // Minimal custom implementation
+  // Minimal sub process
   import child_process from 'node:child_process';
-
-  function myBrowser (url, signal, logger) {
+  async function myBrowser (url, signal, logger) {
     const spawned = child_process.spawn('/bin/mybrowser', ['-headless', url], { signal });
     await new Promise((resolve, reject) => {
       spawned.on('error', (error) => {
diff --git a/eslint.config.js b/eslint.config.js
index 2967c1e..3272e57 100644
--- a/eslint.config.js
+++ b/eslint.config.js
@@ -15,6 +15,8 @@ export default [
     rules: {
       'comma-dangle': 'off',
       'multiline-ternary': 'off',
+      'no-throw-literal': 'off',
+      'object-shorthand': 'off',
       'operator-linebreak': ['error', 'before'],
     }
   }
diff --git a/src/browsers.js b/src/browsers.js
index 64e9d36..10c6fd2 100644
--- a/src/browsers.js
+++ b/src/browsers.js
@@ -6,7 +6,7 @@ import os from 'node:os';
 import path from 'node:path';
 
 import which from 'which';
-import { concatGenFn } from './util.js';
+import { concatGenFn, globalSignal } from './util.js';
 
 const QTAP_DEBUG = process.env.QTAP_DEBUG === '1';
 const tempDirs = [];
@@ -96,7 +96,10 @@ const LocalBrowser = {
   /**
    * Create a new temporary directory and return its name.
    *
-   * The newly created directory will automatically will cleaned up.
+   * This creates subdirectories inside Node.js `os.tmpdir`, which honors
+   * any TMPDIR, TMP, or TEMP environment variable.
+   *
+   * The newly created directory is automatically cleaned up at the end of the process.
    *
    * @returns {string}
    */
@@ -279,12 +282,172 @@ async function chromium (paths, url, signal, logger) {
       '--disable-gpu',
       '--disable-dev-shm-usage'
     ]),
-    ...(process.env.CHROMIUM_FLAGS ? process.env.CHROMIUM_FLAGS.split(/\s+/) : []),
+    ...(process.env.CHROMIUM_FLAGS ? process.env.CHROMIUM_FLAGS.split(/\s+/) : (
+      process.env.CI ? ['--no-sandbox'] : [])
+    ),
     url
   ];
   await LocalBrowser.spawn(paths, args, signal, logger);
 }
 
+/**
+ * Known approaches:
+ *
+ * - `Safari <file>`. This does not allow URLs. Safari allows only local files to be passed.
+ *
+ * - `Safari redirect.html`, without other arguments, worked from 2012-2018, as used by Karma.
+ *   This "trampoline" approach involves creating a temporary HTML file
+ *   with `<script>window.location='<url>';</script>`, which we open instead.
+ *   https://github.com/karma-runner/karma-safari-launcher/blob/v1.0.0/index.js
+ *   https://github.com/karma-runner/karma/blob/v0.3.5/lib/launcher.js#L213
+ *   https://github.com/karma-runner/karma/commit/5513fd66ae
+ *
+ *   This is no longer viable after macOS 10.14 Mojave, because macOS SIP prompts the user
+ *   due to our temporary file being outside `~/Library/Containers/com.apple.Safari`.
+ *   https://github.com/karma-runner/karma-safari-launcher/issues/29
+ *
+ * - `open -F -W -n -b com.apple.Safari <url>`. This starts correctly, but doesn't expose
+ *   a PID to cleanly end the process.
+ *   https://github.com/karma-runner/karma-safari-launcher/issues/29
+ *
+ * - `Safari container/redirect.html`. macOS SIP denies this by default for the same reason.
+ *   But, as long as you grant an exemption to Terminal to write to Safari's container, or
+ *   grant it Full Disk Access, this is viable.
+ *   https://github.com/flutter/engine/pull/27567
+ *   https://github.com/marcoscaceres/karma-safaritechpreview-launcher/issues/7
+ *
+ *   It seems that GitHub CI has pre-approved limited access in its macOS images, to make
+ *   this work [1][2]. This might be viable if it is tolerable to prompt on first local use,
+ *   and require granting said access to the Terminal in general (which has lasting
+ *   consequences beyond QTap).
+ *
+ * - native app Swift/ObjectiveC proxy. This reportedly works but requires
+ *   a binary which requires compilation and makes auditing significantly harder.
+ *   https://github.com/karma-runner/karma-safari-launcher/issues/29
+ *   https://github.com/muthu90ec/karma-safarinative-launcher/
+ *
+ * - `osascript -e <script>`
+ *   As of macOS 13 Ventura (or earlier?), this results in a prompt for
+ *   "Terminal wants access to control Safari", from which osascript will eventually
+ *   timeout and report "Safari got an error: AppleEvent timed out".
+ *
+ *   While past discussions suggest that GitHub CI has this pre-approved [1][2],
+ *   as of writing in Jan 2025 with macOS 13 images, this approval does not include
+ *   access from Terminal to Safari, thus causing the same "AppleEvent timed out".
+ *
+ *   https://github.com/brandonocasey/karma-safari-applescript-launcher
+ *   https://github.com/brandonocasey/karma-safari-applescript-launcher/issues/5
+ *
+ * - `osascript MyScript.scpt`. This avoids the need for quote escaping in the URL, by
+ *   injecting it properly as a parameter instead. Used by Google's karma-webkit-launcher
+ *   https://github.com/google/karma-webkit-launcher/commit/31a2ad8037
+ *
+ * - `safaridriver -p <port>`, and then make an HTTP request to create a session,
+ *   navigate the session, and to delete the session. This addresses all the concerns,
+ *   and seems to be the best as of 2025. The only downside is that it requires a bit
+ *   more code (available port, and HTTP requests).
+ *   https://github.com/flutter/engine/pull/33757
+ *
+ * See also:
+ * - Unresolved as of writing, https://github.com/testem/testem/issues/1387
+ * - Unresolved as of writing, https://github.com/emberjs/data/issues/7170
+ *
+ * [1]: https://github.com/actions/runner-images/issues/4201
+ * [2]: https://github.com/actions/runner-images/issues/7531
+ */
+let pSafariDriverPort = null;
+
+async function launchSafariDriver (safaridriverBin, logger) {
+  async function findAvailablePort () {
+    const net = await import('node:net');
+    return new Promise((resolve, reject) => {
+      const srv = net.createServer();
+      srv.listen(0, () => {
+        const port = srv.address().port;
+        srv.close(() => resolve(port));
+      });
+    });
+  }
+
+  const port = await findAvailablePort();
+  LocalBrowser.spawn(safaridriverBin, ['-p', port], globalSignal, logger);
+  return port;
+}
+
+async function safari (url, signal, logger) {
+  if (!pSafariDriverPort) {
+    // Support overriding via SAFARIDRIVER_BIN to Safari Technology Preview.
+    // https://developer.apple.com/documentation/webkit/testing-with-webdriver-in-safari
+    const safaridriverBin = process.env.SAFARIDRIVER_BIN || which.sync('safaridriver', { nothrow: true });
+    if (process.platform !== 'darwin' || !safaridriverBin) {
+      throw new Error('Safari requires macOS and safaridriver');
+    }
+    pSafariDriverPort = launchSafariDriver(safaridriverBin, logger);
+  } else {
+    // This is not an optimization. Safari can only be claimed by one safaridriver.
+    logger.debug('safaridriver_reuse', 'Found existing safaridriver process');
+  }
+  const port = await pSafariDriverPort;
+
+  // https://developer.apple.com/documentation/webkit/macos-webdriver-commands-for-safari-12-and-later
+  async function webdriverReq (method, endpoint, body) {
+    // Since Node.js 18, connecting to "localhost" favours IPv6 (::1), whereas safaridriver
+    // listens exclusively on IPv4 (127.0.0.1). This was fixed in Node.js 20 by trying both.
+    // https://github.com/nodejs/node/issues/40702
+    // https://github.com/nodejs/node/pull/44731
+    // https://github.com/node-fetch/node-fetch/issues/1624
+    const resp = await fetch(`http://127.0.0.1:${port}${endpoint}`, {
+      method: method,
+      body: JSON.stringify(body)
+    });
+    const data = await resp.json();
+    if (!resp.ok) {
+      throw `HTTP ${resp.status} ${data?.value?.error}, ${data?.value?.message || ''}`;
+    }
+    return data.value;
+  }
+
+  let sessionId;
+  for (let i = 1; i <= 20; i++) {
+    try {
+      const session = await webdriverReq('POST', '/session/', { capabilities: { browserName: 'safari' } });
+      sessionId = session.sessionId;
+      // Connected!
+      break;
+    } catch (e) {
+      if (e && (e.code === 'ECONNREFUSED' || (e.cause && e.cause.code === 'ECONNREFUSED'))) {
+        // Wait another 10ms-200ms for safaridriver to start, upto ~2s in total.
+        logger.debug('safaridriver_waiting', `Attempt #${i}: ${e.code || e.cause.code}. Try again in ${i * 10}ms.`);
+        await new Promise(resolve => setTimeout(resolve, i * 10));
+        continue;
+      }
+      logger.warning('safaridriver_session_error', e);
+      throw new Error('Failed to create new session');
+    }
+  }
+
+  try {
+    await webdriverReq('POST', `/session/${sessionId}/url`, { url: url });
+  } catch (e) {
+    logger.warning('safaridriver_url_error', e);
+    throw new Error('Failed to create new tab');
+  }
+
+  // NOTE: If we didn't support concurrency, the `signal` could kill the safaridriver process,
+  // which would automatically closes our tabs, not needing an 'abort' listener and DELETE.
+  await new Promise((resolve, reject) => {
+    signal.addEventListener('abort', async () => {
+      try {
+        await webdriverReq('DELETE', `/session/${sessionId}`);
+        resolve();
+      } catch (e) {
+        logger.warning('safaridriver_delete_error', e);
+        reject(new Error('Unable to stop safaridriver session'));
+      }
+    });
+  });
+}
+
 export default {
   LocalBrowser,
 
@@ -292,9 +455,7 @@ export default {
   chrome: chromium.bind(null, concatGenFn(getChromePaths, getChromiumPaths, getEdgePaths)),
   chromium: chromium.bind(null, concatGenFn(getChromiumPaths, getChromePaths, getEdgePaths)),
   edge: chromium.bind(null, concatGenFn(getEdgePaths)),
-
-  //
-  // TODO: safari: [],
+  safari,
 
   // TODO: browserstack
   // - browserstack/firefox_45
diff --git a/src/server.js b/src/server.js
index 9f536f1..d0b1de8 100644
--- a/src/server.js
+++ b/src/server.js
@@ -99,7 +99,7 @@ class ControlServer {
     const controller = new AbortController();
     const summary = { ok: true };
 
-    const CLIENT_IDLE_TIMEOUT = 5000;
+    const CLIENT_IDLE_TIMEOUT = 60_000;
     const CLIENT_IDLE_INTERVAL = 1000;
     let clientIdleTimer = null;
 
@@ -137,21 +137,21 @@ class ControlServer {
     };
 
     const tapFinishFinder = tapFinished({ wait: 0 }, () => {
-      logger.debug('browser_tap_finished', 'Requesting browser stop');
+      logger.debug('browser_tap_finished', 'Test has finished, stopping browser');
 
       stopBrowser('QTap: browser_tap_finished');
     });
 
     const tapParser = new TapParser();
     tapParser.on('bailout', (reason) => {
+      logger.warning('browser_tap_bailout', `Test ended unexpectedly, stopping browser. Reason: ${reason}`);
       summary.ok = false;
-      logger.debug('browser_tap_bailout', reason);
 
       stopBrowser('QTap: browser_tap_bailout');
     });
     tapParser.once('fail', () => {
+      logger.debug('browser_tap_fail', 'Results indicate at least one test has failed assertions');
       summary.ok = false;
-      logger.debug('browser_tap_fail', 'One or more tests failed');
     });
     // Debugging
     // tapParser.on('assert', logger.debug.bind(logger, 'browser_tap_assert'));
@@ -176,7 +176,7 @@ class ControlServer {
     // creating tons of timers when processing a large batch of test results back-to-back.
     clientIdleTimer = setTimeout(function qtapClientTimeout () {
       if ((performance.now() - browser.clientIdleActive) > CLIENT_IDLE_TIMEOUT) {
-        logger.debug('browser_idle_timeout', 'Requesting browser stop');
+        logger.warning('browser_idle_timeout', `Browser timed out after ${CLIENT_IDLE_TIMEOUT}ms, stopping browser`);
         // TODO:
         // Produce a tap line to report this test failure to CLI output/reporters.
         summary.ok = false;
@@ -318,7 +318,12 @@ class ControlServer {
 
     const clientId = url.searchParams.get('qtap_clientId');
     if (url.pathname === '/' && clientId !== null) {
-      this.logger.debug('respond_static_testfile', clientId);
+      const browser = this.browsers.get(clientId);
+      if (browser) {
+        browser.logger.debug('browser_connected', 'Browser connected! Serving test file.');
+      } else {
+        this.logger.debug('respond_static_testfile', clientId);
+      }
       resp.writeHead(200, { 'Content-Type': MIME_TYPES[ext] || MIME_TYPES.html });
       resp.write(await this.getTestFile(clientId));
       resp.end();
diff --git a/src/util.js b/src/util.js
index 5211d6c..05e481e 100644
--- a/src/util.js
+++ b/src/util.js
@@ -20,6 +20,9 @@ export const MIME_TYPES = {
   woff: 'font/woff',
 };
 
+export const globalController = new AbortController();
+export const globalSignal = globalController.signal;
+
 /**
  * @param {number} msDuration
  * @returns {string} Something like "0.7", "2", or "3.1"