From 1f5bffc4de0b874b6ec70d8d5f215dcabe3383aa Mon Sep 17 00:00:00 2001 From: sycatle Date: Thu, 16 Jul 2026 22:58:57 +0200 Subject: [PATCH] refactor: drop dead code and refresh comments after the per-connection move - Delete the now-unused CdpContext::new_full (its only caller switched to new_with_shared_context); have _new_inner build the shared BrowserContext and delegate, so the struct's field list lives in one place. - Extract drive_module_eval, shared verbatim by load_module and load_inline_module. - Update comments that still described the deleted process-wide V8 lock and the single-LocalSet model to the per-connection reality, drop the dangling reference to the removed obscura-js/src/v8_lock.rs, and soften the ISOLATE_CREATE_LOCK note now that the main-thread warmup is the real guard against the InitializeBuiltinJSDispatchTable segfault. Refs #430. --- crates/obscura-browser/src/page.rs | 2 +- crates/obscura-cdp/src/dispatch.rs | 101 ++++++++--------------------- crates/obscura-cdp/src/server.rs | 8 +-- crates/obscura-js/src/runtime.rs | 98 ++++++++++------------------ 4 files changed, 69 insertions(+), 140 deletions(-) diff --git a/crates/obscura-browser/src/page.rs b/crates/obscura-browser/src/page.rs index f4df050..9245d9e 100644 --- a/crates/obscura-browser/src/page.rs +++ b/crates/obscura-browser/src/page.rs @@ -1308,7 +1308,7 @@ impl Page { /// V8 isolate handle for this page's runtime, if it has been initialized. /// Lets the CDP dispatcher arm a per-command watchdog (which bounds any one - /// command so a hung page cannot hold the process-wide V8 lock forever) + /// command so a hung page cannot hold this connection's V8 lock forever) /// without taking `&mut self`. pub fn isolate_handle(&self) -> Option { self.js.as_ref().map(|js| js.isolate_handle()) diff --git a/crates/obscura-cdp/src/dispatch.rs b/crates/obscura-cdp/src/dispatch.rs index de2ff67..a766c9f 100644 --- a/crates/obscura-cdp/src/dispatch.rs +++ b/crates/obscura-cdp/src/dispatch.rs @@ -100,21 +100,6 @@ impl CdpContext { Self::_new_inner(proxy, stealth, user_agent, None, allow_file_access, false) } - /// Kitchen-sink constructor that also threads `allow_private_network` - /// (issue #33). Older `new_with_*` builders stay as-is. - pub fn new_full( - proxy: Option, - stealth: bool, - user_agent: Option, - storage_dir: Option, - allow_file_access: bool, - allow_private_network: bool, - ) -> Self { - Self::_new_inner( - proxy, stealth, user_agent, storage_dir, allow_file_access, allow_private_network, - ) - } - /// Build a context that reuses an already-constructed, shared /// [`BrowserContext`]. The thread-per-connection server (issue #430 / /// "Option 2") gives each connection its own `CdpContext` on its own OS @@ -122,6 +107,11 @@ impl CdpContext { /// connections must still share one cookie jar and one HTTP client. Passing /// the same `Arc` to every connection's context does that. pub fn new_with_shared_context(default_context: Arc) -> Self { + // Pre-seed with the default-frame execution context ids that + // `Runtime.enable` (1) and post-navigation re-emission (2) advertise via + // Runtime.executionContextCreated. Anything else has to be registered + // explicitly (Page.createIsolatedWorld), otherwise + // Runtime.{evaluate,callFunctionOn} should reject it per CDP spec. let mut valid_context_ids = HashSet::new(); valid_context_ids.insert(1); valid_context_ids.insert(2); @@ -160,32 +150,7 @@ impl CdpContext { allow_private_network, ); ctx.allow_file_access = allow_file_access; - let default_context = Arc::new(ctx); - // Pre-seed with the default-frame execution context ids that - // `Runtime.enable` (1) and post-navigation re-emission (2) advertise - // via Runtime.executionContextCreated. Anything else has to be - // registered explicitly (Page.createIsolatedWorld), otherwise - // Runtime.{evaluate,callFunctionOn} should reject it per CDP spec. - let mut valid_context_ids = HashSet::new(); - valid_context_ids.insert(1); - valid_context_ids.insert(2); - - CdpContext { - pages: Vec::new(), - sessions: HashMap::new(), - pending_events: Vec::new(), - default_context, - page_counter: 0, - preload_scripts: Vec::new(), - preload_counter: 0, - fetch_intercept: FetchInterceptState::new(), - intercept_tx: None, - isolated_worlds: Vec::new(), - valid_context_ids, - next_isolated_context_id: 100, - io_streams: crate::domains::io::IoStreamStore::default(), - v8_lock: Arc::new(tokio::sync::Mutex::new(())), - } + Self::new_with_shared_context(Arc::new(ctx)) } /// Claim the next isolated-world execution context id and register it as @@ -250,7 +215,7 @@ impl CdpContext { } } -/// Whether a CDP method can be served WITHOUT acquiring the global V8 lock. +/// Whether a CDP method can be served WITHOUT acquiring the per-connection V8 lock. /// /// Methods listed here were audited to confirm they do not transitively /// call into a `JsRuntime`. They either don't touch any `Page` at all, or @@ -295,41 +260,31 @@ fn is_v8_free_method(method: &str) -> bool { pub async fn dispatch(req: &CdpRequest, ctx: &mut CdpContext) -> CdpResponse { // headless_chrome (and older Puppeteer) wrap every CDP call inside // Target.sendMessageToTarget. Unwrap and recurse BEFORE acquiring the - // V8 lock — the recursive dispatch will acquire it for the inner call, - // and tokio Mutex is not reentrant. + // per-connection V8 lock — the recursive dispatch will acquire it for the + // inner call, and tokio Mutex is not reentrant. if req.method == "Target.sendMessageToTarget" { return dispatch_send_message_to_target(req, ctx).await; } - // Issue #19: V8 fatal abort under concurrent CDP work. + // Issue #430: keep this connection's V8 work serialized on its own thread. // - // Every CDP handler below may end up calling into a per-Page `JsRuntime` - // (each owning its own V8 Isolate). All of them run on a single OS - // thread (current_thread tokio + LocalSet). When two pages' V8-touching - // futures interleave across an `.await` (which `process_with_interception` - // can trigger by handling new CDP messages while a navigation task is in - // flight on `spawn_local`), V8 trips the - // `heap->isolate() == Isolate::TryGetCurrent()` invariant and aborts the - // process via `V8_Fatal` — no Rust panic, just `abort(3)`. + // Every CDP handler below may call into a per-Page `JsRuntime` (each owning + // its own V8 Isolate). With the thread-per-connection server each connection + // runs on its own OS thread, so isolates never collide across connections. + // Within one connection, though, a nav task spawned by + // `process_with_interception` runs while this processor keeps pumping other + // CDP messages, so two of this connection's pages could still interleave V8 + // work on this one thread and trip + // `heap->isolate() == Isolate::TryGetCurrent()`. The per-connection lock + // (`ctx.v8_lock`) keeps each handler contiguous: V8 fully exits one Isolate + // before the next of this connection's pages is allowed in. It is + // per-connection, not process-wide, so other connections run in parallel. // - // Holding the process-wide V8 lock around the entire dispatch keeps each - // handler contiguous on the thread: V8 fully exits one Isolate before - // the next page is allowed in. This converts the abort into latency. - // It overshoots — 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; that's tracked as the larger #19 follow-up. - // - // Optimization: methods that demonstrably never touch V8 bypass the - // lock. During Puppeteer's newPage() setup ~8 such calls (Page.enable, - // Runtime.enable, Page.getFrameTree, Page.addScriptToEvaluateOnNewDocument, - // Page.createIsolatedWorld, etc.) used to serialize behind any - // in-flight V8 work on a sibling page. Bypassing keeps the setup chain - // running while a separate nav executes. Each listed method was audited - // to confirm it never reaches `JsRuntime::execute_script` or DOM - // mutation that re-enters V8; `get_session_page_mut` (which can - // trigger `suspend_js`/`resume_js`) is NOT in the list. + // Optimization: methods that demonstrably never touch V8 bypass the lock + // (Puppeteer's newPage() setup issues ~8 such calls). Each listed method was + // audited to confirm it never reaches `JsRuntime::execute_script` or DOM + // mutation that re-enters V8; `get_session_page_mut` (which can trigger + // `suspend_js`/`resume_js`) is NOT in the list. let _v8_guard = if is_v8_free_method(&req.method) { None } else { @@ -341,8 +296,8 @@ pub async fn dispatch(req: &CdpRequest, ctx: &mut CdpContext) -> CdpResponse { // Per-command V8 watchdog. The lock above keeps each handler contiguous on // the thread, but it does not bound how long a handler runs: a hung page (a - // runaway Runtime.evaluate, a synchronous DOM op) would hold the - // process-wide V8 lock and wedge every other session forever. The one-shot + // runaway Runtime.evaluate, a synchronous DOM op) would hold this + // connection's V8 lock and wedge its other sessions forever. The one-shot // CLI uses a process-level hard deadline for this; the long-running server // cannot force-exit, so we terminate just the offending isolate instead. // OBSCURA_CDP_COMMAND_TIMEOUT_MS tunes the bound (0 disables); the default diff --git a/crates/obscura-cdp/src/server.rs b/crates/obscura-cdp/src/server.rs index 3764ac8..06e76da 100644 --- a/crates/obscura-cdp/src/server.rs +++ b/crates/obscura-cdp/src/server.rs @@ -178,7 +178,7 @@ pub async fn start_with_full_serve_options( // reuses it (one cookie jar, one HTTP client) while running on its own OS // thread, so each page's V8 isolate is confined to a single thread and the // #430 cross-page abort cannot happen (V8's TryGetCurrent check is - // per-thread). This is the "Option 2" fix named in obscura-js/src/v8_lock.rs. + // per-thread). This is the thread-per-runtime fix for #430. let mut bctx = obscura_browser::BrowserContext::with_storage_and_network( "default".to_string(), proxy, @@ -460,8 +460,8 @@ async fn cdp_processor( loop { // Drain any deferred messages from the previous interception window // before pulling new ones off the wire. Each is processed with no - // nav-task spawn_local in flight, so dispatch's v8_lock can claim - // the only Isolate cleanly. + // nav-task spawn_local in flight, so this connection's only entered + // Isolate is the one dispatch is about to touch. let msg = if let Some(d) = deferred.pop_front() { d } else { @@ -710,7 +710,7 @@ async fn process_with_interception( // `heap->isolate() == Isolate::TryGetCurrent()` invariant and aborts // the process via `V8_Fatal`. // - // The `obscura_js::v8_lock` mutex doesn't save us here: it's a + // This connection's `ctx.v8_lock` doesn't save us here: it's a // `tokio::sync::Mutex` that is released around `.await`s inside V8 // ops, so it doesn't actually keep the V8 enter/exit pair contiguous // on the thread. diff --git a/crates/obscura-js/src/runtime.rs b/crates/obscura-js/src/runtime.rs index 2ced500..da5122b 100644 --- a/crates/obscura-js/src/runtime.rs +++ b/crates/obscura-js/src/runtime.rs @@ -15,12 +15,15 @@ use crate::ops::{build_extension, ObscuraState, StoredNetworkResponseBody}; static SNAPSHOT: &[u8] = include_bytes!(env!("OBSCURA_SNAPSHOT_PATH")); /// Serializes V8 isolate construction across OS threads. The thread-per- -/// connection server (issue #430 "Option 2") builds isolates on many threads; -/// the V8 platform is process-global and concurrent first-time isolate setup is -/// not safe to run from several threads at once (it segfaults). Construction is -/// fast and rare (once per page), so serializing it costs nothing measurable. -/// Isolate *execution* stays fully parallel: once built, each isolate runs on -/// its own thread with no shared lock. +/// connection server (issue #430) builds isolates on many threads. The main +/// thread already warms up V8 once before any connection thread starts (see the +/// `ObscuraJsRuntime::new` warmup in `obscura-cdp` server startup), which is +/// what actually prevents the `InitializeBuiltinJSDispatchTable` segfault of a +/// first isolate built off the main thread. This lock is defense-in-depth: it +/// keeps two connections from running V8's isolate setup concurrently in case +/// any residual first-time process init races. Construction is rare and fast, so +/// serializing it costs nothing measurable; isolate *execution* stays fully +/// parallel, each isolate on its own thread with no shared lock. static ISOLATE_CREATE_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); #[derive(Debug, Clone)] @@ -55,7 +58,7 @@ pub struct WatchdogToken { /// Arm a V8 termination watchdog directly from an isolate handle, with no /// runtime borrow. The CDP dispatcher uses this to bound every command so a -/// hung page cannot hold the process-wide V8 lock forever. Pair with +/// hung page cannot hold this connection's V8 lock forever. Pair with /// [`WatchdogToken::stop`]; if `stop` returns true, clear the termination flag /// via [`ObscuraJsRuntime::cancel_termination`] before reusing the isolate. pub fn spawn_watchdog(handle: IsolateHandle, budget: std::time::Duration) -> WatchdogToken { @@ -690,20 +693,25 @@ impl ObscuraJsRuntime { } }; - let mut result = Box::pin(self.runtime.mod_evaluate(module_id)); - let deadline = tokio::time::Instant::now() + budget; + // Return as soon as the module finishes evaluating rather than waiting + // for the loop to go fully idle: a page timer (setInterval) keeps the + // loop busy forever and would otherwise burn the whole budget (#374). + self.drive_module_eval(module_id, budget_ms, &format!("Module {}", url)) + .await; + Ok(()) + } - // Return as soon as the module finishes evaluating instead of waiting - // for the event loop to go fully idle: a page timer (setInterval) keeps - // the loop busy forever and would otherwise burn the whole budget, so a - // module that had already evaluated got abandoned (issue #374). - // - // Drive it WITHOUT the #430 isolate leak: pump the event loop one - // self-contained tick at a time (poll_event_loop exits the isolate per - // call) and poll mod_evaluate alongside it, instead of cancelling a - // `run_event_loop` future with a tokio timeout — a fired timeout drops - // that future mid-poll and leaves the isolate entered on the thread, - // aborting the process once a second page enters its own isolate. + /// Drive a just-started module evaluation to completion, or up to + /// `budget_ms`, WITHOUT the #430 isolate leak: pump the event loop one + /// self-contained tick at a time (`poll_event_loop` exits the isolate per + /// call) and poll the `mod_evaluate` future alongside it, instead of + /// cancelling a `run_event_loop` future with a tokio timeout (a fired timeout + /// drops that future mid-poll and leaves the isolate entered on the thread). + /// A module eval error or timeout is logged under `what` and swallowed: + /// neither is fatal to rendering the rest of the page. + async fn drive_module_eval(&mut self, module_id: deno_core::ModuleId, budget_ms: u64, what: &str) { + let deadline = tokio::time::Instant::now() + tokio::time::Duration::from_millis(budget_ms); + let mut result = Box::pin(self.runtime.mod_evaluate(module_id)); loop { let done = std::future::poll_fn(|cx| { let _ = self @@ -718,19 +726,18 @@ impl ObscuraJsRuntime { match done { Some(Ok(())) => break, Some(Err(e)) => { - tracing::warn!("Module eval error: {}", e); + tracing::warn!("{} eval error: {}", what, e); break; } None => { if tokio::time::Instant::now() >= deadline { - tracing::warn!("Module evaluation timed out after {}ms: {}", budget_ms, url); + tracing::warn!("{} evaluation timed out after {}ms", what, budget_ms); break; } tokio::time::sleep(tokio::time::Duration::from_millis(1)).await; } } } - Ok(()) } pub async fn load_inline_module(&mut self, code: &str, base_url: &str, budget_ms: u64) -> Result<(), String> { @@ -757,45 +764,12 @@ impl ObscuraJsRuntime { } }; - let mut result = Box::pin(self.runtime.mod_evaluate(module_id)); - let deadline = tokio::time::Instant::now() + budget; - - // Drive the event loop, but return as soon as the module finishes - // evaluating rather than waiting for the loop to go fully idle. A page - // timer (Vite's HMR / React-Refresh client installs a setInterval) keeps - // the loop busy forever; waiting for idle burned the whole budget on this - // preamble module and starved the later module that mounts the app, - // leaving #root empty (issue #374). - // - // Cancellation-safe pump (see #430 / load_module): never cancel a - // `run_event_loop` future with a tokio timeout, which drops it mid-poll - // and leaves the isolate entered on the thread. - loop { - let done = std::future::poll_fn(|cx| { - let _ = self - .runtime - .poll_event_loop(cx, deno_core::PollEventLoopOptions::default()); - std::task::Poll::Ready(match std::future::Future::poll(result.as_mut(), cx) { - std::task::Poll::Ready(r) => Some(r), - std::task::Poll::Pending => None, - }) - }) - .await; - match done { - Some(Ok(())) => break, - Some(Err(e)) => { - tracing::warn!("Inline module eval error: {}", e); - break; - } - None => { - if tokio::time::Instant::now() >= deadline { - tracing::warn!("Inline module timed out after {}ms", budget_ms); - break; - } - tokio::time::sleep(tokio::time::Duration::from_millis(1)).await; - } - } - } + // Return as soon as the module finishes evaluating rather than waiting + // for idle: Vite's HMR / React-Refresh client installs a setInterval that + // keeps the loop busy forever, and waiting for idle burned the whole + // budget on this preamble module and starved the module that mounts the + // app, leaving #root empty (issue #374). + self.drive_module_eval(module_id, budget_ms, "Inline module").await; Ok(()) }