diff --git a/Cargo.toml b/Cargo.toml index 4bc66eb..f8de046 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,3 +43,10 @@ uuid = { version = "1", features = ["v4"] } thiserror = "2" anyhow = "1" base64 = "0.22" + +# Unwinding (the Rust default) is REQUIRED for the anti-panic protocol: ops wrap +# their bodies in std::panic::catch_unwind so a panic degrades to an error return +# instead of unwinding into V8's FFI frame and aborting the process. panic="abort" +# would turn every catchable op panic into a hard crash. +[profile.release] +panic = "unwind" diff --git a/crates/obscura-browser/src/page.rs b/crates/obscura-browser/src/page.rs index c1271bb..ccbedd1 100644 --- a/crates/obscura-browser/src/page.rs +++ b/crates/obscura-browser/src/page.rs @@ -295,6 +295,17 @@ impl Page { let script_deadline = tokio::time::Instant::now() + tokio::time::Duration::from_millis(script_deadline_ms); + // Hard backstop over the WHOLE script-execution phase. Inline scripts + // run back-to-back with no await between them, so neither the soft + // deadline above (only checked between scripts) nor the per-script guard + // can interrupt a page that burns the budget across many synchronous + // scripts (the real-world SPA / anti-bot busy-loop hang). This watchdog + // terminates the isolate if cumulative synchronous script work overruns. + let exec_wd = self + .js + .as_mut() + .map(|js| js.arm_watchdog(std::time::Duration::from_millis(script_deadline_ms + 1000))); + #[derive(Debug)] struct ScriptInfo { src: Option, @@ -596,6 +607,10 @@ impl Page { // On busy sites this could keep the V8 lock held for tens of // seconds, wedging the entire CDP dispatcher (see triage for // issue series around the 40-site compat sweep). + // A single run_event_loop poll that pins the thread inside V8 makes + // the per-poll tokio timeouts below useless, so guard the whole loop + // with a watchdog that fires ~250ms past its 500ms deadline. + let settle_wd = js.arm_watchdog(std::time::Duration::from_millis(750)); let deadline = tokio::time::Instant::now() + tokio::time::Duration::from_millis(500); let mut idle_count = 0u32; loop { @@ -626,6 +641,12 @@ impl Page { } } } + js.disarm_watchdog(settle_wd); + } + if let Some(token) = exec_wd { + if let Some(js) = self.js.as_mut() { + js.disarm_watchdog(token); + } } } @@ -694,11 +715,11 @@ impl Page { return; } if let Some(js) = &mut self.js { - let _ = tokio::time::timeout( - tokio::time::Duration::from_millis(max_ms), - js.run_event_loop(), - ) - .await; + // Bounded against both async idle and synchronous microtask storms: + // a plain tokio timeout cannot preempt a page that pins the thread + // inside V8 (the real-world SPA hang), so settle drives the loop + // through the watchdog-guarded path. + let _ = js.run_event_loop_bounded(max_ms).await; } } @@ -1010,6 +1031,13 @@ impl Page { _ => 0, }; + // Same hazard as the post-script settle: a synchronous poll can pin + // the thread past the 5s network-idle deadline, so arm a watchdog + // that terminates the isolate ~500ms past it. + let netidle_wd = self + .js + .as_mut() + .map(|js| js.arm_watchdog(std::time::Duration::from_millis(5500))); let deadline = tokio::time::Instant::now() + tokio::time::Duration::from_secs(5); let mut idle_since: Option = None; @@ -1043,6 +1071,11 @@ impl Page { } } + if let Some(token) = netidle_wd { + if let Some(js) = self.js.as_mut() { + js.disarm_watchdog(token); + } + } self.lifecycle = LifecycleState::NetworkIdle; } @@ -1075,6 +1108,27 @@ impl Page { self.dom.as_ref() } + /// Like [`Self::evaluate`] but bounded by a V8 watchdog so a runaway + /// expression cannot hang the process. A non-zero `timeout` of zero falls + /// back to the unbounded path. + pub fn evaluate_with_timeout( + &mut self, + expression: &str, + timeout: std::time::Duration, + ) -> serde_json::Value { + if let Some(js) = &mut self.js { + match js.evaluate_with_timeout(expression, timeout) { + Ok(val) => val, + Err(e) => { + tracing::debug!("JS eval error/timeout for '{}': {}", &expression[..expression.len().min(80)], e); + serde_json::Value::Null + } + } + } else { + self.evaluate(expression) + } + } + pub fn evaluate(&mut self, expression: &str) -> serde_json::Value { if let Some(js) = &mut self.js { match js.evaluate(expression) { diff --git a/crates/obscura-cli/src/main.rs b/crates/obscura-cli/src/main.rs index 919fcec..7163113 100644 --- a/crates/obscura-cli/src/main.rs +++ b/crates/obscura-cli/src/main.rs @@ -513,6 +513,22 @@ async fn run_fetch( eprintln!("Fetching {}...", url_str); } + // Process-level hard deadline. A synchronous hang inside a Rust op invoked + // from page JS cannot be cancelled by tokio (there is no await to interrupt) + // nor by the V8 watchdog (terminate_execution only unwinds JS bytecode, not + // native Rust running beneath a V8->op call). As an absolute backstop so one + // fetch can never wedge the worker, a daemon thread force-exits if the whole + // operation overruns timeout + wait + grace. A normal fetch returns first and + // the process exits before this fires. + { + let hard = Duration::from_secs(timeout_secs.saturating_add(wait_secs).saturating_add(10)); + std::thread::spawn(move || { + std::thread::sleep(hard); + eprintln!("obscura: hard timeout exceeded ({}s); forcing exit", hard.as_secs()); + std::process::exit(124); + }); + } + match timeout(Duration::from_secs(timeout_secs), page.navigate_with_wait(url_str, wait_condition)).await { Ok(result) => result.map_err(|e| anyhow::anyhow!("Failed to navigate to {}: {}", url_str, e))?, Err(_) => anyhow::bail!( @@ -540,7 +556,9 @@ async fn run_fetch( } if let Some(ref expr) = eval { - let result = page.evaluate(expr); + // Bound the eval by the same budget as navigation so a runaway + // expression (infinite loop, never-settling sync work) cannot hang. + let result = page.evaluate_with_timeout(expr, Duration::from_secs(timeout_secs)); let rendered = match result { serde_json::Value::String(s) => s, serde_json::Value::Null => "null".to_string(), diff --git a/crates/obscura-dom/src/tree.rs b/crates/obscura-dom/src/tree.rs index 3f2d22e..4f3f912 100644 --- a/crates/obscura-dom/src/tree.rs +++ b/crates/obscura-dom/src/tree.rs @@ -234,6 +234,40 @@ impl DomTree { if parent_id == child_id { return; } + // Appending an ancestor of the parent under that parent makes the + // parent/child graph cyclic, and every later descendants()/children()/ + // textContent walk (none carry a visited set) would loop forever, pinning + // the thread in native Rust where neither tokio nor the V8 watchdog can + // interrupt it. Per the DOM spec this is a HierarchyRequestError; treat it + // as a no-op, like the self-append guard above. Only a node that already + // has children can be an ancestor, so a fresh/leaf child (the common + // append) skips the walk: O(1) hot path, O(depth) only when relocating a + // populated subtree. + { + let inner = self.inner.borrow(); + let child_has_children = inner.nodes.get(child_id.index()) + .and_then(|n| n.as_ref()) + .map(|n| n.first_child.is_some()) + .unwrap_or(false); + if child_has_children { + let mut cur = inner.nodes.get(parent_id.index()) + .and_then(|n| n.as_ref()) + .and_then(|n| n.parent); + let mut steps = 0usize; + while let Some(p) = cur { + if p == child_id { + return; + } + steps += 1; + if steps > inner.nodes.len() { + return; // pre-existing corruption: refuse rather than risk a cycle + } + cur = inner.nodes.get(p.index()) + .and_then(|n| n.as_ref()) + .and_then(|n| n.parent); + } + } + } self.detach(child_id); let mut inner = self.inner.borrow_mut(); @@ -284,6 +318,35 @@ impl DomTree { } }; + // Inserting the parent itself, or any ancestor of the parent, as a child + // of that parent would create a cycle (same non-terminating-walk hang as + // append_child). Reject it, matching the self-insert guard above. Gate on + // the inserted node actually having children, so the common case (insert a + // fresh node) stays O(1). + { + let inner = self.inner.borrow(); + let new_has_children = inner.nodes.get(new_sibling_id.index()) + .and_then(|n| n.as_ref()) + .map(|n| n.first_child.is_some()) + .unwrap_or(false); + if new_has_children { + let mut cur = Some(parent_id); + let mut steps = 0usize; + while let Some(p) = cur { + if p == new_sibling_id { + return; + } + steps += 1; + if steps > inner.nodes.len() { + return; + } + cur = inner.nodes.get(p.index()) + .and_then(|n| n.as_ref()) + .and_then(|n| n.parent); + } + } + } + self.detach(new_sibling_id); let mut inner = self.inner.borrow_mut(); @@ -443,6 +506,14 @@ impl DomTree { while let Some(current) = stack.pop() { result.push(current); + // Defense in depth: a well-formed subtree has at most nodes.len() + // descendants. Exceeding that means the parent/child graph is cyclic + // (which the append_child / insert_before guards prevent); stop rather + // than grow the stack and result forever and wedge the engine. On a + // valid tree this bound is never reached, so the hot path is unchanged. + if result.len() > inner.nodes.len() { + break; + } let mut child = inner.nodes.get(current.index()) .and_then(|n| n.as_ref()) @@ -515,18 +586,22 @@ impl DomTree { }; if last_child_is_text { + // Re-read last_child without unwrap: if it vanished between the two + // borrows, fall through to appending a fresh text node rather than + // panicking (a panic here aborts the whole engine via V8_Fatal). let last_child_id = { let inner = self.inner.borrow(); inner.nodes.get(parent_id.index()) .and_then(|n| n.as_ref()) .and_then(|n| n.last_child) - .unwrap() }; - let mut inner = self.inner.borrow_mut(); - if let Some(Some(node)) = inner.nodes.get_mut(last_child_id.index()) { - if let NodeData::Text { contents } = &mut node.data { - contents.push_str(text); - return; + if let Some(last_child_id) = last_child_id { + let mut inner = self.inner.borrow_mut(); + if let Some(Some(node)) = inner.nodes.get_mut(last_child_id.index()) { + if let NodeData::Text { contents } = &mut node.data { + contents.push_str(text); + return; + } } } } @@ -737,6 +812,47 @@ mod tests { assert_eq!(tree.get_element_by_id("nonexistent"), None); } + #[test] + fn test_reparent_cycle_is_rejected() { + // document -> html -> body -> div. Moving an ancestor under one of its + // own descendants would make the parent/child graph cyclic and hang + // every later descendants() walk. Both append_child and insert_before + // must reject it as a no-op (DOM HierarchyRequestError). + let tree = DomTree::new(); + let doc = tree.document(); + let mk = |n: &str| { + tree.new_node(NodeData::Element { + name: QualName::new(None, ns!(html), LocalName::from(n)), + attrs: vec![], + template_contents: None, + mathml_annotation_xml_integration_point: false, + }) + }; + let html = mk("html"); + let body = mk("body"); + let div = mk("div"); + tree.append_child(doc, html); + tree.append_child(html, body); + tree.append_child(body, div); + + let before = tree.descendants(doc).len(); + assert_eq!(before, 3); + + // append_child: html is an ancestor of div -> must be a no-op, no cycle. + tree.append_child(div, html); + assert_eq!(tree.descendants(doc).len(), before, "cyclic append must be a no-op"); + assert_eq!(tree.descendants(div).len(), 0, "div must stay a leaf"); + + // insert_before: html is an ancestor of body (div's parent) -> no-op. + tree.insert_before(div, html); + assert_eq!(tree.descendants(doc).len(), before, "cyclic insert_before must be a no-op"); + + // self-append / self-insert remain no-ops (existing guards). + tree.append_child(div, div); + tree.insert_before(div, div); + assert_eq!(tree.descendants(doc).len(), before); + } + #[test] fn test_append_text_merges() { let tree = DomTree::new(); diff --git a/crates/obscura-js/src/ops.rs b/crates/obscura-js/src/ops.rs index 53a97f0..66dfa40 100644 --- a/crates/obscura-js/src/ops.rs +++ b/crates/obscura-js/src/ops.rs @@ -83,6 +83,22 @@ pub type SharedState = Rc>; #[op2] #[string] fn op_dom(state: &OpState, #[string] cmd: String, #[string] arg1: String, #[string] arg2: String) -> String { + // Anti-panic boundary: a panic in a DOM op would unwind through deno_core + // into V8's FFI frame, where V8_Fatal calls abort(3) and takes the whole + // engine (and every CDP client) down. Catch it so one malformed selector or + // inconsistent tree node degrades to a null result for that single call. + // No per-call clone: on the happy path this is just a landing pad, so the + // hot DOM path (querySelector/getAttribute/...) pays nothing measurable. + std::panic::catch_unwind(std::panic::AssertUnwindSafe(move || { + op_dom_inner(state, cmd, arg1, arg2) + })) + .unwrap_or_else(|_| { + tracing::error!("op_dom panicked; returning null"); + "null".to_string() + }) +} + +fn op_dom_inner(state: &OpState, cmd: String, arg1: String, arg2: String) -> String { let gs = state.borrow::().clone(); let gs = gs.borrow(); let dom = match &gs.dom { diff --git a/crates/obscura-js/src/runtime.rs b/crates/obscura-js/src/runtime.rs index efec5d3..32dff83 100644 --- a/crates/obscura-js/src/runtime.rs +++ b/crates/obscura-js/src/runtime.rs @@ -27,6 +27,15 @@ pub struct ObscuraJsRuntime { object_counter: u64, } +/// Handle to an armed V8 execution watchdog (see [`ObscuraJsRuntime::arm_watchdog`]). +/// Holds the cancel channel and the watchdog thread; pass it back to +/// `disarm_watchdog` to stop the watchdog and learn whether it fired. +pub struct WatchdogToken { + pair: std::sync::Arc<(std::sync::Mutex, std::sync::Condvar)>, + join: Option>, + fired: std::sync::Arc, +} + impl ObscuraJsRuntime { pub fn new() -> Self { Self::with_base_url("about:blank") @@ -671,6 +680,109 @@ impl ObscuraJsRuntime { .map_err(|e| format!("Event loop error: {}", e)) } + /// Arm a hard wall-clock backstop on synchronous V8 work. A page stuck in a + /// synchronous loop or a microtask storm pins the OS thread inside V8, so + /// `tokio::time::timeout` (which can only cancel at await points) never + /// fires. This spawns a watchdog thread that terminates the isolate once + /// `budget` elapses, forcing V8 to throw an uncatchable error and hand + /// control back. Always balance with [`Self::disarm_watchdog`]. + pub fn arm_watchdog(&mut self, budget: std::time::Duration) -> WatchdogToken { + let handle = self.runtime.v8_isolate().thread_safe_handle(); + let pair = std::sync::Arc::new((std::sync::Mutex::new(false), std::sync::Condvar::new())); + let fired = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + let pair_c = pair.clone(); + let fired_c = fired.clone(); + let join = std::thread::spawn(move || { + let (lock, cvar) = &*pair_c; + let mut cancelled = lock.lock().unwrap(); + let deadline = std::time::Instant::now() + budget; + loop { + let remaining = deadline.saturating_duration_since(std::time::Instant::now()); + if remaining.is_zero() { + fired_c.store(true, std::sync::atomic::Ordering::SeqCst); + handle.terminate_execution(); + return; + } + let (guard, _) = cvar.wait_timeout(cancelled, remaining).unwrap(); + cancelled = guard; + if *cancelled { + return; + } + } + }); + WatchdogToken { pair, join: Some(join), fired } + } + + /// Stop a watchdog armed by [`Self::arm_watchdog`]. If it had already fired + /// (terminated the isolate), clear V8's termination flag so the isolate is + /// usable again, and return `true`. + pub fn disarm_watchdog(&mut self, mut token: WatchdogToken) -> bool { + { + let (lock, cvar) = &*token.pair; + *lock.lock().unwrap() = true; + cvar.notify_one(); + } + if let Some(j) = token.join.take() { + let _ = j.join(); + } + let fired = token.fired.load(std::sync::atomic::Ordering::SeqCst); + if fired { + self.runtime.v8_isolate().cancel_terminate_execution(); + tracing::warn!("V8 watchdog fired: terminated a synchronous overrun"); + } + fired + } + + /// Drive the event loop for at most `budget_ms`, bounded against BOTH async + /// idle (tokio timeout) and synchronous hangs (V8 watchdog). A microtask + /// storm that pins the thread is terminated ~500ms past the budget; a + /// well-behaved page returns as soon as the loop goes idle. + pub async fn run_event_loop_bounded(&mut self, budget_ms: u64) -> Result<(), String> { + if budget_ms == 0 { + return self.run_event_loop().await; + } + let budget = std::time::Duration::from_millis(budget_ms); + let token = self.arm_watchdog(budget + std::time::Duration::from_millis(500)); + let result = tokio::time::timeout(budget, self.run_event_loop()).await; + self.disarm_watchdog(token); + match result { + Ok(Ok(())) => Ok(()), + Ok(Err(e)) if e.contains("execution terminated") => Ok(()), + Ok(Err(e)) => Err(e), + // tokio idle-timeout is the normal "settled" exit, not an error. + Err(_) => Ok(()), + } + } + + /// Like [`Self::evaluate`] but bounded by a V8 watchdog, so a `--eval` + /// expression that loops forever (or awaits a promise that never settles in + /// synchronous form) cannot hang the process. + pub fn evaluate_with_timeout( + &mut self, + expression: &str, + timeout: std::time::Duration, + ) -> Result { + if timeout.is_zero() { + return self.evaluate(expression); + } + let wrapped = Self::wrap_expression(expression); + let token = self.arm_watchdog(timeout); + let result = self.runtime.execute_script("", wrapped); + let fired = self.disarm_watchdog(token); + match result { + Ok(v) if !fired => self.v8_to_json(v), + Ok(_) => Err("eval timed out".to_string()), + Err(e) => { + let msg = e.to_string(); + if fired || msg.contains("execution terminated") { + Err("eval timed out".to_string()) + } else { + Err(format!("JS error: {}", msg)) + } + } + } + } + pub async fn resolve_promises(&mut self) { // Default settle: just pump until idle or 5s. let _ = tokio::time::timeout(