corysanin/tubestation
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bug 1894631 - Add CancelingImport state in ScriptLoadRequest::State. r=jonco

Browse Source 
In the following module graph:

0.html
    +---- 1.mjs
        +---- 2.mjs (modulepreload)
            +---- 3.mjs (modulepreload)
            +---- 4.mjs
        +---- non_existing.mjs

Fetching non_existing.mjs will fail, which will notify its parent module
1.mjs with ModuleErrored(), and then 1.mjs will cancel its imports.

The sequence is as follows:
1. 1.mjs cancels 2.mjs, 2.mjs changes to Cancel state
2. 2.mjs cancels 3.mjs, 3.mjs is already preloaded, its state still remains Finished.
3. 2.mjs cancels 4.mjs, 4.mjs changes to Cancel state.

Now, 2.mjs will call ChildLoadComplete[1], which will call 1.mjs
ModuleErrored()[2] again (The 1st time is called when loading non_existing.mjs failed)

Now 1.mjs wants to cancel 2.mjs again, and 2.mjs has been canceled
previously, so it will do AssertAllImportsCanceled() check[3].

However, 3.mjs has been fetched by <modulepreload> and is in Finished
state, which triggers the assertion failure.

To fix this, I add a new state in ScriptLoadRequest::State called
CancelingImport, to fix the problem the CancelImport() call is called by
non_existing.mjs and 2.mjs.

[1]: https://searchfox.org/mozilla-central/rev/f1532761de0b60337e42c6c3f525288a523dabef/js/loader/ModuleLoadRequest.cpp#100
[2]: https://searchfox.org/mozilla-central/rev/f1532761de0b60337e42c6c3f525288a523dabef/js/loader/ModuleLoaderBase.cpp#954
[3]: https://searchfox.org/mozilla-central/rev/f1532761de0b60337e42c6c3f525288a523dabef/js/loader/ModuleLoadRequest.cpp#86

Differential Revision: https://phabricator.services.mozilla.com/D209218
This commit is contained in:
Yoshi Cheng-Hao Huang
2024-05-07 16:03:20 +00:00
parent 3c82f6e2c6
commit d48c09dbea
8 changed files with 56 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
dom/base/test/jsmodules/importmaps/bug_1894631_module_1.mjs Normal file
View File

@@ -0,0 +1,5 @@
/* eslint-disable import/no-named-default, import/no-unresolved, import/named */
import { default as default_2 } from "./bug_1894631_module_2.mjs";
import { default as default_non } from "./non_existing.mjs";
module1_loaded = true;

3
dom/base/test/jsmodules/importmaps/bug_1894631_module_2.mjs Normal file
View File

@@ -0,0 +1,3 @@
/* eslint-disable import/no-named-default */
import { default as default_3 } from "./bug_1894631_module_3.mjs";
import { default as default_4 } from "./bug_1894631_module_4.mjs";

1
dom/base/test/jsmodules/importmaps/bug_1894631_module_3.mjs Normal file
View File

@@ -0,0 +1 @@
export default 3;

1
dom/base/test/jsmodules/importmaps/bug_1894631_module_4.mjs Normal file
View File

@@ -0,0 +1 @@
export default 4;

5
dom/base/test/jsmodules/importmaps/mochitest.toml
View File

@@ -6,6 +6,10 @@ support-files = [
"bug_1893164_module_1.mjs",
"bug_1893164_module_2.mjs",
"bug_1893164_module_3.mjs",
"bug_1894631_module_1.mjs",
"bug_1894631_module_2.mjs",
"bug_1894631_module_3.mjs",
"bug_1894631_module_4.mjs",
"classic_script.js",
"module_chain_1.mjs",
"module_chain_2.mjs",
@@ -43,3 +47,4 @@ support-files = [
["test_importMap_with_nonexisting_module.html"]
["test_dynamic_importMap_with_external_script.html"]
["test_dynamic_importMap_load_completes.html"]
["test_shared_submodules_with_modulepreload.html"]

30
dom/base/test/jsmodules/importmaps/test_shared_submodules_with_modulepreload.html Normal file
View File

@@ -0,0 +1,30 @@
<!DOCTYPE html>
<meta charset=utf-8>
<title>Test module cancel won't trigger an assert</title>
<script src="/tests/SimpleTest/SimpleTest.js"></script>
<link rel="modulepreload" href="./bug_1894631_module_2.mjs" />
<link rel="modulepreload" href="./bug_1894631_module_3.mjs" />
<link rel="modulepreload" href="./non_existing.mjs" />
<script src="./bug_1894631_module_1.mjs" type="module" id="module_1"></script>
<script>
var module1_loaded = false;
var module1_error = false;
SimpleTest.waitForExplicitFinish();
const module1 = document.getElementById("module_1");
module1.addEventListener("error", (event) => {
info("error event");
module1_error = true;
});
function testLoaded() {
ok(module1_error, "module_1.mjs should fire an error event");
ok(!module1_loaded, "module_1.mjs should not be loaded");
SimpleTest.finish();
}
</script>
<body onload='testLoaded()'>
</body>

10
js/loader/ModuleLoadRequest.cpp
View File

@@ -160,7 +160,7 @@ void ModuleLoadRequest::ModuleErrored() {
LOG(("ScriptLoadRequest (%p): Module errored", this));
if (IsCanceled()) {
if (IsCanceled() || IsCancelingImports()) {
return;
}
@@ -222,6 +222,12 @@ void ModuleLoadRequest::CheckModuleDependenciesLoaded() {
}
void ModuleLoadRequest::CancelImports() {
State origState = mState;
// To prevent reentering ModuleErrored() for this request via mImports[i]'s
// ChildLoadComplete().
mState = State::CancelingImports;
for (size_t i = 0; i < mImports.Length(); i++) {
if (mLoader->IsFetchingAndHasWaitingRequest(mImports[i])) {
LOG(("CancelImports import %p is fetching and has waiting\n",
@@ -230,6 +236,8 @@ void ModuleLoadRequest::CancelImports() {
}
mImports[i]->Cancel();
}
mState = origState;
}
void ModuleLoadRequest::LoadFinished() {

2
js/loader/ScriptLoadRequest.h
View File

@@ -124,6 +124,7 @@ class ScriptLoadRequest : public nsISupports,
Fetching,
Compiling,
LoadingImports,
CancelingImports,
Ready,
Canceled
};
@@ -139,6 +140,7 @@ class ScriptLoadRequest : public nsISupports,
bool IsFetching() const { return mState == State::Fetching; }
bool IsCompiling() const { return mState == State::Compiling; }
bool IsLoadingImports() const { return mState == State::LoadingImports; }
bool IsCancelingImports() const { return mState == State::CancelingImports; }
bool IsCanceled() const { return mState == State::Canceled; }
bool IsPendingFetchingError() const {