Bug 166926

Created attachment 298582 [details] Patch WIP: prototype. It just works.

*** Bug 166488 has been marked as a duplicate of this bug. ***

Attachment 298582 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 298961 [details] WIP

Created attachment 298976 [details] WIP

It works! Adding tests now. And extracted JSC part in https://bugs.webkit.org/show_bug.cgi?id=167099.

Created attachment 299030 [details] WIP

Created attachment 299492 [details] Patch

Comment on attachment 299492 [details] Patch Attachment 299492 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2933189 New failing tests: http/tests/security/import-script-crossorigin-loads-omit.html http/tests/security/import-script-crossorigin-loads-error.html

Created attachment 299495 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 299492 [details] Patch Attachment 299492 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2933195 New failing tests: http/tests/security/import-script-crossorigin-loads-omit.html http/tests/security/import-script-crossorigin-loads-error.html

Created attachment 299496 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Created attachment 299511 [details] Patch

Comment on attachment 299511 [details] Patch Attachment 299511 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2935010 New failing tests: imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html

Created attachment 299512 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6

(In reply to comment #14) > Comment on attachment 299511 [details] > Patch > > Attachment 299511 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.webkit.org/results/2935010 > > New failing tests: > imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/ > location-protocol-setter-non-broken.html This seems unrelated.

Comment on attachment 299511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299511&action=review > Source/WebCore/bindings/js/CachedScriptFetcher.cpp:45 > +{ > + return requestScriptWithCache(document, sourceURL, String()); > +} Is this default implementation ever used? If not, pure virtual function is more appropriate. If it is used, what does it mean to have String() for crossOriginMode? > Source/WebCore/bindings/js/CachedScriptFetcher.cpp:59 > - request.setAsPotentiallyCrossOrigin(m_crossOriginMode, document); > + request.setAsPotentiallyCrossOrigin(crossOriginMode, document); It's strange that this function takes crossOriginMode but there's also m_crossOriginMode. > Source/WebCore/bindings/js/ScriptModuleLoader.cpp:81 > + Remove this blank line? > Source/WebCore/bindings/js/ScriptModuleLoader.cpp:84 > + return makeUnexpected(ASCIILiteral("Module name constructs an invalid URL.")); I think "Module name does not resolve to a valid URL" is a better way of phrasing this. > Source/WebCore/bindings/js/ScriptModuleLoader.cpp:88 > JSC::JSInternalPromise* ScriptModuleLoader::resolve(JSC::JSGlobalObject* jsGlobalObject, JSC::ExecState* exec, JSC::JSModuleLoader*, JSC::JSValue moduleNameValue, JSC::JSValue importerModuleKey, JSC::JSValue) Please add a change log about why we're deleting code in this function. > Source/WebCore/bindings/js/ScriptModuleLoader.cpp:112 > + promise->reject(TypeError, ASCIILiteral("Importer module key is not Symbol or String.")); I think these error message should say "Importer module key is not *a* Symbol or *a* String" (talked with Joe about this). > Source/WebCore/bindings/js/ScriptModuleLoader.cpp:123 > + auto iterator = m_requestURLToResponseURLMap.find(importerModuleRequestURL); > + if (iterator == m_requestURLToResponseURLMap.end()) { > + promise->reject(TypeError, ASCIILiteral("Importer module key is an invalid URL.")); I'm a bit confused here. We've already checked that importerModuleRequestURL is a valid URL above. When do we hit this case? Also, are you intentionally fixing the error message here to not say "response URL"? > Source/WebCore/dom/InlineClassicScript.cpp:49 > + // https://github.com/tc39/proposal-dynamic-import/blob/master/HTML Integration.md > + // If the fetcher is not module script, credential mode is always "omit". > + return requestScriptWithCache(document, sourceURL, ASCIILiteral("omit")); It's not great that this comment is repeated four times in different places. What if we just passed in a value of new enum type like "enum class ScriptType { Module, Classic };" and then requestScriptWithCache just implemented this logic? Alternatively, we can move isClassicScript() and isModuleScript() in LoadableScript to CachedScriptFetcher and check its value. > LayoutTests/ChangeLog:8 > + * http/tests/misc/import-absolute-url-expected.txt: Added. We should consider converting these tests to web-platform-tests and contribute back. > LayoutTests/ChangeLog:62 > + * js/dom/modules/script-tests/import-from-loaded-classic-finish.js: Added. > + * js/dom/modules/script-tests/import-from-loaded-classic.js: Added. > + * js/dom/modules/script-tests/import-from-loaded-module-finish.js: Added. > + * js/dom/modules/script-tests/import-from-loaded-module.js: Added. These files should be in js/dom/modules/resources. script-tests are supposed to contain test JS files (i.e. ones that assert conditions, etc...), not JS files used by test cases. > LayoutTests/http/tests/security/contentSecurityPolicy/1.1/import-scriptnonce-allowed.html:16 > + function ok() > + { > + if (++count === 6) > + done("PASS"); > + } It's not great that we can't tell which case failed if this test starts failing. Why don't we pass in some identifier (e.g. number 1-6) as an argument? and then keep results like let results = []; function ok(number) { results.push(number); } window.onload = () => { let resultString = results.join(', '); ... } > LayoutTests/http/tests/security/contentSecurityPolicy/1.1/import-scriptnonce-blocked.html:43 > + setTimeout(function () { > + done("PASS"); > + }, 100); Do we really need to wait 100ms? Why can't we just wait for load event? module should delay the load event, right? Ideally, we want to use the same logic for blocked case & allowed case so that it's clear there's no bug in the tests themselves. In fact, I usually prefer interleaving test cases that should succeed with test cases that fail so that when a test case fails, I can be sure that there is no other reason a resource loading is failing. > LayoutTests/http/tests/security/import-module-crossorigin-loads.html:17 > +// Executed with "omit". Perhaps add the link to the spec as you've done in the code? > LayoutTests/http/tests/security/import-script-crossorigin-loads-error.html:16 > +// Executed with "omit". Ditto about adding a link. > LayoutTests/http/tests/security/import-script-crossorigin-loads-omit.html:16 > +// Executed with "omit". Ditto. > LayoutTests/http/tests/security/resources/cors-deny.php:10 > + if (strtolower($_GET["credentials"]) == "true") { > + header("Access-Control-Allow-Credentials: true"); > + } else { > + header("Access-Control-Allow-Credentials: false"); > + } Omit curly brackets around single line statements? > LayoutTests/js/dom/modules/import-execution-order.html:18 > + await import(`./script-tests/module-execution-order-mixed.js`); It looks like you forgot svn add these files. Again, they should be in resources directory. > LayoutTests/js/dom/modules/import-simple.html:19 > + await import(`./script-tests/module-src-simple.js`); It looks like you're missing module-src-simple.js as well.

Comment on attachment 299511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299511&action=review Thanks for your review. I'll upload the new revised patch with the changes that are pointed in this review. I'll add a new class, ScriptElementCachedScriptFetcher. This should be the base class of LoadableScript and InlineClassicScript. And scattered operations are collected and unified into this. >> Source/WebCore/bindings/js/CachedScriptFetcher.cpp:45 >> +} > > Is this default implementation ever used? If not, pure virtual function is more appropriate. > If it is used, what does it mean to have String() for crossOriginMode? Yes, it is used. When the instance of CachedScriptFetcher is created with CachedScriptFetcher::create(), this requestModuleScript is used. This is used when the fetcher is not coupled with any script tag. For example, the hander in HTML, like `<body onload="SCRIPT CODE">`. If we evaluate that "SCRIPT CODE", the fetcher of that becomes CachedScriptFetcher. >> Source/WebCore/bindings/js/CachedScriptFetcher.cpp:59 >> + request.setAsPotentiallyCrossOrigin(crossOriginMode, document); > > It's strange that this function takes crossOriginMode but there's also m_crossOriginMode. Since this cross origin mode use is different under the context. This m_crossOriginMode is the original cross origin mode of the script tag (or null if the fetcher is not coupled with any script tag). 1. If the code is module script, we inherit the original cross origin mode. 2. If the code is classic script, we do not inherit the original `corssorigin` attribute of the classic script. We just use `"omit"`. 3. If the code is not coupled with any script tag, the crossorigin mode becomes null. We should not set the above value to m_crossOriginMode since LoadableClassicScript uses this requestScriptWithCache to load the classic script. In that case, requestScriptWithCache should use the original cross origin mode. That's why we take the cross origin mode as a parameter. I'll drop this m_crossOriginMode from CachedScriptFetcher. And I'll create a new class, ScriptElementCachedScriptFetcher. That will be inherited from LoadableScript and InlineClassicScript. And ScriptElementCachedScriptFetcher has the m_crossOriginMode. >> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:81 >> + > > Remove this blank line? Fixed. >> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:84 >> + return makeUnexpected(ASCIILiteral("Module name constructs an invalid URL.")); > > I think "Module name does not resolve to a valid URL" is a better way of phrasing this. Sounds nice. Fixed. >> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:88 >> JSC::JSInternalPromise* ScriptModuleLoader::resolve(JSC::JSGlobalObject* jsGlobalObject, JSC::ExecState* exec, JSC::JSModuleLoader*, JSC::JSValue moduleNameValue, JSC::JSValue importerModuleKey, JSC::JSValue) > > Please add a change log about why we're deleting code in this function. We extracted the resolving part from resolve to use it in importModule hook function. I'll note it in the ChangeLog. >> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:112 >> + promise->reject(TypeError, ASCIILiteral("Importer module key is not Symbol or String.")); > > I think these error message should say "Importer module key is not *a* Symbol or *a* String" (talked with Joe about this). Sounds nice. Fixed. >> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:123 >> + promise->reject(TypeError, ASCIILiteral("Importer module key is an invalid URL.")); > > I'm a bit confused here. We've already checked that importerModuleRequestURL is a valid URL above. > When do we hit this case? Also, are you intentionally fixing the error message here to not say "response URL"? I think currently it never happens. Change it to assertion. >> Source/WebCore/dom/InlineClassicScript.cpp:49 >> + return requestScriptWithCache(document, sourceURL, ASCIILiteral("omit")); > > It's not great that this comment is repeated four times in different places. > What if we just passed in a value of new enum type like "enum class ScriptType { Module, Classic };" > and then requestScriptWithCache just implemented this logic? > Alternatively, we can move isClassicScript() and isModuleScript() in LoadableScript to CachedScriptFetcher and check its value. I'll change this part with ScriptElementCachedScriptFetcher. I'll move this to ScriptElementCachedScriptFetcher. And use isClassicScript() and isModuleScript() to select which cross origin mode should be used. >> LayoutTests/ChangeLog:8 >> + * http/tests/misc/import-absolute-url-expected.txt: Added. > > We should consider converting these tests to web-platform-tests and contribute back. Sounds nice. I'll open a bug for that. >> LayoutTests/ChangeLog:62 >> + * js/dom/modules/script-tests/import-from-loaded-module.js: Added. > > These files should be in js/dom/modules/resources. > script-tests are supposed to contain test JS files (i.e. ones that assert conditions, etc...), > not JS files used by test cases. As discussed on the IRC, I'll move them to resources/. self-contained module tests are basically included in JSTests. >> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/import-scriptnonce-allowed.html:16 >> + } > > It's not great that we can't tell which case failed if this test starts failing. > Why don't we pass in some identifier (e.g. number 1-6) as an argument? > and then keep results like > let results = []; > function ok(number) { results.push(number); } > window.onload = () => { > let resultString = results.join(', '); > ... > } Sounds nice. propagating the different identifier in ./resources/import-scriptnonce-allowedx.js and sort it to check is nice (since the order of the loading is not defind.). >> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/import-scriptnonce-blocked.html:43 >> + }, 100); > > Do we really need to wait 100ms? > Why can't we just wait for load event? > module should delay the load event, right? > Ideally, we want to use the same logic for blocked case & allowed case > so that it's clear there's no bug in the tests themselves. > > In fact, I usually prefer interleaving test cases that should succeed with test cases that fail > so that when a test case fails, I can be sure that there is no other reason a resource loading is failing. Ah, right. While the `import` itself is not related to the script tag (it does not dispatch any events), in this test, all the script tags are just ignored because of the invalid nonce. We do not need to wait 100ms. Maybe, we can combine the allowed test case with this. >> LayoutTests/http/tests/security/import-module-crossorigin-loads.html:17 >> +// Executed with "omit". > > Perhaps add the link to the spec as you've done in the code? Sounds nice. Added >> LayoutTests/http/tests/security/import-script-crossorigin-loads-error.html:16 >> +// Executed with "omit". > > Ditto about adding a link. Done. >> LayoutTests/http/tests/security/import-script-crossorigin-loads-omit.html:16 >> +// Executed with "omit". > > Ditto. Done. >> LayoutTests/http/tests/security/resources/cors-deny.php:10 >> + } > > Omit curly brackets around single line statements? Right. Done. >> LayoutTests/js/dom/modules/import-execution-order.html:18 >> + await import(`./script-tests/module-execution-order-mixed.js`); > > It looks like you forgot svn add these files. Again, they should be in resources directory. These files are reused. And right, we should move them to resources/. >> LayoutTests/js/dom/modules/import-simple.html:19 >> + await import(`./script-tests/module-src-simple.js`); > > It looks like you're missing module-src-simple.js as well. Done.

Created attachment 299810 [details] Patch

Created attachment 299812 [details] Patch

Created attachment 299913 [details] Patch

Comment on attachment 299913 [details] Patch Attachment 299913 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2956626 New failing tests: js/dom/modules/module-execution-order-mixed-with-classic-scripts.html http/tests/misc/module-absolute-url.html js/dom/modules/import-from-handler.html js/dom/modules/import-execution-order.html js/dom/modules/import-from-javascript-url.html inspector/model/script-sourceType.html js/dom/modules/module-execution-order-mixed.html js/dom/modules/import-from-module.html http/tests/misc/import-absolute-url.html js/dom/modules/module-src-simple.html js/dom/modules/module-src-dynamic.html js/dom/modules/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html js/dom/modules/import-simple.html js/dom/modules/nomodule-has-no-effect-on-module-src.html js/dom/modules/module-not-found-error-event-with-src-and-import.html

Created attachment 299915 [details] Archive of layout-test-results from ews113 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Oops, assertion condition is invalid, fix and upload soon.

Created attachment 299916 [details] Patch

Updated.

Comment on attachment 299916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299916&action=review > Source/WebCore/bindings/js/ScriptModuleLoader.cpp:76 > + // 2. If specifier does not start with the character U+002F SOLIDUS (/), the two-character sequence U+002E FULL STOP, U+002F SOLIDUS (./), > + // or the three-character sequence U+002E FULL STOP, U+002E FULL STOP, U+002F SOLIDUS (../), return failure and abort these steps. It's a bit noisy to have all these comments. Since there are only three steps, it might be better just omit them and just have URL. > Source/WebCore/dom/ScriptElementCachedScriptFetcher.cpp:42 > + String crossOriginMode = m_crossOriginMode; > + if (isClassicScript()) > + crossOriginMode = ASCIILiteral("omit"); > + return requestScriptWithCache(document, sourceURL, crossOriginMode); Why not just requestScriptWithCache(document, sourceURL, isClassicScript() ? ASCIILiteral("omit") : m_crossOriginMode) That'd avoid unnecessary ref-churn to StringImpl. > LayoutTests/http/tests/security/contentSecurityPolicy/1.1/import-scriptnonce.html:18 > + if (array.length === 6) { > + alert(array.sort().toString()); > + done("PASS"); Why don't we just use js-test.js and use shouldBe to compare results? > LayoutTests/http/tests/security/contentSecurityPolicy/1.1/import-scriptnonce.html:34 > + <script type="module" nonce="noncynonce"> We should probably also verify that load/error events are fired / not fired as expected. > LayoutTests/js/dom/modules/import-from-loaded-classic.html:11 > +// Module will be executed asynchronously. > +window.jsTestIsAsync = true; It's better if this code waited for load event and checked some global state being changed by the module script being loaded instead of a module script, which is the test subject, calling to the test framework directly. > LayoutTests/js/dom/modules/import-from-loaded-module.html:11 > +// Module will be executed asynchronously. > +window.jsTestIsAsync = true; Dotto. > LayoutTests/js/dom/modules/import-from-module.html:19 > + await import(`./resources/module-src-simple.js`); Again, it's probably better if this file itself was checking some state instead of module-src-simple.js directly calling to the test framework.

Comment on attachment 299916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299916&action=review Thanks! >> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:76 >> + // or the three-character sequence U+002E FULL STOP, U+002E FULL STOP, U+002F SOLIDUS (../), return failure and abort these steps. > > It's a bit noisy to have all these comments. > Since there are only three steps, it might be better just omit them and just have URL. OK. Done. >> Source/WebCore/dom/ScriptElementCachedScriptFetcher.cpp:42 >> + return requestScriptWithCache(document, sourceURL, crossOriginMode); > > Why not just > requestScriptWithCache(document, sourceURL, isClassicScript() ? ASCIILiteral("omit") : m_crossOriginMode) > That'd avoid unnecessary ref-churn to StringImpl. Sounds nice. Fixed. >> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/import-scriptnonce.html:18 >> + done("PASS"); > > Why don't we just use js-test.js and use shouldBe to compare results? This is because this test is under CSP. Eval is not allowd. So almost all the test functions in js-test is invalid. >> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/import-scriptnonce.html:34 >> + <script type="module" nonce="noncynonce"> > > We should probably also verify that load/error events are fired / not fired as expected. OK, changed. >> LayoutTests/js/dom/modules/import-from-loaded-classic.html:11 >> +window.jsTestIsAsync = true; > > It's better if this code waited for load event and checked some global state being changed by the module script being loaded > instead of a module script, which is the test subject, calling to the test framework directly. In terms of the events, import operator is not coupled with the script tag. So it does not dispatch any events. The import operator is asynchronously done in the classic script, it will be done after the onload event. So we do not have the way to detect that the import is correctly done without (1) polling the result in HTML side or (2) cooperating with the script. And I think the (1), polling the result with something like setInterval, is not good design. So, for this test, I'll take the (2) design. 1. This HTML will load the simple classic script "import-from-loaded-classic.js". 2. In this script, we will perform import. 3. imported script changes some global state 4. in the (1)'s classic script, we will acknowledge the completion of the importing by using the promise (that is returned by the import operator). 5. Then, call the code in HTML, it checks the state change. >> LayoutTests/js/dom/modules/import-from-loaded-module.html:11 >> +window.jsTestIsAsync = true; > > Dotto. Done. >> LayoutTests/js/dom/modules/import-from-module.html:19 >> + await import(`./resources/module-src-simple.js`); > > Again, it's probably better if this file itself was checking some state instead of > module-src-simple.js directly calling to the test framework. Done.

Committed r211280: <http://trac.webkit.org/changeset/211280>