fix(preview): relaunch the shared browser after it disconnects (#146)#158
Open
truffle-dev wants to merge 1 commit into
Open
fix(preview): relaunch the shared browser after it disconnects (#146)#158truffle-dev wants to merge 1 commit into
truffle-dev wants to merge 1 commit into
Conversation
…wright#146) getOrCreateBrowser and getOrCreatePreviewContext both returned their cached handle unconditionally. When the shared Chromium process dies out of band -- a renderer crash that propagates, an OOM kill, or an external process sweep -- the Browser disconnects but the module-level reference stays non-null, so every later phantom_preview_page and browser_* call hands back a dead handle and fails for the rest of the process lifetime. That is the ghostwright#146 symptom: "Target/Page crashed" or "...has been closed" across the whole tool surface with no recovery short of a restart. Gate both warm-cache returns on liveness. getOrCreateBrowser checks browser.isConnected() and drops the stale reference so the existing launch path relaunches a fresh process. getOrCreatePreviewContext checks the context's browser via currentContext.browser()?.isConnected() and drops the dead context so it rebuilds on the relaunched browser. The recovery is lazy (next call), which matches the module's existing lazy-singleton shape and needs no disconnected-event listener lifecycle. Two regression tests drive the crash path with a fake browser whose __disconnect() flips isConnected() to false without going through close(): one asserts getOrCreateBrowser relaunches, one asserts getOrCreatePreviewContext rebuilds on a fresh browser. The fakes gain isConnected() and context.browser() to cover the new call surface.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #146.
The bug
getOrCreateBrowserandgetOrCreatePreviewContextinsrc/ui/preview.tsboth return their cached handle unconditionally (if (browser) return browser,if (currentContext) return currentContext). When the shared Chromium process dies out of band — a renderer crash that propagates to the browser process, an OOM kill, or an external process sweep — theBrowserdisconnects but the module-level reference stays non-null. Every laterphantom_preview_pageandbrowser_*call then hands back a dead handle, so the whole tool surface fails withTarget/Page crashedor...has been closedfor the rest of the process lifetime, with no recovery short of a restart.I hit both faces of this while using the browser tools (the
...has been closedmode and thePage crashedmode are the same root cause — a disconnected shared browser that is never re-created); the issue thread has the process-level evidence (orphanedchrome-headless-shellprocesses from prior sessions, a crashed renderer that is never relaunched).The fix
Gate both warm-cache returns on liveness:
getOrCreateBrowserchecksbrowser.isConnected()and, if false, drops the stale reference so the existing launch path relaunches a fresh process.getOrCreatePreviewContextchecks the context's browser viacurrentContext.browser()?.isConnected()and, if dead, drops the cached context so it rebuilds on the relaunched browser instead of returning a corpse.Recovery is lazy (on the next call), which matches the module's existing lazy-singleton shape and needs no
disconnected-event listener lifecycle to manage. The launch-failure recovery (try/finallyclearingbrowserPromise), theshuttingDownguard, and cookie rotation are all unchanged.Tests
Two regression tests in
preview-correctness.test.tsdrive the crash path with a fake browser whose__disconnect()flipsisConnected()tofalsewithout going throughclose()(simulating an out-of-band death): one assertsgetOrCreateBrowserrelaunches, one assertsgetOrCreatePreviewContextrebuilds on a fresh browser. The fakes gainisConnected()andcontext.browser()to cover the new call surface.