diff --git a/.changeset/direct-probes-for-a-serialising-driver.md b/.changeset/direct-probes-for-a-serialising-driver.md new file mode 100644 index 00000000..1bdbb88d --- /dev/null +++ b/.changeset/direct-probes-for-a-serialising-driver.md @@ -0,0 +1,12 @@ +--- +"@wdio/devtools-service": patch +"@wdio/nightwatch-devtools": patch +--- + +Stop the WDIO service deadlocking a mobile-web Appium session. `beforeCommand` issues its probes — the collector drain, the per-action snapshot's two scripts plus `url`/`title`, and the `__wdioSnapMark` tag — from inside the hook wrapping the command it is observing. Desktop chromedriver tolerates that re-entrancy; Appium serialises per session, so each probe enqueued behind the command it was meant to observe and neither resolved. Measured on an emulator: a two-command mobile-web spec passes in 1.6 s without the service and took 6 m 13 s of timeouts with it, every command at the WDIO timeout, with Chrome still on its new-tab page. + +The probes now go straight to the driver's HTTP endpoint for a session whose driver serialises, which is the only escape that does not change the ordering guarantee the pre-action snapshot depends on — the alternative, not awaiting in the hook, trades "state BEFORE this action executes" for every adapter and platform. + +The transport moved to `core` rather than being copied: Nightwatch has needed exactly this since its own command queue posed the same problem, and its `helpers/webdriverHttp.ts` now delegates to it, keeping only the part that is genuinely framework-specific — walking Nightwatch's internal config for the driver's host and port. Two things the Nightwatch version could not do are in the core one because the service needs them: https, and basic auth from the connection's `user`/`key`, since a cloud grid answers 401 without it and a probe that silently 401s reads as a capture gap rather than an error. + +Gated on `isAppiumSession`, not on being native. A native session skips these probes entirely, so the one that needed this is the mobile **web** session — it has a document and is driven through Appium. Desktop keeps `browser.*`, which carries WDIO's own retries and interceptors, because the re-entrancy is only fatal where the driver serialises. A session whose address is not knowable from the connection options also keeps the normal path: guessing localhost would aim a probe at whatever else is listening there. diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 4c8ecce6..a97888f5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -20,6 +20,7 @@ export * from './attempt-tracker.js' export * from './screenshot-artifact.js' export * from './video-slice.js' export * from './with-timeout.js' +export * from './webdriver-http.js' export * from './assert-patcher.js' export * from './element-snapshot.js' export * from './element-scripts.js' diff --git a/packages/core/src/webdriver-http.ts b/packages/core/src/webdriver-http.ts new file mode 100644 index 00000000..384be822 --- /dev/null +++ b/packages/core/src/webdriver-http.ts @@ -0,0 +1,231 @@ +// Direct WebDriver HTTP transport for the capture probes. +// +// A probe issued from inside an adapter's own command hook re-enters the driver +// while the command it is meant to observe is still in flight. Where the driver +// serialises per session that probe enqueues behind that command and neither +// one resolves: Nightwatch's command queue does it always, Appium does it for a +// mobile-web session (#374, measured at 6m13s of timeouts against 1.6s without +// capture attached). Going straight to the driver's HTTP endpoint bypasses the +// queue the client library owns, which is the only escape that does not change +// the ordering guarantee the pre-action snapshot depends on. + +import http from 'node:http' +import https from 'node:https' +import { errorMessage } from './error.js' + +/** Ceiling on a single driver request — a driver that stops answering must not + * hold a capture open longer than the adapter's settle window. */ +export const WEBDRIVER_REQUEST_TIMEOUT_MS = 5000 + +/** Where a driver is reachable, and what it needs to answer. Resolved by each + * adapter from its own client, since no two expose it the same way. */ +export interface WebDriverAddress { + hostname: string + port: number + /** Defaults to http. A cloud grid is https, and a plain-http transport + * reaches it as a connection error rather than an auth failure. */ + protocol?: string + /** Base prefix ahead of `/session`, e.g. `/wd/hub`. WDIO's default is `/`. */ + path?: string + /** Basic-auth pair for a cloud grid; sent only when both are present. */ + user?: string + key?: string + headers?: Record + /** Adapter's logger. Core carries no logging dependency of its own. */ + onWarn?: (message: string) => void +} + +/** `/wd/hub` and `/` both reach us; only the former belongs in a URL. */ +function basePrefix(path: string | undefined): string { + if (!path || path === '/') { + return '' + } + return path.endsWith('/') ? path.slice(0, -1) : path +} + +/** An IPv6 literal has to be bracketed in a URL, or its own colons read as the + * port separator and the endpoint is unparseable — which would take a driver + * that is perfectly reachable and silently disable every direct probe. */ +function formatHost(hostname: string): string { + const isIpv6Literal = hostname.includes(':') && !hostname.startsWith('[') + return isIpv6Literal ? `[${hostname}]` : hostname +} + +export function sessionEndpoint( + address: WebDriverAddress, + sessionId: string, + path: string +): string { + const protocol = address.protocol ?? 'http' + const prefix = basePrefix(address.path) + const host = formatHost(address.hostname) + return `${protocol}://${host}:${address.port}${prefix}/session/${sessionId}/${path}` +} + +function authHeaders(address: WebDriverAddress): Record { + if (!address.user || !address.key) { + return {} + } + const encoded = Buffer.from(`${address.user}:${address.key}`).toString( + 'base64' + ) + return { authorization: `Basic ${encoded}` } +} + +/** A W3C error payload: `value` carries `error`/`message` instead of the + * command's result. Shape-checked rather than status-checked because + * chromedriver answers some failures with a 200. */ +function isWebdriverError(value: unknown): boolean { + return ( + typeof value === 'object' && + value !== null && + !Array.isArray(value) && + typeof (value as { error?: unknown }).error === 'string' + ) +} + +function requestHeaders( + address: WebDriverAddress, + payload: string | undefined +): Record { + return { + ...authHeaders(address), + ...address.headers, + ...(payload + ? { + 'content-type': 'application/json', + 'content-length': String(Buffer.byteLength(payload)) + } + : {}) + } +} + +function resolveFromResponse( + res: http.IncomingMessage, + endpoint: string, + warn: (message: string) => void, + resolve: (value: T | null) => void +): void { + let raw = '' + let settled = false + // Every path below has to land exactly once. `end` and `close` both fire on + // a healthy response, and a reset fires `error` before either. + const settle = (value: T | null) => { + if (!settled) { + settled = true + resolve(value) + } + } + res.on('data', (chunk: string | Buffer) => { + raw += chunk + }) + res.on('end', () => { + try { + const value = JSON.parse(raw).value + // A W3C error answers 200-shaped JSON whose `value` is + // `{error, message, stacktrace}` — an OBJECT where the caller expects + // its payload. Casting that through as `T` put an error object into a + // screencast frame's `data`, and the run's whole trace was then lost to + // `Buffer.from(object)` at export. + settle(isWebdriverError(value) ? null : ((value as T) ?? null)) + } catch { + warn(`Failed to parse response from ${endpoint}`) + settle(null) + } + }) + // A stream `error` with no listener is thrown, which would take the process + // down over a probe; a reset after headers would otherwise never settle. + res.on('error', (err: Error) => { + warn(`Response failed (${endpoint}): ${errorMessage(err)}`) + settle(null) + }) + // Fires after `end` on a healthy response, where `settle` is already spent. + // Reaching it first means the response was truncated. + res.on('close', () => settle(null)) +} + +/** Resolves the W3C `value` field, or null on any transport/parse/timeout + * failure — a probe is best-effort and never fails the user's test. */ +export function webdriverRequest( + address: WebDriverAddress, + endpoint: string, + method: 'GET' | 'POST', + body?: unknown +): Promise { + const payload = body === undefined ? undefined : JSON.stringify(body) + const transport = endpoint.startsWith('https:') ? https : http + const warn = address.onWarn ?? (() => {}) + return new Promise((resolve) => { + // `request` throws SYNCHRONOUSLY on an endpoint it cannot parse (a bare + // IPv6 hostname is the reachable case). Unguarded that rejects the promise + // instead of resolving null, and the rejection surfaces in the command + // hook as a failure of the user's command rather than a skipped probe. + let req: http.ClientRequest + try { + req = transport.request( + endpoint, + { method, headers: requestHeaders(address, payload) }, + (res) => resolveFromResponse(res, endpoint, warn, resolve) + ) + } catch (err) { + warn(`Request could not be issued (${endpoint}): ${errorMessage(err)}`) + resolve(null) + return + } + req.on('error', (err) => { + warn(`Request failed (${endpoint}): ${errorMessage(err)}`) + resolve(null) + }) + req.setTimeout(WEBDRIVER_REQUEST_TIMEOUT_MS, () => { + warn(`Request timed out (${endpoint})`) + req.destroy() + resolve(null) + }) + if (payload) { + req.write(payload) + } + req.end() + }) +} + +/** GET `/session/:id/`. Null when the session is gone or the call fails. */ +export function webdriverGet( + address: WebDriverAddress, + sessionId: string, + path: string +): Promise { + return webdriverRequest( + address, + sessionEndpoint(address, sessionId, path), + 'GET' + ) +} + +/** POST `/session/:id/`. Null when the session is gone or the call fails. */ +export function webdriverPost( + address: WebDriverAddress, + sessionId: string, + path: string, + body: unknown +): Promise { + return webdriverRequest( + address, + sessionEndpoint(address, sessionId, path), + 'POST', + body + ) +} + +/** Run a script in the page, outside whatever queue the client library owns. + * `body` is a function body, matching what `browser.execute` accepts. */ +export function webdriverExecute( + address: WebDriverAddress, + sessionId: string, + body: string, + args: unknown[] = [] +): Promise { + return webdriverPost(address, sessionId, 'execute/sync', { + script: body, + args + }) +} diff --git a/packages/core/tests/webdriver-http.test.ts b/packages/core/tests/webdriver-http.test.ts new file mode 100644 index 00000000..c4530cec --- /dev/null +++ b/packages/core/tests/webdriver-http.test.ts @@ -0,0 +1,238 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest' +import http from 'node:http' +import type { AddressInfo } from 'node:net' +import { + sessionEndpoint, + webdriverExecute, + webdriverGet, + webdriverPost, + type WebDriverAddress +} from '../src/webdriver-http.js' + +type Received = { + url?: string + method?: string + headers?: http.IncomingHttpHeaders + body?: string +} + +// A real driver rather than a mocked `http` module: the behaviours under test +// are all wire-level (a W3C error answered with 200, a malformed body, the +// auth header) and a mock would assert the mock. +let server: http.Server +let received: Received = {} +let respond: (res: http.ServerResponse) => void + +const address = (): WebDriverAddress => ({ + hostname: '127.0.0.1', + port: (server.address() as AddressInfo).port +}) + +beforeAll( + () => + new Promise((resolve) => { + server = http.createServer((req, res) => { + let body = '' + req.on('data', (c) => { + body += c + }) + req.on('end', () => { + received = { + url: req.url, + method: req.method, + headers: req.headers, + body + } + respond(res) + }) + }) + server.listen(0, '127.0.0.1', () => resolve()) + }) +) + +afterAll( + () => + new Promise((resolve) => { + server.close(() => resolve()) + }) +) + +const ok = (value: unknown) => (res: http.ServerResponse) => { + res.writeHead(200, { 'content-type': 'application/json' }) + res.end(JSON.stringify({ value })) +} + +describe('sessionEndpoint', () => { + it('defaults to http and omits a root path prefix', () => { + expect( + sessionEndpoint({ hostname: 'h', port: 4723, path: '/' }, 'sess', 'url') + ).toBe('http://h:4723/session/sess/url') + }) + + it('keeps a real base prefix and honours https', () => { + expect( + sessionEndpoint( + { hostname: 'h', port: 443, path: '/wd/hub', protocol: 'https' }, + 'sess', + 'title' + ) + ).toBe('https://h:443/wd/hub/session/sess/title') + }) + + it('does not double the slash when the prefix carries a trailing one', () => { + expect( + sessionEndpoint({ hostname: 'h', port: 1, path: '/wd/hub/' }, 's', 'url') + ).toBe('http://h:1/wd/hub/session/s/url') + }) +}) + +describe('webdriverGet', () => { + it('resolves the W3C value field', async () => { + respond = ok('https://example.com/') + await expect(webdriverGet(address(), 'sess', 'url')).resolves.toBe( + 'https://example.com/' + ) + expect(received.method).toBe('GET') + expect(received.url).toBe('/session/sess/url') + }) + + // chromedriver answers some failures with a 200, so the shape is the signal. + it('answers null for a W3C error payload delivered as 200', async () => { + respond = ok({ error: 'no such window', message: 'gone', stacktrace: '' }) + await expect(webdriverGet(address(), 'sess', 'url')).resolves.toBeNull() + }) + + it('answers null for a body that is not JSON', async () => { + respond = (res) => { + res.writeHead(200) + res.end('gateway error') + } + await expect(webdriverGet(address(), 'sess', 'url')).resolves.toBeNull() + }) + + it('answers null rather than throwing when the driver is unreachable', async () => { + // Port 1 is not listening; a probe must never fail the user's test. + await expect( + webdriverGet({ hostname: '127.0.0.1', port: 1 }, 'sess', 'url') + ).resolves.toBeNull() + }) + + it('reports a failure through the caller-supplied logger', async () => { + const warnings: string[] = [] + await webdriverGet( + { hostname: '127.0.0.1', port: 1, onWarn: (m) => warnings.push(m) }, + 'sess', + 'url' + ) + expect(warnings).toHaveLength(1) + expect(warnings[0]).toContain('Request failed') + }) +}) + +describe('webdriverPost', () => { + it('sends a JSON body with a matching content-length', async () => { + respond = ok(null) + await webdriverPost(address(), 'sess', 'execute/sync', { script: 'x' }) + expect(received.method).toBe('POST') + expect(received.body).toBe('{"script":"x"}') + expect(received.headers?.['content-type']).toBe('application/json') + expect(received.headers?.['content-length']).toBe( + String(Buffer.byteLength('{"script":"x"}')) + ) + }) +}) + +describe('webdriverExecute', () => { + it('posts the body as a script with empty args by default', async () => { + respond = ok(42) + await expect( + webdriverExecute(address(), 'sess', 'return 41 + 1') + ).resolves.toBe(42) + expect(received.url).toBe('/session/sess/execute/sync') + expect(JSON.parse(received.body ?? '{}')).toEqual({ + script: 'return 41 + 1', + args: [] + }) + }) +}) + +describe('authentication', () => { + it('sends basic auth when both user and key are present', async () => { + respond = ok('ok') + const addr = { ...address(), user: 'u', key: 'k' } + await webdriverGet(addr, 'sess', 'url') + const expected = Buffer.from('u:k').toString('base64') + expect(received.headers?.authorization).toBe(`Basic ${expected}`) + }) + + // Half a credential pair is not a credential; sending `Basic dTo=` would + // turn a missing key into a 401 that reads as a wrong password. + it('sends no auth header when only one half is present', async () => { + respond = ok('ok') + await webdriverGet({ ...address(), user: 'u' }, 'sess', 'url') + expect(received.headers?.authorization).toBeUndefined() + }) +}) + +// An IPv6 driver is REACHABLE, so answering null for it would silently drop +// every direct probe — the drain, the snapshot, the screenshot, url and title. +// Bracketing is what makes the URL parseable at all. +describe('an IPv6 driver address', () => { + it('brackets the literal so the colons are not read as a port', () => { + expect( + sessionEndpoint({ hostname: '::1', port: 4723 }, 'sess', 'url') + ).toBe('http://[::1]:4723/session/sess/url') + }) + + it('leaves an already-bracketed literal alone', () => { + expect( + sessionEndpoint({ hostname: '[::1]', port: 4723 }, 'sess', 'url') + ).toBe('http://[::1]:4723/session/sess/url') + }) + + it('does not bracket a hostname or an IPv4 address', () => { + expect( + sessionEndpoint({ hostname: 'localhost', port: 1 }, 's', 'url') + ).toBe('http://localhost:1/session/s/url') + expect( + sessionEndpoint({ hostname: '127.0.0.1', port: 1 }, 's', 'url') + ).toBe('http://127.0.0.1:1/session/s/url') + }) +}) + +// A probe must never fail the user's test. A stream `error` with no listener +// is thrown rather than caught, and a response reset after headers would +// otherwise leave the promise pending forever — which is the same hang this +// transport exists to remove. +describe('a response that fails mid-stream', () => { + // The reset has to land in a LATER tick than the write, or the client sees a + // clean EOF and takes the parse-failure path instead of the reset one. + it('settles null on a reset after headers instead of hanging', async () => { + respond = (res) => { + res.writeHead(200, { 'content-length': '999' }) + res.write('{"value":1}') + setTimeout(() => res.destroy(), 20) + } + await expect( + Promise.race([ + webdriverGet(address(), 'sess', 'url'), + new Promise((r) => setTimeout(() => r('HUNG'), 3000)) + ]) + ).resolves.toBeNull() + }) + + it('reports the reset through the caller-supplied logger', async () => { + const warnings: string[] = [] + respond = (res) => { + res.writeHead(200, { 'content-length': '999' }) + res.write('{"value":1}') + setTimeout(() => res.destroy(), 20) + } + await webdriverGet( + { ...address(), onWarn: (m) => warnings.push(m) }, + 'sess', + 'url' + ) + expect(warnings.join('\n')).toMatch(/Response failed/) + }) +}) diff --git a/packages/nightwatch-devtools/src/helpers/webdriverHttp.ts b/packages/nightwatch-devtools/src/helpers/webdriverHttp.ts index 18a3c9ec..8b830451 100644 --- a/packages/nightwatch-devtools/src/helpers/webdriverHttp.ts +++ b/packages/nightwatch-devtools/src/helpers/webdriverHttp.ts @@ -7,17 +7,16 @@ // Unguarded, one such probe stranded the whole per-action snapshot capture; the // in-page script probe merely timed out, leaving empty a11y trees. -import http from 'node:http' import logger from '@wdio/logger' -import { errorMessage } from '@wdio/devtools-core' +import { + webdriverGet as coreGet, + webdriverPost as corePost, + type WebDriverAddress +} from '@wdio/devtools-core' import type { NightwatchBrowser } from '../types.js' const log = logger('@wdio/nightwatch-devtools:webdriverHttp') -/** Ceiling on a single driver request — a driver that stops answering must not - * hold a capture open longer than the adapter's settle window. */ -const REQUEST_TIMEOUT_MS = 5000 - type LooseRec = Record const getProp = (obj: unknown, key: string): unknown => @@ -80,85 +79,19 @@ export function resolveWebDriverAddress(browser: NightwatchBrowser): { return { driverHost, driverPort } } -function sessionEndpoint( - browser: NightwatchBrowser, - path: string -): string | undefined { - const sessionId = (browser as unknown as { sessionId?: string }).sessionId - if (!sessionId) { - return undefined - } +/** The core transport's address, plus this adapter's logger. Nightwatch always + * speaks plain http to a local driver — it has no cloud-grid path of its own. */ +function address(browser: NightwatchBrowser): WebDriverAddress { const { driverHost, driverPort } = resolveWebDriverAddress(browser) - return `http://${driverHost}:${driverPort}/session/${sessionId}/${path}` -} - -/** A W3C error payload: `value` carries `error`/`message` instead of the - * command's result. Shape-checked rather than status-checked because chromedriver - * answers some failures with a 200. */ -function isWebdriverError(value: unknown): boolean { - return ( - typeof value === 'object' && - value !== null && - !Array.isArray(value) && - typeof (value as { error?: unknown }).error === 'string' - ) + return { + hostname: driverHost, + port: driverPort, + onWarn: (message: string) => log.warn(message) + } } -/** Resolves the W3C `value` field, or null on any transport/parse/timeout - * failure — a probe is best-effort and never fails the user's test. */ -function request( - endpoint: string, - method: 'GET' | 'POST', - body?: unknown -): Promise { - const payload = body === undefined ? undefined : JSON.stringify(body) - return new Promise((resolve) => { - const req = http.request( - endpoint, - { - method, - headers: payload - ? { - 'content-type': 'application/json', - 'content-length': Buffer.byteLength(payload) - } - : undefined - }, - (res) => { - let raw = '' - res.on('data', (chunk: string | Buffer) => { - raw += chunk - }) - res.on('end', () => { - try { - const value = JSON.parse(raw).value - // A W3C error answers 200-shaped JSON whose `value` is - // `{error, message, stacktrace}` — an OBJECT where the caller - // expects its payload. Casting that through as `T` put an error - // object into a screencast frame's `data`, and the run's whole - // trace was then lost to `Buffer.from(object)` at export. - resolve(isWebdriverError(value) ? null : ((value as T) ?? null)) - } catch { - log.warn(`Failed to parse response from ${endpoint}`) - resolve(null) - } - }) - } - ) - req.on('error', (err) => { - log.warn(`Request failed (${endpoint}): ${errorMessage(err)}`) - resolve(null) - }) - req.setTimeout(REQUEST_TIMEOUT_MS, () => { - log.warn(`Request timed out (${endpoint})`) - req.destroy() - resolve(null) - }) - if (payload) { - req.write(payload) - } - req.end() - }) +function sessionId(browser: NightwatchBrowser): string | undefined { + return (browser as unknown as { sessionId?: string }).sessionId } /** GET `/session/:id/`. Null when the session is gone or the call fails. */ @@ -166,8 +99,8 @@ export function webdriverGet( browser: NightwatchBrowser, path: string ): Promise { - const endpoint = sessionEndpoint(browser, path) - return endpoint ? request(endpoint, 'GET') : Promise.resolve(null) + const id = sessionId(browser) + return id ? coreGet(address(browser), id, path) : Promise.resolve(null) } /** POST `/session/:id/`. Null when the session is gone or the call fails. */ @@ -176,8 +109,10 @@ export function webdriverPost( path: string, body: unknown ): Promise { - const endpoint = sessionEndpoint(browser, path) - return endpoint ? request(endpoint, 'POST', body) : Promise.resolve(null) + const id = sessionId(browser) + return id + ? corePost(address(browser), id, path, body) + : Promise.resolve(null) } /** Run a script in the page, outside the command queue. `body` is a function diff --git a/packages/service/src/action-snapshot.ts b/packages/service/src/action-snapshot.ts index f1db78c0..4dccf4f3 100644 --- a/packages/service/src/action-snapshot.ts +++ b/packages/service/src/action-snapshot.ts @@ -15,6 +15,7 @@ import { } from '@wdio/devtools-core' import { isNativeAppSession, type ActionSnapshot } from '@wdio/devtools-shared' import { mobilePlatform } from './mobile.js' +import { directProbes } from './direct-probes.js' import { INTERNAL_COMMANDS } from './constants.js' import { wdioRunnerId } from './wdio-runner-id.js' @@ -113,22 +114,36 @@ export function captureActionSnapshot( // A mobile BROWSER session takes the web path below: it has a document, and // the native path would read its HTML through the page-source XML parser. const native = isNativeAppSession(browser.capabilities) + // A driver that serialises per session deadlocks on a probe issued from + // inside the command hook, so those go straight to it (#374). + const direct = directProbes(browser) return coreCapture({ command, timestamp, runner: wdioRunnerId(browser), - runScript: native ? undefined : (src) => browser.execute(reviveScript(src)), - takeScreenshot: () => browser.takeScreenshot().catch(() => undefined), + runScript: native + ? undefined + : direct + ? // an element-script src is a self-invoking IIFE, and `execute/sync` + // takes a function body — the string form of `reviveScript`. + (src: string) => direct.runScript(`return (${src})`) + : (src: string) => browser.execute(reviveScript(src)), + takeScreenshot: + direct?.takeScreenshot ?? + (() => browser.takeScreenshot().catch(() => undefined)), // url/title are browser-only concepts — they fail with "Method has not // yet been implemented" on native mobile, costing a round-trip each. - getUrl: native ? undefined : () => browser.getUrl().catch(() => undefined), + getUrl: native + ? undefined + : (direct?.getUrl ?? (() => browser.getUrl().catch(() => undefined))), getTitle: native ? undefined - : () => browser.getTitle().catch(() => undefined), + : (direct?.getTitle ?? (() => browser.getTitle().catch(() => undefined))), // On native mobile, use page-source XML to produce structured element // data and an AI-readable snapshot (same approach as @wdio/elements). getPageSource: native - ? () => browser.getPageSource().catch(() => undefined) + ? (direct?.getPageSource ?? + (() => browser.getPageSource().catch(() => undefined))) : undefined, platform: native ? mobilePlatform(browser) : undefined }) diff --git a/packages/service/src/direct-probes.ts b/packages/service/src/direct-probes.ts new file mode 100644 index 00000000..7b7143c8 --- /dev/null +++ b/packages/service/src/direct-probes.ts @@ -0,0 +1,65 @@ +// Capture probes that go straight to the driver instead of back through WDIO. +// +// `beforeCommand` runs inside the command it is observing, so every probe it +// issues re-enters the driver mid-command. Appium serialises per session, so +// the probe enqueues behind that command and neither resolves (#374). Desktop +// chromedriver tolerates the re-entrancy, and `browser.*` carries WDIO's own +// retries and interceptors, so it stays the path wherever it works. + +import { + webdriverExecute, + webdriverGet, + type WebDriverAddress +} from '@wdio/devtools-core' +import { isAppiumSession } from './mobile.js' +import { resolveWebDriverAddress } from './webdriver-address.js' + +/** The subset of probes `beforeCommand` issues. Mirrors the closures + * `captureActionSnapshot` already takes, so rerouting is a swap. */ +export interface DirectProbes { + /** `body` is a function BODY, which is what W3C `execute/sync` takes — the + * drain expression already is one, an element-script IIFE needs wrapping. */ + runScript: (body: string) => Promise + getUrl: () => Promise + getTitle: () => Promise + takeScreenshot: () => Promise + /** The native path's page read. A native session is always an Appium one, so + * leaving this on `browser.*` would keep an in-hook call exactly where the + * serialising driver is guaranteed. */ + getPageSource: () => Promise +} + +const orUndefined = (value: T | null): T | undefined => value ?? undefined + +/** + * Direct probes for a session whose driver serialises commands, or undefined + * when the normal path is safe or the driver's address is not knowable. + * + * Gated on `isAppiumSession` rather than on being native: a native app session + * skips these probes entirely (#372), so the one that needs this is the mobile + * WEB session, which has a document and is driven through Appium. + */ +export function directProbes( + browser: WebdriverIO.Browser +): DirectProbes | undefined { + if (!isAppiumSession(browser)) { + return undefined + } + const address: WebDriverAddress | undefined = resolveWebDriverAddress(browser) + const sessionId = browser.sessionId + if (!address || !sessionId) { + return undefined + } + return { + runScript: (body: string) => + webdriverExecute(address, sessionId, body).then(orUndefined), + getUrl: () => + webdriverGet(address, sessionId, 'url').then(orUndefined), + getTitle: () => + webdriverGet(address, sessionId, 'title').then(orUndefined), + takeScreenshot: () => + webdriverGet(address, sessionId, 'screenshot').then(orUndefined), + getPageSource: () => + webdriverGet(address, sessionId, 'source').then(orUndefined) + } +} diff --git a/packages/service/src/index.ts b/packages/service/src/index.ts index 8dc49f68..08116b59 100644 --- a/packages/service/src/index.ts +++ b/packages/service/src/index.ts @@ -61,6 +61,7 @@ import { PAGE_TRANSITION_COMMANDS } from './constants.js' import { isAppiumSession } from './mobile.js' +import { directProbes } from './direct-probes.js' import { resolveSessionMetadata } from './session-metadata.js' import { stampRunnerMetadata } from './wdio-runner-id.js' import { detectInvocationConfigPath } from './standalone.js' @@ -680,6 +681,14 @@ export default class DevToolsHookService implements Services.ServiceInstance { if (!this.#browser || isNativeAppSession(this.#browser.capabilities)) { return Promise.resolve() } + // Issued from inside beforeCommand, so it takes the direct path on a + // driver that serialises per session (#374). + const direct = directProbes(this.#browser) + if (direct) { + return direct + .runScript('window.__wdioSnapMark = true') + .catch(() => undefined) + } return this.#browser .execute(() => { ;(window as Window & { __wdioSnapMark?: boolean }).__wdioSnapMark = true diff --git a/packages/service/src/session.ts b/packages/service/src/session.ts index 384169e2..591021ff 100644 --- a/packages/service/src/session.ts +++ b/packages/service/src/session.ts @@ -23,6 +23,7 @@ import { RetryTracker, SessionCapturerBase, applyPerformanceData, + collectorDrainExpression, drainCollectorWithRecovery, errorMessage, getRequestType, @@ -31,6 +32,7 @@ import { type CapturedPerformancePayload } from '@wdio/devtools-core' import type { CommandLog } from './types.js' +import { directProbes } from './direct-probes.js' const log = logger('@wdio/devtools-service:SessionCapturer') @@ -365,6 +367,15 @@ export class SessionCapturer extends SessionCapturerBase { * `execute` awaits its startup (which anchors the current DOM). */ async injectIntoCurrentDocument(browser: WebdriverIO.Browser) { const source = await loadInjectableScript() + // Reached from the drain's recovery path, which runs inside beforeCommand. + // Without a BiDi preload (a classic-protocol mobile session has none) the + // first drain always misses the collector and lands here, so this is the + // in-hook call a serialising driver would deadlock on (#374). + const direct = directProbes(browser) + if (direct) { + await direct.runScript(`return ${source}`) + return + } await browser.execute(`return ${source}`) } @@ -402,20 +413,26 @@ export class SessionCapturer extends SessionCapturerBase { // spurious "Cannot read properties of undefined" errors. // forceAnchor: capture the current document before draining — for a final // closing navigation whose async initial anchor hasn't run by teardown. + // Called from inside beforeCommand for a page transition, so on a driver + // that serialises per session both reads take the direct path (#374). + const direct = directProbes(browser) const payload = await drainCollectorWithRecovery({ drain: () => - browser.execute((anchor) => { - if (typeof window.wdioTraceCollector === 'undefined') { - return null - } - if (anchor) { - window.wdioTraceCollector.captureCurrentDom() - } - return window.wdioTraceCollector.getTraceData() - }, forceAnchor), + direct + ? direct.runScript(collectorDrainExpression(forceAnchor)) + : browser.execute((anchor) => { + if (typeof window.wdioTraceCollector === 'undefined') { + return null + } + if (anchor) { + window.wdioTraceCollector.captureCurrentDom() + } + return window.wdioTraceCollector.getTraceData() + }, forceAnchor), injectIntoCurrentDocument: () => this.injectIntoCurrentDocument(browser), - currentUrl: () => browser.getUrl(), + currentUrl: () => + direct ? direct.getUrl().then((u) => u ?? '') : browser.getUrl(), log: (level, message) => log[level](message) }) if (!payload) { diff --git a/packages/service/src/webdriver-address.ts b/packages/service/src/webdriver-address.ts new file mode 100644 index 00000000..4ec9926a --- /dev/null +++ b/packages/service/src/webdriver-address.ts @@ -0,0 +1,48 @@ +// Where this session's driver is reachable, for the probes that must not go +// back through WDIO's own command path. See `core/webdriver-http.ts` for why. + +import logger from '@wdio/logger' +import type { WebDriverAddress } from '@wdio/devtools-core' + +const log = logger('@wdio/devtools-service:webdriverAddress') + +/** WDIO's connection options, which are on the runtime `browser` but not on + * its published type. Spelled once here rather than at each read. */ +type ConnectionOptions = { + protocol?: string + hostname?: string + port?: number + path?: string + user?: string + key?: string + headers?: Record +} + +/** + * The address WDIO built this session against, or undefined when it is not + * knowable — a session created by something other than the standard connection + * options, where guessing localhost would send a probe to the wrong driver. + * + * `user`/`key` carry through because a cloud grid answers 401 without them, + * and a probe that silently 401s is a capture gap rather than a visible error. + */ +export function resolveWebDriverAddress( + browser: WebdriverIO.Browser +): WebDriverAddress | undefined { + const options = browser.options as ConnectionOptions | undefined + const hostname = options?.hostname + const port = options?.port + if (!hostname || !port) { + return undefined + } + return { + hostname, + port, + protocol: options?.protocol, + path: options?.path, + user: options?.user, + key: options?.key, + headers: options?.headers, + onWarn: (message: string) => log.warn(message) + } +} diff --git a/packages/service/tests/direct-probes.test.ts b/packages/service/tests/direct-probes.test.ts new file mode 100644 index 00000000..fdf9344f --- /dev/null +++ b/packages/service/tests/direct-probes.test.ts @@ -0,0 +1,139 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest' +import http from 'node:http' +import type { AddressInfo } from 'node:net' +import { resolveWebDriverAddress } from '../src/webdriver-address.js' +import { directProbes } from '../src/direct-probes.js' +import { captureActionSnapshot } from '../src/action-snapshot.js' + +// Double cast: `WebdriverIO.Browser` carries private fields no stub can +// satisfy, and these probes read only sessionId/options/capabilities. +const browser = (over: Record = {}): WebdriverIO.Browser => + ({ + sessionId: 'sess', + isMobile: true, + options: { hostname: '127.0.0.1', port: 4723, path: '/' }, + ...over + }) as unknown as WebdriverIO.Browser + +describe('resolveWebDriverAddress', () => { + it('reads the connection options WDIO built the session with', () => { + const address = resolveWebDriverAddress( + browser({ + options: { + protocol: 'https', + hostname: 'hub.example.com', + port: 443, + path: '/wd/hub', + user: 'u', + key: 'k' + } + }) + ) + expect(address).toMatchObject({ + protocol: 'https', + hostname: 'hub.example.com', + port: 443, + path: '/wd/hub', + user: 'u', + key: 'k' + }) + }) + + // Guessing localhost would aim a probe at whatever else is listening there. + it('answers undefined when the address is not knowable', () => { + expect(resolveWebDriverAddress(browser({ options: {} }))).toBeUndefined() + expect( + resolveWebDriverAddress(browser({ options: { hostname: 'h' } })) + ).toBeUndefined() + expect( + resolveWebDriverAddress(browser({ options: { port: 4723 } })) + ).toBeUndefined() + }) +}) + +describe('directProbes', () => { + it('serves an Appium session, which is the one that deadlocks', () => { + expect(directProbes(browser())).toBeDefined() + }) + + // Desktop chromedriver tolerates the re-entrancy, and `browser.*` carries + // WDIO's own retries — so it stays the path wherever it works. + it('declines a desktop session', () => { + expect( + directProbes(browser({ isMobile: false, isAndroid: false, isIOS: false })) + ).toBeUndefined() + }) + + it('declines when the driver address is unknown', () => { + expect(directProbes(browser({ options: {} }))).toBeUndefined() + }) + + it('declines before a session exists', () => { + expect(directProbes(browser({ sessionId: undefined }))).toBeUndefined() + }) + + it('exposes the four probes beforeCommand issues', () => { + const probes = directProbes(browser()) + expect(Object.keys(probes ?? {}).sort()).toEqual([ + 'getPageSource', + 'getTitle', + 'getUrl', + 'runScript', + 'takeScreenshot' + ]) + }) +}) + +// The regression #374 exists for: these probes are issued from inside +// `beforeCommand`, and on Appium a `browser.*` call there enqueues behind the +// command it is observing and never resolves. Asserting the TRANSPORT rather +// than a timeout keeps the test fast and deterministic. +describe('captureActionSnapshot on an Appium session (#374)', () => { + let server: http.Server + const seen: string[] = [] + + beforeAll( + () => + new Promise((resolve) => { + server = http.createServer((req, res) => { + seen.push(`${req.method} ${req.url}`) + req.on('data', () => {}) + req.on('end', () => { + res.writeHead(200, { 'content-type': 'application/json' }) + res.end(JSON.stringify({ value: null })) + }) + }) + server.listen(0, '127.0.0.1', () => resolve()) + }) + ) + + afterAll( + () => + new Promise((resolve) => { + server.close(() => resolve()) + }) + ) + + it('reaches the driver directly and never through browser.*', async () => { + const forbidden = (name: string) => () => { + throw new Error(`browser.${name} must not be called from the hook`) + } + const appium = browser({ + options: { + hostname: '127.0.0.1', + port: (server.address() as AddressInfo).port, + path: '/' + }, + capabilities: { platformName: 'Android', browserName: 'chrome' }, + execute: forbidden('execute'), + getUrl: forbidden('getUrl'), + getTitle: forbidden('getTitle'), + takeScreenshot: forbidden('takeScreenshot') + }) + + await expect( + captureActionSnapshot(appium, 'click', 1) + ).resolves.not.toThrow() + expect(seen.some((r) => r.includes('execute/sync'))).toBe(true) + }) +})