Attachment 324219 Details for Bug 178168 – Patch

[patch] Patch

bug-178168-20171019201145.patch (text/plain), 16.47 KB, created by Yusuke Suzuki on 2017-10-19 04:11:46 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2017-10-19 04:11:46 PDT

Size: 16.47 KB

patch

obsolete

>Subversion Revision: 223683
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 5b0ff42e993db99ad07d898c3c38ea2d9eafecea..103aff3678c849d33764db4445615f6e801c9380 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,52 @@
>+2017-10-19  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 NOBODY (OOPS!).
>+
>+        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-10-18  Mark Lam  <mark.lam@apple.com>
> 
>         RegExpObject::defineOwnProperty() does not need to compare values if no descriptor value is specified.
>diff --git a/Source/JavaScriptCore/builtins/ModuleLoaderPrototype.js b/Source/JavaScriptCore/builtins/ModuleLoaderPrototype.js
>index e6ac55b09399fab8d64ff73cbbd0b9e204ef1fea..112dab13dc6ff734b232711b045ef8989a6580dd 100644
>--- a/Source/JavaScriptCore/builtins/ModuleLoaderPrototype.js
>+++ b/Source/JavaScriptCore/builtins/ModuleLoaderPrototype.js
>@@ -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);
>     });
>diff --git a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
>index 8b08ec65bee43b59259f727b6e558df7202c3ce1..2f8e355bcd914dfcec5f68e5e2abcf05f6a4a79d 100644
>--- a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
>+++ b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
>@@ -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;
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 58014089f2bd8f7877ba8d20bb8aa4ba109071ac..5ca5142637645642b193855fd379228f8389ce16 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2017-10-19  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 NOBODY (OOPS!).
>+
>+        * 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-10-18  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         Don't expose raw HTML in pasteboard to the web content
>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
>index 0000000000000000000000000000000000000000..484e3753519a4cfd8e6b8ae7c9a684ed469b922c
>--- /dev/null
>+++ b/LayoutTests/js/dom/modules/module-fetch-failure-not-cached-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..128fc7e52550731994ca12cbd9135c44d771bed6
>--- /dev/null
>+++ b/LayoutTests/js/dom/modules/module-fetch-failure-not-cached.html
>@@ -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
>index 0000000000000000000000000000000000000000..3e2368435901bb8f219bf51c5fd71ec3ee62f50b
>--- /dev/null
>+++ b/LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..6a5b7dc82653d65be570ce8728f2433a540a2af3
>--- /dev/null
>+++ b/LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache.html
>@@ -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
>index 0000000000000000000000000000000000000000..1b5b82e286de149b1ee0a74c5da87e34d85cf277
>--- /dev/null
>+++ b/LayoutTests/js/dom/modules/script-tests/module-fetch-failure-not-cached.js
>@@ -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
>index 0000000000000000000000000000000000000000..1b5b82e286de149b1ee0a74c5da87e34d85cf277
>--- /dev/null
>+++ b/LayoutTests/js/dom/modules/script-tests/module-integrity-bad-value-success-with-cache.js
>@@ -0,0 +1 @@
>+window.moduleIsLoaded = true;

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 178168: 323893 | 324219 | 326533 | 326539 | 326542 | 326543 | 326544