From ade4de2f5f78be4ded40cac5d63d82ac0922142a Mon Sep 17 00:00:00 2001 From: ldm0 Date: Sun, 4 Oct 2026 16:00:56 +0800 Subject: [PATCH] fix(renderer): preserve source parser until navigation commits Keep the complete source phase-one residence and context resources intact until navigation commit validation and WindowProxy detachment succeed. Retire the parser, input bridge and ScriptVm synchronously after that boundary, and defer source lifecycle termination until commit. Cover replacement success, 204 No Content, and script-environment and WindowProxy ownership failures after a replacement response is Prepared. Verify failed commits preserve the source Inspector context, later body bytes still parse, and the original Document reaches Load. --- .../src/runtime/owner_local_store/entry.rs | 15 +- .../runtime/page_vm/followed_navigation.rs | 40 +-- moli-renderer-v8/src/runtime/page_vm/mod.rs | 58 ++-- .../runtime/phase_one/pending_residence.rs | 12 + .../runtime/phase_one/streaming_residence.rs | 8 + .../src/runtime/tests/open_streaming.rs | 261 ++++++++++++++++++ 6 files changed, 352 insertions(+), 42 deletions(-) diff --git a/moli-renderer-v8/src/runtime/owner_local_store/entry.rs b/moli-renderer-v8/src/runtime/owner_local_store/entry.rs index 9b49afb7ed..2cf54712d5 100644 --- a/moli-renderer-v8/src/runtime/owner_local_store/entry.rs +++ b/moli-renderer-v8/src/runtime/owner_local_store/entry.rs @@ -357,13 +357,20 @@ impl LivePageEntry { &mut self, prepared: PageVmPreparedFollowedNavigationCommit, ) -> Result { - assert!( - self.pending_phase_one_navigation.is_none(), - "a source navigation commit cannot coexist with pending phase one" - ); + // Commit validation and WindowProxy detachment can fail. Keep the + // complete source residence live until they succeed; every operation + // after that boundary is synchronous and infallible. let navigation = self .page_vm_mut() .commit_prepared_followed_location_navigation(prepared)?; + if let Some(pending) = self.pending_phase_one_navigation.take() { + let (residence, mut metadata) = pending.into_parts(); + let mut page_vm = residence.into_navigation_triggered_page_vm(); + metadata.complete_service_worker_follow(&mut page_vm); + self.install_resumed_phase_one_page_vm(page_vm); + } + self.retire_document_lifecycle_turn(); + self.page_vm_mut().retire_committed_main_script_vm(); Ok(navigation) } diff --git a/moli-renderer-v8/src/runtime/page_vm/followed_navigation.rs b/moli-renderer-v8/src/runtime/page_vm/followed_navigation.rs index eea5846928..e4ef9400c4 100644 --- a/moli-renderer-v8/src/runtime/page_vm/followed_navigation.rs +++ b/moli-renderer-v8/src/runtime/page_vm/followed_navigation.rs @@ -17,7 +17,6 @@ use crate::runtime::{ ExternalRawDocumentBodyStream, PageId, PageVmFollowNavigationTurnOutcome, PageVmFollowedNavigationBuildOutcome, PageVmFollowedNavigationMetadata, PageVmInitStage, PageVmPendingPhaseOneNavigation, PendingDocumentLifecycleTurn, RendererBrowserContextRuntime, - RendererDocumentLifecycleTransition, RendererDocumentTerminationReason, RendererLifecycleStartReason, RendererPendingDownloadActivation, RendererPendingDownloadResponse, }; @@ -789,21 +788,6 @@ impl PageVm { | LoadedFollowedLocationNavigation::ExternalDocument { .. }) => loaded, }; - let termination = self.document_lifecycle.request_termination( - self.document_lifecycle.identity(), - RendererDocumentTerminationReason::SupersededByCrossDocumentNavigation, - ); - debug_assert!( - matches!( - termination, - RendererDocumentLifecycleTransition::Recorded(_) - | RendererDocumentLifecycleTransition::Deferred - | RendererDocumentLifecycleTransition::Duplicate - ), - "cross-document navigation should terminate the active renderer document: {termination:?}" - ); - *pending_document_lifecycle_turn = None; - Ok(PageVmDocumentCommitPreparation::Prepared(Box::new( PageVmPreparedFollowedNavigationCommit { initiator_url, @@ -943,16 +927,33 @@ impl PageVm { } = prepared; let env = self.followed_location_navigation_env(); let runtime_hooks = self.runtime_hooks.clone().for_cross_document_commit(); + #[cfg(test)] + let runtime_hooks = { + let mut runtime_hooks = runtime_hooks; + if loaded.has_header_for_test("x-moli-test-missing-navigation-script-environment") { + runtime_hooks.renderer_page_script_environment = None; + } + runtime_hooks + }; let browser_context_runtime = runtime_hooks.browser_context_runtime.clone(); let local_executor = self.local_executor.clone(); let request_client = self.request_client.clone(); let page_id = self.page_id; + #[cfg(test)] + let commit_page_id = + if loaded.has_header_for_test("x-moli-test-mismatched-navigation-page-id") { + PageId::new_for_testing(page_id.as_u64() + 1) + } else { + page_id + }; + #[cfg(not(test))] + let commit_page_id = page_id; let commit_result = (|| { ensure!( runtime_hooks.has_renderer_page_script_environment(), "owner-managed navigation commit requires a renderer Page script environment" ); - self.commit_main_window_proxy_navigation() + self.commit_main_window_proxy_navigation(commit_page_id) })(); if let Err(error) = commit_result { self.reject_failed_followed_location_navigation( @@ -1235,7 +1236,10 @@ impl PageVm { let env = self.followed_location_navigation_env(); let runtime_hooks = self.runtime_hooks.clone().for_cross_document_commit(); if runtime_hooks.has_renderer_page_script_environment() { - self.commit_main_window_proxy_navigation()?; + self.commit_main_window_proxy_navigation(self.page_id)?; + self.retire_committed_main_script_vm(); + } else { + self.record_main_navigation_commit(); } bootstrap_committed_followed_location_navigation( self.page_id, diff --git a/moli-renderer-v8/src/runtime/page_vm/mod.rs b/moli-renderer-v8/src/runtime/page_vm/mod.rs index c80f4109cf..da4aa87156 100644 --- a/moli-renderer-v8/src/runtime/page_vm/mod.rs +++ b/moli-renderer-v8/src/runtime/page_vm/mod.rs @@ -2191,28 +2191,46 @@ impl PageVm { .page_task_producer_routes_match(sources) } - fn commit_main_window_proxy_navigation(&mut self) -> Result<()> { - let Some(mut vm) = self.vm.take() else { - return Err(anyhow::anyhow!( - "main navigation attempted to commit an already retired PageVm" - )); - }; + fn commit_main_window_proxy_navigation(&mut self, page_id: PageId) -> Result<()> { + let vm = self.vm.as_mut().ok_or_else(|| { + anyhow::anyhow!("main navigation attempted to commit an already retired PageVm") + })?; + // ScriptVm validates ownership and proxy identity before detach_global. + // An Err therefore leaves the inspector, context resources and parser + // intact. Keep the ScriptVm until its parser residence has been retired. + vm.detach_main_window_proxy_for_navigation_commit(page_id.as_u64())?; + self.record_main_navigation_commit(); + tracing::debug!( + page_id = self.page_id.as_u64(), + "committed main navigation before replacement realm bootstrap" + ); + Ok(()) + } + + fn record_main_navigation_commit(&mut self) { + let termination = self.document_lifecycle.request_termination( + self.document_lifecycle.identity(), + RendererDocumentTerminationReason::SupersededByCrossDocumentNavigation, + ); + debug_assert!( + matches!( + termination, + RendererDocumentLifecycleTransition::Recorded(_) + | RendererDocumentLifecycleTransition::Deferred + | RendererDocumentLifecycleTransition::Duplicate + ), + "cross-document navigation should terminate the active renderer document: {termination:?}" + ); + } + + pub(in crate::runtime) fn retire_committed_main_script_vm(&mut self) { + let mut vm = self + .vm + .take() + .expect("committed main navigation must retain its source ScriptVm until retirement"); vm.detach_default_inspector_context_for_context_teardown(); vm.close_page_context_resources_for_context_teardown(); - match vm.detach_main_window_proxy_for_navigation_commit(self.page_id.as_u64()) { - Ok(()) => { - tracing::debug!( - page_id = self.page_id.as_u64(), - "committed main navigation before replacement realm bootstrap" - ); - drop(vm); - Ok(()) - } - Err(error) => { - self.vm = Some(vm); - Err(error) - } - } + drop(vm); } // Fresh PageVm bootstrap is a special V8-entry boundary. diff --git a/moli-renderer-v8/src/runtime/phase_one/pending_residence.rs b/moli-renderer-v8/src/runtime/phase_one/pending_residence.rs index d72eb80bb6..b2e78b3e31 100644 --- a/moli-renderer-v8/src/runtime/phase_one/pending_residence.rs +++ b/moli-renderer-v8/src/runtime/phase_one/pending_residence.rs @@ -133,6 +133,18 @@ impl PendingPhaseOneResidence { } } + /// Retire the source parser while keeping its PageVm live for a prepared + /// cross-document navigation commit. This never waits for source input. + pub(in crate::runtime) fn into_navigation_triggered_page_vm(self) -> PageVm { + match self { + Self::ParserBlockingSourceLoad { runtime, .. } + | Self::ClosedInputPageWork { runtime, .. } => { + (*runtime).into_navigation_triggered_page_vm() + } + Self::OpenStreaming(continuation) => continuation.into_navigation_triggered_page_vm(), + } + } + pub(in crate::runtime) async fn resume(self) -> Result { match self { Self::ParserBlockingSourceLoad { runtime, started } diff --git a/moli-renderer-v8/src/runtime/phase_one/streaming_residence.rs b/moli-renderer-v8/src/runtime/phase_one/streaming_residence.rs index 9404fde33c..a1c3416056 100644 --- a/moli-renderer-v8/src/runtime/phase_one/streaming_residence.rs +++ b/moli-renderer-v8/src/runtime/phase_one/streaming_residence.rs @@ -74,6 +74,14 @@ impl PendingStreamingPhaseOneContinuation { self.input.has_ready_input() } + pub(super) fn into_navigation_triggered_page_vm(self) -> PageVm { + let Self { runtime, input, .. } = self; + // Dropping the input receiver cancels the old body bridge without + // waiting for its response to finish. + drop(input); + (*runtime).into_navigation_triggered_page_vm() + } + pub(in crate::runtime) async fn resume(self) -> Result { self.runtime.publish_processing_main_document_phase(); let Self { diff --git a/moli-renderer-v8/src/runtime/tests/open_streaming.rs b/moli-renderer-v8/src/runtime/tests/open_streaming.rs index da5d6200f0..302bbe189a 100644 --- a/moli-renderer-v8/src/runtime/tests/open_streaming.rs +++ b/moli-renderer-v8/src/runtime/tests/open_streaming.rs @@ -21,6 +21,267 @@ struct OpenStreamingPage { activity_wake_rx: RendererExternalActivityTestReceiver, } +const SELF_REPLACE_WHILE_PARSING_HTML: &str = r#""#; + +#[tokio::test(flavor = "multi_thread")] +async fn location_replace_commits_while_source_phase_one_stream_remains_open() { + let (base_url, server) = spawn_owner_wake_server_with_content_type( + "/page", + "
replacement document
", + "text/html", + Duration::ZERO, + ) + .await; + // The timer runs as Page work after the parser has parked waiting for + // more body input. Keep the source stream open throughout the commit. + let mut source = OpenStreamingPage::create(&base_url, SELF_REPLACE_WHILE_PARSING_HTML).await; + + tokio::time::timeout( + Duration::from_secs(5), + recv_page_lifecycle_until( + &mut source.activity_wake_rx, + &source.page, + RendererDocumentLifecycleMilestone::Load, + ), + ) + .await + .expect("self-replacement must reach Load before the source stream reaches EOF"); + tokio::time::timeout(Duration::from_secs(5), source.body_tx.closed()) + .await + .expect("committing the replacement must release the source body receiver"); + assert!( + source.completion_tx.send(Ok(())).is_err(), + "the discarded phase-one bridge must release its completion receiver" + ); + let (reply, _) = source + .page + .run_async_command(RendererPageCommand::EvaluateExpression { + expression: "JSON.stringify([!!document.getElementById('replacement'), document.readyState, history.length, typeof __sourceDocument])".to_owned(), + await_promise: false, + }) + .await + .expect("replacement Document should remain usable after phase-one settlement"); + assert_eq!( + renderer_json_value(reply), + Some(serde_json::json!("[true,\"complete\",1,\"undefined\"]")) + ); + let state = RendererPageTestingHandle::new_for_testing(&source.page) + .current_page_state_async() + .await + .expect("replacement state should be published"); + assert_eq!(state.final_url().as_str(), format!("{base_url}/page")); + assert_eq!( + has_pending_location_navigation_for_test(&source.page).await, + Some(false) + ); + + source + .page + .close_async() + .await + .expect("replacement Page should close"); + server.await.expect("self-replacement server should finish"); +} + +#[tokio::test(flavor = "multi_thread")] +async fn location_replace_without_document_preserves_pending_phase_one_stream() { + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("bind no-Document navigation server"); + let address = listener.local_addr().expect("navigation server address"); + let server = tokio::spawn(async move { + let (mut stream, _) = listener + .accept() + .await + .expect("accept self-replacement request"); + let request = read_owner_wake_http_request_head(&mut stream).await; + assert_eq!(request.lines().next().unwrap(), "GET /page HTTP/1.1"); + stream + .write_all(b"HTTP/1.1 204 No Content\r\nContent-Length: 0\r\nConnection: close\r\n\r\n") + .await + .expect("write no-Document navigation response"); + }); + let mut source = OpenStreamingPage::create( + &format!("http://{address}"), + SELF_REPLACE_WHILE_PARSING_HTML, + ) + .await; + tokio::time::timeout(Duration::from_secs(5), server) + .await + .expect("self-replacement request should reach the server") + .expect("no-Document navigation server should finish"); + // This command is ordered after the checked-out navigation task, so the + // 204 has been settled before the remaining source bytes are delivered. + let (reply, _) = source + .page + .run_async_command(RendererPageCommand::EvaluateExpression { + expression: "__sourceDocument".to_owned(), + await_promise: false, + }) + .await + .expect("the source Document must remain active after a 204 navigation"); + assert_eq!(renderer_json_value(reply), Some(serde_json::json!(true))); + source + .body_tx + .send(b"".to_vec()) + .await + .expect("the pending parser must retain its source body receiver"); + drop(source.body_tx); + source + .completion_tx + .send(Ok(())) + .expect("source body should finish"); + tokio::time::timeout( + Duration::from_secs(5), + recv_page_lifecycle_until( + &mut source.activity_wake_rx, + &source.page, + RendererDocumentLifecycleMilestone::Load, + ), + ) + .await + .expect("the source parser should resume and reach Load after the 204"); + let (reply, _) = source + .page + .run_async_command(RendererPageCommand::EvaluateExpression { + expression: + "JSON.stringify([__sourceDocument, __sourceTailParsed, document.readyState])" + .to_owned(), + await_promise: false, + }) + .await + .expect("source parser tail should execute in the original Document"); + assert_eq!( + renderer_json_value(reply), + Some(serde_json::json!("[true,true,\"complete\"]")) + ); + source + .page + .close_async() + .await + .expect("source Page should close"); +} + +#[tokio::test(flavor = "multi_thread")] +async fn location_replace_commit_precondition_failure_preserves_open_source_stream() { + assert_failed_prepared_navigation_preserves_open_source_stream( + "x-moli-test-missing-navigation-script-environment", + ) + .await; +} + +#[tokio::test(flavor = "multi_thread")] +async fn location_replace_window_proxy_validation_failure_preserves_open_source_stream() { + assert_failed_prepared_navigation_preserves_open_source_stream( + "x-moli-test-mismatched-navigation-page-id", + ) + .await; +} + +async fn assert_failed_prepared_navigation_preserves_open_source_stream( + injection_header: &'static str, +) { + let (base_url, server) = spawn_owner_wake_server_with_content_type_and_headers( + "/page", + "
replacement document
", + "text/html", + vec![(injection_header, "1")], + Duration::ZERO, + ) + .await; + let mut source = OpenStreamingPage::create( + &base_url, + r#""#, + ) + .await; + tokio::time::timeout(Duration::from_secs(5), server) + .await + .expect("the replacement response must exist before testing commit failure") + .expect("prepared navigation failure server should finish"); + + // Owner commands are ordered after the checked-out navigation task. The + // response reached Prepared, but its fallible commit must return a Live + // entry with the original context, inspector and parser still resident. + let messages = dispatch_runtime_protocol_for_test( + &source.page, + serde_json::json!({ + "id": 1, + "method": "Runtime.evaluate", + "params": { + "expression": "window === __sourceWindow && document === __sourceDocument && !document.getElementById('replacement')", + "returnByValue": true + } + }), + ) + .await + .expect("commit validation failure must preserve the source Inspector context"); + let response = runtime_protocol_response_by_id(&messages, 1) + .expect("source Inspector evaluation should reply"); + assert_eq!(response["result"]["result"]["value"], true); + assert_eq!( + has_pending_location_navigation_for_test(&source.page).await, + Some(false) + ); + assert!( + !source.body_tx.is_closed(), + "a failed Prepared commit must retain the source body receiver" + ); + source + .body_tx + .send(b"".to_vec()) + .await + .expect("the retained source parser must accept later body bytes"); + drop(source.body_tx); + source + .completion_tx + .send(Ok(())) + .expect("the original source stream must retain its completion receiver"); + let events = tokio::time::timeout( + Duration::from_secs(5), + recv_page_lifecycle_until( + &mut source.activity_wake_rx, + &source.page, + RendererDocumentLifecycleMilestone::Load, + ), + ) + .await + .expect("the original source parser must resume and reach Load after commit failure"); + assert!( + !events.iter().any(|event| matches!( + event.kind, + RendererDocumentLifecycleEventKind::Terminated { + reason: super::super::RendererDocumentTerminationReason::SupersededByCrossDocumentNavigation, + .. + } + )), + "a failed Prepared commit must not terminate the source lifecycle" + ); + let (reply, _) = source + .page + .run_async_command(RendererPageCommand::EvaluateExpression { + expression: "JSON.stringify([window === __sourceWindow, document === __sourceDocument, __sourceTailParsed, document.readyState])".to_owned(), + await_promise: false, + }) + .await + .expect("the source Document must remain usable after its parser reaches EOF"); + assert_eq!( + renderer_json_value(reply), + Some(serde_json::json!("[true,true,true,\"complete\"]")) + ); + source + .page + .close_async() + .await + .expect("source Page should close after a failed Prepared commit"); +} + #[tokio::test(flavor = "multi_thread")] async fn owner_loop_executes_async_script_while_main_document_stream_remains_open() { assert_open_stream_work_executes_before_eof(ASYNC_SCRIPT_HTML, "async script").await;