From a7221fd74c488251fa0a6dd406b974dd2aec0524 Mon Sep 17 00:00:00 2001 From: CYBAI Date: Mon, 22 Jun 2020 23:25:44 +0900 Subject: [PATCH 1/2] Rollback to recursively check dependency status In https://github.com/servo/servo/pull/26395/files#diff-3fe97584f564214ec8e7ebbf91747e03L253-R318, we moved from `recursive checking` of dependency status to check only the _current module_'s dependency status and its descendant dependency status and also circular dependency status. However, it will cause an issue. For example, if the module dependency is like following ``` a -> b -> c -> d -> e f -> g -> h -> c -> d -> e ``` In this example, if the d module is still under fetching but g is trying to advance to finish. Then, it will cause a panic because module d is g's grand-grand-grand-descendant which means it's still under fetching and we can't instantiate module g. Ideally, we should get rid of the checking in #26903 so, before #26903 fixed, we can just move back to the recursive checking way which will ensure all descendants are not fetching. --- components/script/script_module.rs | 143 ++++++++--------------------- 1 file changed, 40 insertions(+), 103 deletions(-) diff --git a/components/script/script_module.rs b/components/script/script_module.rs index 185a96c5d86..045c6225337 100644 --- a/components/script/script_module.rs +++ b/components/script/script_module.rs @@ -37,7 +37,7 @@ use crate::task::TaskBox; use crate::task_source::TaskSourceName; use encoding_rs::UTF_8; use hyper_serde::Serde; -use indexmap::{IndexMap, IndexSet}; +use indexmap::IndexSet; use ipc_channel::ipc; use ipc_channel::router::ROUTER; use js::jsapi::Handle as RawHandle; @@ -65,7 +65,7 @@ use net_traits::{FetchMetadata, Metadata}; use net_traits::{FetchResponseListener, NetworkError}; use net_traits::{ResourceFetchTiming, ResourceTimingType}; use servo_url::ServoUrl; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::ffi; use std::rc::Rc; use std::str::FromStr; @@ -257,64 +257,51 @@ impl ModuleTree { self.incomplete_fetch_urls.borrow_mut().remove(&dependency); } - /// Find circular dependencies in non-recursive way - /// - /// This function is basically referred to - /// [this blog post](https://breakingcode.wordpress.com/2013/03/11/an-example-dependency-resolution-algorithm-in-python/). - /// - /// The only difference is, in that blog post, its algorithm will throw errors while finding circular - /// dependencies; however, in our use case, we'd like to find circular dependencies so we will just - /// return it. - pub fn find_circular_dependencies(&self, global: &GlobalScope) -> IndexSet { - let module_map = global.get_module_map().borrow(); + /// recursively checks if all of the transitive descendants are + /// in the FetchingDescendants or later status + fn recursive_check_descendants( + module_tree: &ModuleTree, + module_map: &HashMap>, + discovered_urls: &mut HashSet, + ) -> bool { + discovered_urls.insert(module_tree.url.clone()); - // A map for checking dependencies and using the module url as key - let mut module_deps: IndexMap> = module_map - .iter() - .map(|(module_url, module)| { - (module_url.clone(), module.descendant_urls.borrow().clone()) - }) - .collect(); + let descendant_urls = module_tree.descendant_urls.borrow(); - while module_deps.len() != 0 { - // Get all dependencies with no dependencies - let ready: IndexSet = module_deps - .iter() - .filter_map(|(module_url, descendant_urls)| { - if descendant_urls.len() == 0 { - Some(module_url.clone()) - } else { - None + for descendant_url in descendant_urls.iter() { + match module_map.get(&descendant_url.clone()) { + None => return false, + Some(descendant_module) => { + if discovered_urls.contains(&descendant_module.url) { + continue; } - }) - .collect(); - // If there's no ready module but we're still in the loop, - // it means we find circular modules, then we can return them. - if ready.len() == 0 { - return module_deps - .iter() - .map(|(url, _)| url.clone()) - .collect::>(); - } + let descendant_status = descendant_module.get_status(); + if descendant_status < ModuleStatus::FetchingDescendants { + return false; + } - // Remove ready modules from the dependency map - for module_url in ready.iter() { - module_deps.remove(&module_url.clone()); - } + let all_ready_descendants = ModuleTree::recursive_check_descendants( + &descendant_module, + module_map, + discovered_urls, + ); - // Also make sure to remove the ready modules from the - // remaining module dependencies as well - for (_, deps) in module_deps.iter_mut() { - *deps = deps - .difference(&ready) - .into_iter() - .cloned() - .collect::>(); + if !all_ready_descendants { + return false; + } + }, } } - IndexSet::new() + return true; + } + + fn has_all_ready_descendants(&self, global: &GlobalScope) -> bool { + let module_map = global.get_module_map().borrow(); + let mut discovered_urls = HashSet::new(); + + return ModuleTree::recursive_check_descendants(&self, &module_map, &mut discovered_urls); } // We just leverage the power of Promise to run the task for `finish` the owner. @@ -751,35 +738,8 @@ impl ModuleTree { /// step 4-7. fn advance_finished_and_link(&self, global: &GlobalScope) { { - let descendant_urls = self.descendant_urls.borrow(); - - // Check if there's any dependencies under fetching. - // - // We can't only check `incomplete fetches` here because... - // - // For example, module `A` has descendants `B`, `C` - // while `A` has added them to incomplete fetches, it's possible - // `B` has finished but `C` is not yet fired its fetch; in this case, - // `incomplete fetches` will be `zero` but the module is actually not ready - // to finish. Thus, we need to check dependencies directly instead of - // incomplete fetches here. - if !is_all_dependencies_ready(&descendant_urls, &global) { - // When we found the `incomplete fetches` is bigger than zero, - // we will need to check if there's any circular dependency. - // - // If there's no circular dependencies but there are incomplete fetches, - // it means it needs to wait for finish. - // - // Or, if there are circular dependencies, then we need to confirm - // no circular dependencies are fetching. - // - // if there's any circular dependencies and they all proceeds to status - // higher than `FetchingDescendants`, then it means we can proceed to finish. - let circular_deps = self.find_circular_dependencies(&global); - - if circular_deps.len() == 0 || !is_all_dependencies_ready(&circular_deps, &global) { - return; - } + if !self.has_all_ready_descendants(&global) { + return; } } @@ -838,29 +798,6 @@ impl ModuleTree { } } -// Iterate the given dependency urls to see if it and its descendants are fetching or not. -// When a module status is `FetchingDescendants`, it's possible that the module is a circular -// module so we will also check its descendants. -fn is_all_dependencies_ready(dependencies: &IndexSet, global: &GlobalScope) -> bool { - dependencies.iter().all(|dep| { - let module_map = global.get_module_map().borrow(); - match module_map.get(&dep) { - Some(module) => { - let module_descendants = module.get_descendant_urls().borrow(); - - module.get_status() >= ModuleStatus::FetchingDescendants && - module_descendants.iter().all(|descendant_url| { - match module_map.get(&descendant_url) { - Some(m) => m.get_status() >= ModuleStatus::FetchingDescendants, - None => false, - } - }) - }, - None => false, - } - }) -} - #[derive(JSTraceable, MallocSizeOf)] struct ModuleHandler { #[ignore_malloc_size_of = "Measuring trait objects is hard"] From 0c938b3d7563a95f52f9a9adbe98be56f6c8e24c Mon Sep 17 00:00:00 2001 From: CYBAI Date: Mon, 22 Jun 2020 23:29:40 +0900 Subject: [PATCH 2/2] Test imports under more than 3 levels in different modules --- tests/wpt/metadata/MANIFEST.json | 39 +++++++++++++++++++ .../module/nested-imports-a.js | 1 + .../module/nested-imports-b.js | 2 + .../module/nested-imports-c.js | 2 + .../module/nested-imports-d.js | 3 ++ .../module/nested-imports-e.js | 2 + .../module/nested-imports-f.js | 2 + .../module/nested-imports-g.js | 2 + .../module/nested-imports-h.js | 2 + .../module/nested-imports.html | 29 ++++++++++++++ 10 files changed, 84 insertions(+) create mode 100644 tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-a.js create mode 100644 tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-b.js create mode 100644 tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-c.js create mode 100644 tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-d.js create mode 100644 tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-e.js create mode 100644 tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-f.js create mode 100644 tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-g.js create mode 100644 tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-h.js create mode 100644 tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports.html diff --git a/tests/wpt/metadata/MANIFEST.json b/tests/wpt/metadata/MANIFEST.json index 5d2c1f32822..81ced0c7694 100644 --- a/tests/wpt/metadata/MANIFEST.json +++ b/tests/wpt/metadata/MANIFEST.json @@ -334491,6 +334491,38 @@ "e6f5746eb743a338ad6fbd401715fed368e4cf74", [] ], + "nested-imports-a.js": [ + "a127aeb559a0a4d6eedccae19905088fa9fce4b9", + [] + ], + "nested-imports-b.js": [ + "18a5af40cc0ec3a3c10de50f3cd3bffe22c6ec4d", + [] + ], + "nested-imports-c.js": [ + "ec44596aeae7482656b29f1d6aebf6d2ab6a9d54", + [] + ], + "nested-imports-d.js": [ + "cee87849c6258182d6c0085ab504867c197fb8d4", + [] + ], + "nested-imports-e.js": [ + "ec6f0360a608429f774e430a2658f3235d72ed43", + [] + ], + "nested-imports-f.js": [ + "0591e0b3166907bdf94cff3677c2460f9824e082", + [] + ], + "nested-imports-g.js": [ + "86cbe7d3f8e0c29fd7848d9b2626166a0f6f3d30", + [] + ], + "nested-imports-h.js": [ + "a1612912599a4c79307cffe79b43133ceb7f99b0", + [] + ], "nested-missing-export.js": [ "3801ae847afca704cfac9d99428c96851296b8cb", [] @@ -465114,6 +465146,13 @@ {} ] ], + "nested-imports.html": [ + "23bb595d0ebce1fbe14a3b72ce34fc1efa2720fe", + [ + null, + {} + ] + ], "nomodule-attribute.html": [ "656c99b292ac03f401eead1c4798666de61ca91a", [ diff --git a/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-a.js b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-a.js new file mode 100644 index 00000000000..a127aeb559a --- /dev/null +++ b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-a.js @@ -0,0 +1 @@ +import { b } from "./nested-imports-b.js"; diff --git a/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-b.js b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-b.js new file mode 100644 index 00000000000..18a5af40cc0 --- /dev/null +++ b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-b.js @@ -0,0 +1,2 @@ +import { c } from "./nested-imports-c.js"; +export const b = "b"; diff --git a/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-c.js b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-c.js new file mode 100644 index 00000000000..ec44596aeae --- /dev/null +++ b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-c.js @@ -0,0 +1,2 @@ +import { d } from "./nested-imports-d.js"; +export const c = "c"; diff --git a/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-d.js b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-d.js new file mode 100644 index 00000000000..cee87849c62 --- /dev/null +++ b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-d.js @@ -0,0 +1,3 @@ +import { e } from "./nested-imports-e.js"; +import "./resources/delayed-modulescript.py"; +export const d = "d"; diff --git a/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-e.js b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-e.js new file mode 100644 index 00000000000..ec6f0360a60 --- /dev/null +++ b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-e.js @@ -0,0 +1,2 @@ +import { f } from "./nested-imports-f.js"; +export const e = "e"; diff --git a/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-f.js b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-f.js new file mode 100644 index 00000000000..0591e0b3166 --- /dev/null +++ b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-f.js @@ -0,0 +1,2 @@ +import { g } from "./nested-imports-g.js"; +export const f = "f"; diff --git a/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-g.js b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-g.js new file mode 100644 index 00000000000..86cbe7d3f8e --- /dev/null +++ b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-g.js @@ -0,0 +1,2 @@ +import { h } from "./nested-imports-h.js"; +export const g = "g"; diff --git a/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-h.js b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-h.js new file mode 100644 index 00000000000..a1612912599 --- /dev/null +++ b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports-h.js @@ -0,0 +1,2 @@ +import { c } from "./nested-imports-c.js"; +export const h = "h"; diff --git a/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports.html b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports.html new file mode 100644 index 00000000000..23bb595d0eb --- /dev/null +++ b/tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports.html @@ -0,0 +1,29 @@ + +Test imports under more than 3 levels in different modules + + + + + +