1877792 - JSON modules browser integration

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Comment 1

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 1: Change HttpBaseChannel.cpp to allow application/json r=mgaudet!,yulia!

Comment 2

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 2: Change the module map key to include both URL and module type r=mgaudet!,yulia!

The module maps (mFetchingModules and mFetchedModules) in
ModuleLoaderBase was previously only keyed by the URL and used the
nsURIHashKey hashtable key class. This is no longer sufficient, and the key
should also contain the module type.

This patch introduces a new hashtable key class called ModuleMapKey and
changes mFetchingModules and mFetchedModules to use the new key type.

To make this a bit easier to review, this first patch only introduces the new
key type and hard-codes the type to Javascript where the key is constructed.
The hard-corded module types will be fixed in later patches.

Comment 3

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 4: Add CompileJsonModule version for SourceText<Utf8Unit> r=mgaudet!,yulia!

Comment 4

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 5: Use AutoReportFrontendContext in CompileJsonModule for error reporting r=dminor!,mgaudet!

We need to change JSONFullParseHandler::reportError (when compiling JSON
modules), so that we create syntax errors with the required metadata attached
(line, column, filename).

Depends on D160386

Comment 5

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 17: Extend CompileFetchedModule to handle JSON modules r=#spidermonkey-reviewers,#dom-worker-reviewers!,#dom-core!

Depends on D204174

Comment 6

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 3: Update module map methods to take ModuleMapKey instead of nsIURI r=#spidermonkey-reviewers

Comment 7

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 18: Update WPT prefs to enable JSON modules r=#spidermonkey-reviewers

Comment 8

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 19: Update WPT test expectations or JSON modules r=#spidermonkey-reviewers

Comment 9

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 20: Fix URL encoding in JSON module WPT tests r=#spidermonkey-reviewers

We need to encode content_type by calling encodeURIComponent. Otherwise,
the plus sign is treated as a space.

Comment 10

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 6: Change ModuleLoaderBase::ResolveRequestedModules to handle URL and module type r=#spidermonkey-reviewers

This patch is a small step towards the end goal, so we hardcoded the module
type to JS::ModuleType::JavaScript, but that will get changed in a later
patch.

Comment 11

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 7: Add ModuleLoadRequest::mModuleType r=#spidermonkey-reviewers!,#dom-worker-reviewers!,#dom-core

This patch gets rid of most hard-coded JS::ModuleType::JavaScript in
js/loader/ModuleLoaderBase.cpp. However, the module type is still hard-coded
when constructing the ModuleLoadRequest (will be addressed in a later patch).

Comment 12

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 8: Add module type to ModuleLoadRequest::NewVisitedSetForTopLevelImport r=#spidermonkey-reviewers!,#dom-worker-reviewers!,#dom-core!

This patch adds the new parameter to the NewVisitedSetForTopLevelImport
method, but we hard-code the module type (which will be addressed in a later
patch)

Comment 13

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 9: Add module type to ModuleLoaderBase::{CreateStaticImport,CreateDynamicImport} r=#spidermonkey-reviewers!,#dom-worker-reviewers!,#dom-core!

This patch adds a module type parameter to
ModuleLoaderBase::{CreateStaticImport,CreateDynamicImport}, and gets rid of a
bunch of hard-coded JS::ModuleType::JavaScript. However, the module type is
still hard-coded when we call CreateDynamicImport and CreateStaticImport.

Comment 14

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 10: Get the module type from the module request r=#spidermonkey-reviewers

Comment 15

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 11: Only allow expected mime types r=#dom-worker-reviewers!,#dom-core!

Comment 16

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 12: Add JS::IsCyclicModule r=#spidermonkey-reviewers

Comment 17

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 13: Skip setting/clearing the module private for synthetic modules r=#spidermonkey-reviewers

Synthetic modules do not have a module private.

Comment 18

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 14: Return early in ModuleLoaderBase::ResolveRequestedModules for synthetic modules r=#spidermonkey-reviewers

Synthetic modules do not have any dependencies, so calling
GetRequestedModulesCount results in cash. This patch fixes this by making
ResolveRequestedModules return early for synthetic modules.

Comment 19

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 15: Skip ExposeScriptToDebugger/UpdateDebugMetadata for synthetic modules r=#spidermonkey-reviewers

---

Sync does not have GetModuleScript

Comment 20

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1877792 - Part 16: Disable off-thread compilaton for JSON modules r=#spidermonkey-reviewers

This is something we may want to investigate and implement, but it felt out of
scope for the initial implementation.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c7182604e4f
Part 1: Change HttpBaseChannel.cpp to allow application/json r=yulia,necko-reviewers,jesup,valentin
https://hg.mozilla.org/integration/autoland/rev/82a58a80f58a
Part 2: Change the module map key to include both URL and module type r=yulia,jonco
https://hg.mozilla.org/integration/autoland/rev/d73a9f57f03c
Part 3: Update module map methods to take ModuleMapKey instead of nsIURI r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/3cead4149e6b
Part 4: Add CompileJsonModule version for SourceText<Utf8Unit> r=mgaudet,yulia
https://hg.mozilla.org/integration/autoland/rev/5b1e29e4c597
Part 5: Use AutoReportFrontendContext in CompileJsonModule for error reporting r=bthrall
https://hg.mozilla.org/integration/autoland/rev/bd23c96418b6
Part 6: Change ModuleLoaderBase::ResolveRequestedModules to handle URL and module type r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/0674cf646120
Part 7: Add ModuleLoadRequest::mModuleType r=dom-core,spidermonkey-reviewers,dom-worker-reviewers,asuth,jonco,mccr8
https://hg.mozilla.org/integration/autoland/rev/d763241fccaf
Part 8: Add module type to ModuleLoadRequest::NewVisitedSetForTopLevelImport r=spidermonkey-reviewers,dom-worker-reviewers,dom-core,asuth,jonco,mccr8
https://hg.mozilla.org/integration/autoland/rev/95b20763e650
Part 9: Add module type to ModuleLoaderBase::{CreateStaticImport,CreateDynamicImport} r=spidermonkey-reviewers,dom-worker-reviewers,dom-core,asuth,jonco,mccr8
https://hg.mozilla.org/integration/autoland/rev/39a20aee3942
Part 10: Get the module type from the module request r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/fe179004423b
Part 11: Only allow expected mime types r=dom-worker-reviewers,dom-core,asuth,hsivonen
https://hg.mozilla.org/integration/autoland/rev/a4d7565218f3
Part 12: Add JS::IsCyclicModule r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/a1f494b9592a
Part 13: Skip setting/clearing the module private for synthetic modules r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/a3b5b0cde6ce
Part 14: Return early in ModuleLoaderBase::ResolveRequestedModules for synthetic modules  r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/1234a31ff88f
Part 15: Skip ExposeScriptToDebugger/UpdateDebugMetadata for synthetic modules r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/d98ecddf2f59
Part 16: Disable off-thread compilaton for JSON modules r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/48dc4035a14f
Part 17: Extend CompileFetchedModule to handle JSON modules r=spidermonkey-reviewers,dom-core,jonco,mccr8
https://hg.mozilla.org/integration/autoland/rev/662eb52b2a6f
Part 18: Update WPT prefs to enable JSON modules r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/14f104e5ce51
Part 19: Update WPT test expectations or JSON modules r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/19d78d0b6ee4
Part 20: Fix URL encoding in JSON module WPT tests r=spidermonkey-reviewers,jonco

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/47741 for changes under testing/web-platform/tests

Comment 23

[Cristina Horotan [:chorotan]]

Backed out 20 changesets (Bug 1877792) for causing wpt failures at importscripts_mime_local.any.sharedworker.html

Backout: https://hg.mozilla.org/integration/autoland/rev/d3fc59306685de28fb32bbb0111b27564f4b43be

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=19d78d0b6ee4cf742918ef43c9b0246f3054c508&selectedTaskRun=CEutAFJhTKGs6dQXevxrAA.0

Failure logs:
https://treeherder.mozilla.org/logviewer?job_id=471367151&repo=autoland&lineNumber=2606
https://treeherder.mozilla.org/logviewer?job_id=471367172&repo=autoland&lineNumber=2607

Comment 24

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 25

[Jonatan Klemets [:jon4t4n]]

Now, when the code freeze has been lifted, we can try to re-land these patches. Jon, can you queue this for landing again?

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1de3f8d89dec
Part 1: Change HttpBaseChannel.cpp to allow application/json r=yulia,necko-reviewers,jesup,valentin
https://hg.mozilla.org/integration/autoland/rev/67c5323f1554
Part 2: Change the module map key to include both URL and module type r=yulia,jonco
https://hg.mozilla.org/integration/autoland/rev/0c5bcdad91ed
Part 3: Update module map methods to take ModuleMapKey instead of nsIURI r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/579d484a8683
Part 4: Add CompileJsonModule version for SourceText<Utf8Unit> r=mgaudet,yulia
https://hg.mozilla.org/integration/autoland/rev/0e95f906a999
Part 5: Use AutoReportFrontendContext in CompileJsonModule for error reporting r=bthrall
https://hg.mozilla.org/integration/autoland/rev/67b7056ae5db
Part 6: Change ModuleLoaderBase::ResolveRequestedModules to handle URL and module type r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/f0e296342474
Part 7: Add ModuleLoadRequest::mModuleType r=dom-core,spidermonkey-reviewers,dom-worker-reviewers,asuth,jonco,mccr8
https://hg.mozilla.org/integration/autoland/rev/28035acf7788
Part 8: Add module type to ModuleLoadRequest::NewVisitedSetForTopLevelImport r=spidermonkey-reviewers,dom-worker-reviewers,dom-core,asuth,jonco,mccr8
https://hg.mozilla.org/integration/autoland/rev/b83c58b4a868
Part 9: Add module type to ModuleLoaderBase::{CreateStaticImport,CreateDynamicImport} r=spidermonkey-reviewers,dom-worker-reviewers,dom-core,asuth,jonco,mccr8
https://hg.mozilla.org/integration/autoland/rev/9b247d7d4dd3
Part 10: Get the module type from the module request r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/eb947a73a97f
Part 11: Only allow expected mime types r=dom-worker-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/3457e8d54eb5
Part 12: Add JS::IsCyclicModule r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/3fdcdf158eb7
Part 13: Skip setting/clearing the module private for synthetic modules r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/a0db4567cf83
Part 14: Return early in ModuleLoaderBase::ResolveRequestedModules for synthetic modules  r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/66c4ccedeecb
Part 15: Skip ExposeScriptToDebugger/UpdateDebugMetadata for synthetic modules r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/d99e07f9cbeb
Part 16: Disable off-thread compilaton for JSON modules r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/c005e9f8e985
Part 17: Extend CompileFetchedModule to handle JSON modules r=spidermonkey-reviewers,dom-core,jonco,mccr8
https://hg.mozilla.org/integration/autoland/rev/a6641dc244fa
Part 18: Update WPT prefs to enable JSON modules r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/cd4e9f2dfdae
Part 19: Update WPT test expectations or JSON modules r=spidermonkey-reviewers,jonco
https://hg.mozilla.org/integration/autoland/rev/b0bf3998767e
Part 20: Fix URL encoding in JSON module WPT tests r=spidermonkey-reviewers,jonco

Comment 27

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/1de3f8d89dec
https://hg.mozilla.org/mozilla-central/rev/67c5323f1554
https://hg.mozilla.org/mozilla-central/rev/0c5bcdad91ed
https://hg.mozilla.org/mozilla-central/rev/579d484a8683
https://hg.mozilla.org/mozilla-central/rev/0e95f906a999
https://hg.mozilla.org/mozilla-central/rev/67b7056ae5db
https://hg.mozilla.org/mozilla-central/rev/f0e296342474
https://hg.mozilla.org/mozilla-central/rev/28035acf7788
https://hg.mozilla.org/mozilla-central/rev/b83c58b4a868
https://hg.mozilla.org/mozilla-central/rev/9b247d7d4dd3
https://hg.mozilla.org/mozilla-central/rev/eb947a73a97f
https://hg.mozilla.org/mozilla-central/rev/3457e8d54eb5
https://hg.mozilla.org/mozilla-central/rev/3fdcdf158eb7
https://hg.mozilla.org/mozilla-central/rev/a0db4567cf83
https://hg.mozilla.org/mozilla-central/rev/66c4ccedeecb
https://hg.mozilla.org/mozilla-central/rev/d99e07f9cbeb
https://hg.mozilla.org/mozilla-central/rev/c005e9f8e985
https://hg.mozilla.org/mozilla-central/rev/a6641dc244fa
https://hg.mozilla.org/mozilla-central/rev/cd4e9f2dfdae
https://hg.mozilla.org/mozilla-central/rev/b0bf3998767e

Comment 28

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Is this something we should call out in the Fx132 relnotes?

Comment 30

[Jonatan Klemets [:jon4t4n]]

I don't know anything about how and when the release notes are handled, so I will defer to Dan here. That said, this is currently nightly only, and shipping is tracked under bug 1777526, so I don't think we want to mention this in the relnotes for Fx132.

Comment 31

[Dan Minor [:dminor]]

Yes, I think we can wait on the release notes until bug 1777526, unless we normally call out experimental features like this, which I'm not sure we do.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

We do have the ability to add Nightly-only release notes if you want to call it out there to encourage testing. Totally up to you, though.