1968890 - Update error handling for dynamic import in various script loaders

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

This is a follow-up for https://phabricator.services.mozilla.com/D228860

When a dynamic import failed, the script loaders call OnFetchComplete(errorCode) and then CancelDynamicImport()
This is due to the legacy behavior to call FinishDynamicModuleImport.

We could do the error handling in OnFetchComplete, so the error handling code can be shared.
This also requires changing DOM ScriptLoader and Worker ScriptLoader, so I'm moving this change to a new bug.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1968890 - Part 2: Handle dynamic import in OnFetchFailed.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1968890 - Part 3: Call ModuleErrored in OnFetchFailed.

Update the state of the ModuleLoadRequest in OnFetchFailed.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1968890 - Part 4: Handle Dynamic import in LoadRequestedModulesRejected.

Reject the dynamic import when the LoadRequestedModules fails.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1968890 - Part 5: Add ModuleLoadRequest::SetErroredLoadingImports().

To indicate the loading error for a top-level of the module from the dynamic import
is successfully fetched and parsed, but one of its dependencies fails to fetch
or compile.

//Example
import("a.mjs");

// a.mjs
import "./404.mjs";

This patch allows the ModuleLoadRequest::IsErrored() for "a.mjs" returns
true.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1968890 - Part 6: ScriptLoaders update.

OnFetchComplete will do the error handling and update the state, so we
don't need to call Cancel nor LoadFailed.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1968890 - Part 7: Add Cancel method.

Triggers FinishLoadingImportedModule in Cancel() to prevent memory leak.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1968890 - Part 8: Remove FinishDynamicImportAndReject.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1968890 - Part 9: Add test for dynamic import.

This test checks that the TypeError from fetching the sub-modules will
be forwarded to the reject handler of the promise of the dynamic import.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1968890 - Part 1: Dynamic import is not a top-level request.

According to the spec, requests created in HostLoadImportedModule have
the isTopLevel flag set to false. This means both static and dynamic
import requests should also have this flag set to false.

HostLoadImportedModule
Step 14. Fetch a single imported module script ...

Fetch a single imported module script
Step 4. Fetch a single module script given url, fetchClient, destination, options, settingsObject, referrer, moduleRequest, false, and onComplete.

The false argument in the second-to-last parameter of 'Fetch a single
module script' corresponds to the top-levl flag.

Fetch a single module script
given a URL url, an environment settings object fetchClient, a
destination destination, a script fetch options options, an environment
settings object settingsObject, a referrer referrer, an optional
ModuleRequest Record moduleRequest, a boolean isTopLevel, an algorithm
onComplete,

The spec was updated in https://github.com/whatwg/html/issues/7996. We’re
updating the implementation to reflect this change.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2f3764f5f653
https://hg.mozilla.org/integration/autoland/rev/2f9ab5e705d3
Part 1: Dynamic import is not a top-level request. r=jonco,dom-worker-reviewers,edenchuang
https://github.com/mozilla-firefox/firefox/commit/4846001155e8
https://hg.mozilla.org/integration/autoland/rev/06ec60266948
Part 2: Handle dynamic import in OnFetchFailed. r=jonco
https://github.com/mozilla-firefox/firefox/commit/3ef1571f6c31
https://hg.mozilla.org/integration/autoland/rev/ab23423c7353
Part 3: Call ModuleErrored in OnFetchFailed. r=jonco
https://github.com/mozilla-firefox/firefox/commit/59b34ef09eda
https://hg.mozilla.org/integration/autoland/rev/42da5cac3411
Part 4: Handle Dynamic import in LoadRequestedModulesRejected. r=jonco
https://github.com/mozilla-firefox/firefox/commit/d3e0ec0a70a8
https://hg.mozilla.org/integration/autoland/rev/c48ad530cfe2
Part 5: Add ModuleLoadRequest::SetErroredLoadingImports(). r=jonco
https://github.com/mozilla-firefox/firefox/commit/38def54b95ab
https://hg.mozilla.org/integration/autoland/rev/75ae330e1b90
Part 6: ScriptLoaders update. r=jonco,dom-worker-reviewers,edenchuang
https://github.com/mozilla-firefox/firefox/commit/6af0a75aa410
https://hg.mozilla.org/integration/autoland/rev/1b987903eab3
Part 7: Add Cancel method. r=jonco
https://github.com/mozilla-firefox/firefox/commit/111e590ac7b2
https://hg.mozilla.org/integration/autoland/rev/cc48926e1974
Part 8: Remove FinishDynamicImportAndReject. r=jonco
https://github.com/mozilla-firefox/firefox/commit/612601492356
https://hg.mozilla.org/integration/autoland/rev/77947c04dcbe
Part 9: Add test for dynamic import. r=jonco

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ea0e5ede0e2b
https://hg.mozilla.org/integration/autoland/rev/cc6bc73fbbfd
Revert "Bug 1968890 - Part 9: Add test for dynamic import. r=jonco" for causing multiple wpt failures

Comment 13

[amarc]

Backed out for causing multiple wpt failures

• Backout link
• Push with failures
• Failure Log
• Failure Log 2

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to amarc from comment #13)

Backed out for causing multiple wpt failures

• Backout link
• Push with failures
• Failure Log
• Failure Log 2

I'll fix it in bug 1990764

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/43e95dfc628c
https://hg.mozilla.org/integration/autoland/rev/88588284e643
Part 1: Dynamic import is not a top-level request. r=jonco,dom-worker-reviewers,edenchuang
https://github.com/mozilla-firefox/firefox/commit/4396212837fa
https://hg.mozilla.org/integration/autoland/rev/70130eac1346
Part 2: Handle dynamic import in OnFetchFailed. r=jonco
https://github.com/mozilla-firefox/firefox/commit/33105bfe8d48
https://hg.mozilla.org/integration/autoland/rev/a20ad923b1f8
Part 3: Call ModuleErrored in OnFetchFailed. r=jonco
https://github.com/mozilla-firefox/firefox/commit/3837c393b5a4
https://hg.mozilla.org/integration/autoland/rev/52a87b60e974
Part 4: Handle Dynamic import in LoadRequestedModulesRejected. r=jonco
https://github.com/mozilla-firefox/firefox/commit/36f140b26bc9
https://hg.mozilla.org/integration/autoland/rev/c6c863899bf9
Part 5: Add ModuleLoadRequest::SetErroredLoadingImports(). r=jonco
https://github.com/mozilla-firefox/firefox/commit/0bd0adafd831
https://hg.mozilla.org/integration/autoland/rev/9e2a3327cd0f
Part 6: ScriptLoaders update. r=jonco,dom-worker-reviewers,edenchuang
https://github.com/mozilla-firefox/firefox/commit/4efb29fcff60
https://hg.mozilla.org/integration/autoland/rev/02269004c755
Part 7: Add Cancel method. r=jonco
https://github.com/mozilla-firefox/firefox/commit/0c71c535fce5
https://hg.mozilla.org/integration/autoland/rev/fe82822fa4a1
Part 8: Remove FinishDynamicImportAndReject. r=jonco
https://github.com/mozilla-firefox/firefox/commit/c6e5d9e61cd0
https://hg.mozilla.org/integration/autoland/rev/587cbe9d2aaa
Part 9: Add test for dynamic import. r=jonco

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/88588284e643
https://hg.mozilla.org/mozilla-central/rev/70130eac1346
https://hg.mozilla.org/mozilla-central/rev/a20ad923b1f8
https://hg.mozilla.org/mozilla-central/rev/52a87b60e974
https://hg.mozilla.org/mozilla-central/rev/c6c863899bf9
https://hg.mozilla.org/mozilla-central/rev/9e2a3327cd0f
https://hg.mozilla.org/mozilla-central/rev/02269004c755
https://hg.mozilla.org/mozilla-central/rev/fe82822fa4a1
https://hg.mozilla.org/mozilla-central/rev/587cbe9d2aaa

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/17032d74be76
https://hg.mozilla.org/integration/autoland/rev/e47abb2b8ac7
Revert "Bug 1968890 - Part 9: Add test for dynamic import. r=jonco" as potential cause of javascript engine crashes (bug 1991268)

Comment 18

[amarc]

Backed out as potential cause of javascript engine crashes (bug 1991268)

• Backout link

Comment 19

[Cristian Tuns]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/e47abb2b8ac7

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I think this is caused by an ongoing dynamic import, but the document is destroyed so the dynamic import request is canceled.
And when the reject handler is called, the mPayload field has been cleared and triggered the crash.

However, I can't reproduce that crash with a test, when the PromiseJobRunnable is about to execute, its global is dying so it doesn't execute at all.

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1968890 - Part 10: Check if the request has been canceled in reject handler.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/982b4fcd355e
https://hg.mozilla.org/integration/autoland/rev/f3f57e90febf
Part 1: Dynamic import is not a top-level request. r=jonco,dom-worker-reviewers,edenchuang
https://github.com/mozilla-firefox/firefox/commit/01ff6257fd03
https://hg.mozilla.org/integration/autoland/rev/1fe3698ab5d1
Part 2: Handle dynamic import in OnFetchFailed. r=jonco
https://github.com/mozilla-firefox/firefox/commit/c4ba35eab1ff
https://hg.mozilla.org/integration/autoland/rev/360f8b5e412b
Part 3: Call ModuleErrored in OnFetchFailed. r=jonco
https://github.com/mozilla-firefox/firefox/commit/548979d573fa
https://hg.mozilla.org/integration/autoland/rev/e2701bd43ce2
Part 4: Handle Dynamic import in LoadRequestedModulesRejected. r=jonco
https://github.com/mozilla-firefox/firefox/commit/1e5ddf19cf0d
https://hg.mozilla.org/integration/autoland/rev/584beaae1d20
Part 5: Add ModuleLoadRequest::SetErroredLoadingImports(). r=jonco
https://github.com/mozilla-firefox/firefox/commit/8714caa9e3eb
https://hg.mozilla.org/integration/autoland/rev/f06186536887
Part 6: ScriptLoaders update. r=jonco,dom-worker-reviewers,edenchuang
https://github.com/mozilla-firefox/firefox/commit/8b743b6a79d5
https://hg.mozilla.org/integration/autoland/rev/f0d873f64979
Part 7: Add Cancel method. r=jonco
https://github.com/mozilla-firefox/firefox/commit/08904835b890
https://hg.mozilla.org/integration/autoland/rev/a80062ebc58c
Part 8: Remove FinishDynamicImportAndReject. r=jonco
https://github.com/mozilla-firefox/firefox/commit/aab59189a2c2
https://hg.mozilla.org/integration/autoland/rev/b83b3c1baa19
Part 9: Add test for dynamic import. r=jonco
https://github.com/mozilla-firefox/firefox/commit/2dbf29e59b81
https://hg.mozilla.org/integration/autoland/rev/6c40df111eaf
Part 10: Check if the request has been canceled in reject handler. r=jonco

Comment 23

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/f3f57e90febf
https://hg.mozilla.org/mozilla-central/rev/1fe3698ab5d1
https://hg.mozilla.org/mozilla-central/rev/360f8b5e412b
https://hg.mozilla.org/mozilla-central/rev/e2701bd43ce2
https://hg.mozilla.org/mozilla-central/rev/584beaae1d20
https://hg.mozilla.org/mozilla-central/rev/f06186536887
https://hg.mozilla.org/mozilla-central/rev/f0d873f64979
https://hg.mozilla.org/mozilla-central/rev/a80062ebc58c
https://hg.mozilla.org/mozilla-central/rev/b83b3c1baa19
https://hg.mozilla.org/mozilla-central/rev/6c40df111eaf