1980126 - Refactor and tidy up the modules implementation following the loader refactoring

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Jon Coppeard (:jonco)]

Following on from landing the module loader (spec) refactor in bug 1820594, there are a bunch of things we can do to tidy up the code.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1980126 - Part 1: Make LoadModuleResolved/RejectedCallbacks into function pointers r?allstarschh

We usually use function pointers for callbacks into the JS engine.
These are simpler to deal with and also std::function can alocate memory in
some situations which is undesirable.

The patch converts these to function pointers and also refactors
ModuleLoaderBase.cpp where these are defined for the main module loader.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1980126 - Part 2: Make GraphLoadingStateRecordObject background finalized and refactor r?allstarschh

The finalizer for this only frees memory so it doesn't have to be foreground finalized.

The rest of the patch simplifies object creation a little.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1980126 - Part 3: Move dynamic module code to vm/Modules.cpp from builtin/ModuleObject.cpp r?allstarschh

This moves the code that deal with dynamic import from
builting/ModuleObject.cpp to vm/Modules.cpp to be with the rest of the modules
implementation where it belongs.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1980126 - Part 4: Unify JS::FinishLoadingImportedModule API r?allstarschh

This merges the two versions of FinishLoadingImportedModule so there's a single
API that takes a payload rather than separate ones for promise/statePrivate.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1980126 - Part 5: Unify JS::FinishLoadingImportedModuleFailed APIs r?allstarschh

Similar to the previous patch, this makes the FinishLoadingImportedModuleFailed and
FinishLoadingImportedModuleFailedWithPendingException APIs take a single
payload argument.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1980126 - Part 6: Refactor error handling in module load hook implementation r?allstarschh

We use ScopeExit here to ensure we call FinishLoadingImportedModuleFailed on
error. However sometimes we call this directly and then exit without releasing
the guard and so call it again.

The patch cleans this up and makes the guard report the pending exception if
set but still report an exception otherwise. (In future we might think about
insisting that an exception is set if we leave early.)

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1980126 - Part 7: Combine internal FinishLoadingImportedModule failure paths r?allstarschh

This makes it so FinishLoadingImportedModuleFailedWithPendingException gets the
current exception and calls FinishLoadingImportedModuleFailed.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Bug 1980126 - Part 8: Add missing steps for FinishLoadingImportedModule for dynamic import r?allstarschh

This moves some steps related to updating referrer.loadedModules from the
statePrivate path into the common path also used for dynamic imports.

There's still a slight spec difference because we only support module referrers
in our implementation and not script or realm referrers. However we don't ever
use loadedModules for anything except assertions for those referrer kinds.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Bug 1980126 - Part 9: Update test expectations r?allstarschh

Following the previous patch a bunch of tests started passing. This also
updates a few tests that as far as I can tell were always passing evening
though they were marked as possibly failing.

Comment 10

[Jon Coppeard (:jonco)]

Attachment: Bug 1980126 - Part 10: Pass a single payload argument to HostLoadImportedModule callback r?allstarschh

Previously this passed separate statePrivate and promise arguments, only one of
which was used at a time.

Comment 11

[Jon Coppeard (:jonco)]

Attachment: Bug 1980126 - Part 11: Remove unused data from dynamic import context r?allstarschh

The moduleRequest field of DynamicImportContextObject is never used for
anything so can be removed.

I added some spec steps around this code too.

Comment 12

[Jon Coppeard (:jonco)]

Attachment: Bug 1980126 - Part 12: Tidy error handling in StartDynamicModuleImport r?allstarschh

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/20ca6df255f8
https://hg.mozilla.org/integration/autoland/rev/7cc93a61dcf9
Part 1: Make LoadModuleResolved/RejectedCallbacks into function pointers r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/bec492ffcdfb
https://hg.mozilla.org/integration/autoland/rev/dc050749ef8c
Part 2: Make GraphLoadingStateRecordObject background finalized and refactor r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/e7d409880ffa
https://hg.mozilla.org/integration/autoland/rev/82da124b774a
Part 3: Move dynamic module code to vm/Modules.cpp from builtin/ModuleObject.cpp r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/8de76d7f723e
https://hg.mozilla.org/integration/autoland/rev/a88003d84e39
Part 4: Unify JS::FinishLoadingImportedModule API r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/dff70a55ad87
https://hg.mozilla.org/integration/autoland/rev/e87aae5e5702
Part 5: Unify JS::FinishLoadingImportedModuleFailed APIs r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/47cd46fb6bd0
https://hg.mozilla.org/integration/autoland/rev/e8f85784d986
Part 6: Refactor error handling in module load hook implementation r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/1a807053818d
https://hg.mozilla.org/integration/autoland/rev/55b4afc09f65
Part 7: Combine internal FinishLoadingImportedModule failure paths r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/a904cdf9b889
https://hg.mozilla.org/integration/autoland/rev/5648b19b8155
Part 8: Add missing steps for FinishLoadingImportedModule for dynamic import r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/eee20ec5f411
https://hg.mozilla.org/integration/autoland/rev/b710df1831e1
Part 9: Update test expectations r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/d90f9d66748a
https://hg.mozilla.org/integration/autoland/rev/ab86f64d7537
Part 10: Pass a single payload argument to HostLoadImportedModule callback r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/e9878305e68d
https://hg.mozilla.org/integration/autoland/rev/205405c7409c
Part 11: Remove unused data from dynamic import context r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/8a67a2ece6fd
https://hg.mozilla.org/integration/autoland/rev/97c1e6d6cbd7
Part 12: Tidy error handling in StartDynamicModuleImport r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/3e05c6426825
https://hg.mozilla.org/integration/autoland/rev/a608780329b8
apply code formatting via Lando

Comment 14

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/7cc93a61dcf9
https://hg.mozilla.org/mozilla-central/rev/dc050749ef8c
https://hg.mozilla.org/mozilla-central/rev/82da124b774a
https://hg.mozilla.org/mozilla-central/rev/a88003d84e39
https://hg.mozilla.org/mozilla-central/rev/e87aae5e5702
https://hg.mozilla.org/mozilla-central/rev/e8f85784d986
https://hg.mozilla.org/mozilla-central/rev/55b4afc09f65
https://hg.mozilla.org/mozilla-central/rev/5648b19b8155
https://hg.mozilla.org/mozilla-central/rev/b710df1831e1
https://hg.mozilla.org/mozilla-central/rev/ab86f64d7537
https://hg.mozilla.org/mozilla-central/rev/205405c7409c
https://hg.mozilla.org/mozilla-central/rev/97c1e6d6cbd7
https://hg.mozilla.org/mozilla-central/rev/a608780329b8

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2270145ce0e4
https://hg.mozilla.org/integration/autoland/rev/e99ab13be959
Revert "Bug 1980126: apply code formatting via Lando" for causing wpt failures on before-prepare-iframe-success-external-module.html

Comment 16

[Cristina Horotan [:chorotan]]

Backed out for causing wpt failures on before-prepare-iframe-success-external-module.html

Backout link

Push with failures

Failure log

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/07264939b1b9
https://hg.mozilla.org/integration/autoland/rev/833105cacc8d
Part 1: Make LoadModuleResolved/RejectedCallbacks into function pointers r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/4379e049dddf
https://hg.mozilla.org/integration/autoland/rev/1aa8baaa297f
Part 2: Make GraphLoadingStateRecordObject background finalized and refactor r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/c47ad1fc40d7
https://hg.mozilla.org/integration/autoland/rev/195d7b536123
Part 3: Move dynamic module code to vm/Modules.cpp from builtin/ModuleObject.cpp r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/3f057094f7e5
https://hg.mozilla.org/integration/autoland/rev/8ef0f18dddeb
Part 4: Unify JS::FinishLoadingImportedModule API r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/b1a2c70f8536
https://hg.mozilla.org/integration/autoland/rev/5e152ff1e0c5
Part 5: Unify JS::FinishLoadingImportedModuleFailed APIs r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/484bfe7dbcfb
https://hg.mozilla.org/integration/autoland/rev/284683a40c99
Part 6: Refactor error handling in module load hook implementation r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/6094f777fb08
https://hg.mozilla.org/integration/autoland/rev/b770eb79da82
Part 7: Combine internal FinishLoadingImportedModule failure paths r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/012e0cf4f3a8
https://hg.mozilla.org/integration/autoland/rev/a94d628f4895
Part 8: Add missing steps for FinishLoadingImportedModule for dynamic import r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/43cc3a8a5a35
https://hg.mozilla.org/integration/autoland/rev/686dd16d5066
Part 9: Update test expectations r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/428c5600743c
https://hg.mozilla.org/integration/autoland/rev/aa9be607dad3
Part 10: Pass a single payload argument to HostLoadImportedModule callback r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/717acc9e207e
https://hg.mozilla.org/integration/autoland/rev/9cc36384db92
Part 11: Remove unused data from dynamic import context r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/9d287aad458c
https://hg.mozilla.org/integration/autoland/rev/4aae58151731
Part 12: Tidy error handling in StartDynamicModuleImport r=allstarschh
https://github.com/mozilla-firefox/firefox/commit/4a2ccdd31f2d
https://hg.mozilla.org/integration/autoland/rev/1ec8400efa8a
apply code formatting via Lando

Comment 18

[[e-mail]]

Backout merge into mozilla-central

Comment 19

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/833105cacc8d
https://hg.mozilla.org/mozilla-central/rev/1aa8baaa297f
https://hg.mozilla.org/mozilla-central/rev/195d7b536123
https://hg.mozilla.org/mozilla-central/rev/8ef0f18dddeb
https://hg.mozilla.org/mozilla-central/rev/5e152ff1e0c5
https://hg.mozilla.org/mozilla-central/rev/284683a40c99
https://hg.mozilla.org/mozilla-central/rev/b770eb79da82
https://hg.mozilla.org/mozilla-central/rev/a94d628f4895
https://hg.mozilla.org/mozilla-central/rev/686dd16d5066
https://hg.mozilla.org/mozilla-central/rev/aa9be607dad3
https://hg.mozilla.org/mozilla-central/rev/9cc36384db92
https://hg.mozilla.org/mozilla-central/rev/4aae58151731
https://hg.mozilla.org/mozilla-central/rev/1ec8400efa8a