webview: navigate() accepts {waitUntil, timeout}#30645
Open
robobun wants to merge 14 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughWebView navigation methods accept optional waitUntil and timeout options; lifecycle events (DOMContentLoaded or load) drive promise settlement, parent-side timeouts can reject navigations, and Chrome backend matches lifecycle events to committed frame/loader IDs. ChangesWebView Navigation with waitUntil and Timeout Support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Collaborator
Author
|
Updated 4:31 PM PT - May 14th, 2026
❌ @autofix-ci[bot], your commit 3534b57 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30645That installs a local version of the PR into your bun-30645 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webview/JSWebView.cpp`:
- Around line 320-359: The comment says timeouts are armed for symmetry but in
JSWebView::goBack, JSWebView::goForward, and JSWebView::reload the WK_DISPATCH
path returns directly so armNavTimeout(g, timeoutMs) is never called; either
update the comment to state WebKit doesn't arm the timeout, or change the
WK_DISPATCH branches to mirror navigate by saving the promise, calling
armNavTimeout(g, timeoutMs), then returning that promise (i.e., follow
navigate's pattern) so behavior matches the comment; reference the WK_DISPATCH
macro, armNavTimeout, and the navigate implementation when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82a744d8-3bde-42f2-ae9d-10dca0d6447d
📒 Files selected for processing (8)
packages/bun-types/bun.d.tssrc/runtime/webview/ChromeBackend.cppsrc/runtime/webview/JSWebView.cppsrc/runtime/webview/JSWebView.hsrc/runtime/webview/JSWebViewConstructor.cppsrc/runtime/webview/JSWebViewPrototype.cpptest/js/bun/webview/webview-chrome.test.tstest/js/bun/webview/webview-navigate-options.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webview/JSWebViewPrototype.cpp`:
- Around line 243-255: The code currently coerces non-number JS values for
options.timeout via toNumber(); change it to reject any non-number (except
undefined) the same way waitUntil does: after retrieving t (from o->get(...,
"timeout")), if t.isUndefined() skip, otherwise require t.isNumber() and if not
call Bun::ERR::INVALID_ARG_VALUE (using "options.timeout"_s and t) and return
false; then call t.toNumber(g), check RETURN_IF_EXCEPTION, validate finiteness
and non-negativity, and clamp into timeout as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a93f631-b47b-421c-b2f8-d91884de81cd
📒 Files selected for processing (6)
packages/bun-types/bun.d.tssrc/runtime/webview/ChromeBackend.cppsrc/runtime/webview/JSWebView.cppsrc/runtime/webview/JSWebViewConstructor.cppsrc/runtime/webview/JSWebViewPrototype.cpptest/js/bun/webview/webview-navigate-options.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webview/ChromeBackend.cpp`:
- Around line 1204-1206: The branch handling the "Page.loadEventFired" event
must not clear view->m_loading for a stale/cleared loader; modify the check so
m_loading is only set to false when the current loader is still active (e.g.,
require that view->m_loaderId is non-empty or otherwise matches the event's
loader id) before clearing m_loading and calling chainTitle; update the
Page.loadEventFired handling in ChromeBackend.cpp to gate setting
view->m_loading = false behind the loader-id validity check (and then call
chainTitle() only as before when view->m_pendingNavigate is true).
In `@src/runtime/webview/JSWebView.cpp`:
- Around line 348-377: The Chrome branches for JSWebView::reload and
JSWebView::goBack/JSWebView::goForward must set the view loading flag the same
way navigate() does: set m_loading (or call the same setter navigate() uses) to
true when beginning a Chrome navigation (i.e. before calling
CDP::Ops::reload/goBack/goForward and before armNavTimeout), and for the
history-path that queries Page.getNavigationHistory ensure you set m_loading =
true when the history response confirms there is an entry to traverse to; this
will keep view.loading in sync with in-flight Chrome reload/history navigations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5482aa6a-e445-4dee-a7d2-5086baecfde9
📒 Files selected for processing (4)
src/runtime/webview/ChromeBackend.cppsrc/runtime/webview/JSWebView.cppsrc/runtime/webview/JSWebViewConstructor.cpptest/js/bun/webview/webview-navigate-options.test.ts
Collaborator
Author
Status — all CI lanes green, ready to mergeAll review threads resolved. 8/8 mock-CDP tests ( Branch rebased onto CI — #54352 finalEvery individual BuildKite lane passes. Only the aggregate No webview test appears in any BuildKite annotation across builds #54113–54352. This is mergeable as-is — every lane is green; only the sticky aggregate status needs an override. What's in
Deferred — maintainer scope call
|
navigate() (and reload/goBack/goForward) previously settled only on
Page.loadEventFired. A page that holds a subresource connection open
(SSE, long-poll, a hung image) never fires `load`, so `await
view.navigate()` hung forever.
Now accepts Playwright-shaped options:
await view.navigate(url, {
waitUntil: "load" | "domcontentloaded", // default "load"
timeout: 30000, // ms; 0 disables
});
Chrome backend: Page.setLifecycleEventsEnabled is sent in the attach
chain; Page.frameNavigated stashes the main frame id + loaderId; the
Page.lifecycleEvent handler settles the Navigate slot when
frameId/loaderId match and name == DOMContentLoaded (or load).
Subframe events and the about:blank replay never match because
m_frameId/m_loaderId are only set after the user URL commits.
Timeout is a parent-side RunLoop::dispatchAfter timer. It captures a
per-view navigation generation counter; settleSlot bumps the counter,
so a late fire no-ops instead of rejecting the next navigation. No
explicit cancel, no RefPtr lifetime dance.
WebKit backend: parses the same options. `timeout` applies (same
parent-side timer). WKNavigationDelegate has no DOMContentLoaded
hook, so `domcontentloaded` degrades to `load` there — noted in the
type docs.
Tests: new webview-navigate-options.test.ts drives a mock CDP
WebSocket server so the full lifecycleEvent path is exercised without
a Chrome binary; webview-chrome.test.ts gets real-Chrome coverage
(todo-gated where Chrome is absent).
The WebKit branch uses the Misc slot and WK_DISPATCH returns directly; armNavTimeout is intentionally not called there (it would no-op on an empty Navigate slot). Comment now says so instead of implying symmetry.
…eneration bump
Three review findings:
1) A navigate({waitUntil:'domcontentloaded'}) can settle before the
page's `load` fires. If a second navigate() starts in that window,
the first page's trailing Page.lifecycleEvent(load)/loadEventFired
would pass the gate (the old loaderId was still cached) and
chainTitle() would settle the SECOND navigate's promise with the
FIRST page's title. Fix: clear m_loaderId at the start of every
Chrome navigation (beginChromeNavigation helper); lifecycleEvent's
loaderId check and loadEventFired's !m_loaderId.isEmpty() guard
then drop the stale events. loadEventFired also gates on
m_pendingNavigate so an uninitiated load can't enqueue a PageTitle
that races a later navigate. New mock-CDP regression test.
2) bun.d.ts declared back()/forward() but the runtime prototype
registers goBack()/goForward(). Pre-existing, but this PR touched
those lines. Renamed to match runtime.
3) On WebKit, goBack/goForward/reload use the Misc slot and can run
concurrently with a navigate() on the Navigate slot. The
unconditional ++m_navGeneration before WK_DISPATCH was silently
disarming that concurrent navigate's timeout. Moved the waitUntil/
generation/loaderId setup into the Chrome branch only (via
beginChromeNavigation); WebKit leaves Navigate-slot state alone.
- uint32 ms is ~49.7 days, not ~24.8 (that's INT32_MAX ms). Comment-only.
- Constructor's initial navigate (for {url}) drops the returned promise,
so a 30s timeout would surface as unhandledrejection on slow pages.
Pass timeoutMs=0 there instead — the user's first explicit await
serializes behind it anyway.
- goBack/goForward/reload: re-check m_closed after parseNavOptions(),
same as navigate() — option getters can call user code that closes
the view.
…imeout chainTitle() now clears m_loaderId after enqueuing the PageTitle fetch. On a fast page with waitUntil:'domcontentloaded', lifecycleEvent(DCL), lifecycleEvent(load) and loadEventFired can all arrive before the first PageTitle response — each was passing the gate and enqueuing a duplicate PageTitle, and a duplicate response could settle the NEXT navigate()'s promise with the previous page's title. With the clear, only the first trigger chains; subsequent ones for the same document hit the empty- m_loaderId gate. New mock-CDP regression test. Also: the timeout closure no longer clears m_loading — it tracks the REAL load state (flipped by loadEventFired), and a timeout is the user giving up on the wait, not the browser finishing. And the WebKit constructor comment no longer claims equivalence with view.navigate(url) (it passes timeout:0, navigate() defaults 30s).
…omments Split the chainTitle() dedupe from the stale-event gate: m_navTitleChained (set by chainTitle, cleared by beginChromeNavigation) prevents duplicate PageTitle enqueues; m_loaderId stays populated until the NEXT navigation clears it, so Page.loadEventFired can distinguish "this document just finished loading" (m_loaderId non-empty → clear m_loading) from "a prior document's trailing load arrived after a new nav started" (m_loaderId empty → drop). The stale-load test now also asserts view.loading stays true. reload() now sets m_loading = true on Chrome (pre-existing omission in code this PR was already editing). goBack/goForward left as-is since the at-boundary case resolves without navigating. Two stale test comments corrected.
A PageTitle request enqueued by chainTitle() can outlive its navigation: if the navigate's timeout fires (or the user retries from .catch()) before the Runtime.evaluate response arrives, the PageTitle handler's settle(Navigate) would hit the NEW navigate's promise. m_navTitleChained already distinguishes the two — chainTitle() set it, the new beginChromeNavigation() cleared it — so gate the response handler (both success and the generic CDP error path) on it. A stale PageTitle now drops instead of mis-settling. Also: ceil options.timeout before the uint32 cast so values in (0,1) arm a 1ms timer instead of truncating to the 0="disable" sentinel. And one more stale test comment from the 742d651 → b6ea944 refactor.
Pending now carries navGen (the view's m_navGeneration at enqueue time, 0 = ungated). handleResponse drops a Navigate-slot response whose navGen doesn't match the current generation — covers every in-flight request armNavTimeout can orphan (PageNavigate errorText, PageGetNavigationHistory boundary, PageTitle, and the whole TargetCreateTarget → AttachToTarget → PageEnable chain) in one place, instead of the PageTitle-only m_navTitleChained shortcut. Chained responses carry entry.navGen forward. For a stale TargetCreateTarget response (first-navigate attach chain timed out and user retried), Chrome already created the tab; send Target.closeTarget(targetId) so it doesn't leak.
robobun
force-pushed
the
farm/46eec509/webview-navigate-waituntil-timeout
branch
from
16c6719 to
3534b57
Compare
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
1 participant
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.
Problem
await view.navigate(url)settles only onPage.loadEventFired. A page that holds a subresource connection open (SSE, long-poll, a hung<img>) never fires the windowloadevent, so the await hangs forever with no way to bound it.Change
navigate(),reload(),goBack(),goForward()now accept a second options argument:waitUntil— Chrome backendPage.setLifecycleEventsEnabled({enabled:true})is sent in the attach chain right afterPage.enable.Page.frameNavigatedstashes the main frame'sid+loaderId; the newPage.lifecycleEventhandler settles the Navigate slot whenframeId/loaderIdmatch andname == "DOMContentLoaded"(or"load"— satisfiesdomcontentloadedtoo, for same-document navs that skip DCL). Subframe lifecycle events and the about:blank replay thatsetLifecycleEventsEnabledtriggers never match becausem_frameId/m_loaderIdaren't set until the user's URL commits.waitUntil:"load"(the default) keeps usingPage.loadEventFired— no behavior change.waitUntil— WebKit backendWKNavigationDelegateexposes onlydidFinishNavigation(=load)."domcontentloaded"is accepted but degrades to"load"; the type docs say so. Usetimeoutto bound the wait on WebKit.timeoutParent-side
WTF::RunLoop::dispatchAfter(backed by Bun'sWTFTimer→EventLoopTimer). The closure captures only{viewId, backend, navGeneration}; each navigation start and each Navigate-slot settle bumpsm_navGeneration, so a stale timer fires into a generation mismatch and no-ops. No explicit cancel, noRefPtr<DispatchTimer>lifetime dance.timeout: 0skips arming. Works on both backends.Tests
test/js/bun/webview/webview-navigate-options.test.ts(new): a mock CDP WebSocket server speaks just enough protocol to drive the attach chain and emitPage.lifecycleEvent/loadEventFiredon demand. 6 tests cover DCL-settles-without-load, default-load-still-works, reload-with-DCL, timeout-rejects, stale-timer-doesn't-reject-next-nav, option validation. Runs on every CI lane without a Chrome binary.test/js/bun/webview/webview-chrome.test.ts: 5 new real-Chrome tests (todo-gated where Chrome is absent) — aBun.servethat holds an<img>response open forever, subframe-filtering with<iframe srcdoc>, option validation.Gate