Changeset 223331 in webkit for trunk/Source/JavaScriptCore

Timestamp:

Oct 15, 2017, 6:55:16 PM (8 years ago)

Author:

Yusuke Suzuki

Message:

[JSC] Perform module specifier validation at parsing time
​https://bugs.webkit.org/show_bug.cgi?id=178256

Reviewed by Darin Adler.

Source/JavaScriptCore:

This patch make module loader's resolve operation synchronous. And we validate
module's requested module names when instantiating the module instead of satisfying
module's dependencies. This change is not observable to users. But this is precise
to the spec and this optimizes & simplifies the current module loader a bit by
reducing object allocations.

Previously, we have an object called pair in the module loader. This is pair of
module's name and module's record. And we use it to link one module to dependent
modules. Now, it is replaced with module's registry entry.

We also change our loader functions to take a registry entry instead of a module key.
Previous design is due to the consideration that these APIs may be exposed to users
in whatwg/loader spec. However, this won't happen. This change removes unnecessary
repeatedly hash map lookups.

• builtins/ModuleLoaderPrototype.js:

(globalPrivate.newRegistryEntry):
(requestFetch):
(requestInstantiate):
(requestSatisfy):
(link):
(moduleEvaluation):
(loadModule):

• jsc.cpp:

(GlobalObject::moduleLoaderResolve):

• runtime/AbstractModuleRecord.cpp:

(JSC::AbstractModuleRecord::finishCreation):
(JSC::AbstractModuleRecord::hostResolveImportedModule):

• runtime/JSGlobalObject.h:
• runtime/JSModuleLoader.cpp:

(JSC::JSModuleLoader::resolveSync):
(JSC::JSModuleLoader::resolve):

• runtime/JSModuleLoader.h:
• runtime/ModuleLoaderPrototype.cpp:

(JSC::moduleLoaderPrototypeResolveSync):

Source/WebCore:

No behavior change in the current implementation.

• bindings/js/JSDOMWindowBase.cpp:

(WebCore::JSDOMWindowBase::moduleLoaderResolve):

• bindings/js/JSDOMWindowBase.h:
• bindings/js/ScriptModuleLoader.cpp:

(WebCore::ScriptModuleLoader::resolve):

• bindings/js/ScriptModuleLoader.h:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• builtins/ModuleLoaderPrototype.js (modified) (15 diffs)
• jsc.cpp (modified) (2 diffs)
• runtime/AbstractModuleRecord.cpp (modified) (2 diffs)
• runtime/JSGlobalObject.h (modified) (1 diff)
• runtime/JSModuleLoader.cpp (modified) (2 diffs)
• runtime/JSModuleLoader.h (modified) (1 diff)
• runtime/ModuleLoaderPrototype.cpp (modified) (4 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2017-10-15  Yusuke Suzuki  <[email protected]>
+
+        [JSC] Perform module specifier validation at parsing time
+        https://bugs.webkit.org/show_bug.cgi?id=178256
+
+        Reviewed by Darin Adler.
+
+        This patch make module loader's `resolve` operation synchronous. And we validate
+        module's requested module names when instantiating the module instead of satisfying
+        module's dependencies. This change is not observable to users. But this is precise
+        to the spec and this optimizes & simplifies the current module loader a bit by
+        reducing object allocations.
+
+        Previously, we have an object called pair in the module loader. This is pair of
+        module's name and module's record. And we use it to link one module to dependent
+        modules. Now, it is replaced with module's registry entry.
+
+        We also change our loader functions to take a registry entry instead of a module key.
+        Previous design is due to the consideration that these APIs may be exposed to users
+        in whatwg/loader spec. However, this won't happen. This change removes unnecessary
+        repeatedly hash map lookups.
+
+        * builtins/ModuleLoaderPrototype.js:
+        (globalPrivate.newRegistryEntry):
+        (requestFetch):
+        (requestInstantiate):
+        (requestSatisfy):
+        (link):
+        (moduleEvaluation):
+        (loadModule):
+        * jsc.cpp:
+        (GlobalObject::moduleLoaderResolve):
+        * runtime/AbstractModuleRecord.cpp:
+        (JSC::AbstractModuleRecord::finishCreation):
+        (JSC::AbstractModuleRecord::hostResolveImportedModule):
+        * runtime/JSGlobalObject.h:
+        * runtime/JSModuleLoader.cpp:
+        (JSC::JSModuleLoader::resolveSync):
+        (JSC::JSModuleLoader::resolve):
+        * runtime/JSModuleLoader.h:
+        * runtime/ModuleLoaderPrototype.cpp:
+        (JSC::moduleLoaderPrototypeResolveSync):
+
 2017-10-14  Devin Rousso  <[email protected]>
 

trunk/Source/JavaScriptCore/builtins/ModuleLoaderPrototype.js

@@ -100 +100 @@
         linkError: @undefined,
         linkSucceeded: true,
+        evaluated: false,
     };
 }
@@ -141 +142 @@
 // Loader.
 
-function requestFetch(key, parameters, fetcher)
+function requestFetch(entry, parameters, fetcher)
 {
     // https://whatwg.github.io/loader/#request-fetch
@@ -147 +148 @@
     "use strict";
 
-    var entry = this.ensureRegistered(key);
     if (entry.fetch)
         return entry.fetch;
@@ -157 +157 @@
     //     For example, JavaScriptCore shell can provide the hook fetching the resource
     //     from the local file system.
-    var fetchPromise = this.fetch(key, parameters, fetcher).then((source) => {
+    var fetchPromise = this.fetch(entry.key, parameters, fetcher).then((source) => {
         @setStateToMax(entry, @ModuleInstantiate);
         return source;
@@ -165 +165 @@
 }
 
-function requestInstantiate(key, parameters, fetcher)
+function requestInstantiate(entry, parameters, fetcher)
 {
     // https://whatwg.github.io/loader/#request-instantiate
@@ -171 +171 @@
     "use strict";
 
-    var entry = this.ensureRegistered(key);
     if (entry.instantiate)
         return entry.instantiate;
 
-    var instantiatePromise = this.requestFetch(key, parameters, fetcher).then((source) => {
-        var moduleRecord = this.parseModule(entry.key, source);
+    var instantiatePromise = this.requestFetch(entry, parameters, fetcher).then((source) => {
+        var key = entry.key;
+        var moduleRecord = this.parseModule(key, source);
 
         // FIXME: Described in the draft,
@@ -185 +185 @@
         // https://github.com/whatwg/loader/pull/67
 
-        var dependencies = [];
         var dependenciesMap = moduleRecord.dependenciesMap;
-        moduleRecord.registryEntry = entry;
         var requestedModules = this.requestedModules(moduleRecord);
+        var dependencies = @newArrayWithSize(requestedModules.length);
         for (var i = 0, length = requestedModules.length; i < length; ++i) {
-            var depKey = requestedModules[i];
-            var pair = {
-                key: depKey,
-                value: @undefined
-            };
-            @putByValDirect(dependencies, dependencies.length, pair);
-            dependenciesMap.@set(depKey, pair);
+            var depName = requestedModules[i];
+            var depKey = this.resolveSync(depName, key, fetcher);
+            var depEntry = this.ensureRegistered(depKey);
+            @putByValDirect(dependencies, i, depEntry);
+            dependenciesMap.@set(depName, depEntry);
         }
         entry.dependencies = dependencies;
@@ -208 +205 @@
 }
 
-function requestSatisfy(key, parameters, fetcher)
+function requestSatisfy(entry, parameters, fetcher)
 {
     // https://whatwg.github.io/loader/#satisfy-instance
@@ -214 +211 @@
     "use strict";
 
-    var entry = this.ensureRegistered(key);
     if (entry.satisfy)
         return entry.satisfy;
 
-    var satisfyPromise = this.requestInstantiate(key, parameters, fetcher).then((entry) => {
-        var depLoads = [];
+    var satisfyPromise = this.requestInstantiate(entry, parameters, fetcher).then((entry) => {
+        var depLoads = @newArrayWithSize(entry.dependencies.length);
         for (var i = 0, length = entry.dependencies.length; i < length; ++i) {
-            let pair = entry.dependencies[i];
-
-            // Hook point.
-            // 1. Loader.resolve.
-            //     https://whatwg.github.io/loader/#browser-resolve
-            //     Take the name and resolve it to the unique identifier for the resource location.
-            //     For example, take the "jquery" and return the URL for the resource.
-            var promise = this.resolve(pair.key, key, fetcher).then((depKey) => {
-                var depEntry = this.ensureRegistered(depKey);
-
-                // Recursive resolving. The dependencies of this entry is being resolved or already resolved.
-                // Stop tracing the circular dependencies.
-                // But to retrieve the instantiated module record correctly,
-                // we need to wait for the instantiation for the dependent module.
-                // For example, reaching here, the module is starting resolving the dependencies.
-                // But the module may or may not reach the instantiation phase in the loader's pipeline.
-                // If we wait for the Satisfy for this module, it construct the circular promise chain and
-                // rejected by the Promises runtime. Since only we need is the instantiated module, instead of waiting
-                // the Satisfy for this module, we just wait Instantiate for this.
-                if (depEntry.satisfy) {
-                    return depEntry.instantiate.then((entry) => {
-                        pair.value = entry.module;
-                        return entry;
-                    });
-                }
-
+            var depEntry = entry.dependencies[i];
+            var promise = @undefined;
+
+            // Recursive resolving. The dependencies of this entry is being resolved or already resolved.
+            // Stop tracing the circular dependencies.
+            // But to retrieve the instantiated module record correctly,
+            // we need to wait for the instantiation for the dependent module.
+            // For example, reaching here, the module is starting resolving the dependencies.
+            // But the module may or may not reach the instantiation phase in the loader's pipeline.
+            // If we wait for the Satisfy for this module, it construct the circular promise chain and
+            // rejected by the Promises runtime. Since only we need is the instantiated module, instead of waiting
+            // the Satisfy for this module, we just wait Instantiate for this.
+            if (depEntry.satisfy)
+                promise = depEntry.instantiate;
+            else {
                 // Currently, module loader do not pass any information for non-top-level module fetching.
-                return this.requestSatisfy(depKey, @undefined, fetcher).then((entry) => {
-                    pair.value = entry.module;
-                    return entry;
-                });
-            });
-            @putByValDirect(depLoads, depLoads.length, promise);
+                promise = this.requestSatisfy(depEntry, @undefined, fetcher);
+            }
+            @putByValDirect(depLoads, i, promise);
         }
 
@@ -285 +267 @@
         // without constructing the dependency graph by calling dependencyGraph.
         var dependencies = entry.dependencies;
-        for (var i = 0, length = dependencies.length; i < length; ++i) {
-            var pair = dependencies[i];
-            this.link(pair.value.registryEntry, fetcher);
-        }
+        for (var i = 0, length = dependencies.length; i < length; ++i)
+            this.link(dependencies[i], fetcher);
 
         this.moduleDeclarationInstantiation(entry.module, entry.key, fetcher);
@@ -300 +280 @@
 // Module semantics.
 
-function moduleEvaluation(moduleRecord, fetcher)
+function moduleEvaluation(entry, fetcher)
 {
     // http://www.ecma-international.org/ecma-262/6.0/#sec-moduleevaluation
@@ -306 +286 @@
     "use strict";
 
-    if (moduleRecord.evaluated)
+    if (entry.evaluated)
         return;
-    moduleRecord.evaluated = true;
-
-    var entry = moduleRecord.registryEntry;
+    entry.evaluated = true;
 
     // The contents of the [[RequestedModules]] is cloned into entry.dependencies.
     var dependencies = entry.dependencies;
-    for (var i = 0, length = dependencies.length; i < length; ++i) {
-        var pair = dependencies[i];
-        var requiredModuleRecord = pair.value;
-        this.moduleEvaluation(requiredModuleRecord, fetcher);
-    }
-    this.evaluate(entry.key, moduleRecord, fetcher);
+    for (var i = 0, length = dependencies.length; i < length; ++i)
+        this.moduleEvaluation(dependencies[i], fetcher);
+
+    this.evaluate(entry.key, entry.module, fetcher);
 }
 
@@ -344 +320 @@
     // For example, take the "jquery" and return the URL for the resource.
     return this.resolve(moduleName, @undefined, fetcher).then((key) => {
-        return this.requestSatisfy(key, parameters, fetcher);
+        return this.requestSatisfy(this.ensureRegistered(key), parameters, fetcher);
     }).then((entry) => {
         return entry.key;
@@ -359 +335 @@
 
     this.link(entry, fetcher);
-    return this.moduleEvaluation(entry.module, fetcher);
+    return this.moduleEvaluation(entry, fetcher);
 }
 
@@ -375 +351 @@
     "use strict";
 
-    return this.requestSatisfy(key, parameters, fetcher).then((entry) => {
+    return this.requestSatisfy(this.ensureRegistered(key), parameters, fetcher).then((entry) => {
         this.linkAndEvaluateModule(entry.key, fetcher);
         return this.getModuleNamespaceObject(entry.module);

trunk/Source/JavaScriptCore/jsc.cpp

@@ -1659 +1659 @@
 
     static JSInternalPromise* moduleLoaderImportModule(JSGlobalObject*, ExecState*, JSModuleLoader*, JSString*, JSValue, const SourceOrigin&);
-    static JSInternalPromise* moduleLoaderResolve(JSGlobalObject*, ExecState*, JSModuleLoader*, JSValue, JSValue, JSValue);
+    static Identifier moduleLoaderResolve(JSGlobalObject*, ExecState*, JSModuleLoader*, JSValue, JSValue, JSValue);
     static JSInternalPromise* moduleLoaderFetch(JSGlobalObject*, ExecState*, JSModuleLoader*, JSValue, JSValue, JSValue);
     static JSObject* moduleLoaderCreateImportMetaProperties(JSGlobalObject*, ExecState*, JSModuleLoader*, JSValue, JSModuleRecord*, JSValue);
@@ -1841 +1841 @@
 }
 
-JSInternalPromise* GlobalObject::moduleLoaderResolve(JSGlobalObject* globalObject, ExecState* exec, JSModuleLoader*, JSValue keyValue, JSValue referrerValue, JSValue)
+Identifier GlobalObject::moduleLoaderResolve(JSGlobalObject* globalObject, ExecState* exec, JSModuleLoader*, JSValue keyValue, JSValue referrerValue, JSValue)
 {
     VM& vm = globalObject->vm();
-    auto scope = DECLARE_CATCH_SCOPE(vm);
-
-    JSInternalPromiseDeferred* deferred = JSInternalPromiseDeferred::create(exec, globalObject);
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
     scope.releaseAssertNoException();
     const Identifier key = keyValue.toPropertyKey(exec);
-    if (UNLIKELY(scope.exception())) {
-        JSValue exception = scope.exception();
-        scope.clearException();
-        return deferred->reject(exec, exception);
-    }
-
-    if (key.isSymbol()) {
-        auto result = deferred->resolve(exec, keyValue);
-        scope.releaseAssertNoException();
-        return result;
-    }
+    RETURN_IF_EXCEPTION(scope, { });
+
+    if (key.isSymbol())
+        return key;
+
     if (referrerValue.isUndefined()) {
         auto directoryName = currentWorkingDirectory();
-        if (!directoryName)
-            return deferred->reject(exec, createError(exec, ASCIILiteral("Could not resolve the current working directory.")));
-        auto result = deferred->resolve(exec, jsString(exec, resolvePath(directoryName.value(), ModuleName(key.impl()))));
-        scope.releaseAssertNoException();
-        return result;
+        if (!directoryName) {
+            throwException(exec, scope, createError(exec, ASCIILiteral("Could not resolve the current working directory.")));
+            return { };
+        }
+        return Identifier::fromString(&vm, resolvePath(directoryName.value(), ModuleName(key.impl())));
     }
 
     const Identifier referrer = referrerValue.toPropertyKey(exec);
-    if (UNLIKELY(scope.exception())) {
-        JSValue exception = scope.exception();
-        scope.clearException();
-        return deferred->reject(exec, exception);
-    }
+    RETURN_IF_EXCEPTION(scope, { });
 
     if (referrer.isSymbol()) {
         auto directoryName = currentWorkingDirectory();
-        if (!directoryName)
-            return deferred->reject(exec, createError(exec, ASCIILiteral("Could not resolve the current working directory.")));
-        auto result = deferred->resolve(exec, jsString(exec, resolvePath(directoryName.value(), ModuleName(key.impl()))));
-        scope.releaseAssertNoException();
-        return result;
+        if (!directoryName) {
+            throwException(exec, scope, createError(exec, ASCIILiteral("Could not resolve the current working directory.")));
+            return { };
+        }
+        return Identifier::fromString(&vm, resolvePath(directoryName.value(), ModuleName(key.impl())));
     }
 
     // If the referrer exists, we assume that the referrer is the correct absolute path.
     auto directoryName = extractDirectoryName(referrer.impl());
-    if (!directoryName)
-        return deferred->reject(exec, createError(exec, makeString("Could not resolve the referrer name '", String(referrer.impl()), "'.")));
-    auto result = deferred->resolve(exec, jsString(exec, resolvePath(directoryName.value(), ModuleName(key.impl()))));
-    scope.releaseAssertNoException();
-    return result;
+    if (!directoryName) {
+        throwException(exec, scope, createError(exec, makeString("Could not resolve the referrer name '", String(referrer.impl()), "'.")));
+        return { };
+    }
+    return Identifier::fromString(&vm, resolvePath(directoryName.value(), ModuleName(key.impl())));
 }
 

trunk/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp

@@ -55 +55 @@
     Base::finishCreation(vm);
     ASSERT(inherits(vm, info()));
-    putDirect(vm, Identifier::fromString(&vm, ASCIILiteral("registryEntry")), jsUndefined());
-    putDirect(vm, Identifier::fromString(&vm, ASCIILiteral("evaluated")), jsBoolean(false));
 
     auto scope = DECLARE_THROW_SCOPE(vm);
@@ -150 +148 @@
     auto scope = DECLARE_THROW_SCOPE(vm);
     JSValue moduleNameValue = identifierToJSValue(exec, moduleName);
-    JSValue pair = m_dependenciesMap->JSMap::get(exec, moduleNameValue);
+    JSValue entry = m_dependenciesMap->JSMap::get(exec, moduleNameValue);
     RETURN_IF_EXCEPTION(scope, nullptr);
     scope.release();
-    return jsCast<AbstractModuleRecord*>(pair.get(exec, Identifier::fromString(exec, "value")));
+    return jsCast<AbstractModuleRecord*>(entry.get(exec, Identifier::fromString(exec, "module")));
 }
 

trunk/Source/JavaScriptCore/runtime/JSGlobalObject.h

@@ -190 +190 @@
     ModuleLoaderImportModulePtr moduleLoaderImportModule;
 
-    typedef JSInternalPromise* (*ModuleLoaderResolvePtr)(JSGlobalObject*, ExecState*, JSModuleLoader*, JSValue, JSValue, JSValue);
+    typedef Identifier (*ModuleLoaderResolvePtr)(JSGlobalObject*, ExecState*, JSModuleLoader*, JSValue, JSValue, JSValue);
     ModuleLoaderResolvePtr moduleLoaderResolve;
 

trunk/Source/JavaScriptCore/runtime/JSModuleLoader.cpp

@@ -203 +203 @@
 }
 
-JSInternalPromise* JSModuleLoader::resolve(ExecState* exec, JSValue name, JSValue referrer, JSValue scriptFetcher)
+Identifier JSModuleLoader::resolveSync(ExecState* exec, JSValue name, JSValue referrer, JSValue scriptFetcher)
 {
     if (Options::dumpModuleLoadingState())
@@ -211 +211 @@
     if (globalObject->globalObjectMethodTable()->moduleLoaderResolve)
         return globalObject->globalObjectMethodTable()->moduleLoaderResolve(globalObject, exec, this, name, referrer, scriptFetcher);
-    JSInternalPromiseDeferred* deferred = JSInternalPromiseDeferred::create(exec, globalObject);
-    deferred->resolve(exec, name);
-    return deferred->promise();
+    return name.toPropertyKey(exec);
+}
+
+JSInternalPromise* JSModuleLoader::resolve(ExecState* exec, JSValue name, JSValue referrer, JSValue scriptFetcher)
+{
+    VM& vm = exec->vm();
+    auto scope = DECLARE_CATCH_SCOPE(vm);
+
+    JSInternalPromiseDeferred* deferred = JSInternalPromiseDeferred::create(exec, exec->lexicalGlobalObject());
+    scope.releaseAssertNoException();
+    const Identifier moduleKey = resolveSync(exec, name, referrer, scriptFetcher);
+    if (UNLIKELY(scope.exception())) {
+        JSValue exception = scope.exception();
+        scope.clearException();
+        return deferred->reject(exec, exception);
+    }
+    auto result = deferred->resolve(exec, identifierToJSValue(vm, moduleKey));
+    scope.releaseAssertNoException();
+    return result;
 }
 

trunk/Source/JavaScriptCore/runtime/JSModuleLoader.h

@@ -73 +73 @@
     JSInternalPromise* importModule(ExecState*, JSString* moduleName, JSValue parameters, const SourceOrigin& referrer);
     JSInternalPromise* resolve(ExecState*, JSValue name, JSValue referrer, JSValue scriptFetcher);
+    Identifier resolveSync(ExecState*, JSValue name, JSValue referrer, JSValue scriptFetcher);
     JSInternalPromise* fetch(ExecState*, JSValue key, JSValue parameters, JSValue scriptFetcher);
     JSObject* createImportMetaProperties(ExecState*, JSValue key, JSModuleRecord*, JSValue scriptFetcher);

trunk/Source/JavaScriptCore/runtime/ModuleLoaderPrototype.cpp

@@ -28 +28 @@
 
 #include "BuiltinNames.h"
+#include "CatchScope.h"
 #include "CodeProfiling.h"
 #include "Error.h"
@@ -53 +54 @@
 static EncodedJSValue JSC_HOST_CALL moduleLoaderPrototypeModuleDeclarationInstantiation(ExecState*);
 static EncodedJSValue JSC_HOST_CALL moduleLoaderPrototypeResolve(ExecState*);
+static EncodedJSValue JSC_HOST_CALL moduleLoaderPrototypeResolveSync(ExecState*);
 static EncodedJSValue JSC_HOST_CALL moduleLoaderPrototypeFetch(ExecState*);
 static EncodedJSValue JSC_HOST_CALL moduleLoaderPrototypeGetModuleNamespaceObject(ExecState*);
@@ -87 +89 @@
     requestedModules               moduleLoaderPrototypeRequestedModules               DontEnum|Function 1
     resolve                        moduleLoaderPrototypeResolve                        DontEnum|Function 2
+    resolveSync                    moduleLoaderPrototypeResolveSync                    DontEnum|Function 2
     fetch                          moduleLoaderPrototypeFetch                          DontEnum|Function 3
 @end
@@ -183 +186 @@
 }
 
+EncodedJSValue JSC_HOST_CALL moduleLoaderPrototypeResolveSync(ExecState* exec)
+{
+    VM& vm = exec->vm();
+    auto scope = DECLARE_CATCH_SCOPE(vm);
+
+    JSModuleLoader* loader = jsDynamicCast<JSModuleLoader*>(vm, exec->thisValue());
+    if (!loader)
+        return JSValue::encode(jsUndefined());
+    auto result = loader->resolveSync(exec, exec->argument(0), exec->argument(1), exec->argument(2));
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    return JSValue::encode(identifierToJSValue(vm, result));
+}
+
 EncodedJSValue JSC_HOST_CALL moduleLoaderPrototypeFetch(ExecState* exec)
 {