Closed Bug 1240072 (modules-0) Opened 8 years ago Closed 6 years ago

Implement milestone 0 <script type="module">

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 2 open bugs)

Details

(Keywords: DevAdvocacy)

Attachments

(4 files, 10 obsolete files)

5.11 KB, patch
sicking
: review+
Details | Diff | Splinter Review
26.77 KB, patch
shu
: review+
Details | Diff | Splinter Review
73.42 KB, patch
jonco
: review+
Details | Diff | Splinter Review
46.76 KB, patch
sicking
: review+
Details | Diff | Splinter Review
This bug is to add milestone 0 support for JS modules in the browser.  

This will be in a restricted form initially, either for chrome or nightly only.  This will let us test our implementation and the proposed spec.

Work on the spec is currently ongoing - the pull request is here: https://github.com/whatwg/html/pull/443
Can we target B2G as well?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #1)
Yes, the eventual plan is to enable this everywhere.
Although the full module loader is specced in terms of promises and would make most sense as a self hosted JS component, my current plan is to implement this mainly in C++ by changing nsScriptLoader and associated classes.  

The choice is either to reflect a bunch of gecko/C++ stuff into JS or to reflect a bunch of JS stuff into gecko.  This milesstone 0 spec is written mainly in terms of things which today are implemented in C++ so I think it makes sense to do it this way round.
Depends on: 1241183
Attached patch 1 - jsapi-changes - WIP (obsolete) — Splinter Review
Extend the JS API to allow compilation of modules etc.  Work in progress.
Attached patch 2 - module-scripts - WIP (obsolete) — Splinter Review
Add support for non-async module scripts.  Work in progress.
Attached patch 3 - module-scripts-async - WIP (obsolete) — Splinter Review
Add support for async module scripts.  Work in progress.
Attached patch 1-add-js-apis (obsolete) — Splinter Review
Attachment #8716397 - Attachment is obsolete: true
Attached patch 2-dom-js-utils (obsolete) — Splinter Review
Attachment #8716399 - Attachment is obsolete: true
Attached patch 3-script-loader (obsolete) — Splinter Review
Attachment #8716400 - Attachment is obsolete: true
Attached patch 4-test-code (obsolete) — Splinter Review
Comment on attachment 8723068 [details] [diff] [review]
3-script-loader

I think this patch is about ready.  It's quite large, so apologies for that up front.  This turned out more complicated than I anticipated.

The changes are as follows:

Make nsScriptLoader and associated classes cycle collectable, as we're going to have references to JS module objects.
   
Add an nsModuleLoadRequest class derived from nsScriptLoadRequest to represent an request to load a module.  There is one of these per top level module script and per module import.
   
Add an nsModuleScript class.  This corresponds to the spec's "module script" and there is one per loaded module.  One of these can satisfy multiple load requests if they are for the same module.

Each nsModuleLoadRequest has a MozPromise that is completed when the script is ready.  Ready for a module means ready to execute - in other words fetched, parsed (compiled) and all of its descendants (imported modules) also ready.  This happens before it or any of its descendants are instantiated or executed.  These promises are used to trigger instantiation and execution of the top level module (which internally executes any dependant modules).

The "module map" concept is implemented as two maps, one containing an already fetched module script and one containing a promise for an in-flight fetch.  This promise is created on demand and is used to wait for the fetch to complete, which is before fetching of its descendants happens.

There may be better ways to structure this as it is quite tied to the script loader at the moment, but the reason it's like this is because it reuses the fetching infrastructure and because the order of module execution is tied in with that of scripts.  This may have to change in the future to implement worker modules if we want to have a single module loader implementation, although how that's going to work is not obvious right now.

All of the module loading is predicated on the document being chrome, so this doesn't affect web content at all.
Attachment #8723068 - Flags: feedback?(jonas)
Do you have time to give feedback / review patches for this?
Flags: needinfo?(bugs)
Flags: needinfo?(bugs)
Attachment #8723068 - Flags: feedback?(jonas) → review?(jonas)
Comment on attachment 8723068 [details] [diff] [review]
3-script-loader

Review of attachment 8723068 [details] [diff] [review]:
-----------------------------------------------------------------

The main problem that I found is that the function names here are really not very descriptive. It's very unclear which parts of "do network request", "found dependencies", "downloaded dependencies" have been done when "Load" or "Fetch" is done.

Other than that, and the comments below, it looks good. It was really hard to verify that this does the right thing, so please make sure you have plenty of tests. Also let me know if there was anything in particular that you wanted me to look at.

r- so that we can get better names figured out and the CORS stuff looked at.

::: dom/base/nsScriptLoader.cpp
@@ +124,5 @@
> +
> +  nsModuleLoadRequest(nsIScriptElement* aElement,
> +                      uint32_t aVersion,
> +                      CORSMode aCORSMode,
> +                      const SRIMetadata &aIntegrity,

Nit: Please put the '&' together with the type.

@@ +142,5 @@
> +  // Is this a request for a top level module script or an import?
> +  bool mIsTopLevel;
> +
> +  // The base URL used for resolving relative module imports.
> +  RefPtr<nsIURI> mBaseURL;

use nsCOMPtr when holding on to XPCOM interfaces. nsCOMPtr provides additional debug-only assertions which are useful.

@@ +173,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsModuleLoadRequest, nsScriptLoadRequest)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mBaseURL)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mLoader)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mParent)

Use NS_IMPL_CYCLE_COLLECTION_INHERITED to save yourself some boilerplate.

@@ +179,5 @@
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mImports)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsModuleLoadRequest, nsScriptLoadRequest)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mBaseURL)

Technically I don't think you need to include mBaseURL here. I think we often count on nsIURIs not participating in cycles, so I think adding it just adds code and CPU cycles.

@@ +216,5 @@
> +  nsScriptLoadRequest::Cancel();
> +  mModuleScript = nullptr;
> +  mProgress = nsScriptLoadRequest::Progress::Ready;
> +  for (size_t i = 0; i < mImports.Length(); i++) {
> +    mImports[i]->Cancel();

Might this cancel a sub-module load that is also being used by other top-level modules? If so, is that bad?

@@ +241,5 @@
> +  // A module that was found to be marked as fetching in the module map has now
> +  // been loaded.
> +
> +  mModuleScript = mLoader->GetFetchedModule(mURI);
> +  mLoader->StartFetchingModuleDescendants(this);

Shouldn't we start fetching imports before the top-level script has finished downloading? Or are we planning on adding that later?

@@ +245,5 @@
> +  mLoader->StartFetchingModuleDescendants(this);
> +}
> +
> +void
> +nsModuleLoadRequest::LoadFinished()

The difference between "FetchFinished" and "LoadFinished" is not entirely clear. Historically they have both "fetch" and "load" has meant "download from the network".

@@ +274,5 @@
> +
> +class nsModuleScript final : public nsISupports
> +{
> +  RefPtr<nsScriptLoader> mLoader;
> +  RefPtr<nsIURI> mBaseURL;

nsCOMPtr

@@ +435,5 @@
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsScriptLoader)
> +NS_INTERFACE_MAP_END
> +
> +NS_IMPL_CYCLE_COLLECTION_CLASS(nsScriptLoader)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsScriptLoader)

Use NS_IMPL_CYCLE_COLLECTION

@@ +711,5 @@
> +{
> +  MOZ_ASSERT(!aRequest->mModuleScript);
> +
> +  nsresult rv = CreateModuleScript(aRequest);
> +  SetModuleFetchFinished(aRequest, rv);

Better names here are definitely needed. I would have expected "fetch" to only include the network part, and not also include processing, which appears to be the case.

@@ +718,5 @@
> +  aRequest->mScriptTextBuf = nullptr;
> +  aRequest->mScriptTextLength = 0;
> +
> +  if (NS_SUCCEEDED(rv)) {
> +    StartFetchingModuleDescendants(aRequest);

You're calling this both in FetchFinished and in ProcessFetchedModuleSource. Is that really accurate?

@@ +746,5 @@
> +                              context->GetNativeContext());
> +  entryScript.TakeOwnershipOfErrorReporting();
> +
> +  bool oldProcessingScriptTag = context->GetProcessingScriptTag();
> +  context->SetProcessingScriptTag(true);

I *think* that the only thing that SetProcessingScriptTag does is that it enables document.write() calls during the execution of the script.

Is that something that we want to do? I think it's not enabled for <script async> for example, so I suspect it should not be for modules either.

But maybe SetProcessingScriptTag does other things too? Sorry, it's been too long since I worked on this code and don't remember the details.

@@ +857,5 @@
> +
> +  if (!StringBeginsWith(aSpecifier, NS_LITERAL_STRING("/")) &&
> +      !StringBeginsWith(aSpecifier, NS_LITERAL_STRING("./")) &&
> +      !StringBeginsWith(aSpecifier, NS_LITERAL_STRING("../")))
> +  {

'{' should be moved to the previous line. But see below. I don't understand why this logic is needed at all.

@@ +861,5 @@
> +  {
> +    return nullptr;
> +  }
> +
> +  rv = NS_NewURI(getter_AddRefs(uri), aSpecifier, nullptr, aScript->BaseURL());

I don't understand why this function doesn't simply call this directly? I.e. why try to parse as an absolute URI first and then as a relative URI? The URI parser will handle that automatically.

One thing that you're not properly handling here for example is the relative URLs '#', '..' and '.' (which are a quite valid URLs, though unlikely to be used in this context)

@@ +943,5 @@
> +    size_t len = JS_GetStringLength(str);
> +    size_t byteLen = (len + 1) * sizeof(char16_t);
> +    char16_t* chars = static_cast<char16_t*>(moz_xmalloc(byteLen));
> +    Range<char16_t> dest(chars, len + 1);
> +    if (!JS_CopyStringChars(cx, dest, str)) {

Use nsAutoJSString

@@ +1007,5 @@
> +                 &nsModuleLoadRequest::LoadFailed);
> +}
> +
> +RefPtr<GenericPromise>
> +nsScriptLoader::StartFetchingModuleScriptTree(nsModuleLoadRequest* aRequest,

Maybe name this StartFetchingModuleAndDependencies or some such. My first guess was that this fetch a top-level module since it sounds like it fetches a whole tree, whereas in reality it's only fetching a subtree.

@@ +1081,5 @@
> +  }
> +
> +  JS::Rooted<JSFunction*> func(aCx);
> +  func = JS_NewFunction(aCx, HostResolveImportedModule, 2, 0,
> +                        "HostResolveImportedModule");

Is this not something we can accomplish with WebIDL? That would probably also significantly simplify HostResolveImportedModule

@@ +1149,5 @@
>    nsCOMPtr<nsIInterfaceRequestor> prompter(do_QueryInterface(docshell));
>  
> +  nsSecurityFlags securityFlags;
> +  if (aRequest->IsModuleRequest()) {
> +    securityFlags = nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS;

Is this really the desired mode for module loads? Seems very unwise to switch script loading to use CORS by default given that it's so common to do cross-site loading without CORS for normal scripts.

Seems like a very minimal security benefit for a very large source of frustration.

But if the spec says to use CORS then of course that's what we should do.

@@ +1151,5 @@
> +  nsSecurityFlags securityFlags;
> +  if (aRequest->IsModuleRequest()) {
> +    securityFlags = nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS;
> +    if (aRequest->mCORSMode == CORS_NONE) {
> +      securityFlags |= nsILoadInfo::SEC_COOKIES_OMIT;

This definitely looks wrong. Can you point me to where the spec defines how cookies and CORS should be handled?

Setting SEC_COOKIES_OMIT means that no cookies are sent for even same-site requests. If that's really what we want we should introduce a new value for the mCORSMode enum rather than reuse CORS_NONE which means something completely different.

@@ +1167,3 @@
>      securityFlags |= nsILoadInfo::SEC_COOKIES_SAME_ORIGIN;
>    } else if (aRequest->mCORSMode == CORS_USE_CREDENTIALS) {
> +      securityFlags |= nsILoadInfo::SEC_COOKIES_INCLUDE;

Indentation here is wrong.

@@ +1588,5 @@
> +    StartFetchingModuleDescendants(modReq);
> +    if (aElement->GetScriptAsync()) {
> +      mLoadingAsyncRequests.AppendElement(request);
> +    } else {
> +      AddDeferRequest(request);

This seems strange to me. Are inline <script type=module>'s really defined to not execute as soon as any dependencies are loaded? The Defer queue never runs until the whole document is done parsing. Is that really the intent?

@@ +2482,5 @@
> +
> +  // If it's async, move it to the loaded list.  aRequest->mIsAsync really
> +  // _should_ be in a list, but the consequences if it's not are bad enough we
> +  // want to avoid trying to move it if it's not.
> +  if (aRequest->mIsAsync) {

Based on the comment it sounds like you want to move non-async scripts always. But the code below never moves non-async scripts to the loaded list.

@@ +2483,5 @@
> +  // If it's async, move it to the loaded list.  aRequest->mIsAsync really
> +  // _should_ be in a list, but the consequences if it's not are bad enough we
> +  // want to avoid trying to move it if it's not.
> +  if (aRequest->mIsAsync) {
> +    MOZ_ASSERT(aRequest->isInList());

Name this IsInLoadedList?
Attachment #8723068 - Flags: review?(jonas) → review-
(In reply to Jonas Sicking (:sicking) from comment #13)

Great, thanks for all the comments.

> The main problem that I found is that the function names here are really not
> very descriptive. It's very unclear which parts of "do network request",
> "found dependencies", "downloaded dependencies" have been done when "Load"
> or "Fetch" is done.

In general I tried to name things based on the section names in the spec, but I agree it's confusing (in the spec the 'fetch' operations on modules include parsing the module source).  

I think the following changes will make things more obvious:
  Fetch* -> FetchAndParse*
  Load* -> FetchAndParseSubtree*

> @@ +216,5 @@
> > +  nsScriptLoadRequest::Cancel();
> > +  mModuleScript = nullptr;
> > +  mProgress = nsScriptLoadRequest::Progress::Ready;
> > +  for (size_t i = 0; i < mImports.Length(); i++) {
> > +    mImports[i]->Cancel();
> 
> Might this cancel a sub-module load that is also being used by other
> top-level modules? If so, is that bad?

No, because module load requests are not shared in that way (although nsModuleScripts are).

> @@ +241,5 @@
> > +  // A module that was found to be marked as fetching in the module map has now
> > +  // been loaded.
> > +
> > +  mModuleScript = mLoader->GetFetchedModule(mURI);
> > +  mLoader->StartFetchingModuleDescendants(this);
> 
> Shouldn't we start fetching imports before the top-level script has finished
> downloading? Or are we planning on adding that later?

Yes, but we don't have the means to do this yet.

At the moment we download a script, parse it and then extract the list of imported modules to fetch.  Incremental parsing is being investigated in bug 1154987 which may eventually allow us to get this information sooner and start fetching imported modules right away.

> @@ +718,5 @@
> > +  aRequest->mScriptTextBuf = nullptr;
> > +  aRequest->mScriptTextLength = 0;
> > +
> > +  if (NS_SUCCEEDED(rv)) {
> > +    StartFetchingModuleDescendants(aRequest);
> 
> You're calling this both in FetchFinished and in ProcessFetchedModuleSource.
> Is that really accurate?

Yes.  In ProcessFetchedModuleSource it's used to fetch the imports when a module has just been fetched and parsed.  In FetchFinished it's used for a request that found its requested module was either already loaded or currently being loaded, after waiting for that module to become available.  In the latter case then this creates new module requests for the imports which may be immediately satisfied.  

> @@ +746,5 @@
> > +                              context->GetNativeContext());
> > +  entryScript.TakeOwnershipOfErrorReporting();
> > +
> > +  bool oldProcessingScriptTag = context->GetProcessingScriptTag();
> > +  context->SetProcessingScriptTag(true);
> 
> I *think* that the only thing that SetProcessingScriptTag does is that it
> enables document.write() calls during the execution of the script.

I did some digging and it seems this changes the loading behaviour if we're inside a script tag so that session history is correct if a script sets location.href (originally in bug 72197).  This does happens for async script execution too.  I think we do want this to be the same for modules as it is for
scripts.

> @@ +861,5 @@
> > +  {
> > +    return nullptr;
> > +  }
> > +
> > +  rv = NS_NewURI(getter_AddRefs(uri), aSpecifier, nullptr, aScript->BaseURL());
> 
> I don't understand why this function doesn't simply call this directly? I.e.
> why try to parse as an absolute URI first and then as a relative URI? The
> URI parser will handle that automatically.

This is to disallow bare word import specifiers in particular (and any other valid relative URLs except those explicitly allowed).  From the spec:

'This restriction is in place so that in the future we can allow custom module loaders to give special meaning to "bare" import specifiers, like import "jquery" or import "web/crypto". For now any such imports will fail, instead of being treated as relative URLs.'

I'll add a comment to this effect.

> @@ +1081,5 @@
> > +  }
> > +
> > +  JS::Rooted<JSFunction*> func(aCx);
> > +  func = JS_NewFunction(aCx, HostResolveImportedModule, 2, 0,
> > +                        "HostResolveImportedModule");
> 
> Is this not something we can accomplish with WebIDL? That would probably
> also significantly simplify HostResolveImportedModule

I don't know much about WebIDL.  I read some docs but didn't see anything that obviously applied to this case.  Can you point me at an example where we use it for something like this?

> @@ +1149,5 @@
> >    nsCOMPtr<nsIInterfaceRequestor> prompter(do_QueryInterface(docshell));
> >  
> > +  nsSecurityFlags securityFlags;
> > +  if (aRequest->IsModuleRequest()) {
> > +    securityFlags = nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS;
> 
> Is this really the desired mode for module loads? 

Yes, the spec says to always use CORS for fetching modules.

> This definitely looks wrong. Can you point me to where the spec defines how
> cookies and CORS should be handled?

It's step 16 / 6 under "prepare a script":

https://html.spec.whatwg.org/multipage/scripting.html#prepare-a-script 

> @@ +1588,5 @@
> > +    StartFetchingModuleDescendants(modReq);
> > +    if (aElement->GetScriptAsync()) {
> > +      mLoadingAsyncRequests.AppendElement(request);
> > +    } else {
> > +      AddDeferRequest(request);
> 
> This seems strange to me. Are inline <script type=module>'s really defined
> to not execute as soon as any dependencies are loaded? The Defer queue never
> runs until the whole document is done parsing. Is that really the intent?

That's how I read the spec, yes.  In "prepare a script" step 18 handles both external and inline module scripts in the same way: "Add the element to the end of the list of scripts that will execute when the document has finished parsing" (second if block).

> @@ +2482,5 @@
> > +
> > +  // If it's async, move it to the loaded list.  aRequest->mIsAsync really
> > +  // _should_ be in a list, but the consequences if it's not are bad enough we
> > +  // want to avoid trying to move it if it's not.
> > +  if (aRequest->mIsAsync) {
> 
> Based on the comment it sounds like you want to move non-async scripts
> always. But the code below never moves non-async scripts to the loaded list.

AUII non-async module scripts don't need to move to different list because they are processed in order as they become ready, it's just async scripts that need to move from the loading list to the loaded list.

I totally don't understand the second sentence of the comment though.  I moved this block from somewhere else and didn't want to delete it.
(In reply to Jon Coppeard (:jonco) (PTO until 11th April) from comment #14)
> I think the following changes will make things more obvious:
>   Fetch* -> FetchAndParse*
>   Load* -> FetchAndParseSubtree*

Sounds good. I'd like to look over the resulting names once a new patch is ready.

> > @@ +718,5 @@
> > > +  aRequest->mScriptTextBuf = nullptr;
> > > +  aRequest->mScriptTextLength = 0;
> > > +
> > > +  if (NS_SUCCEEDED(rv)) {
> > > +    StartFetchingModuleDescendants(aRequest);
> > 
> > You're calling this both in FetchFinished and in ProcessFetchedModuleSource.
> > Is that really accurate?
> 
> Yes.  In ProcessFetchedModuleSource it's used to fetch the imports when a
> module has just been fetched and parsed.  In FetchFinished it's used for a
> request that found its requested module was either already loaded or
> currently being loaded, after waiting for that module to become available. 
> In the latter case then this creates new module requests for the imports
> which may be immediately satisfied.  

This is still quite confusing to me. Please grab me on irc and lets chat.

> > @@ +1081,5 @@
> > > +  }
> > > +
> > > +  JS::Rooted<JSFunction*> func(aCx);
> > > +  func = JS_NewFunction(aCx, HostResolveImportedModule, 2, 0,
> > > +                        "HostResolveImportedModule");
> > 
> > Is this not something we can accomplish with WebIDL? That would probably
> > also significantly simplify HostResolveImportedModule
> 
> I don't know much about WebIDL.  I read some docs but didn't see anything
> that obviously applied to this case.  Can you point me at an example where
> we use it for something like this?

I'm not really sure what you're trying to accomplish with this code, so I can't really tell if it's doable with WebIDL or not. And I'm also not an expert on WebIDL. Might be worth reaching out to bholley to get his opinion on if we can/should use WebIDL rather than use JSAPI.

> > @@ +1149,5 @@
> > >    nsCOMPtr<nsIInterfaceRequestor> prompter(do_QueryInterface(docshell));
> > >  
> > > +  nsSecurityFlags securityFlags;
> > > +  if (aRequest->IsModuleRequest()) {
> > > +    securityFlags = nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS;
> > 
> > Is this really the desired mode for module loads? 
> 
> Yes, the spec says to always use CORS for fetching modules.
> 
> > This definitely looks wrong. Can you point me to where the spec defines how
> > cookies and CORS should be handled?
> 
> It's step 16 / 6 under "prepare a script":
> 
> https://html.spec.whatwg.org/multipage/scripting.html#prepare-a-script 

This seems really broken to me. Both the use of CORS by default, and the fact that it says to not send cookies at all, not even for same-origin requests, by default.
Attached patch 1-add-js-apis v2 (obsolete) — Splinter Review
Extend the JSAPI to support parsing, instantiation and evaluation of modules including off-main-thread parsing.

This also adds the [[HostDefined]] module field that appeared in ES2017 which is needed by the browser changes.
Attachment #8723066 - Attachment is obsolete: true
Attachment #8740994 - Flags: review?(shu)
Add nsJSUtils methods for module parsing, instantiation and evaluation that call into the JS API.
Attachment #8723067 - Attachment is obsolete: true
Attachment #8740995 - Flags: review?(jonas)
Attached patch script-loader-review-followup (obsolete) — Splinter Review
Here's the review followup for the script loader changes, excluding renamings.
Attachment #8740997 - Flags: review?(jonas)
Attached patch script-loader-rename-methods (obsolete) — Splinter Review
Renamings only in this patch.

I changed the following:
 - nsModuleLoadRequest::FetchFinshed -> ModuleLoaded
 - nsModuleLoadRequest::LoadFinished -> DependenciesLoaded
 - nsScriptLoader::FinishModuleLoad -> ProcessLoadedModuleTree
 - nsScriptLoader::StartFetchingModuleScriptTree -> StartFetchingModuleAndDependencies as suggested
 - nsScriptLoader::SetModuleFetchFinished -> SetModuleFetchFinishedAndResumeWaitingRequests
 - dependents -> dependencies everywhere

I found it hard to come up with names that were pithy but descriptive.  Is this enough to make things understandable do you think.  Better suggestions are welcome.
Attachment #8741016 - Flags: review?(jonas)
Keywords: dev-doc-needed
Comment on attachment 8740994 [details] [diff] [review]
1-add-js-apis v2

Review of attachment 8740994 [details] [diff] [review]:
-----------------------------------------------------------------

The API shuffling is pretty straightforward. Most of my comments below are nits.

I'd like clarification on their intended use and why passing in a non-ModuleObject returns false before r+.

::: js/src/builtin/ModuleObject.cpp
@@ +933,5 @@
> +    if (!args.init(0))
> +        return false;
> +    args.setCallee(value);
> +    args.setThis(ObjectValue(*self));
> +    return Invoke(cx, args);

Waldo recently made an effort to use js::Call instead of Invoke because you can't forget to set the callee and thisv. Use js::Call instead.

::: js/src/builtin/ModuleObject.h
@@ +208,5 @@
>          InitialEnvironmentSlot,
>          EnvironmentSlot,
>          NamespaceSlot,
>          EvaluatedSlot,
> +        HostDefined,

Please add the Slot suffix.

::: js/src/builtin/ReflectParse.cpp
@@ +3746,5 @@
>          pn = parser.parse();
>          if (!pn)
>              return false;
>      } else {
> +        Rooted<GlobalObject*> global(cx, cx->global());

cx->global() already returns a Handle.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +790,5 @@
> +ModuleObject*
> +frontend::CompileModule(JSContext* cx, const ReadOnlyCompileOptions& options,
> +                        SourceBufferHolder& srcBuf)
> +{
> +    Rooted<GlobalObject*> global(cx, cx->global());

cx->global() is already a Handle.

::: js/src/frontend/BytecodeCompiler.h
@@ +42,1 @@
>                ScriptSourceObject** sourceObjectOut = nullptr);

Existing style nits: * and & not next to type name.

::: js/src/jsapi.cpp
@@ +4619,5 @@
> +    cx->global()->setModuleResolveHook(func);
> +}
> +
> +JS_PUBLIC_API(bool)
> +JS::ParseModule(JSContext* cx, const ReadOnlyCompileOptions& options,

Why is this named Parse instead of CompileModule?

@@ +4628,5 @@
> +    CHECK_REQUEST(cx);
> +
> +    AutoLastFrameCheck lfc(cx);
> +
> +    SourceCompressionTask sct(cx);

What's the intention behind compressing the module source? Is it because the toplevel is run-once?

I don't think this is necessary for now -- I have no intuition how module sources might be used in the future by embedding or the user.

This is also not error checked as it is right now -- need to check sct.complete() after compiling.

@@ +4640,5 @@
> +
> +JS_PUBLIC_API(void)
> +JS::SetModuleHostDefinedField(JSObject* module, JS::Value value)
> +{
> +    MOZ_ASSERT(module->is<ModuleObject>());

Unnecessary, as<T> already asserts this.

@@ +4647,5 @@
> +
> +JS_PUBLIC_API(JS::Value)
> +JS::GetModuleHostDefinedField(JSObject* module)
> +{
> +    MOZ_ASSERT(module->is<ModuleObject>());

Unnecessary.

@@ +4658,5 @@
> +    AssertHeapIsIdle(cx);
> +    CHECK_REQUEST(cx);
> +    assertSameCompartment(cx, moduleArg);
> +    if (!moduleArg->is<ModuleObject>())
> +        return false;

Questions for this and the other checks below:

What are the expected uses such that it would pass in a non-ModuleObject?

Returning false here is strange in any case. Why is this an error in the same way that, say, an OOM is inside DeclarationInstantiation? Are both cases handled analogously by embedding?

@@ +4687,5 @@
> +    assertSameCompartment(cx, moduleArg);
> +    if (!moduleArg->is<ModuleObject>())
> +        return false;
> +
> +    RootedModuleObject module(cx, &moduleArg->as<ModuleObject>());

Rooting seems unnecessary.

@@ +4701,5 @@
> +    assertSameCompartment(cx, moduleArg);
> +    if (!moduleArg->is<ModuleObject>())
> +        return nullptr;
> +
> +    RootedModuleObject module(cx, &moduleArg->as<ModuleObject>());

Rooting seems unnecessary.
Attachment #8740994 - Flags: review?(shu)
Attached patch 1-add-js-apis v3Splinter Review
Thanks for the comments.

> What are the expected uses such that it would pass in a non-ModuleObject?

This was me being defensive and adding a check that happened in release builds.  I see that in general we don't do this however and use as() where we expect an object to be a certain subclass, so I've done that instead.
Attachment #8740994 - Attachment is obsolete: true
Attachment #8743799 - Flags: review?(shu)
(In reply to Jon Coppeard (:jonco) from comment #21)
> > What are the expected uses such that it would pass in a non-ModuleObject?
> 
> This was me being defensive and adding a check that happened in release
> builds.  I see that in general we don't do this however and use as() where
> we expect an object to be a certain subclass, so I've done that instead.

By all means, feel free to MOZ_RELEASE_ASSERT it too, if it seems like an easy mistake.

Add an as_with_release_assert() member function template, if you want.
Comment on attachment 8743799 [details] [diff] [review]
1-add-js-apis v3

Review of attachment 8743799 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: js/src/jsapi.h
@@ +4349,5 @@
> + * record to request importation of modules.
> + *
> + * The result is a JavaScript array of string values.  ForOfIterator can be used
> + * to extract the individual strings.
> + */

Worth commenting here that this function is otherwise infallible, and would return false iff moduleRecord isn't a ModuleObject.

@@ +4356,5 @@
> +                    JS::MutableHandleObject rval);
> +
> +/*
> + * Get the script associated with a module.
> + */

Same deal as above.
Attachment #8743799 - Flags: review?(shu) → review+
Comment on attachment 8741016 [details] [diff] [review]
script-loader-rename-methods

Review of attachment 8741016 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, thanks!
Attachment #8741016 - Flags: review?(jonas) → review+
Updated patch with review comments applied.
Attachment #8723068 - Attachment is obsolete: true
Attachment #8740997 - Attachment is obsolete: true
Attachment #8741016 - Attachment is obsolete: true
Attachment #8749699 - Flags: review+
Attached patch 4-test-code v2Splinter Review
Thanks for the reviews.  This is the final patch - it's just test code.
Attachment #8723069 - Attachment is obsolete: true
Attachment #8749722 - Flags: review?(jonas)
Comment on attachment 8749722 [details] [diff] [review]
4-test-code v2

Review of attachment 8749722 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me

I didn't review these too carefully, but looks good to me.
Attachment #8749722 - Flags: review?(jonas) → review+
Keywords: leave-open
Keywords: DevAdvocacy
Flags: platform-rel?
platform-rel: --- → ?
Depends on: 1281194
platform-rel: ? → ---
Depends on: 1292614
Depends on: 1295978
Any update on the status of implementation? Looks like something was landed.
Depends on: 1330682
Any thoughts from Mozilla on the small proposal for a nomodule="" attribute at https://github.com/whatwg/html/pull/2261 ? It would be ideal if all browsers were able to ship that as part of their initial <script type="module"> implementation, as if they don't, its utility for authors becomes much lower. The other 3 engines are on-board.
Thoughts on comment 31, bz? Seems uncontroversial to me.
Flags: needinfo?(bzbarsky)
So the idea is to allow fallback for non-module supporting browsers?  And scripts of types other than "module" will not run when nomodule is specified on them?

Seems ok to me, but please send an intent for this part to get more eyes on it.
Flags: needinfo?(bzbarsky)
Depends on: 1330900
Depends on: 1337780
Depends on: 1340865
Depends on: 1330688
Depends on: 1342416
Depends on: 1361369
Depends on: 1361373
Assignee: jcoppeard → nobody
Depends on: 1358882
Depends on: 1362098
Depends on: 1362101
As jonco has helpfully noted in e.g. bug 1362101, as we implement in Chrome we're finding spec holes around various edge cases and trying to fix them.

My initial hope was that we could squash them all, write lots of good web platform tests, and then provide an update to all the other browsers saying "hey, we fixed all the edge cases and have tests for you" so they could do a single pass benefiting from our work. But probably it is better to keep you all in the loop, since the fixes are taking longer than I'd like and we keep discovering new small problems (including ones discovered Firefox).

So let me give a quick update on the in-flight PRs:

- Fix error cases to consistently send to scriptEl.onerror vs. window.onerror correctly, and re-propagate old errors: https://github.com/whatwg/html/pull/2595 (bug 1362101)
- Make inline module scripts actually instantiate: https://github.com/whatwg/html/pull/2604
- Don't handle impossible errors in HostResolveImportedModule: https://github.com/whatwg/html/pull/2625

and currently-unsolved issues:

- Circular module graphs instantiate too early: https://github.com/whatwg/html/issues/2629 (bug 1361988)
- Module script errors maybe shouldn't propagate to descendant scripts so much: https://github.com/whatwg/html/issues/2630

Our intention is still to provide good web platform tests for all these bug fixes as we go. Let us know how else we can help.
Depends on: 1362119
Depends on: 1374239
Depends on: 1362842
Depends on: 1382020
Depends on: 1388728
Depends on: 1346482
Depends on: 1391584
Depends on: 1393429
Depends on: 1372258
Depends on: 1395635
Depends on: 1283534
Depends on: 1406151
Blocks: 1342012
Depends on: 1379934
Depends on: 1419513
Depends on: 1420420
Alias: modules-0
Depends on: 1365187
Depends on: 1428002
Depends on: 1430145
Depends on: 1432137
Depends on: 1435439
Depends on: 1438139
Blocks: 1451545
Depends on: 1453559
No longer depends on: 1283534
This is shipping in FF 60.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
No longer blocks: 1451545
Assignee: nobody → jcoppeard
You need to log in before you can comment on or make changes to this bug.