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

15 files changed, 252 insertions, 208 deletions

diff --git a/js/src/builtin/Module.js b/js/src/builtin/Module.js
index 64d62d216..86b44880d 100644
--- a/js/src/builtin/Module.js
+++ b/js/src/builtin/Module.js
@@ -69,8 +69,13 @@ function ModuleSetStatus(module, newStatus)
 {
     assert(newStatus >= MODULE_STATUS_ERRORED && newStatus <= MODULE_STATUS_EVALUATED,
            "Bad new module status in ModuleSetStatus");
-    if (newStatus !== MODULE_STATUS_ERRORED)
-        assert(newStatus > module.status, "New module status inconsistent with current status");
+
+    // Note that under OOM conditions we can fail the module instantiation
+    // process even after modules have been marked as instantiated.
+    assert((module.status <= MODULE_STATUS_INSTANTIATED &&
+            newStatus === MODULE_STATUS_UNINSTANTIATED) ||
+           newStatus > module.status,
+           "New module status inconsistent with current status");
 
     UnsafeSetReservedSlot(module, MODULE_OBJECT_STATUS_SLOT, newStatus);
 }
@@ -80,20 +85,15 @@ function ModuleSetStatus(module, newStatus)
 // Returns an object describing the location of the resolved export or
 // indicating a failure.
 //
-// On success this returns: { resolved: true, module, bindingName }
-//
-// There are three failure cases:
+// On success this returns a resolved binding record: { module, bindingName }
 //
-//  - The resolution failure can be blamed on a particular module.
-//    Returns: { resolved: false, module, ambiguous: false }
+// There are two failure cases:
 //
-//  - No culprit can be determined and the resolution failure was due to star
-//    export ambiguity.
-//    Returns: { resolved: false, module: null, ambiguous: true }
+//  - If no definition was found or the request is found to be circular, *null*
+//    is returned.
 //
-//  - No culprit can be determined and the resolution failure was not due to
-//    star export ambiguity.
-//    Returns: { resolved: false, module: null, ambiguous: false }
+//  - If the request is found to be ambiguous, the string `"ambiguous"` is
+//    returned.
 //
 function ModuleResolveExport(exportName, resolveSet = [])
 {
@@ -106,53 +106,47 @@ function ModuleResolveExport(exportName, resolveSet = [])
     let module = this;
 
     // Step 2
-    assert(module.status !== MODULE_STATUS_ERRORED, "Bad module status in ResolveExport");
-    
-    // Step 3
     for (let i = 0; i < resolveSet.length; i++) {
         let r = resolveSet[i];
         if (r.module === module && r.exportName === exportName) {
             // This is a circular import request.
-            return {resolved: false, module: null, ambiguous: false};
+            return null;
         }
     }
 
-    // Step 4
+    // Step 3
     _DefineDataProperty(resolveSet, resolveSet.length, {module: module, exportName: exportName});
 
-    // Step 5
+    // Step 4
     let localExportEntries = module.localExportEntries;
     for (let i = 0; i < localExportEntries.length; i++) {
         let e = localExportEntries[i];
         if (exportName === e.exportName)
-            return {resolved: true, module, bindingName: e.localName};
+            return {module, bindingName: e.localName};
     }
 
-    // Step 6
+    // Step 5
     let indirectExportEntries = module.indirectExportEntries;
     for (let i = 0; i < indirectExportEntries.length; i++) {
         let e = indirectExportEntries[i];
         if (exportName === e.exportName) {
             let importedModule = CallModuleResolveHook(module, e.moduleRequest,
                                                        MODULE_STATUS_UNINSTANTIATED);
-            let resolution = callFunction(importedModule.resolveExport, importedModule, e.importName,
-                                          resolveSet);
-            if (!resolution.resolved && !resolution.module)
-                resolution.module = module;
-            return resolution;
+            return callFunction(importedModule.resolveExport, importedModule, e.importName,
+                                resolveSet);
         }
     }
 
-    // Step 7
+    // Step 6
     if (exportName === "default") {
         // A default export cannot be provided by an export *.
-        return {resolved: false, module: null, ambiguous: false};
+        return null;
     }
 
-    // Step 8
+    // Step 7
     let starResolution = null;
 
-    // Step 9
+    // Step 8
     let starExportEntries = module.starExportEntries;
     for (let i = 0; i < starExportEntries.length; i++) {
         let e = starExportEntries[i];
@@ -160,27 +154,31 @@ function ModuleResolveExport(exportName, resolveSet = [])
                                                    MODULE_STATUS_UNINSTANTIATED);
         let resolution = callFunction(importedModule.resolveExport, importedModule, exportName,
                                       resolveSet);
-        if (!resolution.resolved && (resolution.module || resolution.ambiguous))
+        if (resolution === "ambiguous")
             return resolution;
 
-        if (resolution.resolved) {
+        if (resolution !== null) {
             if (starResolution === null) {
                 starResolution = resolution;
             } else {
                 if (resolution.module !== starResolution.module ||
                     resolution.bindingName !== starResolution.bindingName)
                 {
-                    return {resolved: false, module: null, ambiguous: true};
+                    return "ambiguous";
                 }
             }
         }
     }
 
-    // Step 10
-    if (starResolution !== null)
-        return starResolution;
+    // Step 9
+    return starResolution;
+}
 
-    return {resolved: false, module: null, ambiguous: false};
+function IsResolvedBinding(resolution)
+{
+    assert(resolution === "ambiguous" || typeof resolution === "object",
+           "Bad module resolution result");
+    return typeof resolution === "object" && resolution !== null;
 }
 
 // 15.2.1.18 GetModuleNamespace(module)
@@ -189,12 +187,12 @@ function GetModuleNamespace(module)
     // Step 1
     assert(IsModule(module), "GetModuleNamespace called with non-module");
 
-    // Step 2
+    // Steps 2-3
     assert(module.status !== MODULE_STATUS_UNINSTANTIATED &&
-           module.status !== MODULE_STATUS_ERRORED,
+           module.status !== MODULE_STATUS_EVALUATED_ERROR,
            "Bad module status in GetModuleNamespace");
 
-    // Step 3
+    // Step 4
     let namespace = module.namespace;
 
     if (typeof namespace === "undefined") {
@@ -203,7 +201,7 @@ function GetModuleNamespace(module)
         for (let i = 0; i < exportedNames.length; i++) {
             let name = exportedNames[i];
             let resolution = callFunction(module.resolveExport, module, name);
-            if (resolution.resolved)
+            if (IsResolvedBinding(resolution))
                 _DefineDataProperty(unambiguousNames, unambiguousNames.length, name);
         }
         namespace = ModuleNamespaceCreate(module, unambiguousNames);
@@ -225,7 +223,7 @@ function ModuleNamespaceCreate(module, exports)
     for (let i = 0; i < exports.length; i++) {
         let name = exports[i];
         let binding = callFunction(module.resolveExport, module, name);
-        assert(binding.resolved, "Failed to resolve binding");
+        assert(IsResolvedBinding(binding), "Failed to resolve binding");
         AddModuleNamespaceBinding(ns, name, binding.module, binding.bindingName);
     }
 
@@ -236,11 +234,6 @@ function GetModuleEnvironment(module)
 {
     assert(IsModule(module), "Non-module passed to GetModuleEnvironment");
 
-    // Check for a previous failed attempt to instantiate this module. This can
-    // only happen due to a bug in the module loader.
-    if (module.status === MODULE_STATUS_ERRORED)
-        ThrowInternalError(JSMSG_MODULE_INSTANTIATE_FAILED, module.status);
-
     let env = UnsafeGetReservedSlot(module, MODULE_OBJECT_ENVIRONMENT_SLOT);
     assert(env === undefined || IsModuleEnvironment(env),
            "Module environment slot contains unexpected value");
@@ -248,19 +241,6 @@ function GetModuleEnvironment(module)
     return env;
 }
 
-function RecordModuleError(module, error)
-{
-    // Set the module's status to 'errored' to indicate a failed module
-    // instantiation and record the exception. The environment slot is also
-    // reset to 'undefined'.
-
-    assert(IsObject(module) && IsModule(module), "Non-module passed to RecordModuleError");
-
-    ModuleSetStatus(module, MODULE_STATUS_ERRORED);
-    UnsafeSetReservedSlot(module, MODULE_OBJECT_ERROR_SLOT, error);
-    UnsafeSetReservedSlot(module, MODULE_OBJECT_ENVIRONMENT_SLOT, undefined);
-}
-
 function CountArrayValues(array, value)
 {
     let count = 0;
@@ -280,6 +260,16 @@ function ArrayContains(array, value)
     return false;
 }
 
+function HandleModuleInstantiationFailure(module)
+{
+    // Reset the module to the "uninstantiated" state. Don't reset the
+    // environment slot as the environment object will be required by any
+    // possible future instantiation attempt.
+    ModuleSetStatus(module, MODULE_STATUS_UNINSTANTIATED);
+    UnsafeSetReservedSlot(module, MODULE_OBJECT_DFS_INDEX_SLOT, undefined);
+    UnsafeSetReservedSlot(module, MODULE_OBJECT_DFS_ANCESTOR_INDEX_SLOT, undefined);
+}
+
 // 15.2.1.16.4 ModuleInstantiate()
 function ModuleInstantiate()
 {
@@ -301,38 +291,30 @@ function ModuleInstantiate()
 
     // Steps 4-5
     try {
-        InnerModuleDeclarationInstantiation(module, stack, 0);
+        InnerModuleInstantiation(module, stack, 0);
     } catch (error) {
         for (let i = 0; i < stack.length; i++) {
             let m = stack[i];
 
-            assert(m.status === MODULE_STATUS_INSTANTIATING ||
-                   m.status === MODULE_STATUS_ERRORED,
-                   "Bad module status after failed instantiation");
-
-            RecordModuleError(m, error);
+            assert(m.status === MODULE_STATUS_INSTANTIATING,
+                   "Expected instantiating status during failed instantiation");
+            HandleModuleInstantiationFailure(m);
         }
 
-        if (stack.length === 0 &&
-            typeof(UnsafeGetReservedSlot(module, MODULE_OBJECT_ERROR_SLOT)) === "undefined")
-        {
-            // This can happen due to OOM when appending to the stack.
-            assert(error === "out of memory",
-                   "Stack must contain module unless we hit OOM");
-            RecordModuleError(module, error);
-        }
+        // Handle OOM when appending to the stack or over-recursion errors.
+        if (stack.length === 0)
+            HandleModuleInstantiationFailure(module);
 
-        assert(module.status === MODULE_STATUS_ERRORED,
-               "Bad module status after failed instantiation");
-        assert(SameValue(UnsafeGetReservedSlot(module, MODULE_OBJECT_ERROR_SLOT), error),
-               "Module has different error set after failed instantiation");
+        assert(module.status === MODULE_STATUS_UNINSTANTIATED,
+               "Expected uninstantiated status after failed instantiation");
 
         throw error;
     }
 
     // Step 6
-    assert(module.status == MODULE_STATUS_INSTANTIATED ||
-           module.status == MODULE_STATUS_EVALUATED,
+    assert(module.status === MODULE_STATUS_INSTANTIATED ||
+           module.status === MODULE_STATUS_EVALUATED ||
+           module.status === MODULE_STATUS_EVALUATED_ERROR,
            "Bad module status after successful instantiation");
 
     // Step 7
@@ -344,8 +326,8 @@ function ModuleInstantiate()
 }
 _SetCanonicalName(ModuleInstantiate, "ModuleInstantiate");
 
-// 15.2.1.16.4.1 InnerModuleDeclarationInstantiation(module, stack, index)
-function InnerModuleDeclarationInstantiation(module, stack, index)
+// 15.2.1.16.4.1 InnerModuleInstantiation(module, stack, index)
+function InnerModuleInstantiation(module, stack, index)
 {
     // Step 1
     // TODO: Support module records other than source text module records.
@@ -353,42 +335,40 @@ function InnerModuleDeclarationInstantiation(module, stack, index)
     // Step 2
     if (module.status === MODULE_STATUS_INSTANTIATING ||
         module.status === MODULE_STATUS_INSTANTIATED ||
-        module.status === MODULE_STATUS_EVALUATED)
+        module.status === MODULE_STATUS_EVALUATED ||
+        module.status === MODULE_STATUS_EVALUATED_ERROR)
     {
         return index;
     }
 
     // Step 3
-    if (module.status === MODULE_STATUS_ERRORED)
-        throw module.error;
-
-    // Step 4
     assert(module.status === MODULE_STATUS_UNINSTANTIATED,
           "Bad module status in ModuleDeclarationInstantiation");
 
-    // Steps 5
+    // Step 4
     ModuleSetStatus(module, MODULE_STATUS_INSTANTIATING);
 
-    // Step 6-8
+    // Steps 5-7
     UnsafeSetReservedSlot(module, MODULE_OBJECT_DFS_INDEX_SLOT, index);
     UnsafeSetReservedSlot(module, MODULE_OBJECT_DFS_ANCESTOR_INDEX_SLOT, index);
     index++;
 
-    // Step 9
+    // Step 8
     _DefineDataProperty(stack, stack.length, module);
 
-    // Step 10
+    // Step 9
     let requestedModules = module.requestedModules;
     for (let i = 0; i < requestedModules.length; i++) {
         let required = requestedModules[i];
-        let requiredModule = CallModuleResolveHook(module, required, MODULE_STATUS_ERRORED);
+        let requiredModule = CallModuleResolveHook(module, required, MODULE_STATUS_UNINSTANTIATED);
 
-        index = InnerModuleDeclarationInstantiation(requiredModule, stack, index);
+        index = InnerModuleInstantiation(requiredModule, stack, index);
 
         assert(requiredModule.status === MODULE_STATUS_INSTANTIATING ||
                requiredModule.status === MODULE_STATUS_INSTANTIATED ||
-               requiredModule.status === MODULE_STATUS_EVALUATED,
-               "Bad required module status after InnerModuleDeclarationInstantiation");
+               requiredModule.status === MODULE_STATUS_EVALUATED ||
+               requiredModule.status === MODULE_STATUS_EVALUATED_ERROR,
+               "Bad required module status after InnerModuleInstantiation");
 
         assert((requiredModule.status === MODULE_STATUS_INSTANTIATING) ===
                ArrayContains(stack, requiredModule),
@@ -404,16 +384,16 @@ function InnerModuleDeclarationInstantiation(module, stack, index)
         }
     }
 
-    // Step 11
+    // Step 10
     ModuleDeclarationEnvironmentSetup(module);
 
-    // Steps 12-13
+    // Steps 11-12
     assert(CountArrayValues(stack, module) === 1,
            "Current module should appear exactly once in the stack");
     assert(module.dfsAncestorIndex <= module.dfsIndex,
            "Bad DFS ancestor index");
 
-    // Step 14
+    // Step 13
     if (module.dfsAncestorIndex === module.dfsIndex) {
         let requiredModule;
         do {
@@ -434,11 +414,9 @@ function ModuleDeclarationEnvironmentSetup(module)
     for (let i = 0; i < indirectExportEntries.length; i++) {
         let e = indirectExportEntries[i];
         let resolution = callFunction(module.resolveExport, module, e.exportName);
-        assert(resolution.resolved || resolution.module,
-               "Unexpected failure to resolve export in ModuleDeclarationEnvironmentSetup");
-        if (!resolution.resolved) {
-            return ResolutionError(resolution, "indirectExport", e.exportName,
-                                   e.lineNumber, e.columnNumber)
+        if (!IsResolvedBinding(resolution)) {
+            ThrowResolutionError(module, resolution, "indirectExport", e.exportName,
+                                 e.lineNumber, e.columnNumber);
         }
     }
 
@@ -458,12 +436,9 @@ function ModuleDeclarationEnvironmentSetup(module)
         } else {
             let resolution = callFunction(importedModule.resolveExport, importedModule,
                                           imp.importName);
-            if (!resolution.resolved && !resolution.module)
-                resolution.module = module;
-
-            if (!resolution.resolved) {
-                return ResolutionError(resolution, "import", imp.importName,
-                                       imp.lineNumber, imp.columnNumber);
+            if (!IsResolvedBinding(resolution)) {
+                ThrowResolutionError(module, resolution, "import", imp.importName,
+                                     imp.lineNumber, imp.columnNumber);
             }
 
             CreateImportBinding(env, imp.localName, resolution.module, resolution.bindingName);
@@ -473,38 +448,58 @@ function ModuleDeclarationEnvironmentSetup(module)
     InstantiateModuleFunctionDeclarations(module);
 }
 
-// 15.2.1.16.4.3 ResolutionError(module)
-function ResolutionError(resolution, kind, name, line, column)
+function ThrowResolutionError(module, resolution, kind, name, line, column)
 {
-    let module = resolution.module;
-    assert(module !== null,
-           "Null module passed to ResolutionError");
-
-    assert(module.status === MODULE_STATUS_UNINSTANTIATED ||
-           module.status === MODULE_STATUS_INSTANTIATING,
-           "Unexpected module status in ResolutionError");
+    assert(module.status === MODULE_STATUS_INSTANTIATING,
+           "Unexpected module status in ThrowResolutionError");
 
     assert(kind === "import" || kind === "indirectExport",
-           "Unexpected kind in ResolutionError");
+           "Unexpected kind in ThrowResolutionError");
 
     assert(line > 0,
            "Line number should be present for all imports and indirect exports");
 
+    let ambiguous = resolution === "ambiguous";
+
     let errorNumber;
-    if (kind === "import") {
-        errorNumber = resolution.ambiguous ? JSMSG_AMBIGUOUS_IMPORT
-                                           : JSMSG_MISSING_IMPORT;
-    } else {
-        errorNumber = resolution.ambiguous ? JSMSG_AMBIGUOUS_INDIRECT_EXPORT
-                                           : JSMSG_MISSING_INDIRECT_EXPORT;
-    }
+    if (kind === "import")
+        errorNumber = ambiguous ? JSMSG_AMBIGUOUS_IMPORT : JSMSG_MISSING_IMPORT;
+    else
+        errorNumber = ambiguous ? JSMSG_AMBIGUOUS_INDIRECT_EXPORT : JSMSG_MISSING_INDIRECT_EXPORT;
 
     let message = GetErrorMessage(errorNumber) + ": " + name;
     let error = CreateModuleSyntaxError(module, line, column, message);
-    RecordModuleError(module, error);
     throw error;
 }
 
+function GetModuleEvaluationError(module)
+{
+    assert(IsObject(module) && IsModule(module),
+           "Non-module passed to GetModuleEvaluationError");
+    assert(module.status === MODULE_STATUS_EVALUATED_ERROR,
+           "Bad module status in GetModuleEvaluationError");
+    return UnsafeGetReservedSlot(module, MODULE_OBJECT_EVALUATION_ERROR_SLOT);
+}
+
+function RecordModuleEvaluationError(module, error)
+{
+    // Set the module's EvaluationError slot to indicate a failed module
+    // evaluation.
+
+    assert(IsObject(module) && IsModule(module),
+           "Non-module passed to RecordModuleEvaluationError");
+
+    if (module.status === MODULE_STATUS_EVALUATED_ERROR) {
+        // It would be nice to assert that |error| is the same as the one we
+        // previously recorded, but that's not always true in the case of out of
+        // memory and over-recursion errors.
+        return;
+    }
+
+    ModuleSetStatus(module, MODULE_STATUS_EVALUATED_ERROR);
+    UnsafeSetReservedSlot(module, MODULE_OBJECT_EVALUATION_ERROR_SLOT, error);
+}
+
 // 15.2.1.16.5 ModuleEvaluate()
 function ModuleEvaluate()
 {
@@ -515,9 +510,9 @@ function ModuleEvaluate()
     let module = this;
 
     // Step 2
-    if (module.status !== MODULE_STATUS_ERRORED &&
-        module.status !== MODULE_STATUS_INSTANTIATED &&
-        module.status !== MODULE_STATUS_EVALUATED)
+    if (module.status !== MODULE_STATUS_INSTANTIATED &&
+        module.status !== MODULE_STATUS_EVALUATED &&
+        module.status !== MODULE_STATUS_EVALUATED_ERROR)
     {
         ThrowInternalError(JSMSG_BAD_MODULE_STATUS);
     }
@@ -535,27 +530,20 @@ function ModuleEvaluate()
             assert(m.status === MODULE_STATUS_EVALUATING,
                    "Bad module status after failed evaluation");
 
-            RecordModuleError(m, error);
+            RecordModuleEvaluationError(m, error);
         }
 
-        if (stack.length === 0 &&
-            typeof(UnsafeGetReservedSlot(module, MODULE_OBJECT_ERROR_SLOT)) === "undefined")
-        {
-            // This can happen due to OOM when appending to the stack.
-            assert(error === "out of memory",
-                  "Stack must contain module unless we hit OOM");
-            RecordModuleError(module, error);
-        }
+        // Handle OOM when appending to the stack or over-recursion errors.
+        if (stack.length === 0)
+            RecordModuleEvaluationError(module, error);
 
-        assert(module.status === MODULE_STATUS_ERRORED,
+        assert(module.status === MODULE_STATUS_EVALUATED_ERROR,
                "Bad module status after failed evaluation");
-        assert(SameValue(UnsafeGetReservedSlot(module, MODULE_OBJECT_ERROR_SLOT), error),
-               "Module has different error set after failed evaluation");
 
         throw error;
     }
 
-    assert(module.status == MODULE_STATUS_EVALUATED,
+    assert(module.status === MODULE_STATUS_EVALUATED,
            "Bad module status after successful evaluation");
     assert(stack.length === 0,
            "Stack should be empty after successful evaluation");
@@ -571,19 +559,19 @@ function InnerModuleEvaluation(module, stack, index)
     // TODO: Support module records other than source text module records.
 
     // Step 2
-    if (module.status === MODULE_STATUS_EVALUATING ||
-        module.status === MODULE_STATUS_EVALUATED)
-    {
+    if (module.status === MODULE_STATUS_EVALUATED_ERROR)
+        throw GetModuleEvaluationError(module);
+
+    if (module.status === MODULE_STATUS_EVALUATED)
         return index;
-    }
 
     // Step 3
-    if (module.status === MODULE_STATUS_ERRORED)
-        throw module.error;
+    if (module.status === MODULE_STATUS_EVALUATING)
+        return index;
 
     // Step 4
     assert(module.status === MODULE_STATUS_INSTANTIATED,
-          "Bad module status in ModuleEvaluation");
+          "Bad module status in InnerModuleEvaluation");
 
     // Step 5
     ModuleSetStatus(module, MODULE_STATUS_EVALUATING);
@@ -605,8 +593,8 @@ function InnerModuleEvaluation(module, stack, index)
 
         index = InnerModuleEvaluation(requiredModule, stack, index);
 
-        assert(requiredModule.status == MODULE_STATUS_EVALUATING ||
-               requiredModule.status == MODULE_STATUS_EVALUATED,
+        assert(requiredModule.status === MODULE_STATUS_EVALUATING ||
+               requiredModule.status === MODULE_STATUS_EVALUATED,
               "Bad module status after InnerModuleEvaluation");
 
         assert((requiredModule.status === MODULE_STATUS_EVALUATING) ===
diff --git a/js/src/builtin/ModuleObject.cpp b/js/src/builtin/ModuleObject.cpp
index 30e7120c0..a9a257178 100644
--- a/js/src/builtin/ModuleObject.cpp
+++ b/js/src/builtin/ModuleObject.cpp
@@ -18,10 +18,10 @@
 using namespace js;
 using namespace js::frontend;
 
-static_assert(MODULE_STATUS_ERRORED < MODULE_STATUS_UNINSTANTIATED &&
-              MODULE_STATUS_UNINSTANTIATED < MODULE_STATUS_INSTANTIATING &&
+static_assert(MODULE_STATUS_UNINSTANTIATED < MODULE_STATUS_INSTANTIATING &&
               MODULE_STATUS_INSTANTIATING < MODULE_STATUS_INSTANTIATED &&
-              MODULE_STATUS_INSTANTIATED < MODULE_STATUS_EVALUATED,
+              MODULE_STATUS_INSTANTIATED < MODULE_STATUS_EVALUATED &&
+              MODULE_STATUS_EVALUATED < MODULE_STATUS_EVALUATED_ERROR,
               "Module statuses are ordered incorrectly");
 
 template<typename T, Value ValueGetter(const T* obj)>
@@ -273,12 +273,12 @@ IndirectBindingMap::trace(JSTracer* trc)
 }
 
 bool
-IndirectBindingMap::putNew(JSContext* cx, HandleId name,
-                           HandleModuleEnvironmentObject environment, HandleId localName)
+IndirectBindingMap::put(JSContext* cx, HandleId name,
+                        HandleModuleEnvironmentObject environment, HandleId localName)
 {
     RootedShape shape(cx, environment->lookup(cx, localName));
     MOZ_ASSERT(shape);
-    if (!map_.putNew(name, Binding(environment, shape))) {
+    if (!map_.put(name, Binding(environment, shape))) {
         ReportOutOfMemory(cx);
         return false;
     }
@@ -359,7 +359,7 @@ ModuleNamespaceObject::addBinding(JSContext* cx, HandleAtom exportedName,
     RootedModuleEnvironmentObject environment(cx, &targetModule->initialEnvironment());
     RootedId exportedNameId(cx, AtomToId(exportedName));
     RootedId localNameId(cx, AtomToId(localName));
-    return bindings->putNew(cx, exportedNameId, environment, localNameId);
+    return bindings->put(cx, exportedNameId, environment, localNameId);
 }
 
 const char ModuleNamespaceObject::ProxyHandler::family = 0;
@@ -660,6 +660,9 @@ ModuleObject::finalize(js::FreeOp* fop, JSObject* obj)
 ModuleEnvironmentObject*
 ModuleObject::environment() const
 {
+//-    MOZ_ASSERT(status() != MODULE_STATUS_ERRORED);
+//+    MOZ_ASSERT(!hadEvaluationError());
+
     Value value = getReservedSlot(EnvironmentSlot);
     if (value.isUndefined())
         return nullptr;
@@ -723,7 +726,7 @@ void
 ModuleObject::init(HandleScript script)
 {
     initReservedSlot(ScriptSlot, PrivateValue(script));
-    initReservedSlot(StatusSlot, Int32Value(MODULE_STATUS_ERRORED));
+    initReservedSlot(StatusSlot, Int32Value(MODULE_STATUS_UNINSTANTIATED));
 }
 
 void
@@ -827,7 +830,8 @@ ModuleObject::script() const
 static inline void
 AssertValidModuleStatus(ModuleStatus status)
 {
-    MOZ_ASSERT(status >= MODULE_STATUS_ERRORED && status <= MODULE_STATUS_EVALUATED);
+    MOZ_ASSERT(status >= MODULE_STATUS_UNINSTANTIATED &&
+               status <= MODULE_STATUS_EVALUATED_ERROR);
 }
 
 ModuleStatus
@@ -838,11 +842,17 @@ ModuleObject::status() const
     return status;
 }
 
+bool
+ModuleObject::hadEvaluationError() const
+{
+    return status() == MODULE_STATUS_EVALUATED_ERROR;
+}
+
 Value
-ModuleObject::error() const
+ModuleObject::evaluationError() const
 {
-    MOZ_ASSERT(status() == MODULE_STATUS_ERRORED);
-    return getReservedSlot(ErrorSlot);
+    MOZ_ASSERT(hadEvaluationError());
+    return getReservedSlot(EvaluationErrorSlot);
 }
 
 Value
@@ -1005,7 +1015,7 @@ ModuleObject::Evaluate(JSContext* cx, HandleModuleObject self)
 
 DEFINE_GETTER_FUNCTIONS(ModuleObject, namespace_, NamespaceSlot)
 DEFINE_GETTER_FUNCTIONS(ModuleObject, status, StatusSlot)
-DEFINE_GETTER_FUNCTIONS(ModuleObject, error, ErrorSlot)
+DEFINE_GETTER_FUNCTIONS(ModuleObject, evaluationError, EvaluationErrorSlot)
 DEFINE_GETTER_FUNCTIONS(ModuleObject, requestedModules, RequestedModulesSlot)
 DEFINE_GETTER_FUNCTIONS(ModuleObject, importEntries, ImportEntriesSlot)
 DEFINE_GETTER_FUNCTIONS(ModuleObject, localExportEntries, LocalExportEntriesSlot)
@@ -1020,7 +1030,7 @@ GlobalObject::initModuleProto(JSContext* cx, Handle<GlobalObject*> global)
     static const JSPropertySpec protoAccessors[] = {
         JS_PSG("namespace", ModuleObject_namespace_Getter, 0),
         JS_PSG("status", ModuleObject_statusGetter, 0),
-        JS_PSG("error", ModuleObject_errorGetter, 0),
+        JS_PSG("evaluationError", ModuleObject_evaluationErrorGetter, 0),
         JS_PSG("requestedModules", ModuleObject_requestedModulesGetter, 0),
         JS_PSG("importEntries", ModuleObject_importEntriesGetter, 0),
         JS_PSG("localExportEntries", ModuleObject_localExportEntriesGetter, 0),
diff --git a/js/src/builtin/ModuleObject.h b/js/src/builtin/ModuleObject.h
index be0315215..5a809c865 100644
--- a/js/src/builtin/ModuleObject.h
+++ b/js/src/builtin/ModuleObject.h
@@ -107,8 +107,8 @@ class IndirectBindingMap
 
     void trace(JSTracer* trc);
 
-    bool putNew(JSContext* cx, HandleId name,
-                HandleModuleEnvironmentObject environment, HandleId localName);
+    bool put(JSContext* cx, HandleId name,
+             HandleModuleEnvironmentObject environment, HandleId localName);
 
     size_t count() const {
         return map_.count();
@@ -218,7 +218,7 @@ class ModuleObject : public NativeObject
         EnvironmentSlot,
         NamespaceSlot,
         StatusSlot,
-        ErrorSlot,
+        EvaluationErrorSlot,
         HostDefinedSlot,
         RequestedModulesSlot,
         ImportEntriesSlot,
@@ -238,8 +238,8 @@ class ModuleObject : public NativeObject
                   "EnvironmentSlot must match self-hosting define");
     static_assert(StatusSlot == MODULE_OBJECT_STATUS_SLOT,
                   "StatusSlot must match self-hosting define");
-    static_assert(ErrorSlot == MODULE_OBJECT_ERROR_SLOT,
-                  "ErrorSlot must match self-hosting define");
+    static_assert(EvaluationErrorSlot == MODULE_OBJECT_EVALUATION_ERROR_SLOT,
+                  "EvaluationErrorSlot must match self-hosting define");
     static_assert(DFSIndexSlot == MODULE_OBJECT_DFS_INDEX_SLOT,
                   "DFSIndexSlot must match self-hosting define");
     static_assert(DFSAncestorIndexSlot == MODULE_OBJECT_DFS_ANCESTOR_INDEX_SLOT,
@@ -269,7 +269,8 @@ class ModuleObject : public NativeObject
     ModuleEnvironmentObject* environment() const;
     ModuleNamespaceObject* namespace_();
     ModuleStatus status() const;
-    Value error() const;
+    bool hadEvaluationError() const;
+    Value evaluationError() const;
     Value hostDefinedField() const;
     ArrayObject& requestedModules() const;
     ArrayObject& importEntries() const;
diff --git a/js/src/builtin/SelfHostingDefines.h b/js/src/builtin/SelfHostingDefines.h
index a06c2aa62..6afa641b1 100644
--- a/js/src/builtin/SelfHostingDefines.h
+++ b/js/src/builtin/SelfHostingDefines.h
@@ -99,15 +99,15 @@
 
 #define MODULE_OBJECT_ENVIRONMENT_SLOT        2
 #define MODULE_OBJECT_STATUS_SLOT             4
-#define MODULE_OBJECT_ERROR_SLOT              5
+#define MODULE_OBJECT_EVALUATION_ERROR_SLOT   5 // 4
 #define MODULE_OBJECT_DFS_INDEX_SLOT          16
 #define MODULE_OBJECT_DFS_ANCESTOR_INDEX_SLOT 17
 
-#define MODULE_STATUS_ERRORED        0
-#define MODULE_STATUS_UNINSTANTIATED 1
-#define MODULE_STATUS_INSTANTIATING  2
-#define MODULE_STATUS_INSTANTIATED   3
-#define MODULE_STATUS_EVALUATING     4
-#define MODULE_STATUS_EVALUATED      5
+#define MODULE_STATUS_UNINSTANTIATED  0
+#define MODULE_STATUS_INSTANTIATING   1
+#define MODULE_STATUS_INSTANTIATED    2
+#define MODULE_STATUS_EVALUATING      3
+#define MODULE_STATUS_EVALUATED       4
+#define MODULE_STATUS_EVALUATED_ERROR 5
 
 #endif
diff --git a/js/src/builtin/TestingFunctions.cpp b/js/src/builtin/TestingFunctions.cpp
index 025620766..e93ebeec3 100644
--- a/js/src/builtin/TestingFunctions.cpp
+++ b/js/src/builtin/TestingFunctions.cpp
@@ -3585,6 +3585,9 @@ GetModuleEnvironmentNames(JSContext* cx, unsigned argc, Value* vp)
         return false;
     }
 
+//-    if (module->status() == MODULE_STATUS_ERRORED) {
+//+    if (module->hadEvaluationError()) {
+
     RootedModuleEnvironmentObject env(cx, GetModuleEnvironment(cx, args[0]));
     Rooted<IdVector> ids(cx, IdVector(cx));
     if (!JS_Enumerate(cx, env, &ids))
@@ -3622,6 +3625,9 @@ GetModuleEnvironmentValue(JSContext* cx, unsigned argc, Value* vp)
         return false;
     }
 
+//-    if (module->status() == MODULE_STATUS_ERRORED) {
+//+    if (module->hadEvaluationError()) {
+
     RootedModuleEnvironmentObject env(cx, GetModuleEnvironment(cx, args[0]));
     RootedString name(cx, args[1].toString());
     RootedId id(cx);
diff --git a/js/src/jit-test/tests/modules/bug-1284486.js b/js/src/jit-test/tests/modules/bug-1284486.js
index 08286393a..6fbd67d9d 100644
--- a/js/src/jit-test/tests/modules/bug-1284486.js
+++ b/js/src/jit-test/tests/modules/bug-1284486.js
@@ -1,9 +1,8 @@
-// This tests that attempting to perform ModuleDeclarationInstantation a second
-// time after a failure re-throws the same error.
+// This tests that module instantiation can succeed when executed a second

+// time after a failure.
 //
 // The first attempt fails becuase module 'a' is not available. The second
-// attempt fails because of the previous failure (it would otherwise succeed as
-// 'a' is now available).
+// attempt succeeds as 'a' is now available.
 
 load(libdir + "dummyModuleResolveHook.js");
 
@@ -25,12 +24,9 @@ let a = moduleRepo['a'] = parseModule("export var a = 1; export var b = 2;");
 let d = moduleRepo['d'] = parseModule("import { a } from 'c'; a;");
 
 threw = false;
-let e2;
 try {
     d.declarationInstantiation();
 } catch (exc) {
     threw = true;
-    e2 = exc;
 }
-assertEq(threw, true);
-assertEq(e1, e2);
+assertEq(threw, false);
diff --git a/js/src/jit-test/tests/modules/bug-1420420-2.js b/js/src/jit-test/tests/modules/bug-1420420-2.js
new file mode 100644
index 000000000..0511e8126
--- /dev/null
+++ b/js/src/jit-test/tests/modules/bug-1420420-2.js
@@ -0,0 +1,19 @@
+// Test re-instantiation module after failure.
+
+load(libdir + "asserts.js");
+load(libdir + "dummyModuleResolveHook.js");
+
+moduleRepo["good"] = parseModule(`export let x`);
+
+moduleRepo["y1"] = parseModule(`export let y`);
+moduleRepo["y2"] = parseModule(`export let y`);
+moduleRepo["bad"] = parseModule(`export* from "y1"; export* from "y2";`);
+
+moduleRepo["a"] = parseModule(`import* as ns from "good"; import {y} from "bad";`);
+
+let b = moduleRepo["b"] = parseModule(`import "a";`);
+let c = moduleRepo["c"] = parseModule(`import "a";`);
+
+assertThrowsInstanceOf(() => b.declarationInstantiation(), SyntaxError);
+assertThrowsInstanceOf(() => c.declarationInstantiation(), SyntaxError);
+
diff --git a/js/src/jit-test/tests/modules/bug-1420420-3.js b/js/src/jit-test/tests/modules/bug-1420420-3.js
new file mode 100644
index 000000000..236c023b9
--- /dev/null
+++ b/js/src/jit-test/tests/modules/bug-1420420-3.js
@@ -0,0 +1,9 @@
+if (!('stackTest' in this))
+   quit();
+
+let a = parseModule(`throw new Error`);
+a.declarationInstantiation();
+stackTest(function() {
+    a.evaluation();
+});
+
diff --git a/js/src/jit-test/tests/modules/bug-1420420-4.js b/js/src/jit-test/tests/modules/bug-1420420-4.js
new file mode 100644
index 000000000..721c770bc
--- /dev/null
+++ b/js/src/jit-test/tests/modules/bug-1420420-4.js
@@ -0,0 +1,16 @@
+load(libdir + "asserts.js");
+load(libdir + "dummyModuleResolveHook.js");
+
+moduleRepo["a"] = parseModule(`throw undefined`);
+
+let b = moduleRepo["b"] = parseModule(`import "a";`);
+let c = moduleRepo["c"] = parseModule(`import "a";`);
+
+b.declarationInstantiation();
+c.declarationInstantiation();
+
+let count = 0;
+try { b.evaluation() } catch (e) { count++; }
+try { c.evaluation() } catch (e) { count++; }
+assertEq(count, 2);
+
diff --git a/js/src/jit-test/tests/modules/bug-1420420.js b/js/src/jit-test/tests/modules/bug-1420420.js
new file mode 100644
index 000000000..cd1101c76
--- /dev/null
+++ b/js/src/jit-test/tests/modules/bug-1420420.js
@@ -0,0 +1,19 @@
+// Test re-instantiation module after failure.
+
+load(libdir + "asserts.js");
+load(libdir + "dummyModuleResolveHook.js");
+
+moduleRepo["good"] = parseModule(`export let x`);
+
+moduleRepo["y1"] = parseModule(`export let y`);
+moduleRepo["y2"] = parseModule(`export let y`);
+moduleRepo["bad"] = parseModule(`export* from "y1"; export* from "y2";`);
+
+moduleRepo["a"] = parseModule(`import {x} from "good"; import {y} from "bad";`);
+
+let b = moduleRepo["b"] = parseModule(`import "a";`);
+let c = moduleRepo["c"] = parseModule(`import "a";`);
+
+assertThrowsInstanceOf(() => b.declarationInstantiation(), SyntaxError);
+assertThrowsInstanceOf(() => c.declarationInstantiation(), SyntaxError);
+
diff --git a/js/src/js.msg b/js/src/js.msg
index 9c508ebbd..587481864 100644
--- a/js/src/js.msg
+++ b/js/src/js.msg
@@ -579,7 +579,6 @@ MSG_DEF(JSMSG_MISSING_IMPORT,            0, JSEXN_SYNTAXERR, "import not found")
 MSG_DEF(JSMSG_AMBIGUOUS_IMPORT,          0, JSEXN_SYNTAXERR, "ambiguous import")
 MSG_DEF(JSMSG_MISSING_NAMESPACE_EXPORT,  0, JSEXN_SYNTAXERR, "export not found for namespace")
 MSG_DEF(JSMSG_MISSING_EXPORT,            1, JSEXN_SYNTAXERR, "local binding for export '{0}' not found")
-MSG_DEF(JSMSG_MODULE_INSTANTIATE_FAILED, 0, JSEXN_INTERNALERR, "attempt to re-instantiate module after failure")
 MSG_DEF(JSMSG_BAD_MODULE_STATUS,         0, JSEXN_INTERNALERR, "module record has unexpected status")
 
 // Promise
diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
index cf5880e03..3e0c63811 100644
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@@ -4739,18 +4739,6 @@ JS::GetModuleScript(JSContext* cx, JS::HandleObject moduleArg)
     return moduleArg->as<ModuleObject>().script();
 }
 
-JS_PUBLIC_API(bool)
-JS::IsModuleErrored(JSObject* moduleArg)
-{
-    return moduleArg->as<ModuleObject>().status() == MODULE_STATUS_ERRORED;
-}
-
-JS_PUBLIC_API(JS::Value)
-JS::GetModuleError(JSObject* moduleArg)
-{
-    return moduleArg->as<ModuleObject>().error();
-}
-
 JS_PUBLIC_API(JSObject*)
 JS_New(JSContext* cx, HandleObject ctor, const JS::HandleValueArray& inputArgs)
 {
diff --git a/js/src/jsapi.h b/js/src/jsapi.h
index 9c3bf8151..76781cf06 100644
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -4395,12 +4395,6 @@ GetRequestedModules(JSContext* cx, JS::HandleObject moduleRecord);
 extern JS_PUBLIC_API(JSScript*)
 GetModuleScript(JSContext* cx, JS::HandleObject moduleRecord);
 
-extern JS_PUBLIC_API(bool)
-IsModuleErrored(JSObject* moduleRecord);
-
-extern JS_PUBLIC_API(JS::Value)
-GetModuleError(JSObject* moduleRecord);
-
 } /* namespace JS */
 
 extern JS_PUBLIC_API(bool)
diff --git a/js/src/vm/EnvironmentObject.cpp b/js/src/vm/EnvironmentObject.cpp
index 7834940f1..871ae3f19 100644
--- a/js/src/vm/EnvironmentObject.cpp
+++ b/js/src/vm/EnvironmentObject.cpp
@@ -491,7 +491,7 @@ ModuleEnvironmentObject::createImportBinding(JSContext* cx, HandleAtom importNam
     RootedId importNameId(cx, AtomToId(importName));
     RootedId localNameId(cx, AtomToId(localName));
     RootedModuleEnvironmentObject env(cx, &module->initialEnvironment());
-    if (!importBindings().putNew(cx, importNameId, env, localNameId)) {
+    if (!importBindings().put(cx, importNameId, env, localNameId)) {
         ReportOutOfMemory(cx);
         return false;
     }
diff --git a/js/src/vm/SelfHosting.cpp b/js/src/vm/SelfHosting.cpp
index 0dfeffc36..f98e968b8 100644
--- a/js/src/vm/SelfHosting.cpp
+++ b/js/src/vm/SelfHosting.cpp
@@ -2087,7 +2087,6 @@ intrinsic_CreateNamespaceBinding(JSContext* cx, unsigned argc, Value* vp)
     // the slot directly.
     RootedShape shape(cx, environment->lookup(cx, name));
     MOZ_ASSERT(shape);
-    MOZ_ASSERT(environment->getSlot(shape->slot()).isMagic(JS_UNINITIALIZED_LEXICAL));
     environment->setSlot(shape->slot(), args[2]);
     args.rval().setUndefined();
     return true;