Issue #618 - Align error handling for module scripts with the spec (again)

This updates module implementation to match spec regarding handling of
instantiation errors, after it was changed yet again, this time to not remember
instantiation errors, but instead immediately rethrow applicable ones.

Ref: BZ 1420420

5 files changed, 79 insertions, 58 deletions

diff --git a/dom/script/ModuleLoadRequest.cpp b/dom/script/ModuleLoadRequest.cpp
index d622143044..a12fcb1629 100644
--- a/dom/script/ModuleLoadRequest.cpp
+++ b/dom/script/ModuleLoadRequest.cpp
@@ -83,7 +83,7 @@ ModuleLoadRequest::ModuleLoaded()
   // been loaded.
 
   mModuleScript = mLoader->GetFetchedModule(mURI);
-  if (!mModuleScript || mModuleScript->IsErrored()) {
+  if (!mModuleScript || mModuleScript->HasParseError()) {
     ModuleErrored();
     return;
   }
@@ -95,7 +95,7 @@ void
 ModuleLoadRequest::ModuleErrored()
 {
   mLoader->CheckModuleDependenciesLoaded(this);
-  MOZ_ASSERT(!mModuleScript || mModuleScript->IsErrored());
+  MOZ_ASSERT(!mModuleScript || mModuleScript->HasParseError());
 
   CancelImports();
   SetReady();
diff --git a/dom/script/ModuleScript.cpp b/dom/script/ModuleScript.cpp
index 28b97a3cb5..1bf9d0b0fc 100644
--- a/dom/script/ModuleScript.cpp
+++ b/dom/script/ModuleScript.cpp
@@ -26,7 +26,8 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ModuleScript)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mLoader)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mBaseURL)
   tmp->UnlinkModuleRecord();
-  tmp->mError.setUndefined();
+  tmp->mParseError.setUndefined();
+  tmp->mErrorToRethrow.setUndefined();
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(ModuleScript)
@@ -35,7 +36,8 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 
 NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(ModuleScript)
   NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mModuleRecord)
-  NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mError)
+  NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mParseError)
+  NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mErrorToRethrow)
 NS_IMPL_CYCLE_COLLECTION_TRACE_END
 
 NS_IMPL_CYCLE_COLLECTING_ADDREF(ModuleScript)
@@ -48,7 +50,8 @@ ModuleScript::ModuleScript(ScriptLoader *aLoader, nsIURI* aBaseURL)
   MOZ_ASSERT(mLoader);
   MOZ_ASSERT(mBaseURL);
   MOZ_ASSERT(!mModuleRecord);
-  MOZ_ASSERT(mError.isUndefined());
+  MOZ_ASSERT(!HasParseError());
+  MOZ_ASSERT(!HasErrorToRethrow());
 }
 
 void
@@ -74,7 +77,8 @@ void
 ModuleScript::SetModuleRecord(JS::Handle<JSObject*> aModuleRecord)
 {
   MOZ_ASSERT(!mModuleRecord);
-  MOZ_ASSERT(mError.isUndefined());
+  MOZ_ASSERT(!HasParseError());
+  MOZ_ASSERT(!HasErrorToRethrow());
 
   mModuleRecord = aModuleRecord;
 
@@ -85,37 +89,24 @@ ModuleScript::SetModuleRecord(JS::Handle<JSObject*> aModuleRecord)
 }
 
 void
-ModuleScript::SetPreInstantiationError(const JS::Value& aError)
+ModuleScript::SetParseError(const JS::Value& aError)
 {
   MOZ_ASSERT(!aError.isUndefined());
+  MOZ_ASSERT(!HasParseError());
+  MOZ_ASSERT(!HasErrorToRethrow());
 
   UnlinkModuleRecord();
-  mError = aError;
-
+  mParseError = aError;
   HoldJSObjects(this);
 }
 
-bool
-ModuleScript::IsErrored() const
-{
-  if (!mModuleRecord) {
-    MOZ_ASSERT(!mError.isUndefined());
-    return true;
-  }
-
-  return JS::IsModuleErrored(mModuleRecord);
-}
-
-JS::Value
-ModuleScript::Error() const
+void
+ModuleScript::SetErrorToRethrow(const JS::Value& aError)
 {
-  MOZ_ASSERT(IsErrored());
-
-  if (!mModuleRecord) {
-    return mError;
-  }
+  MOZ_ASSERT(!aError.isUndefined());
+  MOZ_ASSERT(!HasErrorToRethrow());
 
-  return JS::GetModuleError(mModuleRecord);
+  mErrorToRethrow = aError;
 }
 
 } // dom namespace
diff --git a/dom/script/ModuleScript.h b/dom/script/ModuleScript.h
index 5713598592..f765aa0fad 100644
--- a/dom/script/ModuleScript.h
+++ b/dom/script/ModuleScript.h
@@ -23,7 +23,8 @@ class ModuleScript final : public nsISupports
   RefPtr<ScriptLoader> mLoader;
   nsCOMPtr<nsIURI> mBaseURL;
   JS::Heap<JSObject*> mModuleRecord;
-  JS::Heap<JS::Value> mError;
+  JS::Heap<JS::Value> mParseError;
+  JS::Heap<JS::Value> mErrorToRethrow;
 
   ~ModuleScript();
 
@@ -35,14 +36,17 @@ public:
                nsIURI* aBaseURL);
 
   void SetModuleRecord(JS::Handle<JSObject*> aModuleRecord);
-  void SetPreInstantiationError(const JS::Value& aError);
+  void SetParseError(const JS::Value& aError);
+  void SetErrorToRethrow(const JS::Value& aError);
 
   ScriptLoader* Loader() const { return mLoader; }
   JSObject* ModuleRecord() const { return mModuleRecord; }
   nsIURI* BaseURL() const { return mBaseURL; }
 
-  bool IsErrored() const;
-  JS::Value Error() const;
+  JS::Value ParseError() const { return mParseError; }
+  JS::Value ErrorToRethrow() const { return mErrorToRethrow; }
+  bool HasParseError() const { return !mParseError.isUndefined(); }
+  bool HasErrorToRethrow() const { return !mErrorToRethrow.isUndefined(); }
 
   void UnlinkModuleRecord();
 };
diff --git a/dom/script/ScriptLoader.cpp b/dom/script/ScriptLoader.cpp
index 1426c30c9c..3dbbcaceae 100644
--- a/dom/script/ScriptLoader.cpp
+++ b/dom/script/ScriptLoader.cpp
@@ -493,7 +493,7 @@ ScriptLoader::ProcessFetchedModuleSource(ModuleLoadRequest* aRequest)
     return rv;
   }
 
-  if (!aRequest->mModuleScript->IsErrored()) {
+  if (!aRequest->mModuleScript->HasParseError()) {
     StartFetchingModuleDependencies(aRequest);
   }
 
@@ -568,7 +568,7 @@ ScriptLoader::CreateModuleScript(ModuleLoadRequest* aRequest)
         return NS_ERROR_FAILURE;
       }
 
-      moduleScript->SetPreInstantiationError(error);
+      moduleScript->SetParseError(error);
       aRequest->ModuleErrored();
       return NS_OK;
     }
@@ -580,8 +580,6 @@ ScriptLoader::CreateModuleScript(ModuleLoadRequest* aRequest)
     nsCOMArray<nsIURI> urls;
     rv = ResolveRequestedModules(aRequest, urls);
     if (NS_FAILED(rv)) {
-      // ResolveRequestedModules sets pre-instanitation error on failure.
-      MOZ_ASSERT(moduleScript->IsErrored());
       aRequest->ModuleErrored();
       return NS_OK;
     }
@@ -627,7 +625,7 @@ HandleResolveFailure(JSContext* aCx, ModuleScript* aScript,
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
-  aScript->SetPreInstantiationError(error);
+  aScript->SetParseError(error);
   return NS_OK;
 }
 
@@ -723,7 +721,6 @@ ResolveRequestedModules(ModuleLoadRequest* aRequest, nsCOMArray<nsIURI> &aUrls)
     }
 
     // Let url be the result of resolving a module specifier given module script and requested.
-    ModuleScript* ms = aRequest->mModuleScript;
     nsCOMPtr<nsIURI> uri = ResolveModuleSpecifier(ms, specifier);
     if (!uri) {
       nsresult rv = HandleResolveFailure(cx, ms, specifier);
@@ -746,7 +743,7 @@ void
 ScriptLoader::StartFetchingModuleDependencies(ModuleLoadRequest* aRequest)
 {
   MOZ_ASSERT(aRequest->mModuleScript);
-  MOZ_ASSERT(!aRequest->mModuleScript->IsErrored());
+  MOZ_ASSERT(!aRequest->mModuleScript->HasParseError());
   MOZ_ASSERT(!aRequest->IsReadyToRun());
 
   aRequest->mProgress = ModuleLoadRequest::Progress::FetchingImports;
@@ -843,7 +840,7 @@ HostResolveImportedModule(JSContext* aCx, unsigned argc, JS::Value* vp)
   ModuleScript* ms = script->Loader()->GetFetchedModule(uri);
   MOZ_ASSERT(ms, "Resolved module not found in module map");
 
-  MOZ_ASSERT(!ms->IsErrored());
+  MOZ_ASSERT(!ms->HasParseError());
 
   *vp = JS::ObjectValue(*ms->ModuleRecord());
   return true;
@@ -871,18 +868,16 @@ void
 ScriptLoader::CheckModuleDependenciesLoaded(ModuleLoadRequest* aRequest)
 {
   RefPtr<ModuleScript> moduleScript = aRequest->mModuleScript;
-  if (moduleScript && !moduleScript->IsErrored()) {
-    for (auto childRequest : aRequest->mImports) {
-      ModuleScript* childScript = childRequest->mModuleScript;
-      if (!childScript) {
-        // Load error
-        aRequest->mModuleScript = nullptr;
-        return;
-      } else if (childScript->IsErrored()) {
-        // Script error
-        moduleScript->SetPreInstantiationError(childScript->Error());
-        return;
-      }
+  if (!moduleScript || moduleScript->HasParseError()) {
+    return;
+  }
+
+  for (auto childRequest : aRequest->mImports) {
+    ModuleScript* childScript = childRequest->mModuleScript;
+    if (!childScript) {
+      aRequest->mModuleScript = nullptr;
+      // Load error on script load request; bail.
+      return;
     }
   }
 }
@@ -892,7 +887,7 @@ ScriptLoader::ProcessLoadedModuleTree(ModuleLoadRequest* aRequest)
 {
   if (aRequest->IsTopLevel()) {
     ModuleScript* moduleScript = aRequest->mModuleScript;
-    if (moduleScript && !moduleScript->IsErrored()) {
+    if (moduleScript && !moduleScript->HasErrorToRethrow()) {
       if (!InstantiateModuleTree(aRequest)) {
         aRequest->mModuleScript = nullptr;
       }
@@ -906,6 +901,28 @@ ScriptLoader::ProcessLoadedModuleTree(ModuleLoadRequest* aRequest)
   }
 }
 
+JS::Value
+ScriptLoader::FindFirstParseError(ModuleLoadRequest* aRequest)
+{
+  MOZ_ASSERT(aRequest);
+
+  ModuleScript* moduleScript = aRequest->mModuleScript;
+  MOZ_ASSERT(moduleScript);
+
+  if (moduleScript->HasParseError()) {
+    return moduleScript->ParseError();
+  }
+
+  for (ModuleLoadRequest* childRequest : aRequest->mImports) {
+    JS::Value error = FindFirstParseError(childRequest);
+    if (!error.isUndefined()) {
+      return error;
+    }
+  }
+
+  return JS::UndefinedValue();
+}
+
 bool
 ScriptLoader::InstantiateModuleTree(ModuleLoadRequest* aRequest)
 {
@@ -916,6 +933,14 @@ ScriptLoader::InstantiateModuleTree(ModuleLoadRequest* aRequest)
 
   ModuleScript* moduleScript = aRequest->mModuleScript;
   MOZ_ASSERT(moduleScript);
+
+  JS::Value parseError = FindFirstParseError(aRequest);
+  if (!parseError.isUndefined()) {
+    // Parse error found in the requested script
+    moduleScript->SetErrorToRethrow(parseError);
+    return true;
+  }
+
   MOZ_ASSERT(moduleScript->ModuleRecord());
 
   nsAutoMicroTask mt;
@@ -937,7 +962,7 @@ ScriptLoader::InstantiateModuleTree(ModuleLoadRequest* aRequest)
       return false;
     }
     MOZ_ASSERT(!exception.isUndefined());
-    // Ignore the exception. It will be recorded in the module record.
+    moduleScript->SetErrorToRethrow(exception);
   }
 
   return true;
@@ -1462,7 +1487,7 @@ ScriptLoader::ProcessScriptElement(nsIScriptElement *aElement)
     } else {
       AddDeferRequest(request);
     }
-    if (!modReq->mModuleScript->IsErrored()) {
+    if (!modReq->mModuleScript->HasParseError()) {
       StartFetchingModuleDependencies(modReq);
     }
     return false;
@@ -1966,9 +1991,9 @@ ScriptLoader::EvaluateScript(ScriptLoadRequest* aRequest)
       MOZ_ASSERT(!request->mOffThreadToken);
 
       ModuleScript* moduleScript = request->mModuleScript;
-      if (moduleScript->IsErrored()) {
-        // Module has an error status
-        JS::Rooted<JS::Value> error(cx, moduleScript->Error());
+      if (moduleScript->HasErrorToRethrow()) {
+        // Module has an error status to be rethrown
+        JS::Rooted<JS::Value> error(cx, moduleScript->ErrorToRethrow());
         JS_SetPendingException(cx, error);
         return NS_OK; // An error is reported by AutoEntryScript.
       }
diff --git a/dom/script/ScriptLoader.h b/dom/script/ScriptLoader.h
index b07dd4ec66..db2eeed31a 100644
--- a/dom/script/ScriptLoader.h
+++ b/dom/script/ScriptLoader.h
@@ -583,6 +583,7 @@ private:
   nsresult ProcessFetchedModuleSource(ModuleLoadRequest* aRequest);
   void CheckModuleDependenciesLoaded(ModuleLoadRequest* aRequest);
   void ProcessLoadedModuleTree(ModuleLoadRequest* aRequest);
+  JS::Value FindFirstParseError(ModuleLoadRequest* aRequest);
   bool InstantiateModuleTree(ModuleLoadRequest* aRequest);
   void StartFetchingModuleDependencies(ModuleLoadRequest* aRequest);