From 32ddf93885c18175cbd3d54183aaf6869b4452eb Mon Sep 17 00:00:00 2001 From: SGavrl <277843151+SGavrl@users.noreply.github.com> Date: Fri, 5 Jun 2026 21:07:08 +0200 Subject: [PATCH] Fix/engine hardening (#249) * fix(dom): read insert_before prev_sibling after detach to avoid a sibling cycle insert_before captured the reference node's prev_sibling BEFORE detaching the node being inserted. When page JS inserts a node before its own immediate previous sibling (a no-op reorder frameworks do constantly), detach moves that pointer, so the stale prev_id spliced new.next_sibling = new. The resulting sibling self-cycle made descendants() inner next_sibling walk loop forever in native Rust, uninterruptible by tokio or the V8 watchdog. This hung ebay.com, found by a 1500-page crash/hang sweep. Read prev_sibling after detach so it reflects the post-detach tree. Cap the inner sibling-collection loops in descendants() and log when a descendants cap fires, as defense in depth so a sibling cycle from any path cannot hang the engine. Add test_insert_before_previous_sibling_no_cycle. ebay.com now renders (192K chars) instead of hanging. Verified: 33/33 obscura-dom tests, obstacle course 33/33. * fix(cli): settle async --eval work before reading the page (#248) --eval ran after the selector check and returned its value immediately, so an expression that only kicks off async work (e.g. fetch().then(t => DOM write)) returned the still-pending Promise as {} and any --selector/--dump were ignored. When --eval is combined with --selector and/or --dump, run the eval to kick off its async work, settle the event loop again so it completes, then read the page (selector wait + dump) instead of returning the Promise. A bare --eval still returns its value unchanged, so synchronous expressions are not affected, and the eval stays bounded by evaluate_with_timeout. dump becomes Option so an explicit --dump is distinguishable from the default. Verified: the repro now returns the fetched content, a plain JSON.stringify eval is unchanged, 37 + 16 cli tests pass. --- crates/obscura-cli/src/main.rs | 63 +++++++++++++++++++++----------- crates/obscura-dom/src/tree.rs | 65 ++++++++++++++++++++++++++++++---- 2 files changed, 101 insertions(+), 27 deletions(-) diff --git a/crates/obscura-cli/src/main.rs b/crates/obscura-cli/src/main.rs index 7163113..c57bc11 100644 --- a/crates/obscura-cli/src/main.rs +++ b/crates/obscura-cli/src/main.rs @@ -89,8 +89,12 @@ enum Command { Fetch { url: String, - #[arg(long, default_value = "html")] - dump: DumpFormat, + // Default is html. Kept as Option so we can tell whether --dump was + // explicitly passed: a bare --eval returns its own value, while --eval + // combined with --dump (or --selector) runs the eval, lets its async + // work settle, then reads the page (issue #248). + #[arg(long)] + dump: Option, #[arg(long)] selector: Option, @@ -463,7 +467,7 @@ async fn run_multi_worker_serve( async fn run_fetch( url_str: &str, - dump: DumpFormat, + dump: Option, selector: Option, wait_secs: u64, timeout_secs: u64, @@ -477,6 +481,12 @@ async fn run_fetch( storage_dir: Option, allow_private_network: bool, ) -> anyhow::Result<()> { + // Whether the user explicitly passed --dump. With --eval also present this + // decides whether we return the eval value or read the page after the + // eval's async work settles (issue #248). + let dump_specified = dump.is_some(); + let dump = dump.unwrap_or(DumpFormat::Html); + // --dump original short-circuits the browser stack entirely: fetch the raw // response body via HTTP and stream the bytes verbatim. Useful for binary // payloads (images, fonts, …) and any non-HTML resource where parsing the @@ -548,6 +558,33 @@ async fn run_fetch( // pages stay fast. page.settle(wait_secs.saturating_mul(1000)).await; + if let Some(ref expr) = eval { + // 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)); + + // A bare --eval (no --selector, no --dump) returns the eval value + // directly, so synchronous expressions (JSON.stringify, ...) are + // unchanged. + if !dump_specified && selector.is_none() { + let rendered = match result { + serde_json::Value::String(s) => s, + serde_json::Value::Null => "null".to_string(), + other => other.to_string(), + }; + write_or_print(rendered, output.as_ref()).await?; + context.save_cookies(); + return Ok(()); + } + + // --eval combined with --selector and/or --dump: the eval typically + // kicks off async work (a fetch promise, a timer) that writes the DOM. + // Drive the event loop again so that work completes, then fall through + // to the selector wait and the dump path instead of returning the + // still-pending eval value (issue #248). + page.settle(wait_secs.saturating_mul(1000)).await; + } + if let Some(ref sel) = selector { let found = wait_for_selector(&mut page, sel, wait_secs).await; if !found { @@ -555,20 +592,6 @@ async fn run_fetch( } } - if let Some(ref expr) = eval { - // 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(), - other => other.to_string(), - }; - write_or_print(rendered, output.as_ref()).await?; - context.save_cookies(); - return Ok(()); - } - let rendered = match dump { DumpFormat::Html => dump_html(&page), DumpFormat::Text => dump_text(&mut page), @@ -1143,7 +1166,7 @@ mod tests { .expect("clap should accept --dump original"); match args.command { Some(Command::Fetch { dump, .. }) => { - assert_eq!(dump, DumpFormat::Original); + assert_eq!(dump, Some(DumpFormat::Original)); } _ => panic!("expected Fetch command"), } @@ -1406,7 +1429,7 @@ mod tests { fn matcher_still_uses_fetch_variant() { let cmd = Some(Command::Fetch { url: "https://x".to_string(), - dump: super::DumpFormat::Html, + dump: Some(super::DumpFormat::Html), selector: None, wait: 5, timeout: 30, @@ -1495,7 +1518,7 @@ mod tests { .expect("clap should accept --dump assets"); match args.command { Some(Command::Fetch { dump, .. }) => { - assert_eq!(dump, DumpFormat::Assets); + assert_eq!(dump, Some(DumpFormat::Assets)); } _ => panic!("expected Fetch command"), } diff --git a/crates/obscura-dom/src/tree.rs b/crates/obscura-dom/src/tree.rs index 4f3f912..1b2e471 100644 --- a/crates/obscura-dom/src/tree.rs +++ b/crates/obscura-dom/src/tree.rs @@ -306,14 +306,10 @@ impl DomTree { if existing_id == new_sibling_id { return; } - let (parent_id, prev_id) = { + let parent_id = { let inner = self.inner.borrow(); - let node = match inner.nodes.get(existing_id.index()).and_then(|n| n.as_ref()) { - Some(n) => n, - None => return, - }; - match node.parent { - Some(p) => (p, node.prev_sibling), + match inner.nodes.get(existing_id.index()).and_then(|n| n.as_ref()).and_then(|n| n.parent) { + Some(p) => p, None => return, } }; @@ -349,6 +345,17 @@ impl DomTree { self.detach(new_sibling_id); + // Read existing's prev AFTER detaching new. If new was existing's + // immediate previous sibling, detach moved that pointer; using the + // pre-detach value would splice new.next_sibling = new (a self-cycle) + // and hang every later sibling walk. This is what hung ebay.com. + let prev_id = { + let inner = self.inner.borrow(); + inner.nodes.get(existing_id.index()) + .and_then(|n| n.as_ref()) + .and_then(|n| n.prev_sibling) + }; + let mut inner = self.inner.borrow_mut(); if let Some(Some(node)) = inner.nodes.get_mut(new_sibling_id.index()) { @@ -496,6 +503,10 @@ impl DomTree { let mut children_to_push = Vec::new(); while let Some(child_id) = first { children_to_push.push(child_id); + if children_to_push.len() > inner.nodes.len() { + eprintln!("obscura: sibling-chain cap hit at node {} - cycle", node_id.index()); + break; + } first = inner.nodes.get(child_id.index()) .and_then(|n| n.as_ref()) .and_then(|n| n.next_sibling); @@ -512,6 +523,11 @@ impl DomTree { // 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() { + eprintln!( + "obscura: descendants() cap hit at node {} ({} nodes) - tree has a cycle", + node_id.index(), + inner.nodes.len() + ); break; } @@ -521,6 +537,10 @@ impl DomTree { let mut children_to_push = Vec::new(); while let Some(child_id) = child { children_to_push.push(child_id); + if children_to_push.len() > inner.nodes.len() { + eprintln!("obscura: sibling-chain cap hit at node {} - cycle", current.index()); + break; + } child = inner.nodes.get(child_id.index()) .and_then(|n| n.as_ref()) .and_then(|n| n.next_sibling); @@ -853,6 +873,37 @@ mod tests { assert_eq!(tree.descendants(doc).len(), before); } + #[test] + fn test_insert_before_previous_sibling_no_cycle() { + // Inserting a node before its own immediate previous sibling is a no-op + // reorder that frameworks do constantly. It used to splice + // next_sibling = self via a prev_id captured before detach, hanging every + // later sibling walk (this hung ebay.com). The result must stay a + // well-formed [a, b] with no cycle. + 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 parent = mk("div"); + let a = mk("a"); + let b = mk("b"); + tree.append_child(doc, parent); + tree.append_child(parent, a); + tree.append_child(parent, b); // parent -> [a, b] + + // a is already b's previous sibling; this reorder must not create a cycle. + tree.insert_before(b, a); + + let kids = tree.descendants(parent); + assert_eq!(kids, vec![a, b], "order preserved, no cycle"); + } + #[test] fn test_append_text_merges() { let tree = DomTree::new();