From 1e9e9478fbb4c1c0f1a51dc1533282a1adf03235 Mon Sep 17 00:00:00 2001 From: SGavrl <277843151+SGavrl@users.noreply.github.com> Date: Fri, 5 Jun 2026 07:04:09 +0000 Subject: [PATCH] Fix/v8 watchdog panic safety (#239) * dom: complete detached documents; add ProgressEvent and parseHTMLUnsafe createHTMLDocument returned a document whose body was null: parsing the skeleton string as innerHTML of collapsed through the fragment parser and kept only . Build head>title and body explicitly instead. The DOMParser doc node also gains appendChild/removeChild/insertBefore and a doctype getter, and createDocument carries its doctype argument. This lets dom/common.js setupRangeTests() finish (it builds fixtures with new Document(), createHTMLDocument(), createDocument()) instead of aborting at harness setup, so the dom/ranges files run. Range-collapse 186/186, Range-attributes 1/1, Range-cloneRange 55/62, StaticRange-constructor 14/17. The large O(ranges x points) matrix files (Range-set, comparePoint, compareBoundaryPoints) still time out: the boundary-point comparison walks the tree through per-step DOM ops, which is too slow for those matrices. Speeding that up (a Rust node-position op) is a separate change. Also add globalThis.ProgressEvent (Event subclass with lengthComputable/ loaded/total) and the static Document.parseHTMLUnsafe(html). * perf(dom): compute Range tree-order in Rust ops The Range boundary-point algorithms compared nodes by walking ancestor chains and sibling lists from JS, one DOM op per step. The dom/ranges matrices (testRanges x testPoints, thousands of comparisons) made that time out. Add node_index, compare_order, and node_root ops that do the walk in Rust over the DOM tree (slab lookups, no per-step FFI), and use them in _rngNodeIndex, _rngOrder, and _rngRoot. Range-comparePoint 3534/5580, Range-isPointInRange 5197/5733, Range-compareBoundaryPoints 6441/9313, all completing in under 10s where they previously did not finish. Not on any engine hot path (querySelector, mutation, navigation, fetch do not use these). * fileapi: real byte-backed Blob/File, FileReader state machine, Request.blob Blob was a one-line stub that string-joined its parts (wrong size for binary or multibyte input) and exposed only text(). Replace it with a Uint8Array-backed Blob: normalizes string/ArrayBuffer/typed-array/Blob parts, correct size, slice(), text()/arrayBuffer()/bytes(), type normalization (lowercased, cleared when non-ASCII), the endings option (native CRLF normalization), and Symbol.toStringTag. File adds name/lastModified, the 2-argument requirement, and its own toStringTag. FileReader becomes a real state machine: EMPTY/LOADING/DONE, throws InvalidStateError when a read starts while already LOADING, and fires loadstart/progress/load/loadend (and abort) to both on* handlers and addEventListener. readAsText/DataURL/ArrayBuffer/BinaryString produce correct results. Request gains blob(). FileReader-multiple-reads 6/6, File-constructor-endings 10/11, Blob-slice 141/150. * encoding: TextDecoder supports legacy charsets via encoding_rs TextDecoder was a UTF-8-only JS shim that ignored its label, so new TextDecoder('shift_jis'|'euc-jp'|'big5'|'gbk'|'windows-1252'|'iso-8859-1') .decode() returned garbage. That matters for scraping: pages and scripts decode legacy-encoded bytes (XHR/fetch responses, File/Blob text) all the time. Back it with encoding_rs (already used by the net layer for response bodies): obscura-net gains label_name() and decode_with_label(), exposed to JS as op_encoding_for_label and op_text_decode. The constructor validates the label (RangeError) and sets the canonical .encoding/.fatal/.ignoreBOM; decode() supports fatal mode (TypeError on invalid input) and BOM handling. Plain UTF-8 non-fatal decoding keeps a pure-JS fast path (no op, no JSON), so Response.text()/Blob.text() on large bodies are unchanged; the op only runs for legacy encodings or fatal mode. textdecoder-labels 222/222, textdecoder-fatal 36/36, byte-order-marks 3/3, utf16-surrogates 10/10, single-byte-decoder 168/504. * selection: real Selection over Range (single instance per document) getSelection() returned a fresh plain object each call, so it was not a Selection instance, had no identity (window.getSelection() !== document.getSelection()), and was missing removeRange/empty/collapseToStart/collapseToEnd/setPosition/ setBaseAndExtent/modify while collapse/extend/getRangeAt were broken stubs. That was ~4862 failing 'selection[method] is not a function' subtests. Replace it with a real Selection class built on the live Range: one cached instance per document (window.getSelection() === document.getSelection(), null for documents with no defaultView), an internal range + direction, derived anchor/focus/isCollapsed/type/rangeCount, and the full method set (getRangeAt with IndexSizeError, addRange, removeRange, removeAllRanges/empty, collapse/setPosition, collapseToStart/End, extend, setBaseAndExtent, selectAllChildren, containsNode, toString) with the spec DOMException names. modify() stays a no-op (needs visual layout). No ops.rs changes; not on any hot path. setBaseAndExtent 120/120, removeAllRanges 104/104, collapseToStartEnd 51/51, type 26/26, extend-exception 1/1. * forms: HTMLInputElement valueAsNumber/valueAsDate/stepUp/stepDown These were absent. Implement the number/date conversion API for the applicable types (number, range, date, month, week, time, datetime-local): parsers and formatters per type (Date.UTC for date/datetime-local, ISO-week Monday math for week, months-since-epoch for month), valueAsNumber get/set, valueAsDate get/set, and stepUp/stepDown(n) with the step scale/default per type, step-base snapping, and min/max clamping. Non-applicable types and step="any" throw InvalidStateError; non-finite valueAsNumber throws TypeError; range with empty value reads as the midpoint (50 for 0..100). Plus min/max/step attribute reflection. All on Element accessors gated by input type, only evaluated on explicit access; not on any hot path. input-stepup 51/53, input-valueasnumber 60/64, input-valueasdate 27/30. * perf(encoding): skip label op for no-arg TextDecoder (Response/Blob text path) new TextDecoder() with no argument (what Response.text()/Blob.text() and most pages use) is UTF-8; resolve that without the op_encoding_for_label round trip. Explicit labels still validate via the op. * core: HTMLDialogElement, Node.normalize, Range.createContextualFragment HTMLDialogElement was a bare alias of Element, so ~40 Core files crashed at the first dialog.show()/showModal(). Add the dialog members on Element.prototype (matching the popover/input convention): open reflection, returnValue (internal slot, String coercion), oncancel/onclose defaulting to null, show()/showModal() /close()/requestClose() with the open-state transitions, InvalidStateError throwing (show-on-modal, showModal-on-open, showModal-disconnected), the async non-bubbling close event, the synchronous cancelable cancel event (no re-entrant cancel), beforetoggle/toggle, and closedBy. matches(':modal') reports a modally shown dialog (like :popover-open). Modal top-layer/focus/render stays unhandled (layout). Node.normalize() now merges adjacent Text nodes and drops empty ones (was a no-op). Range.createContextualFragment(html) parses a fragment. createContextualFragment 34/35, dialog-open 3/3, dialog-return-value 1/1, dialog-showModal 5/10, dialog-requestclose 22/75. Not on hot paths. * custom-elements: CustomElementRegistry class, getName, ElementInternals, HTMLUnknownElement customElements was a bare object literal, so 'CustomElementRegistry is not defined' crashed ~22 Core files at top level (idlharness/feature-detect), and HTMLUnknownElement/ElementInternals/attachInternals/getName were missing. Promote the registry to a real CustomElementRegistry class (window.customElements is now an instance, so 'define in CustomElementRegistry.prototype' works and the class is exposed). Add spec define() validation/throw order (TypeError for a non-constructor, SyntaxError for an invalid name, NotSupportedError for a duplicate name/constructor or reentrant define), getName(), and a SyntaxError reject in whenDefined for invalid names. The existing define -> upgrade -> connectedCallback path (used by lit/Polymer on real pages) is preserved. Add HTMLUnknownElement, ElementInternals (setFormValue/setValidity/checkValidity/ validity/validationMessage/form/states), and Element.attachInternals() with its NotSupportedError cases. Does NOT include the [[HTMLConstructor]] (new.target) construction rework or the reaction wiring into appendChild/setAttribute (hot paths) - deferred to avoid element-model and perf risk. getName 4/4. * shadow-dom: ShadowRoot.adoptedStyleSheets (mirror Document) ShadowRoot lacked adoptedStyleSheets; constructible-stylesheet adoption into a shadow root threw. Add the getter/setter on ShadowRoot.prototype, matching Document.adoptedStyleSheets. * conformance: charset/URL encoding override, IDL reflection, Range and Storage fixes Encoding: track the detected document character encoding and expose it as document.characterSet/charset/inputEncoding instead of hardcoding UTF-8. Add the WHATWG URL encoding override for <a>/<area> hrefs, so a legacy-charset document percent-encodes the query in its own encoding (EUC-JP U+8108 becomes %CC%AE). ASCII delimiters stay literal so structured queries are unaffected, and unmappable code points become %26%23NNN%3B. UTF-8 documents take an early-return so the href getter hot path is unchanged. fetch()/XHR text() now decode using the response Content-Type charset instead of always UTF-8. Adds op_url_encode_query and a document_encoding op; encoding state is plumbed through Page and the runtime. Reflection: install the reflected IDL attribute layer once on Element.prototype (no per-element defineProperty, so element creation and query paths are untouched): global attributes (title, lang, dir, accessKey, slot, autofocus, hidden, tabIndex) with correct type coercion and enum canonicalization, plus the ARIAMixin (aria-* reflected as the ariaXxx nullable DOMString properties). Range: compareBoundaryPoints ToUint16-converts the how argument (NaN/Infinity no longer false-throw) and raises a real WrongDocumentError DOMException for ranges that do not share a root. Storage: reimplement localStorage/sessionStorage as a WHATWG Proxy so property access (localStorage.foo = x, delete, in, Object.keys) stays in sync with length, key() and iteration. * conformance: HTML attribute folding, compareDocumentPosition, URLSearchParams, DOMParser, fetch UA Next WPT Core batch, all verified live and benchmarked for no hot-path regression. DOM: - compareDocumentPosition rewritten to spec: real PRECEDING/FOLLOWING via the compare_order op, CONTAINS/CONTAINED_BY, and DISCONNECTED|IMPLEMENTATION_SPECIFIC for different roots. Adds the DOCUMENT_POSITION_* and missing nodeType constants on Node and its prototype. WPT getPosition() depends on this, so it cascades into the Range comparePoint/compareBoundaryPoints/isPointInRange expectations. - HTML elements now ASCII-lowercase attribute names in getAttribute/setAttribute/ removeAttribute/hasAttribute, so el.setAttribute('accessKey') reflects via getAttribute('accesskey'). The fold is gated behind a cheap uppercase scan and a namespaceURI HTML guard (SVG/foreign attributes keep their case), and getAttribute only folds on a miss, so the all-lowercase hot path is unchanged (benchmarked flat). - Range comparePoint/isPointInRange ToUint32-coerce the offset (negative throws IndexSizeError); createTreeWalker/createNodeIterator keep an explicit whatToShow of 0. URL: - URLSearchParams uses a proper form-urlencoded byte decoder (partial/invalid escapes stay literal instead of decodeURIComponent throwing on the whole string), 2-arg has(name,value)/delete(name,value), and a sequence-pair TypeError. Adds URL.parse and URL.canParse statics. Other: - fetch()/XHR send a default Chrome User-Agent (the navigation path set one but scripted requests went out with none, so UA-gated servers rejected them); honors an explicit override. - DOMParser documents expose URL/compatMode/characterSet/readyState/styleSheets/ defaultView and createTreeWalker/createNodeIterator; XMLDocument is defined globally. * perf(dom): O(1) querySelector('#id') via the id index querySelector('#id') went through the full selector engine, scanning descendants in tree order until the first match (O(n), ~68us per call over a 2000-element document in the micro-bench). A bare ASCII id selector now resolves through the existing id index in O(1): ~68us -> ~1.25us per call, about 55x faster, with no change to any other path (create/getAttribute/setAttribute/querySelectorAll/href all flat). querySelector by id is one of the hottest scraping operations. The fast path is conservative: only a bare `#name` selector with an ASCII letter/underscore-led identifier takes it; escapes, combinators, commas, whitespace, compounds, and non-ASCII fall through to the full engine unchanged. For a scoped element.querySelector it returns the indexed element only when it is a strict descendant of the scope, and falls back to the scan otherwise, so duplicate-id scoped queries stay correct. Also makes the id index keep the FIRST element in tree order per id (or_insert at parse time) instead of the last, so getElementById and querySelector('#id') return the first-in-tree-order element on duplicate ids, per spec (previously last). * fix: bound synchronous V8 hangs with a termination watchdog and add op panic safety Heavy pages could hang obscura fetch indefinitely: tokio::time::timeout only cancels at await points, so synchronous V8 work (a page's script bootstrap, a microtask storm) pinned the thread and ran unbounded. Add a reusable V8 terminate_execution watchdog (arm_watchdog/disarm_watchdog) and apply it to every place that runs synchronous V8 work: the post-load settle, both navigation event-loop pumps, the whole script-execution phase, and --eval. A CLI process-level hard deadline backstops the rest, since a hang inside a Rust op cannot be cancelled by tokio or terminate_execution. Also harden against panics: op_dom is wrapped in catch_unwind so a DOM-op panic degrades to a null result instead of unwinding into V8's FFI frame and aborting the process; the append_text unwrap is removed; panic = unwind is pinned in the release profile. Verified: obstacle course 33/33, no perf regression (per-op deltas within the measurement noise floor), 6 of 8 previously-hanging real sites now render and the other 2 exit deterministically at the hard deadline. * fix(dom): reject cyclic reparenting so descendants() cannot hang the engine appendChild/insertBefore only guarded the self-append case (parent == child), never the case where the moved node is an ancestor of the new parent. When page JS does node.appendChild(ancestorOfNode) the parent/child graph becomes cyclic; the next querySelector runs descendants() (a DFS with no visited set) and loops forever, growing a Vec unbounded and pinning the thread in native Rust where neither tokio nor the V8 watchdog can interrupt it. This hung techcrunch and kayak. Reject the cycle at the mutation site per the DOM HierarchyRequestError rule: if the moved node is an inclusive ancestor of its target parent, treat the move as a no-op. The walk is gated on the moved node having children, so a fresh/leaf append (the common case) stays O(1); only relocating a populated subtree pays O(depth). descendants() also gets a result-length cap as defense in depth so a cycle from any future path cannot wedge the engine. techcrunch and kayak now render (37K and 1.26M chars). Verified: new test_reparent_cycle_is_rejected, obstacle course 33/33, no perf regression. --- Cargo.toml | 7 ++ crates/obscura-browser/src/page.rs | 64 +++++++++++++-- crates/obscura-cli/src/main.rs | 20 ++++- crates/obscura-dom/src/tree.rs | 128 +++++++++++++++++++++++++++-- crates/obscura-js/src/ops.rs | 16 ++++ crates/obscura-js/src/runtime.rs | 112 +++++++++++++++++++++++++ 6 files changed, 335 insertions(+), 12 deletions(-) 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<String>, @@ -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<tokio::time::Instant> = 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<RefCell<ObscuraState>>; #[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::<SharedState>().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<bool>, std::sync::Condvar)>, + join: Option<std::thread::JoinHandle<()>>, + fired: std::sync::Arc<std::sync::atomic::AtomicBool>, +} + 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<serde_json::Value, String> { + 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("<eval>", 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(