[JSC] Retry module fetching if previous request fails
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 Nov 2017 05:28:34 +0000 (05:28 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 Nov 2017 05:28:34 +0000 (05:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=178168

Reviewed by Saam Barati.

Source/JavaScriptCore:

According to the latest spec, the failed fetching operation can be retried if it is requested again.
For example,

    <script type="module" integrity="shaXXX-bad" src="./A.js"></script>
    <script type="module" integrity="shaXXX-correct" src="./A.js"></script>

When performing the first module fetching, integrity check fails, and the load of this module becomes failed.
But when loading the second module, we do not use the cached failure result in the first module loading.
We retry fetching for "./A.js". In this case, we have a correct integrity and module fetching succeeds.
This is specified in whatwg/HTML[1]. If the fetching fails, we do not cache it.

Interestingly, fetching result and instantiation result will be cached if they succeeds. This is because we would
like to cache modules based on their URLs. As a result,

    <script type="module" integrity="shaXXX-correct" src="./A.js"></script>
    <script type="module" integrity="shaXXX-bad" src="./A.js"></script>

In the above case, the first loading succeeds. And the second loading also succeeds since the succeeded fetching and
instantiation are cached in the module pipeline.

This patch implements the above semantics. Previously, our module pipeline always caches the result. If the fetching
failed, all the subsequent fetching for the same URL fails even if we have different integrity values. We retry fetching
if the previous one fails. As an overview of our change,

1. Fetching result should be cached only if it succeeds. Two or more on-the-fly fetching requests to the same URLs should
   be unified. But if currently executing one fails, other attempts should retry fetching.

2. Instantiation should be cached if fetching succeeds.

3. Satisfying should be cached if it succeeds.

[1]: https://html.spec.whatwg.org/#fetch-a-single-module-script

* builtins/ModuleLoaderPrototype.js:
(requestFetch):
(requestInstantiate):
(requestSatisfy):
(link):
(loadModule):
* runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::init):

LayoutTests:

* js/dom/modules/module-fetch-failure-not-cached-expected.txt: Added.
* js/dom/modules/module-fetch-failure-not-cached.html: Added.
* js/dom/modules/module-integrity-bad-value-success-with-cache-expected.txt: Added.
* js/dom/modules/module-integrity-bad-value-success-with-cache.html: Added.
* js/dom/modules/script-tests/module-fetch-failure-not-cached.js: Added.
* js/dom/modules/script-tests/module-integrity-bad-value-success-with-cache.js: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@224662 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/js/dom/modules/module-fetch-failure-not-cached-expected.txt [new file with mode: 0644]
LayoutTests/js/dom/modules/module-fetch-failure-not-cached.html [new file with mode: 0644]
LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache-expected.txt [new file with mode: 0644]
LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache.html [new file with mode: 0644]
LayoutTests/js/dom/modules/script-tests/module-fetch-failure-not-cached.js [new file with mode: 0644]
LayoutTests/js/dom/modules/script-tests/module-integrity-bad-value-success-with-cache.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/builtins/ModuleLoaderPrototype.js
Source/JavaScriptCore/runtime/JSGlobalObject.cpp

index 6945c63..619275c 100644 (file)
@@ -1,3 +1,17 @@
+2017-11-09  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [JSC] Retry module fetching if previous request fails
+        https://bugs.webkit.org/show_bug.cgi?id=178168
+
+        Reviewed by Saam Barati.
+
+        * js/dom/modules/module-fetch-failure-not-cached-expected.txt: Added.
+        * js/dom/modules/module-fetch-failure-not-cached.html: Added.
+        * js/dom/modules/module-integrity-bad-value-success-with-cache-expected.txt: Added.
+        * js/dom/modules/module-integrity-bad-value-success-with-cache.html: Added.
+        * js/dom/modules/script-tests/module-fetch-failure-not-cached.js: Added.
+        * js/dom/modules/script-tests/module-integrity-bad-value-success-with-cache.js: Added.
+
 2017-11-09  Ryan Haddad  <ryanhaddad@apple.com>
 
         Mark multiple service worker tests as flaky.
diff --git a/LayoutTests/js/dom/modules/module-fetch-failure-not-cached-expected.txt b/LayoutTests/js/dom/modules/module-fetch-failure-not-cached-expected.txt
new file mode 100644 (file)
index 0000000..484e375
--- /dev/null
@@ -0,0 +1,4 @@
+CONSOLE MESSAGE: TypeError: Cannot load script module-fetch-failure-not-cached.js. Failed integrity metadata check.
+
+PASS Module fetch failure is not cached in module pipeline 
+
diff --git a/LayoutTests/js/dom/modules/module-fetch-failure-not-cached.html b/LayoutTests/js/dom/modules/module-fetch-failure-not-cached.html
new file mode 100644 (file)
index 0000000..128fc7e
--- /dev/null
@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>Module fetch failure is not cached in module pipeline</title>
+<script src="../../../resources/testharness.js"></script>
+<script src="../../../resources/testharnessreport.js"></script>
+</head>
+<body>
+<script>
+promise_test(() => {
+    return new Promise(function (resolve, reject) {
+        var scriptWithBadValue = document.createElement('script');
+        scriptWithBadValue.type = 'module';
+        scriptWithBadValue.src = './script-tests/module-fetch-failure-not-cached.js';
+        scriptWithBadValue.integrity = 'sha256-badbeef';
+        scriptWithBadValue.onload = reject;
+        scriptWithBadValue.onerror = function () {
+            assert_equals(window.moduleIsLoaded, undefined);
+            var script = document.createElement('script');
+            script.type = 'module';
+            script.src = './script-tests/module-integrity-bad-value-success-with-cache.js';
+            script.integrity = 'sha256-7iiaipciOq3/cXnCpuOPyoC9GgCQw2F6y84mH4CJrGk=';
+            script.onload = function () {
+                assert_equals(window.moduleIsLoaded, true);
+                resolve();
+            };
+            script.onerror = reject;
+            document.body.appendChild(script);
+        };
+        document.body.appendChild(scriptWithBadValue);
+    });
+}, 'Module fetch failure is not cached in module pipeline');
+</script>
+</body>
+</html>
diff --git a/LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache-expected.txt b/LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache-expected.txt
new file mode 100644 (file)
index 0000000..3e23684
--- /dev/null
@@ -0,0 +1,3 @@
+
+PASS Module integrity check is ignored if target module is already cached 
+
diff --git a/LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache.html b/LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache.html
new file mode 100644 (file)
index 0000000..6a5b7dc
--- /dev/null
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>Module integrity check is ignored if target module is already cached</title>
+<script src="../../../resources/testharness.js"></script>
+<script src="../../../resources/testharnessreport.js"></script>
+</head>
+<body>
+<script>
+promise_test(() => {
+    return new Promise(function (resolve, reject) {
+        var script = document.createElement('script');
+        script.type = 'module';
+        script.src = './script-tests/module-integrity-bad-value-success-with-cache.js';
+        script.integrity = 'sha256-7iiaipciOq3/cXnCpuOPyoC9GgCQw2F6y84mH4CJrGk=';
+        script.onload = function () {
+            assert_equals(window.moduleIsLoaded, true);
+            var scriptWithBadValue = document.createElement('script');
+            scriptWithBadValue.type = 'module';
+            scriptWithBadValue.src = './script-tests/module-integrity-bad-value-success-with-cache.js';
+            scriptWithBadValue.integrity = 'sha256-badbeef';
+            scriptWithBadValue.onload = resolve;
+            scriptWithBadValue.onerror = reject;
+            document.body.appendChild(scriptWithBadValue);
+        };
+        script.onerror = reject;
+        document.body.appendChild(script);
+    });
+}, 'Module integrity check is ignored if target module is already cached');
+</script>
+</body>
+</html>
diff --git a/LayoutTests/js/dom/modules/script-tests/module-fetch-failure-not-cached.js b/LayoutTests/js/dom/modules/script-tests/module-fetch-failure-not-cached.js
new file mode 100644 (file)
index 0000000..1b5b82e
--- /dev/null
@@ -0,0 +1 @@
+window.moduleIsLoaded = true;
diff --git a/LayoutTests/js/dom/modules/script-tests/module-integrity-bad-value-success-with-cache.js b/LayoutTests/js/dom/modules/script-tests/module-integrity-bad-value-success-with-cache.js
new file mode 100644 (file)
index 0000000..1b5b82e
--- /dev/null
@@ -0,0 +1 @@
+window.moduleIsLoaded = true;
index 283f32f..acff454 100644 (file)
@@ -1,3 +1,52 @@
+2017-11-09  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [JSC] Retry module fetching if previous request fails
+        https://bugs.webkit.org/show_bug.cgi?id=178168
+
+        Reviewed by Saam Barati.
+
+        According to the latest spec, the failed fetching operation can be retried if it is requested again.
+        For example,
+
+            <script type="module" integrity="shaXXX-bad" src="./A.js"></script>
+            <script type="module" integrity="shaXXX-correct" src="./A.js"></script>
+
+        When performing the first module fetching, integrity check fails, and the load of this module becomes failed.
+        But when loading the second module, we do not use the cached failure result in the first module loading.
+        We retry fetching for "./A.js". In this case, we have a correct integrity and module fetching succeeds.
+        This is specified in whatwg/HTML[1]. If the fetching fails, we do not cache it.
+
+        Interestingly, fetching result and instantiation result will be cached if they succeeds. This is because we would
+        like to cache modules based on their URLs. As a result,
+
+            <script type="module" integrity="shaXXX-correct" src="./A.js"></script>
+            <script type="module" integrity="shaXXX-bad" src="./A.js"></script>
+
+        In the above case, the first loading succeeds. And the second loading also succeeds since the succeeded fetching and
+        instantiation are cached in the module pipeline.
+
+        This patch implements the above semantics. Previously, our module pipeline always caches the result. If the fetching
+        failed, all the subsequent fetching for the same URL fails even if we have different integrity values. We retry fetching
+        if the previous one fails. As an overview of our change,
+
+        1. Fetching result should be cached only if it succeeds. Two or more on-the-fly fetching requests to the same URLs should
+           be unified. But if currently executing one fails, other attempts should retry fetching.
+
+        2. Instantiation should be cached if fetching succeeds.
+
+        3. Satisfying should be cached if it succeeds.
+
+        [1]: https://html.spec.whatwg.org/#fetch-a-single-module-script
+
+        * builtins/ModuleLoaderPrototype.js:
+        (requestFetch):
+        (requestInstantiate):
+        (requestSatisfy):
+        (link):
+        (loadModule):
+        * runtime/JSGlobalObject.cpp:
+        (JSC::JSGlobalObject::init):
+
 2017-11-09  Devin Rousso  <webkit@devinrousso.com>
 
         Web Inspector: support undo/redo of insertAdjacentHTML
index e6ac55b..112dab1 100644 (file)
@@ -147,8 +147,22 @@ function requestFetch(entry, parameters, fetcher)
 
     "use strict";
 
-    if (entry.fetch)
-        return entry.fetch;
+    if (entry.fetch) {
+        var currentAttempt = entry.fetch;
+        if (entry.state !== @ModuleFetch)
+            return currentAttempt;
+
+        return currentAttempt.catch((error) => {
+            // Even if the existing fetching request failed, this attempt may succeed.
+            // For example, previous attempt used invalid integrity="" value. But this
+            // request could have the correct integrity="" value. In that case, we should
+            // retry fetching for this request.
+            // https://html.spec.whatwg.org/#fetch-a-single-module-script
+            if (currentAttempt === entry.fetch)
+                entry.fetch = @undefined;
+            return this.requestFetch(entry, parameters, fetcher);
+        });
+    }
 
     // Hook point.
     // 2. Loader.fetch
@@ -170,20 +184,20 @@ function requestInstantiate(entry, parameters, fetcher)
 
     "use strict";
 
+    // entry.instantiate is set if fetch succeeds.
     if (entry.instantiate)
         return entry.instantiate;
 
     var instantiatePromise = this.requestFetch(entry, parameters, fetcher).then((source) => {
+        // https://html.spec.whatwg.org/#fetch-a-single-module-script
+        // Now fetching request succeeds. Then even if instantiation fails, we should cache it.
+        // Instantiation won't be retried.
+        if (entry.instantiate)
+            return entry;
+        entry.instantiate = instantiatePromise;
+
         var key = entry.key;
         var moduleRecord = this.parseModule(key, source);
-
-        // FIXME: Described in the draft,
-        //   4. Fulfill entry.[[Instantiate]] with instance.
-        // But, instantiate promise should be fulfilled with the entry.
-        // We remove this statement because instantiate promise will be
-        // fulfilled without this "force fulfill" operation.
-        // https://github.com/whatwg/loader/pull/67
-
         var dependenciesMap = moduleRecord.dependenciesMap;
         var requestedModules = this.requestedModules(moduleRecord);
         var dependencies = @newArrayWithSize(requestedModules.length);
@@ -200,24 +214,27 @@ function requestInstantiate(entry, parameters, fetcher)
         @setStateToMax(entry, @ModuleSatisfy);
         return entry;
     });
-    entry.instantiate = instantiatePromise;
     return instantiatePromise;
 }
 
-function requestSatisfy(entry, parameters, fetcher)
+function requestSatisfy(entry, parameters, fetcher, visited)
 {
-    // https://whatwg.github.io/loader/#satisfy-instance
+    // https://html.spec.whatwg.org/#internal-module-script-graph-fetching-procedure
 
     "use strict";
 
     if (entry.satisfy)
         return entry.satisfy;
 
+    visited.@add(entry);
     var satisfyPromise = this.requestInstantiate(entry, parameters, fetcher).then((entry) => {
+        if (entry.satisfy)
+            return entry;
+
         var depLoads = @newArrayWithSize(entry.dependencies.length);
         for (var i = 0, length = entry.dependencies.length; i < length; ++i) {
             var depEntry = entry.dependencies[i];
-            var promise = @undefined;
+            var promise;
 
             // Recursive resolving. The dependencies of this entry is being resolved or already resolved.
             // Stop tracing the circular dependencies.
@@ -228,22 +245,24 @@ function requestSatisfy(entry, parameters, fetcher)
             // 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;
+            if (visited.@has(depEntry))
+                promise = this.requestInstantiate(depEntry, @undefined, fetcher);
             else {
                 // Currently, module loader do not pass any information for non-top-level module fetching.
-                promise = this.requestSatisfy(depEntry, @undefined, fetcher);
+                promise = this.requestSatisfy(depEntry, @undefined, fetcher, visited);
             }
             @putByValDirect(depLoads, i, promise);
         }
 
-        return @InternalPromise.internalAll(depLoads).then((modules) => {
+        return @InternalPromise.internalAll(depLoads).then((entries) => {
+            if (entry.satisfy)
+                return entry;
             @setStateToMax(entry, @ModuleLink);
+            entry.satisfy = satisfyPromise;
             return entry;
         });
     });
 
-    entry.satisfy = satisfyPromise;
     return satisfyPromise;
 }
 
@@ -251,7 +270,7 @@ function requestSatisfy(entry, parameters, fetcher)
 
 function link(entry, fetcher)
 {
-    // https://whatwg.github.io/loader/#link
+    // https://html.spec.whatwg.org/#fetch-the-descendants-of-and-instantiate-a-module-script
 
     "use strict";
 
@@ -319,7 +338,7 @@ function loadModule(moduleName, parameters, fetcher)
     // 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.
     return this.resolve(moduleName, @undefined, fetcher).then((key) => {
-        return this.requestSatisfy(this.ensureRegistered(key), parameters, fetcher);
+        return this.requestSatisfy(this.ensureRegistered(key), parameters, fetcher, new @Set);
     }).then((entry) => {
         return entry.key;
     });
@@ -350,7 +369,7 @@ function requestImportModule(key, parameters, fetcher)
 {
     "use strict";
 
-    return this.requestSatisfy(this.ensureRegistered(key), parameters, fetcher).then((entry) => {
+    return this.requestSatisfy(this.ensureRegistered(key), parameters, fetcher, new @Set).then((entry) => {
         this.linkAndEvaluateModule(entry.key, fetcher);
         return this.getModuleNamespaceObject(entry.module);
     });
index f5d7bf1..29c709a 100644 (file)
@@ -563,7 +563,7 @@ void JSGlobalObject::init(VM& vm)
     m_regExpMatchesArrayStructure.set(vm, this, createRegExpMatchesArrayStructure(vm, this));
     m_regExpMatchesArrayWithGroupsStructure.set(vm, this, createRegExpMatchesArrayWithGroupsStructure(vm, this));
 
-    m_moduleRecordStructure.set(vm, this, JSModuleRecord::createStructure(vm, this, m_objectPrototype.get()));
+    m_moduleRecordStructure.set(vm, this, JSModuleRecord::createStructure(vm, this, jsNull()));
     m_moduleNamespaceObjectStructure.set(vm, this, JSModuleNamespaceObject::createStructure(vm, this, jsNull()));
     {
         bool isCallable = false;