Bug 1284486 - Disallow returying ModuleDeclarationInstantiation after error r=shu
authorJon Coppeard <jcoppeard@mozilla.com>
Wed, 13 Jul 2016 10:20:00 +0100
changeset 387159 034f54c5ac438256cfb185eaaab63065a19d2e0d
parent 387158 b15f05a94c716fdbed45b5f46a0fd5b8ae6443ef
child 387160 2cfa9ffe77a49c573340dc5d8880bf8108be967a
push id22898
push userCallek@gmail.com
push dateWed, 13 Jul 2016 13:20:13 +0000
reviewersshu
bugs1284486
milestone50.0a1
Bug 1284486 - Disallow returying ModuleDeclarationInstantiation after error r=shu
js/src/builtin/Module.js
js/src/builtin/ModuleObject.cpp
js/src/builtin/ModuleObject.h
js/src/builtin/SelfHostingDefines.h
js/src/jit-test/tests/modules/bug-1284486.js
js/src/js.msg
js/src/vm/SelfHosting.cpp
--- a/js/src/builtin/Module.js
+++ b/js/src/builtin/Module.js
@@ -178,16 +178,40 @@ function ModuleNamespaceCreate(module, e
         let binding = callFunction(module.resolveExport, module, name);
         assert(binding !== null && binding !== "ambiguous", "Failed to resolve binding");
         AddModuleNamespaceBinding(ns, name, binding.module, binding.bindingName);
     }
 
     return ns;
 }
 
+function GetModuleEnvironment(module)
+{
+    assert(IsModule(module), "Non-module passed to GetModuleEnvironment");
+
+    let env = UnsafeGetReservedSlot(module, MODULE_OBJECT_ENVIRONMENT_SLOT);
+    assert(env === undefined || env === null || IsModuleEnvironment(env),
+          "Module environment slot contains unexpected value");
+
+    // Check for a previous failed attempt to instantiate this module. This can
+    // only happen due to a bug in the module loader.
+    if (env === null)
+        ThrowInternalError(JSMSG_MODULE_INSTANTIATE_FAILED);
+
+    return env;
+}
+
+function RecordInstantationFailure(module)
+{
+    // Set the module's environment slot to 'null' to indicate a failed module
+    // instantiation.
+    assert(IsModule(module), "Non-module passed to RecordInstantationFailure");
+    UnsafeSetReservedSlot(module, MODULE_OBJECT_ENVIRONMENT_SLOT, null);
+}
+
 // 15.2.1.16.4 ModuleDeclarationInstantiation()
 function ModuleDeclarationInstantiation()
 {
     if (!IsObject(this) || !IsModule(this))
         return callFunction(CallModuleMethodIfWrapped, this, "ModuleDeclarationInstantiation");
 
     // Step 1
     let module = this;
@@ -195,56 +219,61 @@ function ModuleDeclarationInstantiation(
     // Step 5
     if (GetModuleEnvironment(module) !== undefined)
         return;
 
     // Step 7
     CreateModuleEnvironment(module);
     let env = GetModuleEnvironment(module);
 
-    // Step 8
-    let requestedModules = module.requestedModules;
-    for (let i = 0; i < requestedModules.length; i++) {
-        let required = requestedModules[i];
-        let requiredModule = HostResolveImportedModule(module, required);
-        callFunction(requiredModule.declarationInstantiation, requiredModule);
-    }
+    try {
+        // Step 8
+        let requestedModules = module.requestedModules;
+        for (let i = 0; i < requestedModules.length; i++) {
+            let required = requestedModules[i];
+            let requiredModule = HostResolveImportedModule(module, required);
+            callFunction(requiredModule.declarationInstantiation, requiredModule);
+        }
 
-    // Step 9
-    let indirectExportEntries = module.indirectExportEntries;
-    for (let i = 0; i < indirectExportEntries.length; i++) {
-        let e = indirectExportEntries[i];
-        let resolution = callFunction(module.resolveExport, module, e.exportName);
-        if (resolution === null)
-            ThrowSyntaxError(JSMSG_MISSING_INDIRECT_EXPORT, e.exportName);
-        if (resolution === "ambiguous")
-            ThrowSyntaxError(JSMSG_AMBIGUOUS_INDIRECT_EXPORT, e.exportName);
-    }
+        // Step 9
+        let indirectExportEntries = module.indirectExportEntries;
+        for (let i = 0; i < indirectExportEntries.length; i++) {
+            let e = indirectExportEntries[i];
+            let resolution = callFunction(module.resolveExport, module, e.exportName);
+            if (resolution === null)
+                ThrowSyntaxError(JSMSG_MISSING_INDIRECT_EXPORT, e.exportName);
+            if (resolution === "ambiguous")
+                ThrowSyntaxError(JSMSG_AMBIGUOUS_INDIRECT_EXPORT, e.exportName);
+        }
 
-    // Step 12
-    let importEntries = module.importEntries;
-    for (let i = 0; i < importEntries.length; i++) {
-        let imp = importEntries[i];
-        let importedModule = HostResolveImportedModule(module, imp.moduleRequest);
-        if (imp.importName === "*") {
-            let namespace = GetModuleNamespace(importedModule);
-            CreateNamespaceBinding(env, imp.localName, namespace);
-        } else {
-            let resolution = callFunction(importedModule.resolveExport, importedModule,
-                                          imp.importName);
-            if (resolution === null)
-                ThrowSyntaxError(JSMSG_MISSING_IMPORT, imp.importName);
-            if (resolution === "ambiguous")
-                ThrowSyntaxError(JSMSG_AMBIGUOUS_IMPORT, imp.importName);
-            CreateImportBinding(env, imp.localName, resolution.module, resolution.bindingName);
+        // Step 12
+        let importEntries = module.importEntries;
+        for (let i = 0; i < importEntries.length; i++) {
+            let imp = importEntries[i];
+            let importedModule = HostResolveImportedModule(module, imp.moduleRequest);
+            if (imp.importName === "*") {
+                let namespace = GetModuleNamespace(importedModule);
+                CreateNamespaceBinding(env, imp.localName, namespace);
+            } else {
+                let resolution = callFunction(importedModule.resolveExport, importedModule,
+                                              imp.importName);
+                if (resolution === null)
+                    ThrowSyntaxError(JSMSG_MISSING_IMPORT, imp.importName);
+                if (resolution === "ambiguous")
+                    ThrowSyntaxError(JSMSG_AMBIGUOUS_IMPORT, imp.importName);
+                CreateImportBinding(env, imp.localName, resolution.module, resolution.bindingName);
+            }
         }
+
+        // Step 16.iv
+        InstantiateModuleFunctionDeclarations(module);
+    } catch (e) {
+        RecordInstantationFailure(module);
+        throw e;
     }
-
-    // Step 16.iv
-    InstantiateModuleFunctionDeclarations(module);
 }
 _SetCanonicalName(ModuleDeclarationInstantiation, "ModuleDeclarationInstantiation");
 
 // 15.2.1.16.5 ModuleEvaluation()
 function ModuleEvaluation()
 {
     if (!IsObject(this) || !IsModule(this))
         return callFunction(CallModuleMethodIfWrapped, this, "ModuleEvaluation");
--- a/js/src/builtin/ModuleObject.cpp
+++ b/js/src/builtin/ModuleObject.cpp
@@ -624,16 +624,18 @@ ModuleObject::finalize(js::FreeOp* fop, 
     if (FunctionDeclarationVector* funDecls = self->functionDeclarations())
         fop->delete_(funDecls);
 }
 
 ModuleEnvironmentObject*
 ModuleObject::environment() const
 {
     Value value = getReservedSlot(EnvironmentSlot);
+    MOZ_ASSERT(!value.isNull());
+
     if (value.isUndefined())
         return nullptr;
 
     return &value.toObject().as<ModuleEnvironmentObject>();
 }
 
 bool
 ModuleObject::hasImportBindings() const
--- a/js/src/builtin/ModuleObject.h
+++ b/js/src/builtin/ModuleObject.h
@@ -5,21 +5,20 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef builtin_ModuleObject_h
 #define builtin_ModuleObject_h
 
 #include "jsapi.h"
 #include "jsatom.h"
 
+#include "builtin/SelfHostingDefines.h"
 #include "gc/Zone.h"
-
 #include "js/GCVector.h"
 #include "js/Id.h"
-
 #include "vm/NativeObject.h"
 #include "vm/ProxyObject.h"
 
 namespace js {
 
 class ModuleEnvironmentObject;
 class ModuleObject;
 
@@ -219,16 +218,19 @@ class ModuleObject : public NativeObject
         StarExportEntriesSlot,
         ImportBindingsSlot,
         NamespaceExportsSlot,
         NamespaceBindingsSlot,
         FunctionDeclarationsSlot,
         SlotCount
     };
 
+    static_assert(EnvironmentSlot == MODULE_OBJECT_ENVIRONMENT_SLOT,
+                  "EnvironmentSlot must match self-hosting define");
+
     static const Class class_;
 
     static bool isInstance(HandleValue value);
 
     static ModuleObject* create(ExclusiveContext* cx, HandleObject enclosingStaticScope);
     void init(HandleScript script);
     void setInitialEnvironment(Handle<ModuleEnvironmentObject*> initialEnvironment);
     void initImportExportData(HandleArrayObject requestedModules,
--- a/js/src/builtin/SelfHostingDefines.h
+++ b/js/src/builtin/SelfHostingDefines.h
@@ -94,9 +94,11 @@
 #define REGEXP_FLAGS_SLOT 2
 
 #define REGEXP_IGNORECASE_FLAG  0x01
 #define REGEXP_GLOBAL_FLAG      0x02
 #define REGEXP_MULTILINE_FLAG   0x04
 #define REGEXP_STICKY_FLAG      0x08
 #define REGEXP_UNICODE_FLAG     0x10
 
+#define MODULE_OBJECT_ENVIRONMENT_SLOT 3
+
 #endif
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/modules/bug-1284486.js
@@ -0,0 +1,23 @@
+// |jit-test| error: InternalError
+
+// This tests that attempting to perform ModuleDeclarationInstantation a second
+// time after a failure throws an error. Doing this would be a bug in the module
+// loader, which is expected to throw away modules if there is an error
+// instantiating them.
+//
+// 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).
+
+let moduleRepo = {};
+setModuleResolveHook(function(module, specifier) {
+    return moduleRepo[specifier];
+});
+try {
+    let b = moduleRepo['b'] = parseModule("export var b = 3; export var c = 4;");
+    let c = moduleRepo['c'] = parseModule("export * from 'a'; export * from 'b';");
+    c.declarationInstantiation();
+} catch (exc) {}
+let a = moduleRepo['a'] = parseModule("export var a = 1; export var b = 2;");
+let d = moduleRepo['d'] = parseModule("import { a } from 'c'; a;");
+d.declarationInstantiation();
--- a/js/src/js.msg
+++ b/js/src/js.msg
@@ -531,15 +531,16 @@ MSG_DEF(JSMSG_REINIT_THIS,       0, JSEX
 // Modules
 MSG_DEF(JSMSG_BAD_DEFAULT_EXPORT,        0, JSEXN_SYNTAXERR, "default export cannot be provided by export *")
 MSG_DEF(JSMSG_MISSING_INDIRECT_EXPORT,   1, JSEXN_SYNTAXERR, "indirect export '{0}' not found")
 MSG_DEF(JSMSG_AMBIGUOUS_INDIRECT_EXPORT, 1, JSEXN_SYNTAXERR, "ambiguous indirect export '{0}'")
 MSG_DEF(JSMSG_MISSING_IMPORT,            1, JSEXN_SYNTAXERR, "import '{0}' not found")
 MSG_DEF(JSMSG_AMBIGUOUS_IMPORT,          1, JSEXN_SYNTAXERR, "ambiguous import '{0}'")
 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")
 
 // Promise
 MSG_DEF(JSMSG_CANNOT_RESOLVE_PROMISE_WITH_ITSELF,       0, JSEXN_TYPEERR, "A promise cannot be resolved with itself.")
 MSG_DEF(JSMSG_PROMISE_CAPABILITY_HAS_SOMETHING_ALREADY, 0, JSEXN_TYPEERR, "GetCapabilitiesExecutor function already invoked with non-undefined values.")
 MSG_DEF(JSMSG_PROMISE_RESOLVE_FUNCTION_NOT_CALLABLE,    0, JSEXN_TYPEERR, "A Promise subclass passed a non-callable value as the resolve function.")
 MSG_DEF(JSMSG_PROMISE_REJECT_FUNCTION_NOT_CALLABLE,     0, JSEXN_TYPEERR, "A Promise subclass passed a non-callable value as the reject function.")
 MSG_DEF(JSMSG_PROMISE_ERROR_IN_WRAPPED_REJECTION_REASON,0, JSEXN_INTERNALERR, "Promise rejection value is a non-unwrappable cross-compartment wrapper.")
--- a/js/src/vm/SelfHosting.cpp
+++ b/js/src/vm/SelfHosting.cpp
@@ -364,16 +364,26 @@ intrinsic_ThrowSyntaxError(JSContext* cx
 {
     CallArgs args = CallArgsFromVp(argc, vp);
     MOZ_ASSERT(args.length() >= 1);
 
     ThrowErrorWithType(cx, JSEXN_SYNTAXERR, args);
     return false;
 }
 
+static bool
+intrinsic_ThrowInternalError(JSContext* cx, unsigned argc, Value* vp)
+{
+    CallArgs args = CallArgsFromVp(argc, vp);
+    MOZ_ASSERT(args.length() >= 1);
+
+    ThrowErrorWithType(cx, JSEXN_INTERNALERR, args);
+    return false;
+}
+
 /**
  * Handles an assertion failure in self-hosted code just like an assertion
  * failure in C++ code. Information about the failure can be provided in args[0].
  */
 static bool
 intrinsic_AssertionFailed(JSContext* cx, unsigned argc, Value* vp)
 {
 #ifdef DEBUG
@@ -2111,33 +2121,16 @@ intrinsic_HostResolveImportedModule(JSCo
         return false;
     }
 
     args.rval().set(result);
     return true;
 }
 
 static bool
-intrinsic_GetModuleEnvironment(JSContext* cx, unsigned argc, Value* vp)
-{
-    CallArgs args = CallArgsFromVp(argc, vp);
-    MOZ_ASSERT(args.length() == 1);
-    RootedModuleObject module(cx, &args[0].toObject().as<ModuleObject>());
-    RootedModuleEnvironmentObject env(cx, module->environment());
-    args.rval().setUndefined();
-    if (!env) {
-        args.rval().setUndefined();
-        return true;
-    }
-
-    args.rval().setObject(*env);
-    return true;
-}
-
-static bool
 intrinsic_CreateModuleEnvironment(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
     MOZ_ASSERT(args.length() == 1);
     RootedModuleObject module(cx, &args[0].toObject().as<ModuleObject>());
     module->createEnvironment();
     args.rval().setUndefined();
     return true;
@@ -2376,16 +2369,17 @@ static const JSFunctionSpec intrinsic_fu
     JS_INLINABLE_FN("IsConstructor", intrinsic_IsConstructor,           1,0,
                     IntrinsicIsConstructor),
     JS_FN("GetBuiltinConstructorImpl", intrinsic_GetBuiltinConstructor, 1,0),
     JS_FN("MakeConstructible",       intrinsic_MakeConstructible,       2,0),
     JS_FN("_ConstructFunction",      intrinsic_ConstructFunction,       2,0),
     JS_FN("ThrowRangeError",         intrinsic_ThrowRangeError,         4,0),
     JS_FN("ThrowTypeError",          intrinsic_ThrowTypeError,          4,0),
     JS_FN("ThrowSyntaxError",        intrinsic_ThrowSyntaxError,        4,0),
+    JS_FN("ThrowInternalError",      intrinsic_ThrowInternalError,      4,0),
     JS_FN("AssertionFailed",         intrinsic_AssertionFailed,         1,0),
     JS_FN("DumpMessage",             intrinsic_DumpMessage,             1,0),
     JS_FN("OwnPropertyKeys",         intrinsic_OwnPropertyKeys,         1,0),
     JS_FN("MakeDefaultConstructor",  intrinsic_MakeDefaultConstructor,  2,0),
     JS_FN("_ConstructorForTypedArray", intrinsic_ConstructorForTypedArray, 1,0),
     JS_FN("DecompileArg",            intrinsic_DecompileArg,            2,0),
     JS_FN("_FinishBoundFunctionInit", intrinsic_FinishBoundFunctionInit, 4,0),
     JS_FN("RuntimeDefaultLocale",    intrinsic_RuntimeDefaultLocale,    0,0),
@@ -2623,17 +2617,17 @@ static const JSFunctionSpec intrinsic_fu
     JS_FN("regexp_exec_no_statics", regexp_exec_no_statics, 2,0),
     JS_FN("regexp_test_no_statics", regexp_test_no_statics, 2,0),
     JS_FN("regexp_construct_no_sticky", regexp_construct_no_sticky, 2,0),
 
     JS_FN("IsModule", intrinsic_IsInstanceOfBuiltin<ModuleObject>, 1, 0),
     JS_FN("CallModuleMethodIfWrapped",
           CallNonGenericSelfhostedMethod<Is<ModuleObject>>, 2, 0),
     JS_FN("HostResolveImportedModule", intrinsic_HostResolveImportedModule, 2, 0),
-    JS_FN("GetModuleEnvironment", intrinsic_GetModuleEnvironment, 1, 0),
+    JS_FN("IsModuleEnvironment", intrinsic_IsInstanceOfBuiltin<ModuleEnvironmentObject>, 1, 0),
     JS_FN("CreateModuleEnvironment", intrinsic_CreateModuleEnvironment, 1, 0),
     JS_FN("CreateImportBinding", intrinsic_CreateImportBinding, 4, 0),
     JS_FN("CreateNamespaceBinding", intrinsic_CreateNamespaceBinding, 3, 0),
     JS_FN("InstantiateModuleFunctionDeclarations",
           intrinsic_InstantiateModuleFunctionDeclarations, 1, 0),
     JS_FN("SetModuleEvaluated", intrinsic_SetModuleEvaluated, 1, 0),
     JS_FN("EvaluateModule", intrinsic_EvaluateModule, 1, 0),
     JS_FN("IsModuleNamespace", intrinsic_IsInstanceOfBuiltin<ModuleNamespaceObject>, 1, 0),