Repository navigation
fix(browser): close the browser when its page cannot be opened - #587
Merged
Merged
Conversation
…er closed close() returned at once while Chromium was launching, so the browser that finished starting afterwards kept running and held its profile folder. The driver now keeps the in-flight start, shares it between callers, and close() waits for it; a start that finishes after a close shuts its browser before returning, and a closed driver starts nothing again.
Contributor
Author
|
Review attestation: ready to merge at A push to this PR makes this attestation stale; the new head needs its own review. |
Contributor
Author
|
Heads-up from #622 (#563): |
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.
Fixes #579. Refs #564 Depends on #566. This branch is stacked on
fix/564-driver-close-during-launch; the diff againstmainincludes #566's commits until it lands. Rebase/mergemainonce #566 merges. ## Problem Inpacks/browser-playwright/src/driver.ts(#launch, reached from#ensurePage),context.newPage()ran outside thetrythat closes a failed start. If it threw, the persistent context stayed open holding the profile folder,#contextstill pointed at it, and the next call launched a second Chromium on the same profile. ## Fix Opening the page (context.pages()/context.newPage()) now sits inside the existing cleanuptry, so a failure resets#contextand closes the browser it started.#launchingis already cleared by thefinallyin#ensurePage, so the next call starts cleanly; the closed/launching state machine from #566 is unchanged. ## Test Newpacks/browser-playwright/test/new-page-failure.spec.tslaunches real Chromium (same pattern asclose-during-launch.spec.ts), makes the first start report no pages and a failingnewPage, and checks: - the firstobserve()rejects and the browser it started is disconnected; - the nextobserve()succeeds on a second launch, with exactly one browser still connected. Against the unfixed driver it fails (launched[0].browser().isConnected()istrue). ## Verification -vitest run packs/browser-playwright apps/runtime/test/{browser-effects,session-preview-real,task-browser}.spec.ts: 9 files, 71 tests passed -pnpm typecheck,pnpm invariants, eslint on changed files: pass -pnpm verify: invariants/typecheck/lint pass; the full test run was on a machine at 100% CPU with other sessions' Chromium processes, and two runs failed different, unrelated timing/EPERM specs (worktree-sweep,managed-worktree,scan-secret-history,package-fetch,scoped-fs, and once the browser pack's 20s timeouts). Every failing file passed when rerun on its own; in the second full run all six browser-pack files passed. No user-visible behavior, API or docs change.