Timothy Olaleke
85739d3f6a
* fix(#19): serialize V8 work across pages with process-wide async lock Concurrent CDP clients (≥5) trip V8's `heap->isolate() == Isolate::TryGetCurrent()` invariant and abort the process via V8_Fatal — no Rust panic, just abort(3). Each Page owns its own JsRuntime/Isolate and they all run on a single OS thread via LocalSet+spawn_local; when two pages' V8-touching futures interleave across an .await, V8 trips the check. This adds a process-wide tokio::sync::Mutex (obscura_js::v8_lock) that is acquired: - At the top of obscura_cdp::dispatch::dispatch (covers every CDP handler that may call into a JsRuntime). - In server.rs's process_with_interception spawn_local that runs page.navigate_with_wait + execute_preload_script (this path bypasses dispatch). The guard keeps each V8-touching block contiguous on the thread: V8 fully exits one Isolate before another page is allowed in. Trade-off: non-V8 handlers (Browser.*, Storage.*, Emulation.*) also serialize, but those are cheap and the safety win dominates. The properly concurrent fix is to pin each JsRuntime to its own OS thread and message-pass; tracked as the larger #19 follow-up. Adds an opt-in smoke test (`--ignored`) that boots the CDP server and runs 5 parallel clients through createTarget+navigate. Note: the deterministic abort needs subresource-fetch yields (page.rs:285 join_all) which data: URLs skip — full repro requires JS-heavy real traffic (issue author used scrapegraphai.com). * fix(#19): suspend other Isolates and defer foreign CDP during interception nav Address maintainer feedback on PR #36 (comment 4328238087): the V8 fatal still reproduces with `Fetch.enable patterns:["*"]` + 5 concurrent `Page.navigate`s. The `obscura_js::v8_lock` mutex was insufficient because V8's actual invariant is "only one Isolate may be entered per OS thread", and the mutex can't enforce that across `.await` points inside V8 ops. Two distinct paths violated the invariant: 1. Foreign CDP messages dispatched while a nav task was running. `process_with_interception`'s `select!` routed every non-Fetch message through `process_cdp_message → dispatch → page::handle::*`. `get_session_page_mut` then `suspend_js`'es other pages (drops their `JsRuntime` → `Isolate::Exit`) while the spawned nav task is parked at `op_fetch_url`'s `resolve_rx.await` with its Isolate still entered. Drop trips `heap->isolate() == Isolate::TryGetCurrent()`. 2. Subsequent navs through `process_with_interception` did not suspend prior pages. After nav-1 completes (page-1's JsRuntime stays alive in `ctx.pages`), nav-2's `init_js` constructs Isolate-2 while Isolate-1 is still current. The next V8 scope unwind aborts via `Context::Exit`. Fix (3 changes in `crates/obscura-cdp/src/server.rs`): - `cdp_processor`: add a `VecDeque<ServerMessage> deferred` drained before each new `rx.recv()`, so deferred messages are processed with no nav task in flight. - `process_with_interception` `select!`: only handle `NewConnection` and `Fetch.{continue,fulfill,fail}Request` inline (safe — no V8 enter on this side); push everything else into `deferred`. - `process_with_interception` entry: explicitly `suspend_js()` every other page in `ctx.pages` before spawning the nav task, mirroring the invariant the regular dispatch path already enforces via `get_session_page_mut`. Verification: - New test `crates/obscura-cdp/tests/concurrent_navigations_with_fetch.rs` is the maintainer's exact repro (5 clients, `Fetch.enable patterns:["*"]`, concurrent `Page.navigate` to https://example.com, auto-reply `Fetch.continueRequest`). Aborts on PR #36 head, passes after fix (5/5 stable runs, ~1.2s each). - Existing data-URL smoke test (`concurrent_navigations`) still passes. - Full workspace test suite (90 tests) still passes. Run with: cargo test -p obscura-cdp --test concurrent_navigations_with_fetch \ -- --nocapture --ignored Honest caveat. This is still the lock/serialize approach — it converts the abort into latency. The properly concurrent fix (Option 2 in issue #19: pin each `JsRuntime` to its own OS thread and message-pass) remains the right long-term direction. * fix(cdp): bound deferred message queue to prevent OOM during stalled navigation PR #36 comment 4341743194: process_with_interception deferred foreign CDP messages into an unbounded VecDeque while a navigation task was in flight. A stalled navigation (e.g. waiting for Fetch interception resolution) could accumulate arbitrary queued messages and OOM the process. Add MAX_DEFERRED_MESSAGES = 256. When the cap is reached, return an explicit CDP error (-32000) to the client instead of silently dropping or growing without bound. All 90 workspace tests pass.
28 lines
1.3 KiB
Rust
28 lines
1.3 KiB
Rust
//! Process-wide async lock that serializes V8 work across all `JsRuntime`s.
|
|
//!
|
|
//! V8's invariant: on any given OS thread, only one `Isolate` may be entered
|
|
//! at a time, and HandleScope/ContextScope stacks must unwind on the Isolate
|
|
//! they belong to. Obscura's CDP server runs every `JsRuntime` (one per Page)
|
|
//! on a single OS thread via `tokio::task::LocalSet` + `spawn_local`. As soon
|
|
//! as two pages' V8-touching futures interleave across an `.await`, V8 trips
|
|
//! its `heap->isolate() == Isolate::TryGetCurrent()` check and aborts the
|
|
//! whole process (no Rust panic; `V8_Fatal` calls `abort(3)`).
|
|
//!
|
|
//! Acquiring this lock around any block that calls `JsRuntime::execute_script`
|
|
//! or `JsRuntime::run_event_loop` keeps that block contiguous on the thread:
|
|
//! V8 fully exits the prior Isolate before the next page is allowed in. This
|
|
//! converts the abort into latency. It is the issue-19 "Option 1" fix.
|
|
//!
|
|
//! The properly concurrent fix is to pin each `JsRuntime` to its own OS
|
|
//! thread (issue-19 "Option 2"); that's a larger refactor tracked separately.
|
|
|
|
use std::sync::OnceLock;
|
|
use tokio::sync::Mutex;
|
|
|
|
static V8_LOCK: OnceLock<Mutex<()>> = OnceLock::new();
|
|
|
|
/// Returns the process-wide V8 serialization lock.
|
|
pub fn global() -> &'static Mutex<()> {
|
|
V8_LOCK.get_or_init(|| Mutex::new(()))
|
|
}
|