- Revision
- 224662
- Author
- utatane....@gmail.com
- Date
- 2017-11-09 21:28:34 -0800 (Thu, 09 Nov 2017)
Log Message
[JSC] Retry module fetching if previous request fails
https://bugs.webkit.org/show_bug.cgi?id=178168
Reviewed by Saam Barati.
Source/_javascript_Core:
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=""
<script type="module" integrity="shaXXX-correct" src=""
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=""
<script type="module" integrity="shaXXX-bad" src=""
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (224661 => 224662)
--- trunk/LayoutTests/ChangeLog 2017-11-10 03:21:23 UTC (rev 224661)
+++ trunk/LayoutTests/ChangeLog 2017-11-10 05:28:34 UTC (rev 224662)
@@ -1,3 +1,17 @@
+2017-11-09 Yusuke Suzuki <utatane....@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 <ryanhad...@apple.com>
Mark multiple service worker tests as flaky.
Added: trunk/LayoutTests/js/dom/modules/module-fetch-failure-not-cached-expected.txt (0 => 224662)
--- trunk/LayoutTests/js/dom/modules/module-fetch-failure-not-cached-expected.txt (rev 0)
+++ trunk/LayoutTests/js/dom/modules/module-fetch-failure-not-cached-expected.txt 2017-11-10 05:28:34 UTC (rev 224662)
@@ -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
+
Added: trunk/LayoutTests/js/dom/modules/module-fetch-failure-not-cached.html (0 => 224662)
--- trunk/LayoutTests/js/dom/modules/module-fetch-failure-not-cached.html (rev 0)
+++ trunk/LayoutTests/js/dom/modules/module-fetch-failure-not-cached.html 2017-11-10 05:28:34 UTC (rev 224662)
@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>Module fetch failure is not cached in module pipeline</title>
+<script src=""
+<script src=""
+</head>
+<body>
+<script>
+promise_test(() => {
+ return new Promise(function (resolve, reject) {
+ var scriptWithBadValue = document.createElement('script');
+ scriptWithBadValue.type = 'module';
+ scriptWithBadValue.src = '';
+ 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.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>
Added: trunk/LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache-expected.txt (0 => 224662)
--- trunk/LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache-expected.txt (rev 0)
+++ trunk/LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache-expected.txt 2017-11-10 05:28:34 UTC (rev 224662)
@@ -0,0 +1,3 @@
+
+PASS Module integrity check is ignored if target module is already cached
+
Added: trunk/LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache.html (0 => 224662)
--- trunk/LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache.html (rev 0)
+++ trunk/LayoutTests/js/dom/modules/module-integrity-bad-value-success-with-cache.html 2017-11-10 05:28:34 UTC (rev 224662)
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>Module integrity check is ignored if target module is already cached</title>
+<script src=""
+<script src=""
+</head>
+<body>
+<script>
+promise_test(() => {
+ return new Promise(function (resolve, reject) {
+ var script = document.createElement('script');
+ script.type = 'module';
+ script.src = '';
+ script.integrity = 'sha256-7iiaipciOq3/cXnCpuOPyoC9GgCQw2F6y84mH4CJrGk=';
+ script._onload_ = function () {
+ assert_equals(window.moduleIsLoaded, true);
+ var scriptWithBadValue = document.createElement('script');
+ scriptWithBadValue.type = 'module';
+ scriptWithBadValue.src = '';
+ 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>
Added: trunk/LayoutTests/js/dom/modules/script-tests/module-fetch-failure-not-cached.js (0 => 224662)
--- trunk/LayoutTests/js/dom/modules/script-tests/module-fetch-failure-not-cached.js (rev 0)
+++ trunk/LayoutTests/js/dom/modules/script-tests/module-fetch-failure-not-cached.js 2017-11-10 05:28:34 UTC (rev 224662)
@@ -0,0 +1 @@
+window.moduleIsLoaded = true;
Added: trunk/LayoutTests/js/dom/modules/script-tests/module-integrity-bad-value-success-with-cache.js (0 => 224662)
--- trunk/LayoutTests/js/dom/modules/script-tests/module-integrity-bad-value-success-with-cache.js (rev 0)
+++ trunk/LayoutTests/js/dom/modules/script-tests/module-integrity-bad-value-success-with-cache.js 2017-11-10 05:28:34 UTC (rev 224662)
@@ -0,0 +1 @@
+window.moduleIsLoaded = true;
Modified: trunk/Source/_javascript_Core/ChangeLog (224661 => 224662)
--- trunk/Source/_javascript_Core/ChangeLog 2017-11-10 03:21:23 UTC (rev 224661)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-11-10 05:28:34 UTC (rev 224662)
@@ -1,3 +1,52 @@
+2017-11-09 Yusuke Suzuki <utatane....@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=""
+ <script type="module" integrity="shaXXX-correct" src=""
+
+ 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=""
+ <script type="module" integrity="shaXXX-bad" src=""
+
+ 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 <web...@devinrousso.com>
Web Inspector: support undo/redo of insertAdjacentHTML
Modified: trunk/Source/_javascript_Core/builtins/ModuleLoaderPrototype.js (224661 => 224662)
--- trunk/Source/_javascript_Core/builtins/ModuleLoaderPrototype.js 2017-11-10 03:21:23 UTC (rev 224661)
+++ trunk/Source/_javascript_Core/builtins/ModuleLoaderPrototype.js 2017-11-10 05:28:34 UTC (rev 224662)
@@ -147,9 +147,23 @@
"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
// https://whatwg.github.io/loader/#browser-fetch
@@ -170,20 +184,20 @@
"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,13 +214,12 @@
@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";
@@ -213,11 +226,15 @@
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 @@
// 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 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 @@
// 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 @@
{
"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);
});
Modified: trunk/Source/_javascript_Core/runtime/JSGlobalObject.cpp (224661 => 224662)
--- trunk/Source/_javascript_Core/runtime/JSGlobalObject.cpp 2017-11-10 03:21:23 UTC (rev 224661)
+++ trunk/Source/_javascript_Core/runtime/JSGlobalObject.cpp 2017-11-10 05:28:34 UTC (rev 224662)
@@ -563,7 +563,7 @@
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;
