diff options
| author | amankumarsingh77 <amankumarsingh7702@gmail.com> | 2026-07-26 20:51:43 +0530 |
|---|---|---|
| committer | amankumarsingh77 <amankumarsingh7702@gmail.com> | 2026-07-26 20:51:43 +0530 |
| commit | 7d2256e584709e4b7c27ab83b61950f00def7046 (patch) | |
| tree | 642775bd745cb59423e762f4df8ff6ea65520ac4 | |
| parent | 083012ef3ede192505a953b7779d0c916dcaa98f (diff) | |
fix(profile): resolve replaced binaries and surface real launch failures
Two failures made a profile launch report the wrong cause.
Linux appends " (deleted)" to /proc/<pid>/exe once the running binary is
unlinked, which is what a google-chrome package upgrade does while the
browser stays open. detectRunningBrowser passed that literal string on as
the executable path, so every launch tried to spawn
"/opt/google/chrome/chrome (deleted)" and hit ENOENT. Executable paths are
now validated against disk, with the marker stripped and argv[0] used as a
fallback; a binary genuinely named "… (deleted)" still wins.
spawn() reports a missing or non-executable binary asynchronously via an
'error' event rather than throwing, and that event was discarded — so
openProfileWindow returned success, no window opened, and the caller
blamed the browser 15s later with a sentinel timeout. The launch now
awaits 'spawn'/'error', returns the real cause, and removes its handshake
page on the failure path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| -rw-r--r-- | src/profile/browser-process.ts | 62 | ||||
| -rw-r--r-- | src/profile/launch.ts | 34 | ||||
| -rw-r--r-- | test/profile/browser-process-test.ts | 71 | ||||
| -rw-r--r-- | test/profile/launch-test.ts | 95 |
4 files changed, 244 insertions, 18 deletions
diff --git a/src/profile/browser-process.ts b/src/profile/browser-process.ts index c0f6178..77bc05c 100644 --- a/src/profile/browser-process.ts +++ b/src/profile/browser-process.ts @@ -92,6 +92,47 @@ export type BrowserCandidate = { readonly explicitUserDataDir?: string; }; +/** + * Linux marks a replaced binary in `/proc/<pid>/exe`: once the file behind a + * running process is unlinked — which is exactly what a `google-chrome` package + * upgrade does while the browser keeps running — the symlink reads + * "/opt/google/chrome/chrome (deleted)". Spawning that string fails with + * ENOENT, so the marker has to come off. + * + * It is stripped only as a fallback, and only when the stripped path exists: a + * binary genuinely named "… (deleted)" must still win. + */ +const DELETED_SUFFIX = " (deleted)"; + +/** The path without Linux's deleted-binary marker, or undefined if unmarked. */ +export const stripDeletedSuffix = (path: string): string | undefined => + path.endsWith(DELETED_SUFFIX) && path.length > DELETED_SUFFIX.length + ? path.slice(0, -DELETED_SUFFIX.length) + : undefined; + +const exists = async (path: string): Promise<boolean> => { + try { + await access(path); + return true; + } catch { + return false; + } +}; + +/** + * A spawnable executable path for a raw readlink/argv value: the value itself + * when it exists, else the same value with the deleted-binary marker removed. + * Undefined when neither form is on disk — a path we cannot spawn must never be + * handed on as if it were usable. + */ +export const spawnableExePath = async (raw: string): Promise<string | undefined> => { + if (raw.length === 0) return undefined; + if (await exists(raw)) return raw; + const stripped = stripDeletedSuffix(raw); + if (stripped && (await exists(stripped))) return stripped; + return undefined; +}; + /** Path comparison that tolerates separator and case differences per platform. */ const samePath = (a: string, b: string): boolean => { const norm = (p: string): string => { @@ -163,14 +204,19 @@ const collectLinux = async (): Promise<ReadonlyArray<BrowserCandidate>> => { continue; } if (!cmdline || hasChildProcessType(cmdline)) continue; - let exePath: string | undefined; + let link: string | undefined; try { - exePath = await readlink(`/proc/${pid}/exe`); + link = await readlink(`/proc/${pid}/exe`); } catch { - // /proc/<pid>/exe is unreadable for processes we don't own; argv[0] is - // the next best thing. - exePath = cmdline.split(" ")[0]; + // /proc/<pid>/exe is unreadable for processes we don't own. } + // argv[0] backs up the symlink in both directions: unreadable for a process + // we don't own, and pointing at an unlinked binary after an in-place + // upgrade. Chrome's browser process rewrites its argv to the bare + // executable path, so on Linux argv[0] is the live install location. + const exePath = + (link ? await spawnableExePath(link) : undefined) ?? + (await spawnableExePath(cmdline.split(" ")[0] ?? "")); const explicit = parseUserDataDirFlag(cmdline); candidates.push({ ...(exePath ? { exePath } : {}), @@ -354,7 +400,11 @@ export const isBrowserRunning = async (): Promise<boolean> => (await detectRunni */ export const resolveBrowserExecutable = async (running?: RunningBrowser): Promise<string | undefined> => { const detected = running ?? (await detectRunningBrowser()); - if (detected.exePath) return detected.exePath; + // Re-check spawnability here too: this function is the single gate every + // launch passes through, and a caller may hand us a RunningBrowser collected + // before the binary was replaced on disk. + const usable = detected.exePath ? await spawnableExePath(detected.exePath) : undefined; + if (usable) return usable; if (process.platform !== "win32") return undefined; const fromRegistry = await winRegistryExecutable(); diff --git a/src/profile/launch.ts b/src/profile/launch.ts index fd93d3d..0088b6d 100644 --- a/src/profile/launch.ts +++ b/src/profile/launch.ts @@ -69,29 +69,47 @@ export const openProfileWindow = async ( if (req.explicitUserDataDir) args.push(`--user-data-dir=${req.explicitUserDataDir}`); args.push(`--profile-directory=${req.profileDir}`, "--new-window", sentinelUrl); - let child: ChildProcess | null = null; + let child: ChildProcess; try { // No shell: arguments are passed as an array, so profile names and paths // containing spaces survive intact on every platform. child = spawn(req.exePath, args, { detached: true, stdio: "ignore" }); - child.unref(); - // The launcher process exits as soon as the running browser takes over the - // command line; an error here means the binary itself could not be started. - child.on("error", () => {}); } catch (e) { await rm(file, { force: true }).catch(() => {}); return err(`could not launch ${req.exePath}: ${e instanceof Error ? e.message : String(e)}`); } + // spawn() reports a missing or non-executable binary ASYNCHRONOUSLY, as an + // 'error' event — it does not throw. Ignoring that event made an ENOENT + // indistinguishable from a browser that simply never opened the window, so + // the caller's 15s sentinel poll reported a timeout for what was really a bad + // executable path. Node guarantees exactly one of 'spawn' or 'error', so + // waiting for it costs nothing and turns the failure into a named cause. + const spawnFailure = await new Promise<string | undefined>((resolve) => { + child.once("spawn", () => resolve(undefined)); + child.once("error", (e: Error) => resolve(e.message)); + }); + // A later 'error' event with no listener would throw; keep one attached for + // the lifetime of the detached launcher. + child.on("error", () => {}); + child.unref(); + + if (spawnFailure) { + await rm(file, { force: true }).catch(() => {}); + return err(`could not launch ${req.exePath}: ${spawnFailure}`); + } + + let live: ChildProcess | null = child; + return ok({ sentinelUrl, cleanup: async () => { await rm(file, { force: true }).catch(() => {}); }, kill: () => { - if (child) { - try { child.kill("SIGTERM"); } catch {} - child = null; + if (live) { + try { live.kill("SIGTERM"); } catch {} + live = null; } }, }); diff --git a/test/profile/browser-process-test.ts b/test/profile/browser-process-test.ts index 29982e5..29b3ddf 100644 --- a/test/profile/browser-process-test.ts +++ b/test/profile/browser-process-test.ts @@ -9,9 +9,16 @@ * * Run: npx tsx test/profile/browser-process-test.ts */ -import { homedir } from "node:os"; +import { chmodSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { homedir, tmpdir } from "node:os"; import { join } from "node:path"; -import { parseUserDataDirFlag, rankBrowserCandidate } from "../../src/profile/browser-process"; +import { + parseUserDataDirFlag, + rankBrowserCandidate, + resolveBrowserExecutable, + spawnableExePath, + stripDeletedSuffix, +} from "../../src/profile/browser-process"; let passed = 0; let failed = 0; @@ -25,7 +32,7 @@ const check = (cond: boolean, label: string): void => { } }; -function main(): void { +async function main(): Promise<void> { // B1: --user-data-dir parsing { check( @@ -101,8 +108,64 @@ function main(): void { ); } + // B6: Linux's deleted-binary marker. An in-place `google-chrome` upgrade + // unlinks the running binary, after which /proc/<pid>/exe reads + // "…/chrome (deleted)" — a path spawn() rejects with ENOENT. + { + check( + stripDeletedSuffix("/opt/google/chrome/chrome (deleted)") === "/opt/google/chrome/chrome", + "B6: marker stripped from a replaced binary's path", + ); + check(stripDeletedSuffix("/opt/google/chrome/chrome") === undefined, "B6: unmarked path left alone"); + check(stripDeletedSuffix(" (deleted)") === undefined, "B6: marker alone is not a path"); + } + + // B7: only a path that is actually on disk may be handed on as spawnable. + { + const root = mkdtempSync(join(tmpdir(), "pi-exe-")); + try { + const real = join(root, "chrome"); + writeFileSync(real, "#!/bin/sh\n"); + chmodSync(real, 0o755); + + check(await spawnableExePath(real) === real, "B7: an existing path is returned unchanged"); + check( + await spawnableExePath(`${real} (deleted)`) === real, + "B7: a replaced binary resolves to the live install path", + ); + check(await spawnableExePath(join(root, "absent")) === undefined, "B7: a missing path yields undefined"); + check(await spawnableExePath("") === undefined, "B7: an empty path yields undefined"); + + // The launch gate must never pass on a path it cannot spawn — that is + // what produced a 15s "couldn't open a window" timeout instead of a + // named error. + check( + await resolveBrowserExecutable({ running: true, exePath: `${real} (deleted)` }) === real, + "B7: resolveBrowserExecutable repairs a replaced-binary path", + ); + if (process.platform !== "win32") { + check( + await resolveBrowserExecutable({ running: true, exePath: join(root, "absent") }) === undefined, + "B7: an unspawnable path is not passed on as usable", + ); + } + + // A binary genuinely named "… (deleted)" must win over the stripped form. + // Created last, so the checks above see only the ordinary case. + const literal = join(root, "chrome (deleted)"); + writeFileSync(literal, "#!/bin/sh\n"); + chmodSync(literal, 0o755); + check( + await spawnableExePath(literal) === literal, + "B7: a real file named '… (deleted)' is preferred over stripping", + ); + } finally { + rmSync(root, { recursive: true, force: true }); + } + } + console.log(`\n${passed} passed, ${failed} failed`); process.exit(failed === 0 ? 0 : 1); } -main(); +void main(); diff --git a/test/profile/launch-test.ts b/test/profile/launch-test.ts new file mode 100644 index 0000000..0061273 --- /dev/null +++ b/test/profile/launch-test.ts @@ -0,0 +1,95 @@ +/** + * Unit tests for openProfileWindow's failure reporting — no browser required. + * + * spawn() reports a missing or non-executable binary asynchronously, as an + * 'error' event, not by throwing. When that event was ignored, a bad executable + * path returned `ok` and the caller's 15s sentinel poll blamed the browser: + * "couldn't open a window in <profile> automatically". These tests pin the + * failure to its real cause instead. + * + * Run: npx tsx test/profile/launch-test.ts + */ +import { existsSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { openProfileWindow } from "../../src/profile/launch"; + +let passed = 0; +let failed = 0; +const check = (cond: boolean, label: string): void => { + if (cond) { + passed++; + console.log(` ✓ ${label}`); + } else { + failed++; + console.error(` ✗ ${label}`); + } +}; + +async function main(): Promise<void> { + const root = mkdtempSync(join(tmpdir(), "pi-launch-")); + try { + // L1: a nonexistent binary — the shape of an in-place browser upgrade, + // where /proc/<pid>/exe still names the unlinked file. + { + const missing = join(root, "chrome (deleted)"); + const r = await openProfileWindow({ exePath: missing, profileDir: "Default" }); + check(!r.success, "L1: a missing executable fails the launch"); + if (!r.success) { + check(r.error.includes(missing), "L1: the error names the path that could not be launched"); + check(r.error.includes("ENOENT"), "L1: the error carries spawn's own cause"); + } + } + + // L2: a file that exists but is not executable — same async 'error' path, + // different errno (EACCES). Skipped where the check is meaningless. + if (process.platform !== "win32" && process.getuid?.() !== 0) { + const notExec = join(root, "not-executable"); + writeFileSync(notExec, "#!/bin/sh\n", { mode: 0o644 }); + const r = await openProfileWindow({ exePath: notExec, profileDir: "Default" }); + check(!r.success, "L2: a non-executable file fails the launch"); + } + + // L3: a failed launch leaves no sentinel file behind. The caller only calls + // cleanup() on the success path, so the failure path owns its own tidying. + { + const before = process.env["TMPDIR"]; + process.env["TMPDIR"] = root; + try { + const r = await openProfileWindow({ exePath: join(root, "absent"), profileDir: "Default" }); + check(!r.success, "L3: launch failed as set up"); + const leftovers = readdirSync(root).filter((n) => n.startsWith("pi-harness-")); + check(leftovers.length === 0, "L3: no handshake page is left on disk after a failed launch"); + } finally { + if (before === undefined) delete process.env["TMPDIR"]; + else process.env["TMPDIR"] = before; + } + } + + // L4: a launcher that starts succeeds, and the sentinel it advertises is a + // real, readable file — the page the browser is asked to open. + { + const stub = join(root, "stub-browser"); + writeFileSync(stub, "#!/bin/sh\nexit 0\n", { mode: 0o755 }); + const r = await openProfileWindow({ exePath: stub, profileDir: "Default" }); + check(r.success, "L4: a startable binary yields a handle"); + if (r.success) { + const sentinel = fileURLToPath(r.data.sentinelUrl); + check(existsSync(sentinel), "L4: the sentinel page exists before the browser is polled"); + await r.data.cleanup(); + check(!existsSync(sentinel), "L4: cleanup removes the sentinel page"); + r.data.kill(); + r.data.kill(); + check(true, "L4: kill is safe to call twice"); + } + } + } finally { + rmSync(root, { recursive: true, force: true }); + } + + console.log(`\n${passed} passed, ${failed} failed`); + process.exit(failed === 0 ? 0 : 1); +} + +void main(); |
