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

@@ -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<obscura_js::runtime::IsolateHandle> {
         self.js.as_ref().map(|js| js.isolate_handle())

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<String>,
-        stealth: bool,
-        user_agent: Option<String>,
-        storage_dir: Option<std::path::PathBuf>,
-        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<BrowserContext>` to every connection's context does that.
     pub fn new_with_shared_context(default_context: Arc<BrowserContext>) -> 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);
+        Self::new_with_shared_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(())),
-        }
     }
 
     /// 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,
+    // per-connection V8 lock — the recursive dispatch will acquire it for the
-    // and tokio Mutex is not reentrant.
+    // 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`
+    // Every CDP handler below may call into a per-Page `JsRuntime` (each owning
-    // (each owning its own V8 Isolate). All of them run on a single OS
+    // its own V8 Isolate). With the thread-per-connection server each connection
-    // thread (current_thread tokio + LocalSet). When two pages' V8-touching
+    // runs on its own OS thread, so isolates never collide across connections.
-    // futures interleave across an `.await` (which `process_with_interception`
+    // Within one connection, though, a nav task spawned by
-    // can trigger by handling new CDP messages while a navigation task is in
+    // `process_with_interception` runs while this processor keeps pumping other
-    // flight on `spawn_local`), V8 trips the
+    // CDP messages, so two of this connection's pages could still interleave V8
-    // `heap->isolate() == Isolate::TryGetCurrent()` invariant and aborts the
+    // work on this one thread and trip
-    // process via `V8_Fatal` — no Rust panic, just `abort(3)`.
+    // `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
+    // Optimization: methods that demonstrably never touch V8 bypass the lock
-    // handler contiguous on the thread: V8 fully exits one Isolate before
+    // (Puppeteer's newPage() setup issues ~8 such calls). Each listed method was
-    // the next page is allowed in. This converts the abort into latency.
+    // audited to confirm it never reaches `JsRuntime::execute_script` or DOM
-    // It overshoots — non-V8 handlers (Browser.*, Storage.*, Emulation.*)
+    // mutation that re-enters V8; `get_session_page_mut` (which can trigger
-    // also serialize — but those are cheap and the safety win dominates.
+    // `suspend_js`/`resume_js`) is NOT in the list.
-    //
-    // 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.
     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
+    // runaway Runtime.evaluate, a synchronous DOM op) would hold this
-    // process-wide V8 lock and wedge every other session forever. The one-shot
+    // 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

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
+        // nav-task spawn_local in flight, so this connection's only entered
-        // the only Isolate cleanly.
+        // 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.

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;
+/// connection server (issue #430) builds isolates on many threads. The main
-/// the V8 platform is process-global and concurrent first-time isolate setup is
+/// thread already warms up V8 once before any connection thread starts (see the
-/// not safe to run from several threads at once (it segfaults). Construction is
+/// `ObscuraJsRuntime::new` warmup in `obscura-cdp` server startup), which is
-/// fast and rare (once per page), so serializing it costs nothing measurable.
+/// what actually prevents the `InitializeBuiltinJSDispatchTable` segfault of a
-/// Isolate *execution* stays fully parallel: once built, each isolate runs on
+/// first isolate built off the main thread. This lock is defense-in-depth: it
-/// its own thread with no shared lock.
+/// 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));
+        // Return as soon as the module finishes evaluating rather than waiting
-        let deadline = tokio::time::Instant::now() + budget;
+        // 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
+    /// Drive a just-started module evaluation to completion, or up to
-        // for the event loop to go fully idle: a page timer (setInterval) keeps
+    /// `budget_ms`, WITHOUT the #430 isolate leak: pump the event loop one
-        // the loop busy forever and would otherwise burn the whole budget, so a
+    /// self-contained tick at a time (`poll_event_loop` exits the isolate per
-        // module that had already evaluated got abandoned (issue #374).
+    /// call) and poll the `mod_evaluate` future alongside it, instead of
-        //
+    /// cancelling a `run_event_loop` future with a tokio timeout (a fired timeout
-        // Drive it WITHOUT the #430 isolate leak: pump the event loop one
+    /// drops that future mid-poll and leaves the isolate entered on the thread).
-        // self-contained tick at a time (poll_event_loop exits the isolate per
+    /// A module eval error or timeout is logged under `what` and swallowed:
-        // call) and poll mod_evaluate alongside it, instead of cancelling a
+    /// neither is fatal to rendering the rest of the page.
-        // `run_event_loop` future with a tokio timeout — a fired timeout drops
+    async fn drive_module_eval(&mut self, module_id: deno_core::ModuleId, budget_ms: u64, what: &str) {
-        // that future mid-poll and leaves the isolate entered on the thread,
+        let deadline = tokio::time::Instant::now() + tokio::time::Duration::from_millis(budget_ms);
-        // aborting the process once a second page enters its own isolate.
+        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));
+        // Return as soon as the module finishes evaluating rather than waiting
-        let deadline = tokio::time::Instant::now() + budget;
+        // for idle: Vite's HMR / React-Refresh client installs a setInterval that
-
+        // keeps the loop busy forever, and waiting for idle burned the whole
-        // Drive the event loop, but return as soon as the module finishes
+        // budget on this preamble module and starved the module that mounts the
-        // evaluating rather than waiting for the loop to go fully idle. A page
+        // app, leaving #root empty (issue #374).
-        // timer (Vite's HMR / React-Refresh client installs a setInterval) keeps
+        self.drive_module_eval(module_id, budget_ms, "Inline module").await;
-        // 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;
-                }
-            }
-        }
         Ok(())
     }