From d2fed1e0d9d608c09e2fc596166306c130b37611 Mon Sep 17 00:00:00 2001 From: ldm0 Date: Thu, 10 Sep 2026 09:17:40 +0800 Subject: [PATCH] fix: ignore document stream replacement during unload Keep a native counter for each unloading Document so open and implicit write/writeln stream replacement preserve its nodes, listeners and URL. Scope guards cover nested callbacks and ancestor teardown without blocking other documents or manually dispatched unload events. Preserve argument conversion and existing XML/dynamic-markup exception checks. Add 34 Browser and main-VM regression scenarios. The 154-case WPT comparison gains one passing case and six passing subtests with no regressions; update the passed ledger to 9,099 cases. Validation: cargo fmt --all; cargo clippy --workspace --all-targets --all-features -- -D warnings; cargo nextest run --no-fail-fast (17,909 passed, 13 skipped). Rebuilt CLI matches the tested WPT binary. --- .../wpt-cross-current/passed-cases.txt | 3 - moli-core/tests/document_open_unload.rs | 155 ++++++++++++++++++ .../context_bootstrap/navigation_events.rs | 5 + .../src/context_bootstrap/window_accessors.rs | 3 +- .../window_accessors/helpers.rs | 2 +- moli-renderer-v8/src/document_runtime.rs | 1 + .../document_runtime/destructive_writes.rs | 25 ++- .../src/document_runtime/runtime_core.rs | 2 + .../context_host/child_documents/lifecycle.rs | 26 ++- .../child_frame_runtime/document.rs | 3 + .../src/native_bridge/document/lifecycle.rs | 11 +- .../script_vm/tests/browser_api/navigation.rs | 44 +++++ 12 files changed, 264 insertions(+), 16 deletions(-) create mode 100644 moli-core/tests/document_open_unload.rs diff --git a/moli-benchmark/wpt-cross-current/passed-cases.txt b/moli-benchmark/wpt-cross-current/passed-cases.txt index 267b73ad68..3976a2be71 100644 --- a/moli-benchmark/wpt-cross-current/passed-cases.txt +++ b/moli-benchmark/wpt-cross-current/passed-cases.txt @@ -5614,7 +5614,6 @@ html/browsers/browsing-the-web/navigating-across-documents/initial-empty-documen html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/window-open-history-length.html html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/window-open-nourl.html html/browsers/browsing-the-web/navigating-across-documents/javascript-url-global-scope.html -html/browsers/browsing-the-web/navigating-across-documents/javascript-url-no-beforeunload.window.js?moli-wpt-script=window html/browsers/browsing-the-web/navigating-across-documents/javascript-url-query-fragment-components.html html/browsers/browsing-the-web/navigating-across-documents/javascript-url-return-value-handling-dynamic.html html/browsers/browsing-the-web/navigating-across-documents/javascript-url-return-value-handling.html @@ -6264,12 +6263,10 @@ html/semantics/forms/form-submission-0/form-double-submit-to-different-origin-fr html/semantics/forms/form-submission-0/form-double-submit.html html/semantics/forms/form-submission-0/form-submit-iframe-then-location-navigate.html html/semantics/forms/form-submission-0/getactionurl.html -html/semantics/forms/form-submission-0/jsurl-form-submit.tentative.html html/semantics/forms/form-submission-0/jsurl-navigation-then-form-submit.html html/semantics/forms/form-submission-0/newline-normalization.html html/semantics/forms/form-submission-0/reparent-form-during-planned-navigation-task.html html/semantics/forms/form-submission-0/request-submit-activation.html -html/semantics/forms/form-submission-0/submit-entity-body.html html/semantics/forms/form-submission-0/url-encoded.html html/semantics/forms/historical.html html/semantics/forms/resetting-a-form/reset-event.html diff --git a/moli-core/tests/document_open_unload.rs b/moli-core/tests/document_open_unload.rs new file mode 100644 index 0000000000..66259a9e9a --- /dev/null +++ b/moli-core/tests/document_open_unload.rs @@ -0,0 +1,155 @@ +use anyhow::Result; +use moli_core::runtime::{Browser, BrowserConfig}; +use moli_test_support::FixtureServer; +use serde_json::Value; +use tokio::time::Duration; +use url::Url; + +async fn stream_operation_during_unload( + event: &str, + operation: &str, + target: &str, +) -> Result { + let server = FixtureServer::spawn().await?; + let browser = Browser::new(BrowserConfig::default())?; + let markup = format!( + r#""# + ); + let mut url = Url::parse(&server.url("/compat/child-dynamic-markup-document"))?; + url.query_pairs_mut().append_pair("markup", &markup); + let result = tokio::time::timeout(Duration::from_secs(10), async { + let mut page = browser.fetch(url.as_str()).await?; + page.evaluate_runtime_expression_with_await_async( + "finished.then(value => JSON.stringify(value))", + true, + ) + .await + }) + .await??; + let result: Value = serde_json::from_str(result["value"].as_str().unwrap())?; + server.shutdown().await; + assert!( + result["error"].is_null(), + "{event}/{operation}/{target}: {result}" + ); + Ok(result) +} + +#[tokio::test(flavor = "multi_thread")] +async fn document_stream_operations_have_no_side_effects_during_unload() -> Result<()> { + for event in ["beforeunload", "pagehide", "unload"] { + for operation in [ + "open", + "prototype-open", + "write", + "writeln", + "prototype-write", + ] { + let result = stream_operation_during_unload(event, operation, "self").await?; + assert_eq!(result["sameRoot"], true, "{event}/{operation}: {result}"); + assert_eq!(result["sameLength"], true); + assert_eq!(result["sameURL"], true); + assert_eq!(result["listenerCount"], 1); + let is_open = operation.ends_with("open"); + assert_eq!(result["returnedDocument"], is_open); + assert_eq!(result["converted"], usize::from(!is_open)); + } + } + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn ancestor_unload_counter_covers_descendant_callbacks() -> Result<()> { + for event in ["pagehide", "visibilitychange", "unload"] { + let result = stream_operation_during_unload(event, "open", "ancestor").await?; + assert_eq!(result["sameRoot"], true, "{event}: {result}"); + assert_eq!(result["sameLength"], true); + assert_eq!(result["listenerCount"], 1); + assert_eq!(result["returnedDocument"], true); + } + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn unload_does_not_block_opening_another_document() -> Result<()> { + for event in ["beforeunload", "pagehide", "unload"] { + let result = stream_operation_during_unload(event, "open", "other").await?; + assert_eq!(result["sameRoot"], false, "{event}: {result}"); + assert_eq!(result["listenerCount"], 0); + assert_eq!(result["returnedDocument"], true); + } + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn synthetic_unload_events_do_not_block_document_open() -> Result<()> { + for event in ["beforeunload", "pagehide", "visibilitychange", "unload"] { + let result = stream_operation_during_unload(event, "open", "synthetic").await?; + assert_eq!(result["sameRoot"], false, "{event}: {result}"); + assert_eq!(result["listenerCount"], 0); + assert_eq!(result["returnedDocument"], true); + } + Ok(()) +} diff --git a/moli-renderer-v8/src/context_bootstrap/navigation_events.rs b/moli-renderer-v8/src/context_bootstrap/navigation_events.rs index 8ec8de4830..64a613878a 100644 --- a/moli-renderer-v8/src/context_bootstrap/navigation_events.rs +++ b/moli-renderer-v8/src/context_bootstrap/navigation_events.rs @@ -669,6 +669,11 @@ fn dispatch_unload_lifecycle_event_for_runtime_owner<'s>( event_type: &str, event: v8::Local<'s, v8::Object>, ) { + let _document_unload = context_host_ptr_from_global_bridge(scope).and_then(|host_ptr| { + let host = unsafe { &*host_ptr }; + super::window_accessors::window_document_handle(scope, owner, host) + .map(|document| host.enter_document_unload(document)) + }); set_navigation_unload_event_active(scope, owner, true); if runtime_window_is_global(scope, owner) { if let Some(host_ptr) = context_host_ptr_from_global_bridge(scope) { diff --git a/moli-renderer-v8/src/context_bootstrap/window_accessors.rs b/moli-renderer-v8/src/context_bootstrap/window_accessors.rs index f12b852fe4..757a93d422 100644 --- a/moli-renderer-v8/src/context_bootstrap/window_accessors.rs +++ b/moli-renderer-v8/src/context_bootstrap/window_accessors.rs @@ -22,7 +22,8 @@ pub(super) use child_context::{ }; pub(crate) use helpers::{current_window_style_viewport, window_host_ptr}; pub(super) use helpers::{ - window_child_context_handle, window_has_discarded_child_browsing_context, + window_child_context_handle, window_document_handle, + window_has_discarded_child_browsing_context, }; pub(super) use interceptors::{ window_indexed_property_definer, window_indexed_property_deleter, diff --git a/moli-renderer-v8/src/context_bootstrap/window_accessors/helpers.rs b/moli-renderer-v8/src/context_bootstrap/window_accessors/helpers.rs index b7c94c5f42..c83c5691f0 100644 --- a/moli-renderer-v8/src/context_bootstrap/window_accessors/helpers.rs +++ b/moli-renderer-v8/src/context_bootstrap/window_accessors/helpers.rs @@ -103,7 +103,7 @@ pub(super) fn window_owner_dispatch_scope<'s>( }) } -pub(super) fn window_document_handle<'s>( +pub(in crate::context_bootstrap) fn window_document_handle<'s>( scope: &mut v8::PinScope<'s, '_>, receiver: v8::Local<'s, v8::Object>, host: &JsContextHost, diff --git a/moli-renderer-v8/src/document_runtime.rs b/moli-renderer-v8/src/document_runtime.rs index c483ad939a..e454377110 100644 --- a/moli-renderer-v8/src/document_runtime.rs +++ b/moli-renderer-v8/src/document_runtime.rs @@ -739,6 +739,7 @@ pub(super) struct DocumentRuntime { resource_loader_binding: Option, script_context_stack: Vec, destructive_write_counters: destructive_writes::DocumentWriteCounters, + document_unload_counters: destructive_writes::DocumentWriteCounters, root_document_parser: Option, post_parse_schedule_invalidated: bool, stylesheet_lifecycle: StylesheetLifecycleState, diff --git a/moli-renderer-v8/src/document_runtime/destructive_writes.rs b/moli-renderer-v8/src/document_runtime/destructive_writes.rs index f1f657c3bf..3d9d2614d0 100644 --- a/moli-renderer-v8/src/document_runtime/destructive_writes.rs +++ b/moli-renderer-v8/src/document_runtime/destructive_writes.rs @@ -5,22 +5,21 @@ use super::{DocumentRuntime, DomHandle}; #[derive(Debug, Default)] pub(super) struct DocumentWriteCounters(Rc>>); -/// Owns an execute-script-element counter through script cleanup, including -/// its microtask checkpoint, but not subsequent tasks or pending TLA work. -/// The shared state avoids borrowing the runtime across JavaScript reentry. -pub(crate) struct IgnoreDestructiveWritesGuard { +/// Holds a document-local writing restriction across JavaScript reentry +/// without borrowing the runtime. Nested scopes release only their own count. +pub(crate) struct DocumentWriteCounterGuard { counters: Rc>>, document: DomHandle, } impl DocumentWriteCounters { - fn enter(&self, document: DomHandle) -> IgnoreDestructiveWritesGuard { + fn enter(&self, document: DomHandle) -> DocumentWriteCounterGuard { let mut counters = self.0.borrow_mut(); let counter = counters.entry(document).or_default(); *counter = counter .checked_add(1) .expect("document write counter overflow"); - IgnoreDestructiveWritesGuard { + DocumentWriteCounterGuard { counters: Rc::clone(&self.0), document, } @@ -31,7 +30,7 @@ impl DocumentWriteCounters { } } -impl Drop for IgnoreDestructiveWritesGuard { +impl Drop for DocumentWriteCounterGuard { fn drop(&mut self) { let mut counters = self.counters.borrow_mut(); let counter = counters @@ -45,16 +44,26 @@ impl Drop for IgnoreDestructiveWritesGuard { } impl DocumentRuntime { + /// Keep the script-element counter through script cleanup and its + /// microtask checkpoint, but not subsequent tasks or pending TLA work. pub(crate) fn enter_ignore_destructive_writes( &self, document: DomHandle, - ) -> IgnoreDestructiveWritesGuard { + ) -> DocumentWriteCounterGuard { self.destructive_write_counters.enter(document) } pub(crate) fn has_ignore_destructive_writes_counter(&self, document: DomHandle) -> bool { self.destructive_write_counters.is_active(document) } + + pub(crate) fn enter_document_unload(&self, document: DomHandle) -> DocumentWriteCounterGuard { + self.document_unload_counters.enter(document) + } + + pub(crate) fn has_document_unload_counter(&self, document: DomHandle) -> bool { + self.document_unload_counters.is_active(document) + } } #[cfg(test)] diff --git a/moli-renderer-v8/src/document_runtime/runtime_core.rs b/moli-renderer-v8/src/document_runtime/runtime_core.rs index 2e76167010..e386b979a5 100644 --- a/moli-renderer-v8/src/document_runtime/runtime_core.rs +++ b/moli-renderer-v8/src/document_runtime/runtime_core.rs @@ -83,6 +83,7 @@ impl DocumentRuntime { resource_loader_binding: None, script_context_stack: Vec::new(), destructive_write_counters: Default::default(), + document_unload_counters: Default::default(), root_document_parser: None, post_parse_schedule_invalidated: false, stylesheet_lifecycle, @@ -215,6 +216,7 @@ impl DocumentRuntime { resource_loader_binding: _, script_context_stack: _, destructive_write_counters: _, + document_unload_counters: _, root_document_parser: _, post_parse_schedule_invalidated: _, stylesheet_lifecycle: _, diff --git a/moli-renderer-v8/src/native_bridge/context_host/child_documents/lifecycle.rs b/moli-renderer-v8/src/native_bridge/context_host/child_documents/lifecycle.rs index c808a2211e..dfde73368b 100644 --- a/moli-renderer-v8/src/native_bridge/context_host/child_documents/lifecycle.rs +++ b/moli-renderer-v8/src/native_bridge/context_host/child_documents/lifecycle.rs @@ -896,6 +896,9 @@ impl JsContextHost { let execution_context_owner = crate::native_bridge::WindowExecutionContextOwner::Frame( action.owner().local_window_id, ); + let _document_unload = self + .child_browsing_context_document_handle(handle) + .map(|document| self.enter_document_unload(document)); dispatch_beforeunload_for_runtime_owner(scope, window); dispatch_pagehide_for_runtime_owner(scope, window); dispatch_unload_for_runtime_owner(scope, window); @@ -932,11 +935,27 @@ impl JsContextHost { .into_iter() .filter_map(|handle| { unsafe { &*host_ptr }.child_browsing_context_document_handle(handle) - .map(|document| (handle, document)) + .map(|document| { + ( + handle, + document, + unsafe { &*host_ptr }.dom_host().owner_document_handle(handle), + ) + }) }) .collect(); - for (handle, document) in documents { + // Keep an ancestor's counter active while its descendants unload, + // and release a completed sibling before entering the next subtree. + let mut unload_guards = Vec::new(); + for (handle, document, parent_document) in documents { + while unload_guards + .last() + .is_some_and(|(document, _)| Some(*document) != parent_document) + { + unload_guards.pop(); + } if unsafe { &*host_ptr }.child_browsing_context_document_handle(handle) == Some(document) { + unload_guards.push((document, unsafe { &*host_ptr }.enter_document_unload(document))); Self::dispatch_child_document_unload_without_beforeunload(scope, host_ptr, handle); } } @@ -970,6 +989,9 @@ impl JsContextHost { action.owner().local_window_id, ); + let _document_unload = unsafe { &*host_ptr } + .child_browsing_context_document_handle(handle) + .map(|document| unsafe { &*host_ptr }.enter_document_unload(document)); dispatch_pagehide_for_runtime_owner(scope, window); if let Some(event) = construct_original_event(scope, "visibilitychange") { let _ = call_object_method(scope, document, "dispatchEvent", &[event.into()]); diff --git a/moli-renderer-v8/src/native_bridge/context_host/child_frame_runtime/document.rs b/moli-renderer-v8/src/native_bridge/context_host/child_frame_runtime/document.rs index 0d109a2cab..40b386cef3 100644 --- a/moli-renderer-v8/src/native_bridge/context_host/child_frame_runtime/document.rs +++ b/moli-renderer-v8/src/native_bridge/context_host/child_frame_runtime/document.rs @@ -592,6 +592,9 @@ impl JsContextHost { { return None; } + if unsafe { &*host_ptr }.has_document_unload_counter(document_handle) { + return None; + } if unsafe { &*host_ptr }.child_document_stream_is_blocked_by_navigation(child_handle) { tracing::debug!( ?child_handle, diff --git a/moli-renderer-v8/src/native_bridge/document/lifecycle.rs b/moli-renderer-v8/src/native_bridge/document/lifecycle.rs index b73e2a26c5..afa3fb232f 100644 --- a/moli-renderer-v8/src/native_bridge/document/lifecycle.rs +++ b/moli-renderer-v8/src/native_bridge/document/lifecycle.rs @@ -123,6 +123,10 @@ fn node_document_write_or_writeln_callback<'s>( if detached_native_handle_for_runtime(scope, runtime_ptr, args.this()).is_some() { let document = args.this(); let stream_was_open = detached_document_write_stream_is_open(scope, document); + if !stream_was_open && unsafe { &*runtime_ptr }.has_document_unload_counter(handle) { + rv.set_undefined(); + return; + } if !stream_was_open { set_detached_document_write_stream_open(scope, document, true); } @@ -141,7 +145,8 @@ fn node_document_write_or_writeln_callback<'s>( let implicit_replacement_session = !runtime.has_active_parser_write_insertion_point() && !runtime.host_document().replace_on_close(); if implicit_replacement_session - && (runtime.has_ignore_destructive_writes_counter(handle) + && (runtime.has_document_unload_counter(handle) + || runtime.has_ignore_destructive_writes_counter(handle) || current_script_ignores_document_write_without_parser_insertion_point(runtime)) { rv.set_undefined(); @@ -243,6 +248,10 @@ pub(in crate::native_bridge) fn node_document_open_callback<'s>( ); return; } + if unsafe { &*runtime_ptr }.has_document_unload_counter(handle) { + rv.set(args.this().into()); + return; + } if detached_native_handle_for_runtime(scope, runtime_ptr, args.this()).is_some() { let document = args.this(); set_detached_document_write_stream_open(scope, document, true); diff --git a/moli-renderer-v8/src/script_vm/tests/browser_api/navigation.rs b/moli-renderer-v8/src/script_vm/tests/browser_api/navigation.rs index 544c2a8fdc..38a053b979 100644 --- a/moli-renderer-v8/src/script_vm/tests/browser_api/navigation.rs +++ b/moli-renderer-v8/src/script_vm/tests/browser_api/navigation.rs @@ -2440,6 +2440,50 @@ fn cross_document_unload_lifecycle_orders_pagehide_before_unload_without_timer() ); } +#[test] +fn main_document_stream_operations_are_suppressed_only_during_unload() { + for event in ["beforeunload", "pagehide", "unload"] { + for operation in ["open", "write", "writeln"] { + let mut vm = new_storage_test_vm("https://document-open-unload.test/source"); + let result = vm + .eval(&format!( + r#"(() => {{ + const doc = document; + if (!doc.documentElement) doc.appendChild(doc.createElement('html')); + const root = doc.documentElement; + root.appendChild(doc.createElement('p')); + let retained = false; + let nestedRetained = false; + let listenerCount = 0; + let converted = 0; + doc.addEventListener('retained-listener', () => listenerCount++); + addEventListener('nested-open', () => {{ + doc.open(); + nestedRetained = doc.documentElement === root; + }}); + addEventListener({event:?}, () => {{ + if ({operation:?} === 'open') doc.open(); + else doc[{operation:?}]({{toString() {{ converted++; return 'changed'; }}}}); + retained = doc.documentElement === root; + dispatchEvent(new Event('nested-open')); + doc.dispatchEvent(new Event('retained-listener')); + }}); + navigation.navigate('/destination'); + doc.open(); + return [retained, nestedRetained, listenerCount, converted, + doc.documentElement !== root].join('|'); + }})()"# + )) + .expect("main document unload stream operations should evaluate"); + assert_eq!( + result, + format!("true|true|1|{}|true", usize::from(operation != "open")), + "{event}/{operation}" + ); + } + } +} + #[test] fn before_unload_handler_coerces_its_result_while_window_event_is_current() { let mut vm = new_storage_test_vm("https://example.com/base");