The module loader should be updated in line with the latest spec changes: https://github.com/whatwg/html/commit/2b408b65a11fe76b6588b744e0ae74d30ff29b43 This fixes some bugs instantiating module graphs and moves some of the complexity of instantiating module graphs into the JS engine from the module loader. This means removing the eager instantiation of modules that was added in bug 1295978. The JS engine changes are in bug 1374239.

Rename module methods in line with spec changes (ModuleDeclarationInstantiation to ModuleInstantiation and ModuleEvaluation to ModuleEvaluate). Add APIs to discover whether a module record is errored and to get the error value.

Comment on attachment 8898368 [details] [diff] [review] bug1388728-jsapi Review of attachment 8898368 [details] [diff] [review]: ----------------------------------------------------------------- The renaming looks good to me; it might be incorrect to check that error is undefined. Thanks for the patch. ::: js/src/builtin/ModuleObject.cpp @@ +816,5 @@ > Value > ModuleObject::error() const > { > + JS::Value error = getReservedSlot(ErrorSlot); > + MOZ_ASSERT(!error.isUndefined() == (status() == MODULE_STATUS_ERRORED)); Apparently, one can throw undefined, so this test and the one in IsModuleErrored need to change. ::: js/src/jsapi.h @@ +4519,5 @@ > extern JS_PUBLIC_API(JS::Value) > GetModuleHostDefinedField(JSObject* module); > > /* > + * Perform the ModuleInstantiate operation on on the give source pre-existing nit: on on => on, the give => the given @@ +4532,3 @@ > > /* > + * Perform the ModuleEvaluate operation on on the give source text module ditto

This patch contains the changes to the script loader and tests to bring them in line with the latest spec. In summary this does the following: - removes eager instantiation of modules as added in bug 1295978 - fixes error handling for modules - renames some operations as in the above patch - some test improvements - enables most web platform tests that were previously failing (there are a couple that still fail) Error handling for a module load request now has three error states: - null module script, caused by failure to fetch a resource - module script with no module record and recorded error, caused by failure to parse module source - module script with module record with errored module script, caused by instantiation or evaluation failure For the first state an error event is fired at the script element. For the others an error event is fired on the global.

Comment on attachment 8898776 [details] [diff] [review] bug1388728-module-loader Review of attachment 8898776 [details] [diff] [review]: ----------------------------------------------------------------- I'm sorry, but I'm having a really hard time reviewing this patch. It seems a lot of changes were made. Some code splitting between patches and better explanation of each patch is probably necessary for me to review it. (I also won't be offended if you want to find someone more knowledgeable of this code to review.) For example, I think it would be helpful to have separate patches for: * Backout of bug 1295978 * Renames of things like in nsJSUtils and some of the variables * Fix error handling and explain the functional difference * test changes Sorry, I know splitting stuff can be a pain, but right now I think any r+ I gave here would be a rubber stamp since I'm not really understanding it. ::: dom/base/nsJSUtils.h @@ +188,3 @@ > > + static nsresult ModuleEvaluate(JSContext* aCx, > + JS::Handle<JSObject*> aModule); FWIW, it would be a lot easier to review a patch like this if rename changes were done in a separate patch. ::: dom/script/ModuleLoadRequest.cpp @@ +108,5 @@ > + mLoader->CheckModuleDependenciesLoaded(this); > + MOZ_ASSERT(!mModuleScript || mModuleScript->IsErrored()); > + > + CancelImports(); > + SetReady(); So if the module is errored we cancel imports, but resolve the ready promise? We don't reject the promise? ::: dom/script/ModuleScript.cpp @@ -50,5 @@ > - > - // Make module's host defined field point to this module script object. > - // This is cleared in the UnlinkModuleRecord(). > - JS::SetModuleHostDefinedField(mModuleRecord, JS::PrivateValue(this)); > - HoldJSObjects(this); It seems the life cycle of this object changed? Can you explain that? I don't really understand why this moved out of the constructor into another method. @@ -65,3 @@ > } > - mModuleRecord = nullptr; > - mException.setUndefined(); Why don't you have to clear mError now? Was this never needed? ::: testing/web-platform/meta/html/semantics/scripting-1/the-script-element/module/instantiation-error-3.html.ini @@ +1,3 @@ > [instantiation-error-3.html] > type: testharness > + disabled: true Why disabled? ::: testing/web-platform/tests/html/semantics/scripting-1/the-script-element/module/compilation-error-1.html @@ +18,5 @@ > assert_equals(log[0].constructor, SyntaxError); > assert_true(log.every(exn => exn === log[0])); > })); > > + function unreachable() { /*log.push("unexpected");*/ } This seems wrong. ::: testing/web-platform/tests/html/semantics/scripting-1/the-script-element/module/errorhandling.html @@ +13,5 @@ > </head> > <body> > <h1>html-script-module-errorHandling</h1> > + //<iframe id="iframe_parseError_Root" src="errorhandling-parseerror-root.html"></iframe> > + //<iframe id="iframe_parseError_Dependent" src="errorhandling-parseerror-dependent.html"></iframe> Commenting out stuff in the WPT test also seems wrong in this file.

(In reply to Ben Kelly [:bkelly] from comment #4) Yes that's a really good point, I'll split it up. Thanks for the comments so far. > So if the module is errored we cancel imports, but resolve the ready > promise? We don't reject the promise? We only reject the promise for fetch failures and other things like OOM. Parse errors, instantiation errors and execution errors are recorded and the reported when we go to execute the module. > It seems the life cycle of this object changed? Can you explain that? I > don't really understand why this moved out of the constructor into another > method. We used to always construct a module script with a valid module record. Now it's possible to construct an 'errored' module script with null module record and a recorded exception. This is the case for parse errors for example. > Why don't you have to clear mError now? Was this never needed? It's cleared in the NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ModuleScript) block now. This now just unlinks the module record. > > [instantiation-error-3.html] > > type: testharness > > + disabled: true > > Why disabled? This test now fails intermittently. I *think* the test code is incorrect but I haven't confirmed this yet. > > + function unreachable() { /*log.push("unexpected");*/ } > > This seems wrong. Ugh, yeah I left debugging changes in. Sorry about that.

Rename ModuleDeclarationInstantiation to ModuleInstantiate and ModuleEvaluation to ModuleEvaluate.

Backout bug 1295978.

Update module error handling. The module loading algorithms in the spec return a possibly-null module script, which itself can be 'errored' in two different ways. So there are four possibilities when loading a module or graph of modules: 1. Fetch error - returns a null ModuleScript This also happens for other unexpected errors like OOM. 2. Pre-instantiation error - returns a ModuleScript with a null module record and a recorded error value. This happens for parse errors and failure to resolve imported module specifiers. The error value is recorded in ModuleScript::mError. 3. Remaining errors - returns a ModuleScript with an errored module record This is the case for instantiation and execution errors. The error is recored in the module record itself and ModuleScript::mError is undefined. 4. Success! -- Case 1 results in firing an event on the script. Cases 2 and 3 proceed the same as success up until we go to execute the script at which point we fire a global error event (in EvaluateScript). Other changes: We now have to attempt to resolve all imported modules before we start fetching any of them. This happens at the end of CreateModuleScript. Failure here is a pre-instantiation error, i.e. case 2. We also have to check all imported modules after they have been loaded and propagate any errors up to the parent (CheckModuleDependenciesLoaded). Hopefully that explains things a little more. It is really complicated. I don't currently know how we can improve that though.

Test code changes.

Thanks for splitting the patches up! I will probably review these monday morning unless you need them sooner. Sorry for the delay.

Comment on attachment 8898906 [details] [diff] [review] bug1388728-3-backout-eager-instantiation Review of attachment 8898906 [details] [diff] [review]: ----------------------------------------------------------------- I guess some other changes happened between bug 1295978 and this one, so its not a direct backout. Looks reasonable, though.

Comment on attachment 8898925 [details] [diff] [review] bug1388728-4-module-loader Review of attachment 8898925 [details] [diff] [review]: ----------------------------------------------------------------- Did my best here. I'm willing to r+ given there are WPT tests. ::: dom/script/ModuleLoadRequest.cpp @@ +48,5 @@ > ScriptLoadRequest::Cancel(); > mModuleScript = nullptr; > mProgress = ScriptLoadRequest::Progress::Ready; > + CancelImports(); > + mReady.RejectIfExists(NS_ERROR_FAILURE, __func__); Is this error code propagated to script? Can we use something more like NS_ERROR_DOM_ABORT_ERR? @@ +102,5 @@ > +ModuleLoadRequest::ModuleErrored() > +{ > + LOG(("ScriptLoadRequest (%p): Module errored", this)); > + > + MOZ_ASSERT(mModuleScript); This doesn't seem right given we call ModuleErrored() from ModuleLoaded() when !mModuleScript. @@ +105,5 @@ > + > + MOZ_ASSERT(mModuleScript); > + > + mLoader->CheckModuleDependenciesLoaded(this); > + MOZ_ASSERT(!mModuleScript || mModuleScript->IsErrored()); So if one of the dependent imports gets a fetch failure we need to return a nullptr mModuleScript here? ::: dom/script/ModuleScript.cpp @@ +21,5 @@ > NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ModuleScript) > NS_IMPL_CYCLE_COLLECTION_UNLINK(mLoader) > NS_IMPL_CYCLE_COLLECTION_UNLINK(mBaseURL) > tmp->UnlinkModuleRecord(); > + tmp->mError.setUndefined(); I'm not sure this is strictly necessary with SCRIPT_HOLDER classes any more, but I don't think it hurts. ::: dom/script/ScriptLoader.cpp @@ +571,5 @@ > message.Append(aSpecifier); > > + JS::Rooted<JS::Value> error(aCx); > + if (!CreateTypeError(aCx, aScript, message, &error)) > + return NS_ERROR_OUT_OF_MEMORY; Please use curly braces even for single line if-statement bodies. @@ +832,5 @@ > +ScriptLoader::CheckModuleDependenciesLoaded(ModuleLoadRequest* aRequest) > +{ > + LOG(("ScriptLoadRequest (%p): Check dependencies loaded", aRequest)); > + > + ModuleScript* moduleScript = aRequest->mModuleScript; Maybe make this a RefPtr<ModuleScript> or note that when you null out mModuleScript below that this pointer could start pointing to dead memory. Not a problem here, but could catch someone changing this in the future. @@ +841,5 @@ > + aRequest->mModuleScript = nullptr; > + LOG(("ScriptLoadRequest (%p): %p failed (load error)", aRequest, childScript)); > + return; > + } else if (childScript->IsErrored()) { > + moduleScript->SetPreInstantiationError(childScript->Error()); So we only propagate the first error? And the ordering of this is defined in the spec? Seems ok, but just want to make sure this matches the spec. I tried looking, but I'm somewhat unfamiliar with es style spec language.

Comment on attachment 8898926 [details] [diff] [review] bug1388728-5-tests Review of attachment 8898926 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/meta/html/semantics/scripting-1/the-script-element/module/instantiation-error-2.html.ini @@ -1,4 @@ > [instantiation-error-2.html] > type: testharness > - [Test that missing exports lead to SyntaxError events on window and load events on script, and that exceptions are remembered] > - expected: FAIL For .ini files like this where all expectations have been removed I think you should just `hg rm` the whole file.

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/932c5acc1f86 Add APIs to query module record errors and rename operations in line with spec r=bbouvier https://hg.mozilla.org/integration/mozilla-inbound/rev/fec5a36cf325 Rename JS APIs for loading modules in line with the spec r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/60a74915871b Remove eager module instantiation r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/d7dcccf15b0d Bug Bug 1388728 - Update module loader error handling to match the spec r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/4c8a48ebf999 Improve module loader mochitests and re-enable failing WPT tests r=bkelly

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9195fb1cc0f1 Disable intermittently failing WPT test that was missed from previous push r=bkelly

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/79ef23ceb1a0 Fix incorrect WPT metadata r=me

https://hg.mozilla.org/mozilla-central/rev/932c5acc1f86 https://hg.mozilla.org/mozilla-central/rev/fec5a36cf325 https://hg.mozilla.org/mozilla-central/rev/60a74915871b https://hg.mozilla.org/mozilla-central/rev/d7dcccf15b0d https://hg.mozilla.org/mozilla-central/rev/4c8a48ebf999 https://hg.mozilla.org/mozilla-central/rev/9195fb1cc0f1 https://hg.mozilla.org/mozilla-central/rev/79ef23ceb1a0