Auto merge of #27034 - CYBAI:fix-nested-modules, r=Manishearth

Fix nested modules while imported under more than 3 levels

This is kind of workaround to fix the issue but #26903 should provide much better solution to remove the checking.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #27029
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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<ServoUrl> {
-        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<ServoUrl, Rc<ModuleTree>>,
+        discovered_urls: &mut HashSet<ServoUrl>,
+    ) -> 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<ServoUrl, IndexSet<ServoUrl>> = module_map
-            .iter()
-            .map(|(module_url, module)| {
-                (module_url.clone(), module.descendant_urls.borrow().clone())
-            })
-            .collect();
-
-        while module_deps.len() != 0 {
-            // Get all dependencies with no dependencies
-            let ready: IndexSet<ServoUrl> = module_deps
-                .iter()
-                .filter_map(|(module_url, descendant_urls)| {
-                    if descendant_urls.len() == 0 {
-                        Some(module_url.clone())
-                    } else {
-                        None
+        let descendant_urls = module_tree.descendant_urls.borrow();
+
+        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::<IndexSet<ServoUrl>>();
-            }
 
-            // Remove ready modules from the dependency map
-            for module_url in ready.iter() {
-                module_deps.remove(&module_url.clone());
-            }
+                    let descendant_status = descendant_module.get_status();
+                    if descendant_status < ModuleStatus::FetchingDescendants {
+                        return false;
+                    }
 
-            // 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::<IndexSet<ServoUrl>>();
+                    let all_ready_descendants = ModuleTree::recursive_check_descendants(
+                        &descendant_module,
+                        module_map,
+                        discovered_urls,
+                    );
+
+                    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<ServoUrl>, 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"]

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",
          [

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";

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";

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";

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";

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";

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";

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";

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";

tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/module/nested-imports.html

@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<title>Test imports under more than 3 levels in different modules</title>
+
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script>
+  setup({ allow_uncaught_exception: true });
+
+  window.log = [];
+
+  const test_load = async_test("should load all modules successfully");
+  window.addEventListener(
+    "load",
+    test_load.step_func_done((ev) => {
+      assert_equals(log.length, 2);
+
+      assert_equals(log[0], 1);
+      assert_equals(log[1], 2);
+    })
+  );
+
+  function unreachable() {
+    log.push("unexpected");
+  }
+</script>
+<script type="module" src="./nested-imports-a.js" onerror="unreachable()"
+    onload="log.push(1)"></script>
+<script type="module" src="./nested-imports-e.js" onerror="unreachable()"
+  onload="log.push(2)"></script>