Remove no-longer needed setTimeouts in test now that messages are queued.
Runner also checks ready_queue when determining doneness.
Co-authored-by: Navid EMAD <design.navid@gmail.com>
At some point recently, we started to process scripts that fail to load (e.g.
404). This stops such scripts from [trying] to be evaluated, and executes the
onerror handler in all script loading paths.
Client.tick drains self.queue (assigning conns to queued transfers) only
at the start. When perform / processMessages releases a batch of conns
back to the pool, those conns sit idle until the next tick — a queued
transfer that could have run this tick waits one Runner iteration
(~20 ms in the test runner) for no reason. Adds a second drainQueue
call after perform so newly-freed conns get picked up immediately.
In practice this matters whenever httpMaxHostOpen / httpMaxConcurrent
is exceeded — pages with N > limit subresources had each "wave" of
queue overflow paying one extra tick of latency.
cache=true is problematic for a few reasons
1 - The current cache implementation is known to cause timing issues; i.e. it
executes callbacks synchronously.
2 - Unlike something like robots.txt or proxy, cache tests need to be explicitly
tested. The response has to include cache headers and the resource loaded
again.
Client.makeRequest used to call self.perform(0) after handing the transfer
to libcurl. That perform() does two things: drives curl_multi_perform (so
bytes hit the wire) AND drains curl_multi_info_read messages, which is
what fires the user-facing header/data/done callbacks.
The issue is that, even in non-cache cases, a request could be immediately
resolved in libcurl, and thus callbacks executed synchronously.
By only calling `curl_multi_perform` on a new request, we prevent this from
happening.
First, KeyValueList.fromJsObject now only iterates own properties. Second
URLSearchParams can now be constructed with another URLSearchParams. This is
a stopgap. The correct solution is for it to accept any iterator, but as a
quick fix for known cases (airbnb.com), this will help.
Adds two ways to opt out of dedicated Web Worker loading entirely. The
Worker constructor still returns a Worker object so calling pages don't
throw, but no script fetch is initiated and the worker scope's eval
never runs (postMessage from the page is queued indefinitely with no
handler to drain it).
* CDP method LP.configureLoading { worker: bool } -- per-session
toggleable at runtime, alongside the existing { subFrame: bool }.
Both fields are now optional so callers can flip one without
resetting the other to its default. Backwards-compatible.
* CLI flag --disable-workers -- process-wide default applying to every
session and to the fetch subcommand. Operators can flip it on without
any driver changes. Mirrors --disable-subframes (#2401) exactly.
## Motivation
Reliably-reproducible SIGABRT in Worker.loadInitialScript whenever a
page constructs a Web Worker AND lightpanda is launched with
--http_proxy. Crash signature:
$msg="V8 fatal callback" location=v8::Context::Exit()
message="Cannot exit non-entered context"
Stack:
_browser.webapi.Worker.loadInitialScript
_browser.webapi.Worker.httpDoneCallback
_network.layer.InterceptionLayer.InterceptContext.doneCallback
_browser.HttpClient.processMessages
_browser.HttpClient.perform
_browser.HttpClient.tick
The Zig-side Enter/Exit pair around the worker's eval doesn't match
v8's entered_contexts stack invariant under that timing -- something
upstream of the loadInitialScript Exit leaves an extra Enter on the
stack, so v8's Utils::ApiCheck (`isolate->context() == *env`) fires
and the process aborts.
Reproducible against any Shopify storefront PDP (e.g.
https://weareallbirds.myshopify.com/products/mens-wool-runners) when
served through any HTTP proxy -- the proxy just adds enough latency
to surface the race; the same code path runs without --http_proxy
but the timing window is too tight to reliably hit. The Allbirds
trigger script is the Shopify web-pixel-extension worker, but ANY
Worker the page constructs hits the same code path.
The proper fix needs the v8 entered-contexts invariant to be
restored end-to-end through the worker eval. That's a deeper dig
into how Worker.loadInitialScript / WorkerGlobalScope.importScript /
ls.local.runMacrotasks compose with v8's microtask queues across
multiple contexts; I tried three intermediate fixes (deferring
loadInitialScript via the frame scheduler when other scripts are
mid-eval, replacing the post-eval cross-context runMacrotasks with
worker-only PerformCheckpoint, and removing runMacrotasks entirely)
and none stopped the crash. The bug is fired from inside the
synchronous tick path before the post-eval microtask handling
runs, which means the leak happens during Script::Run itself and
needs more targeted investigation.
This PR is the workaround so users hitting the SIGABRT on
storefront / analytics-heavy pages have a clean opt-in escape today.
For our use case (product catalog extraction) Workers carry no
extraction signal -- web-pixel sandboxes, analytics SDKs, marketing
tag pixels, etc. -- so disabling them removes a fragile code path
without any downside.
## Implementation
`Session.worker_loading_enabled: bool = true` -- default matches
existing behavior.
`Worker.init` short-circuits AFTER constructing the Worker /
WorkerGlobalScope / arena bookkeeping (so the JS `new Worker(url)`
expression doesn't throw):
if (!session.worker_loading_enabled) {
log.debug(.browser, "worker disabled", .{ .url = resolved_url });
return self;
}
Two ways to flip the flag, mirroring the --disable-subframes pattern:
1. LP.configureLoading { worker: bool } -- both subFrame and worker
are now optional fields in the params struct, so existing callers
passing only { subFrame } continue to work unchanged.
2. --disable-workers CLI flag -- added to CommonOptions (so it
applies to serve, fetch, mcp). New Config.disableWorkers() getter;
Session.init reads it as the initial value.
Total diff: +88 / -3 across 4 files (src/Config.zig,
src/browser/Session.zig, src/browser/webapi/Worker.zig,
src/cdp/domains/lp.zig).
## Verification
Reproducer pattern (puppeteer-core 24.42.0 + tiny CONNECT-tunnel
proxy on 127.0.0.1:9999, scripts in cdp-repros/):
serve --host 127.0.0.1 --port 9222 --http_proxy http://127.0.0.1:9999
serve --host 127.0.0.1 --port 9222 --http_proxy http://127.0.0.1:9999 --disable-workers
Driving https://weareallbirds.myshopify.com/products/mens-wool-runners:
baseline (no --disable-workers): 5/5 SIGABRT in
Worker.loadInitialScript with the v8 fatal callback above.
with --disable-workers: 10/10 successful, returns full
HTML (~1MB), no crash.
Test suite:
make test -> 637 of 637 tests passed (was 636/636 + new
cdp.lp: configureLoading toggles subFrame and worker
independently regression test).
zig fmt --check ./*.zig ./**/*.zig -> clean.
## Notes
* The CDP method is the same domain (LP.configureLoading) and same
shape as --disable-subframes' driver-side opt-in, so existing
Playwright / puppeteer integrations that already toggle
subframes don't need a separate code path -- one CDP call can
flip both.
* worker_loading_enabled = false does NOT remove Worker from the
global namespace (so feature-detection like
`if (typeof Worker !== 'undefined')` still reports true). It just
makes constructed workers no-op. Pages that postMessage to a worker
and wait for a response will hang on that promise forever (or
until the page is torn down). For our extraction use case that's
fine -- we control the worklist timeout anyway -- but it's worth
noting if upstream wants to surface the disabled state more
strongly (e.g. throw from postMessage, or remove the global
entirely behind an even-stricter flag).
* Once the underlying v8 entered-contexts invariant is restored in
Worker.loadInitialScript, this flag becomes a perf / sandboxing
tool rather than a correctness workaround. Worth keeping anyway:
blocking analytics / pixel workers is a reasonable thing to want.
## Related
* #2400 -- the iframe analog to this issue (subframe nav invalidates
executionContextId); same workaround pattern.
* #2401 -- introduced --disable-subframes / LP.configureLoading
{ subFrame } that this PR mirrors exactly for workers.
The HTMLDialogElement constructor was exposed with `open` / `returnValue`
IDL accessors, but the three instance methods that drive the open/close
state were missing. Per HTML §4.11.4 (The dialog element), `show()` sets
the `open` attribute if absent; `showModal()` throws `InvalidStateError`
when the dialog is already open and otherwise sets `open`; `close()`
removes `open`, optionally updates `returnValue`, and fires a non-
bubbling `close` event. The non-rendering steps (focus trap, backdrop,
top-layer placement) are intentional no-ops here — `[open]` reflecting
through to selectors and the `close` event firing are what downstream
CDP clients rely on.
Closes#2434
The build script wrote the version line to stdout, polluting any pipeline
that captures program output via 'zig build run' or similar. Banners
belong on stderr.
The frameNavigated CDP handler sent Page.frameNavigated twice per
navigation and always used the root frame's V8 context for
inspector.contextCreated, even during iframe navigations. This caused
the root frame's inspector context id to be silently re-registered
(and the previous id invalidated) whenever an iframe navigated.
The duplicate Page.frameNavigated masked this bug — Puppeteer happened
to pick up the re-registered context id. Removing the duplicate exposed
the underlying issue: callFunctionOn with the original id failed with
"Cannot find context with specified id".
1 - Track owner of a request (for simpler / more accurate abort (TBD))
2 - Create Transfer upfront, make everything work on Transfer (not Request)
This helps remove ambiguity about cleanup and simplifies layers. For example
Robots request is just another normal request, not a special case. This gives
everything a stable address (the *Transfer which can be looked up by id)
