Skip to content

Commit f1be028

Browse files
committed
fix(browser,js): enforce navigation deadline through synchronous V8 execution
`tokio::time::timeout` only fires at `.await` points. Any synchronous V8 work — script evaluation, module top-level code — holds the tokio executor thread for its entire duration, making the outer async timeout invisible to scripts that never yield. A page with heavy synchronous scripts could run arbitrarily past `--timeout` with no way to interrupt it. Fix: add `navigation_deadline: Option<Instant>` to `Page` and thread it through every execution phase. **Script phase** — each `execute_script_with_timeout` call receives the remaining budget as its hard ceiling. When the budget expires a watchdog thread fires `terminate_execution()` in a tight loop (every 10 ms) so that scripts with `try-catch` error-recovery handlers are still eventually terminated rather than absorbing a single termination call and continuing. Scripts are also skipped entirely once the deadline has passed, cutting the iteration short rather than starting work we know will be cancelled. **Network fetch phase** — each parallel script fetch is wrapped in `tokio::time::timeout(remaining_budget, ...)` so a slow CDN response cannot by itself exhaust the navigation deadline; fetch failures are treated as absent scripts rather than errors. **ES module phase** — V8's `terminate_execution` is catchable by JavaScript `try-catch`, and heavy modules with error-recovery paths run *longer* when disturbed than when left to complete naturally. A threshold guard skips any module when the remaining budget is below 15 s; the module would outlast the deadline regardless, so it is better to skip it cleanly than to start work that cannot be reliably stopped. **Load-events / event-loop drain** — the DOMContentLoaded + load dispatch is capped at the remaining budget (min 50 ms to allow basic event handling). The idle event-loop drain is capped at min(500 ms, remaining) and now checks the deadline on *every* iteration, not only in the timeout branch. **Error surface** — a new `PageError::NavigationTimedOut` variant is returned when `execute_scripts` exits because the deadline was reached, letting the CLI distinguish a timeout from a genuine navigation failure and produce an accurate "Timed out after Ns" message rather than silently returning a partially-rendered page. Also switches `eval_module_with_timeout` from `run_event_loop` to `with_event_loop_promise`: the former waits for *all* pending work in the runtime to drain (blocking forever on a page with a live `setInterval`), while the latter resolves as soon as the module's top-level evaluation completes.
1 parent 88c5006 commit f1be028

6 files changed

Lines changed: 506 additions & 150 deletions

File tree

crates/obscura-browser/src/page.rs

Lines changed: 111 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,12 @@ pub struct Page {
142142
// contract. Includes `Runtime.addBinding` shims so puppeteer's
143143
// `exposeFunction` bindings exist before inline `<script>` tags execute.
144144
preload_scripts: Vec<String>,
145+
/// Hard wall-clock deadline for the *current* navigation. Threaded
146+
/// into the synchronous JS execution path so a single page-supplied
147+
/// script can no longer hold the tokio executor past `--timeout` —
148+
/// `tokio::time::timeout` only fires at await points, and inline
149+
/// `<script>` evaluation has none.
150+
navigation_deadline: Option<std::time::Instant>,
145151
#[cfg(feature = "stealth")]
146152
pub stealth_client: Option<Arc<StealthHttpClient>>,
147153
}
@@ -187,11 +193,29 @@ impl Page {
187193
intercept_block_patterns: Vec::new(),
188194
intercept_tx: None,
189195
preload_scripts: Vec::new(),
196+
navigation_deadline: None,
190197
#[cfg(feature = "stealth")]
191198
stealth_client,
192199
}
193200
}
194201

202+
/// Set a wall-clock deadline that bounds the next navigation. Pass
203+
/// `None` to clear. The deadline is checked between scripts and is
204+
/// also handed to the per-script V8 watchdog so synchronous JS work
205+
/// is terminated when it overruns.
206+
pub fn set_navigation_deadline(&mut self, deadline: Option<std::time::Instant>) {
207+
self.navigation_deadline = deadline;
208+
}
209+
210+
fn navigation_remaining(&self) -> Option<std::time::Duration> {
211+
self.navigation_deadline
212+
.map(|d| d.saturating_duration_since(std::time::Instant::now()))
213+
}
214+
215+
fn navigation_expired(&self) -> bool {
216+
matches!(self.navigation_remaining(), Some(r) if r.is_zero())
217+
}
218+
195219
fn should_block_url(&self, url: &str) -> bool {
196220
if !self.intercept_enabled || self.intercept_block_patterns.is_empty() {
197221
return false;
@@ -387,18 +411,27 @@ impl Page {
387411
}
388412

389413
let client = self.http_client.clone();
414+
let nav_deadline = self.navigation_deadline;
390415
let fetch_futures: Vec<_> = fetch_tasks.iter().map(|(idx, url)| {
391416
let client = client.clone();
392417
let url = url.clone();
393418
let idx = *idx;
394419
async move {
395420
let parsed = Url::parse(&url).unwrap_or_else(|_| Url::parse("about:blank").unwrap());
396-
match client.fetch(&parsed).await {
397-
Ok(resp) => Some((idx, url, resp)),
398-
Err(e) => {
421+
let fetch_budget = nav_deadline
422+
.map(|d| d.saturating_duration_since(std::time::Instant::now()))
423+
.filter(|d| !d.is_zero())
424+
.unwrap_or_else(|| std::time::Duration::from_secs(30));
425+
match tokio::time::timeout(fetch_budget, client.fetch(&parsed)).await {
426+
Ok(Ok(resp)) => Some((idx, url, resp)),
427+
Ok(Err(e)) => {
399428
tracing::warn!("Failed to fetch script {}: {}", url, e);
400429
None
401430
}
431+
Err(_) => {
432+
tracing::debug!("Script fetch deadline exceeded: {}", url);
433+
None
434+
}
402435
}
403436
}
404437
}).collect();
@@ -437,26 +470,58 @@ impl Page {
437470
}
438471

439472
for (i, script) in all_to_execute.iter().enumerate() {
473+
// Hard cut-off when the caller's deadline has elapsed. Without
474+
// this, `--timeout=1` on a script-heavy page would still run
475+
// every remaining `<script>` to completion because the
476+
// synchronous V8 calls below never yield to the tokio
477+
// executor.
478+
if self.navigation_expired() {
479+
break;
480+
}
481+
let script_budget = self
482+
.navigation_remaining()
483+
.unwrap_or_else(|| std::time::Duration::from_secs(5));
484+
440485
if script.src.is_some() {
441486
if let Some((url, code, resp)) = fetched.remove(&i) {
442487
tracing::info!("Executing script ({} bytes): {}", code.len(), url);
443488
self.record_network_event(&url, "GET", "Script", resp.status, &resp.headers, resp.body.len());
444489
if let Some(js) = &mut self.js {
445-
if let Err(e) = js.execute_script_guarded(&url, &code) {
490+
if let Err(e) = js.execute_script_with_timeout(&code, script_budget) {
446491
tracing::warn!("Script error ({}): {}", url, e);
447492
}
448493
}
449494
}
450495
} else if !script.inline.is_empty() {
451496
if let Some(js) = &mut self.js {
452-
if let Err(e) = js.execute_script_guarded("<inline>", &script.inline) {
497+
if let Err(e) = js.execute_script_with_timeout(&script.inline, script_budget) {
453498
tracing::warn!("Inline script error: {}", e);
454499
}
455500
}
456501
}
457502
}
458503

459-
for module_script in &module_scripts {
504+
for module_script in module_scripts.iter() {
505+
if self.navigation_expired() {
506+
break;
507+
}
508+
let module_budget = self
509+
.navigation_remaining()
510+
.unwrap_or_else(|| std::time::Duration::from_secs(10));
511+
512+
// V8 module evaluation cannot be reliably interrupted: JS try-catch
513+
// absorbs terminate_execution(), so the module runs to natural
514+
// completion regardless of the watchdog. Only start a module when
515+
// there is enough budget to absorb its worst-case run time.
516+
const MIN_MODULE_BUDGET: std::time::Duration = std::time::Duration::from_secs(15);
517+
if self.navigation_deadline.is_some() && module_budget < MIN_MODULE_BUDGET {
518+
tracing::info!(
519+
"skipping ES module: only {:.1}s remaining (need {}s min)",
520+
module_budget.as_secs_f64(),
521+
MIN_MODULE_BUDGET.as_secs()
522+
);
523+
break;
524+
}
460525
if let Some(ref src) = module_script.src {
461526
let full_url = if src.starts_with("http://") || src.starts_with("https://") {
462527
src.clone()
@@ -468,7 +533,7 @@ impl Page {
468533

469534
tracing::info!("Loading ES module: {}", full_url);
470535
if let Some(js) = &mut self.js {
471-
match js.load_module(&full_url).await {
536+
match js.load_module(&full_url, module_budget).await {
472537
Ok(()) => {
473538
tracing::info!("ES module loaded: {}", full_url);
474539
self.record_network_event(&full_url, "GET", "Script", 200, &std::collections::HashMap::new(), 0);
@@ -481,29 +546,51 @@ impl Page {
481546
} else if !module_script.inline.is_empty() {
482547
let base = self.url_string();
483548
if let Some(js) = &mut self.js {
484-
if let Err(e) = js.load_inline_module(&module_script.inline, &base).await {
549+
if let Err(e) = js.load_inline_module(&module_script.inline, &base, module_budget).await {
485550
tracing::warn!("Inline ES module error: {}", e);
486551
}
487552
}
488553
}
489554
}
490555

556+
// Compute budgets before mutably borrowing self.js.
557+
let events_budget = self
558+
.navigation_remaining()
559+
.unwrap_or_else(|| std::time::Duration::from_secs(5))
560+
.max(std::time::Duration::from_millis(50));
561+
let event_loop_cap = std::time::Duration::from_millis(500);
562+
let event_loop_budget = self
563+
.navigation_remaining()
564+
.map(|r| r.min(event_loop_cap))
565+
.unwrap_or(event_loop_cap);
566+
491567
if let Some(js) = &mut self.js {
492568
// Spec order: readyState -> interactive, fire DOMContentLoaded on both
493569
// document and window, then readyState -> complete, fire load.
494-
let _ = js.execute_script("<load-events>",
570+
// Allow at least 50 ms so basic event dispatch always completes, but cap
571+
// at the remaining navigation budget so heavy onload handlers don't escape.
572+
let _ = js.execute_script_with_timeout(
495573
"globalThis.__documentReadyState__ = 'interactive';\n\
496574
try { document.dispatchEvent(new Event('DOMContentLoaded', {bubbles:false,cancelable:false})); } catch(e) {}\n\
497575
try { window.dispatchEvent(new Event('DOMContentLoaded', {bubbles:false,cancelable:false})); } catch(e) {}\n\
498576
if (typeof window.onload === 'function') { try { window.onload(); } catch(e) {} }\n\
499577
globalThis.__documentReadyState__ = 'complete';\n\
500-
try { window.dispatchEvent(new Event('load', {bubbles:false,cancelable:false})); } catch(e) {}");
578+
try { window.dispatchEvent(new Event('load', {bubbles:false,cancelable:false})); } catch(e) {}",
579+
events_budget,
580+
);
501581
}
502582

503583
if let Some(js) = &mut self.js {
504-
let deadline = tokio::time::Instant::now() + tokio::time::Duration::from_millis(500);
584+
let deadline = tokio::time::Instant::now() + event_loop_budget;
505585
let mut idle_count = 0u32;
506586
loop {
587+
// Check the deadline on every iteration, not only in the
588+
// timeout branch — otherwise a page that keeps the event
589+
// loop "busy" (microtask spirals, stuck XHRs) can loop here
590+
// indefinitely because the deadline check is unreachable.
591+
if tokio::time::Instant::now() >= deadline {
592+
break;
593+
}
507594
let result = tokio::time::timeout(
508595
tokio::time::Duration::from_millis(10),
509596
js.run_event_loop(),
@@ -525,9 +612,6 @@ impl Page {
525612
Ok(Err(_)) => break,
526613
Err(_) => {
527614
idle_count = 0;
528-
if tokio::time::Instant::now() >= deadline {
529-
break;
530-
}
531615
}
532616
}
533617
}
@@ -795,6 +879,16 @@ impl Page {
795879

796880
self.execute_scripts().await;
797881

882+
// execute_scripts honours the deadline internally and breaks
883+
// early, but `Ok(())` from there is ambiguous: the caller still
884+
// sees "navigation succeeded". Surface the timeout so the CLI
885+
// produces the expected "Timed out navigating" error instead of
886+
// silently returning a partially-rendered page.
887+
if self.navigation_expired() {
888+
self.lifecycle = LifecycleState::Failed;
889+
return Err(PageError::NavigationTimedOut);
890+
}
891+
798892
if let Some(js) = &mut self.js {
799893
if let Ok(new_title) = js.evaluate("document.title") {
800894
if let Some(t) = new_title.as_str() {
@@ -1103,6 +1197,9 @@ pub enum PageError {
11031197

11041198
#[error("Too many redirects (limit {0})")]
11051199
TooManyRedirects(usize),
1200+
1201+
#[error("Navigation deadline exceeded")]
1202+
NavigationTimedOut,
11061203
}
11071204

11081205
impl From<ObscuraNetError> for PageError {

crates/obscura-cli/src/main.rs

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -525,8 +525,25 @@ async fn run_fetch(
525525
eprintln!("Fetching {}...", url_str);
526526
}
527527

528+
// tokio::time::timeout can only fire at await points, so synchronous
529+
// V8 work (page-supplied `<script>` evaluation) used to hold the
530+
// executor for the entire duration of every script regardless of
531+
// `--timeout`. Plumb the deadline into the page itself so each
532+
// script runs under a V8 watchdog with the remaining budget, and
533+
// the navigation aborts cleanly between scripts once the deadline
534+
// is past. The outer `timeout(...)` wrapper still guards async-only
535+
// hangs (slow network, idle wait).
536+
let deadline = std::time::Instant::now() + Duration::from_secs(timeout_secs);
537+
page.set_navigation_deadline(Some(deadline));
538+
528539
match timeout(Duration::from_secs(timeout_secs), page.navigate_with_wait(url_str, wait_condition)).await {
529-
Ok(result) => result.map_err(|e| anyhow::anyhow!("Failed to navigate to {}: {}", url_str, e))?,
540+
Ok(Ok(())) => {}
541+
Ok(Err(obscura_browser::page::PageError::NavigationTimedOut)) => anyhow::bail!(
542+
"Timed out navigating to {} after {}s",
543+
url_str,
544+
timeout_secs
545+
),
546+
Ok(Err(e)) => return Err(anyhow::anyhow!("Failed to navigate to {}: {}", url_str, e)),
530547
Err(_) => anyhow::bail!(
531548
"Timed out navigating to {} after {}s",
532549
url_str,

0 commit comments

Comments
 (0)