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 d62214304..a12fcb162 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 28b97a3cb..1bf9d0b0f 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 571359859..f765aa0fa 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 1426c30c9..3dbbcacea 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 b07dd4ec6..db2eeed31 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);