From 2bc71595327fe7bfce5cc71621863385f8d6f3ff Mon Sep 17 00:00:00 2001 From: ldm0 Date: Thu, 17 Sep 2026 15:34:57 +0800 Subject: [PATCH] fix(modules): settle failed dynamic import fetches Mark a failed fetch in the shared module map before notifying graph clients, so concurrent imports and later cached imports all reject. Route document and child owners through the same completion path. Cover concurrent imports of the same root and a shared dependency, fresh TypeError rejections, and subsequent imports after a cached fetch failure. --- moli-renderer-v8/src/module_runtime/graph.rs | 57 +++-------- .../src/script_vm/native_module.rs | 95 ++++++++++++++++++- 2 files changed, 106 insertions(+), 46 deletions(-) diff --git a/moli-renderer-v8/src/module_runtime/graph.rs b/moli-renderer-v8/src/module_runtime/graph.rs index 915d7f86e2..94ca19de8b 100644 --- a/moli-renderer-v8/src/module_runtime/graph.rs +++ b/moli-renderer-v8/src/module_runtime/graph.rs @@ -831,23 +831,8 @@ impl NativeModuleGraphJob { request: &NativeModuleGraphFetchRequest, source: std::result::Result, ) -> std::result::Result { - let client = request.tree_client.ok_or_else(|| { - ModuleLoadError::new( - ModuleLoadStage::Fetch, - "module graph fetch request was missing its module tree client token", - ) - })?; - if let Err(error) = &source { - let key = request.pending_fetch_key().cloned().ok_or_else(|| { - ModuleLoadError::new( - ModuleLoadStage::Fetch, - "failed module graph fetch request was missing its module map key", - ) - })?; - vm.document_runtime - .mark_native_module_failed(key, error.clone()); - } - self.finish_pending_chromium_tree_fetch_for_client(vm, client, request, source) + let mut owner = NativeModuleTreeDocumentOwner::new(vm); + self.finish_pending_chromium_tree_fetch_for_request_with_owner(&mut owner, request, source) } fn finish_pending_chromium_tree_fetch_for_request_with_owner( @@ -865,28 +850,22 @@ impl NativeModuleGraphJob { "module graph fetch request was missing its module tree client token", ) })?; + if let Err(error) = &source { + let key = request.pending_fetch_key().cloned().ok_or_else(|| { + ModuleLoadError::new( + ModuleLoadStage::Fetch, + "failed module graph fetch request was missing its module map key", + ) + })?; + // Settle the shared fetch before failing this graph, so joined + // clients and later imports observe the cached fetch failure. + owner.mark_module_failed(key, error.clone()); + } self.finish_pending_chromium_tree_fetch_for_client_with_owner( owner, client, request, source, ) } - fn finish_pending_chromium_tree_fetch_for_client( - &mut self, - vm: &mut ScriptVm, - client: module_tree::SingleModuleClientToken, - request: &NativeModuleGraphFetchRequest, - source: std::result::Result, - ) -> std::result::Result { - trace_module_tree_fetch_completed_to_job(client, source.is_ok()); - let outcome = match source { - Ok(source) => module_tree::ModuleFetchOutcome::Fetched(Box::new( - chromium_fetched_source_for_request(source, request)?, - )), - Err(error) => module_tree::ModuleFetchOutcome::Failed(chromium_error(error)), - }; - self.resume_chromium_tree_fetch_outcome(vm, client, outcome) - } - fn finish_pending_chromium_tree_fetch_for_client_with_owner( &mut self, owner: &mut O, @@ -907,16 +886,6 @@ impl NativeModuleGraphJob { self.resume_chromium_tree_fetch_outcome_with_owner(owner, client, outcome) } - fn resume_chromium_tree_fetch_outcome( - &mut self, - vm: &mut ScriptVm, - client: module_tree::SingleModuleClientToken, - outcome: module_tree::ModuleFetchOutcome, - ) -> std::result::Result { - let mut owner = NativeModuleTreeDocumentOwner::new(vm); - self.resume_chromium_tree_fetch_outcome_with_owner(&mut owner, client, outcome) - } - pub(crate) fn resume_chromium_tree_fetch_outcome_with_owner( &mut self, owner: &mut O, diff --git a/moli-renderer-v8/src/script_vm/native_module.rs b/moli-renderer-v8/src/script_vm/native_module.rs index 953d86bd53..f20dd3a2ae 100644 --- a/moli-renderer-v8/src/script_vm/native_module.rs +++ b/moli-renderer-v8/src/script_vm/native_module.rs @@ -5076,8 +5076,8 @@ mod tests { DynamicModuleFetchFailure, DynamicModuleFetchOwnerAdvance, DynamicModuleImportOwner, ModuleAttributesKey, ModuleEntryId, ModuleFetchMetadata, ModuleGraphFetchedSource, ModuleGraphHandle, ModuleImportPhase, ModuleKind, ModuleLoadError, ModuleLoadStage, - ModuleMapKey, ModuleSource, NativeModuleGraphFetchRequest, NativeModuleGraphJob, - NativeModuleGraphJobAdvance, PendingDynamicModuleImport, + ModuleMapEntryState, ModuleMapKey, ModuleSource, NativeModuleGraphFetchRequest, + NativeModuleGraphJob, NativeModuleGraphJobAdvance, PendingDynamicModuleImport, }; use crate::module_script_continuation::NativeDynamicModuleTerminalFanout; use crate::network::ResourceRequestClient; @@ -5470,6 +5470,97 @@ mod tests { ); } + #[test] + fn dynamic_import_fetch_failures_reject_joined_and_cached_imports() { + for shared_dependency in [false, true] { + let mut vm = new_test_vm("https://app.example.test/page.html"); + let second_specifier = if shared_dependency { + "./second.mjs" + } else { + "./first.mjs" + }; + vm.eval(&format!( + r#" +globalThis.__fetchErrors = []; +globalThis.__failedImport = specifier => import(specifier).then( + () => __fetchErrors.push(null), + error => __fetchErrors.push(error) +); +__failedImport('./first.mjs'); +__failedImport({second_specifier:?}); +"queued" +"# + )) + .expect("both imports should enqueue their graph jobs"); + for _ in 0..2 { + assert!(matches!( + vm.run_next_native_dynamic_module_owner_action_selected_task_body(), + MainNativeModuleSelectedTaskApplication::Applied(_) + )); + } + + let (failed_load_id, failed_url) = if shared_dependency { + for (load_id, root) in [(0, "first"), (1, "second")] { + let target = vm + .current_main_dynamic_import_graph_fetch_target(load_id) + .expect("each root should have its own fetch"); + let completion = dynamic_import_completion_with_source( + load_id, + &format!("https://app.example.test/{root}.mjs"), + "import './shared.mjs';", + ); + vm.complete_current_main_dynamic_import_graph_fetch_result( + target, + completion.result, + ) + .expect("the roots should start or join the shared dependency fetch"); + } + (2, "https://app.example.test/shared.mjs") + } else { + (0, "https://app.example.test/first.mjs") + }; + assert_eq!(vm.eval("String(__fetchErrors.length)").unwrap(), "0"); + let target = vm + .current_main_dynamic_import_graph_fetch_target(failed_load_id) + .expect("the shared request should retain its fetch owner"); + vm.complete_current_main_dynamic_import_graph_fetch_result( + target, + Err("HTTP 404".to_owned()), + ) + .expect("fetch failure should reject both graph clients"); + + let key = ModuleMapKey::java_script(url::Url::parse(failed_url).unwrap()); + let entry = vm.document_runtime.native_module_entry_id(&key).unwrap(); + assert_eq!( + vm.document_runtime.native_module_entry_state(entry), + ModuleMapEntryState::Failed, + "a failed request must settle the shared module map entry" + ); + assert_eq!( + vm.eval("JSON.stringify([__fetchErrors.length, __fetchErrors.every(e => e instanceof TypeError), new Set(__fetchErrors).size])") + .unwrap(), + "[2,true,2]", + "both concurrent imports must reject with distinct TypeErrors" + ); + + vm.eval("__failedImport('./first.mjs'); 'queued'").unwrap(); + assert!(matches!( + vm.run_next_native_dynamic_module_owner_action_selected_task_body(), + MainNativeModuleSelectedTaskApplication::Applied(_) + )); + // The selected body leaves Promise reactions to its task-end checkpoint. + vm.perform_script_task_checkpoint(None) + .expect("the cached import task should dispatch its rejection reaction"); + assert_eq!( + vm.eval("JSON.stringify([__fetchErrors.length, __fetchErrors.every(e => e instanceof TypeError), new Set(__fetchErrors).size])") + .unwrap(), + "[3,true,3]", + "a cached fetch failure must reject a later import with a fresh TypeError" + ); + assert!(!vm.has_inflight_dynamic_module_fetch()); + } + } + #[test] fn module_reaction_source_consumes_exactly_one_current_target_per_turn() { let mut vm = new_test_vm("https://module-reaction-one-turn.test/page.html");