Bug 148897

Summary: [ES6] Integrate ES6 Modules into WebCore
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, benjamin, buildbot, commit-queue, darin, dbates, graouts, lzsoft.cja, mathias, m.goleb+bugzilla, rniwa, sam, sbarati, sonny.piers, webkit-bug-importer, webkit
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 149129, 149574, 149576, 161350, 161467, 161470, 161522, 161666, 161674, 161726    
Bug Blocks: 147340    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews114 for mac-yosemite
none
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2
none
Archive of layout-test-results from ews101 for mac-yosemite
none
Patch
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Archive of layout-test-results from ews116 for mac-yosemite
none
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2
none
Patch
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Archive of layout-test-results from ews113 for mac-yosemite
none
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Patch
none
Archive of layout-test-results from ews112 for mac-yosemite
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews100 for mac-yosemite
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Patch
none
Patch
none
Patch
none
Patch
none
WIP
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews114 for mac-yosemite
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews115 for mac-yosemite
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews101 for mac-yosemite
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Archive of layout-test-results from ews116 for mac-yosemite
none
Patch
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Archive of layout-test-results from ews101 for mac-yosemite
none
Archive of layout-test-results from ews112 for mac-yosemite
none
Patch
none
Patch
none
Checking EWS
none
Patch for landing none

Description Yusuke Suzuki 2015-09-05 01:50:01 PDT
Now, it's implemented in JSC shell.
It's time to integrate it to WebCore! (first, we'll enable it only inside WebKit, not expose it to user space, because the loader spec is not mature yet.)
Comment 1 Yusuke Suzuki 2015-09-10 01:24:45 PDT
Created attachment 260914 [details]
Patch

WIP: initial naive implementation, still considering the design
Comment 2 Yusuke Suzuki 2015-09-10 21:03:54 PDT
Created attachment 260987 [details]
Patch

WIP: part2
Comment 3 Yusuke Suzuki 2015-09-14 15:21:22 PDT
Created attachment 261138 [details]
Patch

WIP: part3
Comment 4 Yusuke Suzuki 2015-09-15 03:00:36 PDT
Created attachment 261183 [details]
Patch

WIP: part4
Comment 5 Yusuke Suzuki 2015-09-18 17:36:49 PDT
Created attachment 261547 [details]
Patch

WIP: part5
Comment 6 Yusuke Suzuki 2015-09-18 20:23:36 PDT
Created attachment 261562 [details]
Patch

WIP: part6
Comment 7 WebKit Commit Bot 2015-09-18 20:25:19 PDT
Attachment 261562 [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 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Yusuke Suzuki 2015-09-18 21:16:03 PDT
Created attachment 261570 [details]
Patch

WIP: part7
Comment 9 Yusuke Suzuki 2015-09-21 11:56:13 PDT
Created attachment 261672 [details]
Patch

WIP: part8
Comment 10 Yusuke Suzuki 2015-09-23 14:18:55 PDT
Created attachment 261841 [details]
Patch

WIP: part9, adding tests
Comment 11 Yusuke Suzuki 2015-09-23 15:35:37 PDT
Created attachment 261844 [details]
Patch

WIP: part10, adding more tests
Comment 12 Yusuke Suzuki 2015-09-23 17:31:48 PDT
Created attachment 261852 [details]
Patch
Comment 13 Yusuke Suzuki 2015-09-23 17:41:52 PDT
Created attachment 261854 [details]
Patch
Comment 14 Yusuke Suzuki 2015-09-23 22:01:55 PDT
Created attachment 261861 [details]
Patch
Comment 15 Alex Christensen 2015-09-24 11:31:24 PDT
Comment on attachment 261861 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261861&action=review

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:14439
> +    <ClCompile Include="..\dom\ModuleGraph.cpp">

Add this to DOMAllInOne.cpp.

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:18892
> +    <ClCompile Include="..\bindings\js\ModuleFetcher.cpp">

Add these to JSBindingsAllInOne.cpp
Comment 16 Yusuke Suzuki 2015-09-24 11:49:43 PDT
Created attachment 261880 [details]
Patch
Comment 17 Yusuke Suzuki 2015-09-24 11:50:29 PDT
Comment on attachment 261861 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261861&action=review

Thank you!

>> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:14439
>> +    <ClCompile Include="..\dom\ModuleGraph.cpp">
> 
> Add this to DOMAllInOne.cpp.

Thanks. I've added.

>> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:18892
>> +    <ClCompile Include="..\bindings\js\ModuleFetcher.cpp">
> 
> Add these to JSBindingsAllInOne.cpp

Ditto.
Comment 18 Yusuke Suzuki 2015-09-24 13:22:49 PDT
Created attachment 261889 [details]
Patch
Comment 19 Yusuke Suzuki 2015-09-24 13:46:41 PDT
Created attachment 261892 [details]
Patch
Comment 20 Yusuke Suzuki 2015-09-24 13:49:32 PDT
Created attachment 261893 [details]
Patch
Comment 21 Yusuke Suzuki 2015-09-24 14:02:36 PDT
Comment on attachment 261893 [details]
Patch

I'll split the patch into several small pieces.
Comment 22 Yusuke Suzuki 2015-09-25 15:06:47 PDT
Created attachment 261946 [details]
Patch

WIP: big picture, I'll split the patch into smaller ones
Comment 23 Alexey Proskuryakov 2015-09-25 16:58:57 PDT
Comment on attachment 261946 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261946&action=review

I only looked through parts that seemed more closely related to loading.

> Source/WebCore/bindings/js/ModuleFetcher.cpp:71
> +    request.setCharset(ASCIILiteral("utf-8"));

What would we do when the response includes a Content-Type with a different charset? If we are to only support utf-8, we should detect that as a fatal error, and refuse to decode - but this code forces utf-8 on top of explicit charset. That's a violation of the http spec, which is a lot more influential than gihub specs are.

But also, the requirement to only support utf-8 is a silly political stance. Discussion in https://github.com/whatwg/loader/issues/83 references worker loader, but we intentionally support arbitrary encodings with workers.

> Source/WebCore/bindings/js/ModuleFetcher.cpp:74
> +    // FIXME: Should handle "crossorigin" options.
> +    // Once the spec describes the details, we will follow it.

Are these details any different than for <img> or <video> elements?

> Source/WebCore/bindings/js/ModuleFetcher.cpp:75
> +    request.setInitiator(AtomicString(m_sourceURL.string()));

The initiator field doesn't appear to take URLs - it's either an element, or a predefined constant (not quite sure what the name constructor takes, but that doesn't sound like an URL either).

> Source/WebCore/bindings/js/ModuleFetcher.cpp:126
> +            m_deferred->reject(exec, JSC::Symbol::create(exec->vm(), *(frame->script().moduleLoaderAlreadyReportedErrorSymbol().uid())));

Is this where we get if the web page is closed while loading a module? I'm not sure if rejecting is the right thing to do in this case.

> Source/WebCore/bindings/js/ModuleLoader.cpp:76
> +            completedURL = m_document.completeURL(moduleName, URL(ParsedURLString, referrer));

The ParsedURLString constructor may only be used for strings that originated as URL.string(), and only if the URL was not invalid. I'm not sure where the JSValue, but it seems like it may not be.

> Source/WebCore/bindings/js/ModuleLoader.cpp:120
> +        sourceURL = URL(ParsedURLString, asString(moduleKeyValue)->value(exec));

Ditto.

> Source/WebCore/dom/ScriptElement.cpp:352
> +        String sourceUrl = sourceAttributeValue();

Style nit: should be "sourceURL"

> LayoutTests/js/dom/module-src-dynamic.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Please use HTML5 doctype in new tests.

If these are generated by some script, it should be updated (not in this patch).
Comment 24 Yusuke Suzuki 2015-09-25 17:29:37 PDT
Comment on attachment 261946 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261946&action=review

Thank you for your comments!

>> Source/WebCore/bindings/js/ModuleFetcher.cpp:71
>> +    request.setCharset(ASCIILiteral("utf-8"));
> 
> What would we do when the response includes a Content-Type with a different charset? If we are to only support utf-8, we should detect that as a fatal error, and refuse to decode - but this code forces utf-8 on top of explicit charset. That's a violation of the http spec, which is a lot more influential than gihub specs are.
> 
> But also, the requirement to only support utf-8 is a silly political stance. Discussion in https://github.com/whatwg/loader/issues/83 references worker loader, but we intentionally support arbitrary encodings with workers.

However, it raises severe problem allowing charset for modules because the execution result of the same module is cached.

<script type="module" charset="euc-jp" src="A.js"></script>
<script type="module" charset="utf-8" src="B.js"></script>

A.js:
import "C.js"

B.js:
import "C.js"

In this case, we only execute the C.js once.
And A.js and B.js share the result of C.js.
At that time, we cannot determine which charset should be applied to C.js.
I think forcing utf-8 is good simple solution for this.

We have an alternative solution: when the charset is different, load C.js twice in the above case and distinguish these modules.
But I'm not sure it's worth doing.
Since the module code contains the different syntax and semantics, I think we don't need to maintain the compatibility for the code written in the different charset. They don't contain any module syntax.

"What would we do when the response includes a Content-Type with a different charset? If we are to only support utf-8, we should detect that as a fatal error, and refuse to decode - but this code forces utf-8 on top of explicit charset. That's a violation of the http spec, which is a lot more influential than gihub specs are."

Nice catch, you're right.
We should refuse if the response includes a Content-Type with a different charset.
I'll investigate it and fix the implementation.

>> Source/WebCore/bindings/js/ModuleFetcher.cpp:74
>> +    // Once the spec describes the details, we will follow it.
> 
> Are these details any different than for <img> or <video> elements?

This is similar to the above problem.

<script type="module" crossorigin="use-credentials"  src="A.js"></script>
<script type="module" crossorigin="anonymous" src="B.js"></script>

A.js:
import "C.js"

B.js:
import "C.js"

In the above case, we cannot determine which cross origin option should be applied to the C.js.
This is still discussed.

>> Source/WebCore/bindings/js/ModuleFetcher.cpp:75
>> +    request.setInitiator(AtomicString(m_sourceURL.string()));
> 
> The initiator field doesn't appear to take URLs - it's either an element, or a predefined constant (not quite sure what the name constructor takes, but that doesn't sound like an URL either).

Thanks. In the current case, we don't know which script initiate this code. This is also the similar to the above problem.

<script type="module" src="A.js"></script>
<script type="module" src="B.js"></script>

A.js:
import "C.js"

B.js:
import "C.js"

In the above case, the initiator of the C.js is non-deterministic.
So, I'm now planning to modify the request to allow the other thing (like module key, or URL) as an initiator.

>> Source/WebCore/bindings/js/ModuleFetcher.cpp:126
>> +            m_deferred->reject(exec, JSC::Symbol::create(exec->vm(), *(frame->script().moduleLoaderAlreadyReportedErrorSymbol().uid())));
> 
> Is this where we get if the web page is closed while loading a module? I'm not sure if rejecting is the right thing to do in this case.

No, this is where we get if we encountered an error with resource fetching. For example, when we found 404.
So I think rejecting is the right for this.

>> Source/WebCore/bindings/js/ModuleLoader.cpp:76
>> +            completedURL = m_document.completeURL(moduleName, URL(ParsedURLString, referrer));
> 
> The ParsedURLString constructor may only be used for strings that originated as URL.string(), and only if the URL was not invalid. I'm not sure where the JSValue, but it seems like it may not be.

In the current implementation, this referrerValue's string is ensured it is always valid URL.
Because,
1. We only allow URL or Symbol for module key now by this resolve implementation.
2. The resolved module key will be used as a referrer value. Or if there is no referrer value, it becomes undefined. (referrer value means, the module key of the importing module. so resolve(imported-mod-name, importing-mod-key) will be called).
3. We already filtered symbol case and undefined case, so here, the referrer is always string and valid URL.

But, in the future, once the reflective dynamic module loader is introduced (now it's very early stage), the condition may become different.
So I'll rewrite here to the more defensive code :D (Any value may come here)

>> Source/WebCore/bindings/js/ModuleLoader.cpp:120
>> +        sourceURL = URL(ParsedURLString, asString(moduleKeyValue)->value(exec));
> 
> Ditto.

Fixed.

>> Source/WebCore/dom/ScriptElement.cpp:352
>> +        String sourceUrl = sourceAttributeValue();
> 
> Style nit: should be "sourceURL"

Thanks. Fixed.

>> LayoutTests/js/dom/module-src-dynamic.html:1
>> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> Please use HTML5 doctype in new tests.
> 
> If these are generated by some script, it should be updated (not in this patch).

Thanks. I copied header part from the other Promise tests. I'll fix test cases.
Comment 25 Yusuke Suzuki 2015-09-25 18:24:48 PDT
(In reply to comment #24)

I've fixed hook related things in https://bugs.webkit.org/show_bug.cgi?id=149574 and updated the patch :D
Comment 26 Yusuke Suzuki 2015-09-26 00:59:40 PDT
TODO:
Add tests for SVG script element with type="module"
Comment 27 Yusuke Suzuki 2016-08-20 00:59:57 PDT
Created attachment 286536 [details]
Patch

WIP: Rebaselining & updating
Comment 28 Yusuke Suzuki 2016-08-20 23:01:22 PDT
Created attachment 286556 [details]
Patch

WIP
Comment 29 Yusuke Suzuki 2016-08-20 23:17:09 PDT
Created attachment 286558 [details]
Patch

WIP
Comment 30 Radar WebKit Bug Importer 2016-08-22 08:59:37 PDT
<rdar://problem/27948010>
Comment 31 Yusuke Suzuki 2016-08-25 21:01:31 PDT
Created attachment 287071 [details]
Patch

WIP
Comment 32 Yusuke Suzuki 2016-08-26 11:18:30 PDT
Created attachment 287124 [details]
Patch

WIP
Comment 33 Yusuke Suzuki 2016-08-26 15:50:33 PDT
Created attachment 287165 [details]
Patch

WIP: preloader is fixed
Comment 34 Build Bot 2016-08-26 17:07:33 PDT
Comment on attachment 287165 [details]
Patch

Attachment 287165 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1950143

New failing tests:
js/dom/module-not-found-error-event-with-src-and-import.html
js/dom/module-not-found-error-event-with-src.html
js/dom/module-load-same-module-from-different-entry-point.html
js/dom/module-load-event-with-src.html
js/dom/module-load-same-module-from-different-entry-point-dynamic.html
js/dom/module-execution-order-mixed.html
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
Comment 35 Build Bot 2016-08-26 17:07:38 PDT
Created attachment 287175 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 36 Build Bot 2016-08-26 17:08:02 PDT
Comment on attachment 287165 [details]
Patch

Attachment 287165 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1950166

New failing tests:
js/dom/module-not-found-error-event-with-src-and-import.html
js/dom/module-not-found-error-event-with-src.html
js/dom/module-load-same-module-from-different-entry-point.html
js/dom/module-load-event-with-src.html
js/dom/module-load-same-module-from-different-entry-point-dynamic.html
js/dom/module-execution-order-mixed.html
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
Comment 37 Build Bot 2016-08-26 17:08:07 PDT
Created attachment 287176 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 38 Build Bot 2016-08-26 17:27:46 PDT
Comment on attachment 287165 [details]
Patch

Attachment 287165 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1950242

New failing tests:
js/dom/module-not-found-error-event-with-src-and-import.html
js/dom/module-not-found-error-event-with-src.html
js/dom/module-load-same-module-from-different-entry-point.html
js/dom/module-load-event-with-src.html
js/dom/module-load-same-module-from-different-entry-point-dynamic.html
js/dom/module-execution-order-mixed.html
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
Comment 39 Build Bot 2016-08-26 17:27:53 PDT
Created attachment 287179 [details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5
Comment 40 Build Bot 2016-08-26 19:10:23 PDT
Comment on attachment 287165 [details]
Patch

Attachment 287165 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1950806

New failing tests:
js/dom/module-not-found-error-event-with-src-and-import.html
js/dom/module-not-found-error-event-with-src.html
js/dom/module-load-same-module-from-different-entry-point.html
js/dom/module-load-event-with-src.html
js/dom/module-load-same-module-from-different-entry-point-dynamic.html
js/dom/module-execution-order-mixed.html
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
Comment 41 Build Bot 2016-08-26 19:10:28 PDT
Created attachment 287187 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 42 Yusuke Suzuki 2016-08-28 02:10:06 PDT
Created attachment 287223 [details]
Patch

WIP
Comment 43 Build Bot 2016-08-28 03:23:14 PDT
Comment on attachment 287223 [details]
Patch

Attachment 287223 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1958804

New failing tests:
js/dom/module-not-found-error-event-with-src-and-import.html
js/dom/module-not-found-error-event-with-src.html
js/dom/module-not-found-error-event.html
js/dom/module-load-same-module-from-different-entry-point.html
js/dom/module-load-event-with-src.html
js/dom/module-load-event.html
js/dom/module-load-same-module-from-different-entry-point-dynamic.html
js/dom/module-execution-order-mixed.html
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
Comment 44 Build Bot 2016-08-28 03:23:20 PDT
Created attachment 287225 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 45 Build Bot 2016-08-28 03:26:59 PDT
Comment on attachment 287223 [details]
Patch

Attachment 287223 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1958807

New failing tests:
js/dom/module-not-found-error-event-with-src-and-import.html
js/dom/module-not-found-error-event-with-src.html
js/dom/module-not-found-error-event.html
js/dom/module-load-same-module-from-different-entry-point.html
js/dom/module-load-event-with-src.html
js/dom/module-load-event.html
js/dom/module-load-same-module-from-different-entry-point-dynamic.html
js/dom/module-execution-order-mixed.html
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
Comment 46 Build Bot 2016-08-28 03:27:06 PDT
Created attachment 287226 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 47 Build Bot 2016-08-28 03:37:08 PDT
Comment on attachment 287223 [details]
Patch

Attachment 287223 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1958819

New failing tests:
js/dom/module-not-found-error-event-with-src-and-import.html
js/dom/module-not-found-error-event-with-src.html
js/dom/module-not-found-error-event.html
js/dom/module-incorrect-relative-specifier.html
js/dom/module-load-same-module-from-different-entry-point.html
js/dom/module-load-event-with-src.html
js/dom/module-load-event.html
js/dom/module-load-same-module-from-different-entry-point-dynamic.html
js/dom/module-execution-order-mixed.html
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
Comment 48 Build Bot 2016-08-28 03:37:15 PDT
Created attachment 287227 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 49 Build Bot 2016-08-28 03:37:41 PDT
Comment on attachment 287223 [details]
Patch

Attachment 287223 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1958814

New failing tests:
js/dom/module-not-found-error-event-with-src-and-import.html
js/dom/module-not-found-error-event-with-src.html
js/dom/module-not-found-error-event.html
js/dom/module-load-same-module-from-different-entry-point.html
js/dom/module-load-event-with-src.html
js/dom/module-load-event.html
js/dom/module-load-same-module-from-different-entry-point-dynamic.html
js/dom/module-execution-order-mixed.html
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
Comment 50 Build Bot 2016-08-28 03:37:52 PDT
Created attachment 287228 [details]
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5
Comment 51 Yusuke Suzuki 2016-08-28 23:05:59 PDT
Created attachment 287247 [details]
Patch

WIP
Comment 52 Build Bot 2016-08-29 00:25:26 PDT
Comment on attachment 287247 [details]
Patch

Attachment 287247 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1963922

New failing tests:
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
js/dom/module-not-found-error-event.html
Comment 53 Build Bot 2016-08-29 00:25:30 PDT
Created attachment 287253 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 54 Build Bot 2016-08-29 00:27:53 PDT
Comment on attachment 287247 [details]
Patch

Attachment 287247 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1963917

New failing tests:
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
js/dom/module-not-found-error-event.html
Comment 55 Build Bot 2016-08-29 00:28:00 PDT
Created attachment 287254 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 56 Build Bot 2016-08-29 00:30:33 PDT
Comment on attachment 287247 [details]
Patch

Attachment 287247 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1963921

New failing tests:
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
js/dom/module-not-found-error-event.html
Comment 57 Build Bot 2016-08-29 00:30:39 PDT
Created attachment 287255 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 58 Build Bot 2016-08-29 00:39:17 PDT
Comment on attachment 287247 [details]
Patch

Attachment 287247 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1963930

New failing tests:
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
js/dom/module-not-found-error-event.html
Comment 59 Build Bot 2016-08-29 00:39:25 PDT
Created attachment 287257 [details]
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5
Comment 60 Yusuke Suzuki 2016-08-29 10:10:30 PDT
Created attachment 287277 [details]
Patch

WIP
Comment 61 Yusuke Suzuki 2016-08-29 10:35:44 PDT
Created attachment 287279 [details]
Patch

WIP
Comment 62 Build Bot 2016-08-29 11:38:00 PDT
Comment on attachment 287279 [details]
Patch

Attachment 287279 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1966716

New failing tests:
transitions/default-timing-function.html
js/dom/module-not-found-error-event.html
Comment 63 Build Bot 2016-08-29 11:38:10 PDT
Created attachment 287292 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 64 Build Bot 2016-08-29 11:45:50 PDT
Comment on attachment 287279 [details]
Patch

Attachment 287279 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1966746

New failing tests:
js/dom/module-not-found-error-event.html
Comment 65 Build Bot 2016-08-29 11:45:58 PDT
Created attachment 287294 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 66 Build Bot 2016-08-29 11:49:28 PDT
Comment on attachment 287279 [details]
Patch

Attachment 287279 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1966743

New failing tests:
js/dom/module-not-found-error-event.html
Comment 67 Build Bot 2016-08-29 11:49:34 PDT
Created attachment 287297 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 68 Build Bot 2016-08-29 11:55:45 PDT
Comment on attachment 287279 [details]
Patch

Attachment 287279 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1966757

New failing tests:
js/dom/module-not-found-error-event.html
Comment 69 Build Bot 2016-08-29 11:55:53 PDT
Created attachment 287300 [details]
Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5
Comment 70 Yusuke Suzuki 2016-08-29 14:27:35 PDT
Created attachment 287329 [details]
Patch

WIP
Comment 71 Yusuke Suzuki 2016-08-29 15:02:20 PDT
OK, the tests have passed.
I'm now cleaning up the patch.
Maybe, I'll split the patch into reasonable pieces.
Comment 72 Yusuke Suzuki 2016-08-30 21:04:18 PDT
Created attachment 287479 [details]
Patch

WIP: rebaselining after r205218
Comment 73 Yusuke Suzuki 2016-08-31 01:14:37 PDT
Created attachment 287504 [details]
Patch

WIP
Comment 74 Yusuke Suzuki 2016-08-31 14:52:02 PDT
Created attachment 287545 [details]
Patch

WIP: update. Let's spawn the JSC part next
Comment 75 Build Bot 2016-08-31 16:34:29 PDT
Comment on attachment 287545 [details]
Patch

Attachment 287545 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1982837

New failing tests:
fast/scrolling/ios/scrollTo-at-page-load.html
Comment 76 Build Bot 2016-08-31 16:34:35 PDT
Created attachment 287563 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5
Comment 77 Yusuke Suzuki 2016-08-31 21:44:22 PDT
Created attachment 287596 [details]
Patch

WIP: Rebaselining after r205276 and r205278
Comment 78 Yusuke Suzuki 2016-09-01 11:43:45 PDT
Created attachment 287646 [details]
Patch

WIP: Make ScriptModuleFetcher ActiveDOMObject
Comment 79 Yusuke Suzuki 2016-09-01 15:23:49 PDT
Created attachment 287690 [details]
Patch

WIP: Intentionally allow charset attribute
Comment 80 Yusuke Suzuki 2016-09-01 16:04:00 PDT
Comment on attachment 261946 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261946&action=review

>>> Source/WebCore/bindings/js/ModuleFetcher.cpp:71
>>> +    request.setCharset(ASCIILiteral("utf-8"));
>> 
>> What would we do when the response includes a Content-Type with a different charset? If we are to only support utf-8, we should detect that as a fatal error, and refuse to decode - but this code forces utf-8 on top of explicit charset. That's a violation of the http spec, which is a lot more influential than gihub specs are.
>> 
>> But also, the requirement to only support utf-8 is a silly political stance. Discussion in https://github.com/whatwg/loader/issues/83 references worker loader, but we intentionally support arbitrary encodings with workers.
> 
> However, it raises severe problem allowing charset for modules because the execution result of the same module is cached.
> 
> <script type="module" charset="euc-jp" src="A.js"></script>
> <script type="module" charset="utf-8" src="B.js"></script>
> 
> A.js:
> import "C.js"
> 
> B.js:
> import "C.js"
> 
> In this case, we only execute the C.js once.
> And A.js and B.js share the result of C.js.
> At that time, we cannot determine which charset should be applied to C.js.
> I think forcing utf-8 is good simple solution for this.
> 
> We have an alternative solution: when the charset is different, load C.js twice in the above case and distinguish these modules.
> But I'm not sure it's worth doing.
> Since the module code contains the different syntax and semantics, I think we don't need to maintain the compatibility for the code written in the different charset. They don't contain any module syntax.
> 
> "What would we do when the response includes a Content-Type with a different charset? If we are to only support utf-8, we should detect that as a fatal error, and refuse to decode - but this code forces utf-8 on top of explicit charset. That's a violation of the http spec, which is a lot more influential than gihub specs are."
> 
> Nice catch, you're right.
> We should refuse if the response includes a Content-Type with a different charset.
> I'll investigate it and fix the implementation.

Now, we intentionally use the "character" attribute even if the module script is fetched.
In the above described case, we use the first one, "A". This is somewhat strange, but is consistent to the current spec's "nonce", "crossorigin" etc. attributes handling.

>>> Source/WebCore/bindings/js/ModuleFetcher.cpp:74
>>> +    // Once the spec describes the details, we will follow it.
>> 
>> Are these details any different than for <img> or <video> elements?
> 
> This is similar to the above problem.
> 
> <script type="module" crossorigin="use-credentials"  src="A.js"></script>
> <script type="module" crossorigin="anonymous" src="B.js"></script>
> 
> A.js:
> import "C.js"
> 
> B.js:
> import "C.js"
> 
> In the above case, we cannot determine which cross origin option should be applied to the C.js.
> This is still discussed.

Now the spec describes the behavior.
In the above case, the first fetching request (In this case, request from A) should be used and shared.

>>> Source/WebCore/bindings/js/ModuleFetcher.cpp:75
>>> +    request.setInitiator(AtomicString(m_sourceURL.string()));
>> 
>> The initiator field doesn't appear to take URLs - it's either an element, or a predefined constant (not quite sure what the name constructor takes, but that doesn't sound like an URL either).
> 
> Thanks. In the current case, we don't know which script initiate this code. This is also the similar to the above problem.
> 
> <script type="module" src="A.js"></script>
> <script type="module" src="B.js"></script>
> 
> A.js:
> import "C.js"
> 
> B.js:
> import "C.js"
> 
> In the above case, the initiator of the C.js is non-deterministic.
> So, I'm now planning to modify the request to allow the other thing (like module key, or URL) as an initiator.

Fixed. Now ScriptModuleFetcher uses the element to construct the request.

>>> Source/WebCore/bindings/js/ModuleFetcher.cpp:126
>>> +            m_deferred->reject(exec, JSC::Symbol::create(exec->vm(), *(frame->script().moduleLoaderAlreadyReportedErrorSymbol().uid())));
>> 
>> Is this where we get if the web page is closed while loading a module? I'm not sure if rejecting is the right thing to do in this case.
> 
> No, this is where we get if we encountered an error with resource fetching. For example, when we found 404.
> So I think rejecting is the right for this.

We now changed the ScriptModuleLoader code to the form intentionally using DeferredWrapper.
It won't call the callback when the active DOM objects have been suspended. So the module pipeline stops.
Comment 81 Yusuke Suzuki 2016-09-01 16:06:21 PDT
Created attachment 287698 [details]
Patch

WIP
Comment 82 Yusuke Suzuki 2016-09-01 19:47:45 PDT
Created attachment 287716 [details]
Patch

WIP: almost completed patch. needs testing!
Comment 83 Yusuke Suzuki 2016-09-01 20:52:11 PDT
Created attachment 287725 [details]
Patch

WIP: rebaselining after r205335. JSC part is landed
Comment 84 Yusuke Suzuki 2016-09-01 21:17:07 PDT
Created attachment 287727 [details]
Patch

WIP
Comment 85 Yusuke Suzuki 2016-09-01 22:18:45 PDT
Created attachment 287733 [details]
Patch

WIP
Comment 86 Build Bot 2016-09-01 23:34:46 PDT
Comment on attachment 287733 [details]
Patch

Attachment 287733 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1991993

New failing tests:
js/dom/module-not-found-error-event-with-src-and-import.html
js/dom/module-not-found-error-event-with-src.html
js/dom/module-will-fire-beforeload.html
js/dom/module-incorrect-relative-specifier.html
js/dom/module-load-event-with-src.html
js/dom/module-src-simple.html
js/dom/module-execution-order-mixed.html
Comment 87 Build Bot 2016-09-01 23:34:56 PDT
Created attachment 287742 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 88 Build Bot 2016-09-01 23:38:28 PDT
Comment on attachment 287733 [details]
Patch

Attachment 287733 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1991998

New failing tests:
js/dom/module-not-found-error-event-with-src-and-import.html
js/dom/module-not-found-error-event-with-src.html
js/dom/module-will-fire-beforeload.html
js/dom/module-incorrect-relative-specifier.html
js/dom/module-load-event-with-src.html
js/dom/module-src-simple.html
js/dom/module-execution-order-mixed.html
Comment 89 Build Bot 2016-09-01 23:38:37 PDT
Created attachment 287743 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 90 Yusuke Suzuki 2016-09-01 23:50:55 PDT
Created attachment 287744 [details]
Patch

WIP
Comment 91 Build Bot 2016-09-02 01:30:49 PDT
Comment on attachment 287744 [details]
Patch

Attachment 287744 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1992404

New failing tests:
js/dom/module-execution-error-should-be-propagated-to-onerror.html
js/dom/module-src-simple.html
js/dom/module-type-case-insensitive.html
js/dom/module-load-event-with-src.html
js/dom/module-load-event.html
js/dom/module-load-same-module-from-different-entry-point-dynamic.html
js/dom/module-load-same-module-from-different-entry-point.html
js/dom/module-inline-simple.html
js/dom/module-execution-order-mixed.html
js/dom/module-src-dynamic.html
js/dom/module-execution-order-inline.html
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
js/dom/module-inline-dynamic.html
Comment 92 Build Bot 2016-09-02 01:30:57 PDT
Created attachment 287750 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 93 Yusuke Suzuki 2016-09-02 09:21:26 PDT
Created attachment 287764 [details]
Patch

WIP
Comment 94 Yusuke Suzuki 2016-09-02 11:07:48 PDT
Created attachment 287781 [details]
Patch

WIP: finishing the implementation. adding bunch of tests...
Comment 95 Yusuke Suzuki 2016-09-02 17:18:24 PDT
Created attachment 287836 [details]
Patch

OK. Still I'm adding tests, but early review is welcome! For EWS, now I commented out ES6_MODULES flag check, but this will be fixed when landing
Comment 96 Yusuke Suzuki 2016-09-02 17:22:10 PDT
Comment on attachment 287836 [details]
Patch

Oops. Fixing tests.
Comment 97 Build Bot 2016-09-02 18:34:27 PDT
Comment on attachment 287836 [details]
Patch

Attachment 287836 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1997441

New failing tests:
js/dom/module-not-found-error-event-with-src-and-import.html
js/dom/module-will-fire-beforeload.html
js/dom/module-inline-simple.html
js/dom/module-type-case-insensitive.html
js/dom/module-load-event-with-src.html
js/dom/module-load-same-module-from-different-entry-point-dynamic.html
js/dom/module-load-same-module-from-different-entry-point.html
js/dom/module-src-current-script.html
js/dom/module-src-simple.html
js/dom/module-execution-order-mixed.html
js/dom/module-src-dynamic.html
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
js/dom/module-inline-dynamic.html
Comment 98 Build Bot 2016-09-02 18:34:34 PDT
Created attachment 287843 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 99 Build Bot 2016-09-02 18:44:08 PDT
Comment on attachment 287836 [details]
Patch

Attachment 287836 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1997466

New failing tests:
js/dom/module-not-found-error-event-with-src-and-import.html
js/dom/module-will-fire-beforeload.html
js/dom/module-src-simple.html
js/dom/module-type-case-insensitive.html
js/dom/module-load-event-with-src.html
js/dom/module-load-same-module-from-different-entry-point-dynamic.html
js/dom/module-load-same-module-from-different-entry-point.html
js/dom/module-src-current-script.html
js/dom/module-inline-simple.html
js/dom/module-execution-order-mixed.html
js/dom/module-src-dynamic.html
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
js/dom/module-inline-dynamic.html
Comment 100 Build Bot 2016-09-02 18:44:15 PDT
Created attachment 287844 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 101 Yusuke Suzuki 2016-09-02 19:09:16 PDT
Created attachment 287846 [details]
Patch

Still adding tests. Now tests become green
Comment 102 Ryosuke Niwa 2016-09-02 19:43:04 PDT
Comment on attachment 287846 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287846&action=review

> Source/JavaScriptCore/runtime/PrivateName.h:57
> -    Ref<SymbolImpl> m_uid;
> +    RefPtr<SymbolImpl> m_uid;

Why are we changing this back to RefPtr?

> Source/WebCore/dom/ScriptElement.cpp:432
> +bool ScriptElement::requestModuleScript(const TextPosition& scriptStartPosition)

I'd call this requestModuleScriptGraph or requestScriptModuleDependencies instead since you're loading all its dependences.

> Source/WebCore/dom/ScriptElement.cpp:437
> +    // The browser module loader loads the module sources.
> +    // Since the module involves loading of the dependent modules and the dependent modules does not have corresponding script elements,
> +    // the functionality of loading modules is separated from the script element.
> +

With that rename, I don't think we need this comment.

> Source/WebCore/dom/ScriptElement.cpp:449
> +        // Script element may be detached and attached to a different document while executing the event listener.
> +        if (!m_element.inDocument() || &m_element.document() != originalDocument.ptr())

We should definitely add a test for this.  But WebKit's way of adding a comment like this is to use a local variable of a descriptive name instead.
e.g.
bool didEventListenerDisconnectThisElement = !m_element.inDocument() || &m_element.document() != originalDocument.ptr()

> Source/WebCore/dom/ScriptElement.cpp:467
> +        // Reset line numbering for nested writes.

I don't think we need this comment.

> Source/WebCore/dom/ScriptRunner.cpp:74
> +void ScriptRunner::queueScriptModuleGraphForExecution(ScriptElement* scriptElement, ScriptModuleGraph& moduleGraph, ExecutionType executionType)

Is it possible to introduce an abstract interface from which both ScriptModuleGraph and CachedResourceHandle<CachedScript> (or a new wrapper for it) inherit
so that we don't have to duplicate code like this?
Comment 103 Yusuke Suzuki 2016-09-03 00:29:19 PDT
Comment on attachment 287846 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287846&action=review

Thank you!

>> Source/JavaScriptCore/runtime/PrivateName.h:57
>> +    RefPtr<SymbolImpl> m_uid;
> 
> Why are we changing this back to RefPtr?

This is because  it is the simple way to make this PrivateName copyable.

>> Source/WebCore/dom/ScriptElement.cpp:432
>> +bool ScriptElement::requestModuleScript(const TextPosition& scriptStartPosition)
> 
> I'd call this requestModuleScriptGraph or requestScriptModuleDependencies instead since you're loading all its dependences.

Right. Sounds nice. Fixed.

>> Source/WebCore/dom/ScriptElement.cpp:437
>> +
> 
> With that rename, I don't think we need this comment.

OK. Dropped.

>> Source/WebCore/dom/ScriptElement.cpp:449
>> +        if (!m_element.inDocument() || &m_element.document() != originalDocument.ptr())
> 
> We should definitely add a test for this.  But WebKit's way of adding a comment like this is to use a local variable of a descriptive name instead.
> e.g.
> bool didEventListenerDisconnectThisElement = !m_element.inDocument() || &m_element.document() != originalDocument.ptr()

Nice. Fixed. And I also applied this fix to the requestClassicScript.

>> Source/WebCore/dom/ScriptElement.cpp:467
>> +        // Reset line numbering for nested writes.
> 
> I don't think we need this comment.

OK. Dropped. And I also dropped the same comment in the requestClassicScript.

>> Source/WebCore/dom/ScriptRunner.cpp:74
>> +void ScriptRunner::queueScriptModuleGraphForExecution(ScriptElement* scriptElement, ScriptModuleGraph& moduleGraph, ExecutionType executionType)
> 
> Is it possible to introduce an abstract interface from which both ScriptModuleGraph and CachedResourceHandle<CachedScript> (or a new wrapper for it) inherit
> so that we don't have to duplicate code like this?

Yeah, we can do that. I think it is cleaner to provide a new wrapper, since these two things are managed by the different somewhat ref-counting system. (While we use CachedResourceHandle for CachedScript, ScriptModuleGraph is managed by RefPtr).
Comment 104 Yusuke Suzuki 2016-09-06 15:07:37 PDT
Created attachment 288049 [details]
Patch

Adding tests now
Comment 105 Yusuke Suzuki 2016-09-06 15:12:46 PDT
Comment on attachment 288049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288049&action=review

> Source/WebCore/dom/LoadableScript.h:1
> +/*

Add LoadableScript, LoadableScriptModuleGraph, and LoadableClassicScript.
The latter 2 classes are derived classes from LoadableScript.
Comment 106 Yusuke Suzuki 2016-09-06 15:14:16 PDT
I'll rebaseline the patch.
Comment 107 Yusuke Suzuki 2016-09-06 15:18:34 PDT
Created attachment 288051 [details]
Patch

Adding tests now
Comment 108 Yusuke Suzuki 2016-09-06 15:23:38 PDT
Comment on attachment 288051 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288051&action=review

> Source/WebCore/dom/LoadableClassicScript.h:57
> +    HashCountedSet<LoadableScriptClient*> m_clients;

Oops. This will be dropped.
Comment 109 Yusuke Suzuki 2016-09-06 15:33:33 PDT
Comment on attachment 288051 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288051&action=review

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:925
> +            m_pendingScript = &downcast<LoadableClassicScript>(*scriptElement->loadableScript()).cachedScript();

Oops. This part will be slightly updated & cleaned up.
Comment 110 Yusuke Suzuki 2016-09-06 15:39:06 PDT
Comment on attachment 288051 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288051&action=review

>> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:925
>> +            m_pendingScript = &downcast<LoadableClassicScript>(*scriptElement->loadableScript()).cachedScript();
> 
> Oops. This part will be slightly updated & cleaned up.

I think this XML Document parser's scripting needs refactoring.
In the meantime, I disable "module" feature for the XML document, but this should be enabled later.
Specifically, XMLDocumentParser should use PendingScript to construct such a logic I think.
Comment 111 Yusuke Suzuki 2016-09-06 16:29:43 PDT
Created attachment 288066 [details]
Patch

Fix EFL build, add tests for mime type checking
Comment 112 Yusuke Suzuki 2016-09-06 16:35:28 PDT
Comment on attachment 288066 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288066&action=review

> Source/WebCore/dom/ScriptElement.cpp:178
> +// #endif

Note that this commenting out and TestExpectation is now modified to test modules in EWS.
When landing, we will make this guard back.
Comment 113 Yusuke Suzuki 2016-09-06 16:56:32 PDT
Comment on attachment 288066 [details]
Patch

Discussed with rniwa offline. I'll first split this patch into smaller ones, especially, I'll extract the LoadableScript part.
Comment 114 Ryosuke Niwa 2016-09-06 16:57:45 PDT
Comment on attachment 288051 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288051&action=review

I think we may want to split the part to add LoadableScript as a wrapper for CachedScript as a separate patch so that the related refactoring can be landed separately.  That'll make this patch easier to review & reason through.

> Source/JavaScriptCore/ChangeLog:10
> +        * runtime/PrivateName.h: PrivateName should be copyable. I completely forgot that in the previous patch.
> +        (JSC::PrivateName::PrivateName):
> +        (JSC::PrivateName::uid):

Can we land this in a separate patch?

> Source/WebCore/ChangeLog:28
> +            3. ScriptModuleFetcher

We might want to call this CachedModuleScriptLoader or something since this class doesn't really fetch the module file if it's already cached.

> Source/WebCore/bindings/js/ScriptModuleFetcher.cpp:80
> +    m_loaded = true;

Don't we need to clear m_deferred here?
Also, if we've done that, do we still need a separate m_loaded boolean?

> Source/WebCore/bindings/js/ScriptModuleFetcher.cpp:89
> +    if (m_client)
> +        m_client->notifyFinished(*this);

Why don't we just call this with m_deferred here instead of keeping it around in the fetcher object and calling deferred() in ScriptModuleLoader::notifyFinished ?
That way, we can even use WTFMove() and remove the ownership the callee.

> Source/WebCore/bindings/js/ScriptModuleFetcher.h:73
> +    RefPtr<DeferredWrapper> m_deferred;

We might want to call this m_promise instead.

> Source/WebCore/bindings/js/ScriptModuleGraph.h:49
> +    bool errorOccurred() const { return m_errored; }

I'd call this wasErrored for consistency.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:99
> +    if (importerModuleKey.isSymbol() || importerModuleKey.isUndefined()) {
> +        // When the module is root module, we resolve the specifier with the document.

It's better to define a descriptive local variable. e.g.
bool isRootModule = importerModuleKey.isSymbol() || importerModuleKey.isUndefined().

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:108
> +        // The base URL is a response URL while the module key is a request URL.

I don't know what this comment means.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:110
> +        URL importerModuleResponseURL = m_baseURLMap.get(importerModuleRequestURL);
> +        if (!importerModuleResponseURL.isValid()) {

It's strange that m_baseURLMap contains response URLs.
Having said that, I don't think we need a separate comment about this especially with the error message describing the situation right beneath it.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:133
> -    JSC::JSInternalPromiseDeferred* deferred = JSC::JSInternalPromiseDeferred::create(exec, globalObject);
> +    return JSC::jsCast<JSC::JSInternalPromise*>(resolvePromise(m_document, *exec, *JSC::jsCast<JSDOMGlobalObject*>(globalObject), moduleNameValue, importerModuleKey));
> +}

Why do we need to split into a separate function?

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:219
> +    RefPtr<Element> element = fetcher.element();
> +    RefPtr<DeferredWrapper> deferred = fetcher.deferred();

We can't use Ref<> here?

> Source/WebCore/dom/ScriptElement.cpp:309
> +bool ScriptElement::requestClassicScript(const String& sourceURL)

Can we keep this as the first function so that the diff looks saner?

> Source/WebCore/dom/ScriptElement.cpp:354
> +        if (is<LoadableClassicScript>(m_loadableScript)) {
> +            auto& cachedScript = downcast<LoadableClassicScript>(*m_loadableScript)->cachedScript();
> +            if (!cachedScript.mimeTypeAllowedByNosniff()) {

Can we just add mimeTypeAllowedByNosniff on LoadableScript so that we don't have to do this check?
We can just always return true for modules, right?

> Source/WebCore/dom/ScriptElement.cpp:534
> +bool ScriptElement::attemptToHandleCrossOriginScriptLoad(CachedScript* cachedScript)
> +{

I think this should move to LoadableScript with a name like shouldLoadCrossOriginScript,
and then just have the error spit out in ScriptElement::notifyFinished.
Comment 115 Daniel Bates 2016-09-06 17:00:40 PDT
Comment on attachment 288066 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288066&action=review

> Source/JavaScriptCore/runtime/PrivateName.h:51
> +    SymbolImpl& uid() const { return *m_uid.get(); }

I know that this class has an implicit invariant that it is passed a non-null SymbolImpl. What guarantees that this invariant is not violated and m_uid is not nullptr?

> Source/JavaScriptCore/runtime/PrivateName.h:57
> -    Ref<SymbolImpl> m_uid;
> +    RefPtr<SymbolImpl> m_uid;

This does not seem like the correct idiom to use to make PrivateName copyable. I suggest that we implement a custom copy-constructor to handle the copying of Ref<SymbolImpl>.
Comment 116 Yusuke Suzuki 2016-09-06 17:38:55 PDT
Comment on attachment 288066 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288066&action=review

Thanks!

>> Source/JavaScriptCore/runtime/PrivateName.h:51
>> +    SymbolImpl& uid() const { return *m_uid.get(); }
> 
> I know that this class has an implicit invariant that it is passed a non-null SymbolImpl. What guarantees that this invariant is not violated and m_uid is not nullptr?

We do not allow nullptr SymbolImpl in SymbolImpl users. We may have nullptr UniquedStringImpl* (parent class of SymbolImpl). But in that case, (the wrapper of this class).isSymbol() returns false if the UniquedStringImpl* is nullptr.
For example, Identifier::isSymbol() returns false for nullptr. And Identifier::fromUid(vm, nullptr).isSymbol() is also false.
Before performing downcast (like static_cast<SymbolImpl&>()), we ensure the invariant that SymbolImpl* never becomes nullptr.

>> Source/JavaScriptCore/runtime/PrivateName.h:57
>> +    RefPtr<SymbolImpl> m_uid;
> 
> This does not seem like the correct idiom to use to make PrivateName copyable. I suggest that we implement a custom copy-constructor to handle the copying of Ref<SymbolImpl>.

Looks nice. I'll create the separate patch based on this approach.
Comment 117 Yusuke Suzuki 2016-09-06 17:44:04 PDT
Comment on attachment 288066 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288066&action=review

>>> Source/JavaScriptCore/runtime/PrivateName.h:51
>>> +    SymbolImpl& uid() const { return *m_uid.get(); }
>> 
>> I know that this class has an implicit invariant that it is passed a non-null SymbolImpl. What guarantees that this invariant is not violated and m_uid is not nullptr?
> 
> We do not allow nullptr SymbolImpl in SymbolImpl users. We may have nullptr UniquedStringImpl* (parent class of SymbolImpl). But in that case, (the wrapper of this class).isSymbol() returns false if the UniquedStringImpl* is nullptr.
> For example, Identifier::isSymbol() returns false for nullptr. And Identifier::fromUid(vm, nullptr).isSymbol() is also false.
> Before performing downcast (like static_cast<SymbolImpl&>()), we ensure the invariant that SymbolImpl* never becomes nullptr.

For this class, we have 3 constructors.

1. PrivateName()
2. explicit PrivateName(DescriptionTag, const String& description)
3. explicit PrivateName(SymbolImpl& uid)

In the cases of 1 and 2, since StringImpl::createXXXSymbol() never returns nullptr (And its type is Ref<SymbolImpl>), this PrivateName does not become m_uid = nullptr.
In the case of 3, we ensure this invariant by taking SymbolImpl&.
Comment 118 Yusuke Suzuki 2016-09-08 22:44:23 PDT
Created attachment 288389 [details]
WIP

WIP
Comment 119 Yusuke Suzuki 2016-09-09 17:20:44 PDT
Comment on attachment 288051 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288051&action=review

Thanks!

>> Source/JavaScriptCore/ChangeLog:10
>> +        (JSC::PrivateName::uid):
> 
> Can we land this in a separate patch?

OK, I'll split it soon :)

>> Source/WebCore/ChangeLog:28
>> +            3. ScriptModuleFetcher
> 
> We might want to call this CachedModuleScriptLoader or something since this class doesn't really fetch the module file if it's already cached.

Looks better. Changed.

>> Source/WebCore/bindings/js/ScriptModuleFetcher.cpp:80
>> +    m_loaded = true;
> 
> Don't we need to clear m_deferred here?
> Also, if we've done that, do we still need a separate m_loaded boolean?

Make sense. Changed.

>> Source/WebCore/bindings/js/ScriptModuleFetcher.cpp:89
>> +        m_client->notifyFinished(*this);
> 
> Why don't we just call this with m_deferred here instead of keeping it around in the fetcher object and calling deferred() in ScriptModuleLoader::notifyFinished ?
> That way, we can even use WTFMove() and remove the ownership the callee.

Nice, fixed.

>> Source/WebCore/bindings/js/ScriptModuleFetcher.h:73
>> +    RefPtr<DeferredWrapper> m_deferred;
> 
> We might want to call this m_promise instead.

OK, fixed.

>> Source/WebCore/bindings/js/ScriptModuleGraph.h:49
>> +    bool errorOccurred() const { return m_errored; }
> 
> I'd call this wasErrored for consistency.

Fixed in LoadableScript patch.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:99
>> +        // When the module is root module, we resolve the specifier with the document.
> 
> It's better to define a descriptive local variable. e.g.
> bool isRootModule = importerModuleKey.isSymbol() || importerModuleKey.isUndefined().

Sounds nice. Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:108
>> +        // The base URL is a response URL while the module key is a request URL.
> 
> I don't know what this comment means.

The module key - which maintains identity of each module - should be request URL (URL used when requesting), since we need to detect duplicate module requests before actually sending requests over network.
On the other hand, when we resolve the given module name (`import A from "./A.js")'s "./A.js" part) with a parent module, this "./A.js" should be resolved with the response URL of the parent module.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:110
>> +        if (!importerModuleResponseURL.isValid()) {
> 
> It's strange that m_baseURLMap contains response URLs.
> Having said that, I don't think we need a separate comment about this especially with the error message describing the situation right beneath it.

OK, this comment is dropped. And rename this map more descriptive name, "m_requestURLToResponseURLMap".

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:133
>> +}
> 
> Why do we need to split into a separate function?

Oops, due to previous design of this function. Now, it is not necessary. Merged.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:219
>> +    RefPtr<DeferredWrapper> deferred = fetcher.deferred();
> 
> We can't use Ref<> here?

We now take RefPtr<DeferredWrapper> as this function's argument.

>> Source/WebCore/dom/ScriptElement.cpp:309
>> +bool ScriptElement::requestClassicScript(const String& sourceURL)
> 
> Can we keep this as the first function so that the diff looks saner?

Done in LoadableScript patch and it makes the diff nicer.

>> Source/WebCore/dom/ScriptElement.cpp:354
>> +            if (!cachedScript.mimeTypeAllowedByNosniff()) {
> 
> Can we just add mimeTypeAllowedByNosniff on LoadableScript so that we don't have to do this check?
> We can just always return true for modules, right?

Move this check to the LoadableClassicScript in LoadableScript patch, thanks!

>> Source/WebCore/dom/ScriptElement.cpp:534
>> +{
> 
> I think this should move to LoadableScript with a name like shouldLoadCrossOriginScript,
> and then just have the error spit out in ScriptElement::notifyFinished.

Nice, I've moved this check to the LoadableClassicScript. And now the error is propagated to the ScriptElement through wasErrored().
Comment 120 Yusuke Suzuki 2016-09-09 17:55:20 PDT
Created attachment 288462 [details]
Patch

Update!
Comment 121 Yusuke Suzuki 2016-10-14 13:17:43 PDT
Created attachment 291662 [details]
Patch

Just rebaselining...
Comment 122 Yusuke Suzuki 2016-10-22 00:51:31 PDT
Created attachment 292487 [details]
Patch

WIP
Comment 123 Build Bot 2016-10-22 18:04:47 PDT
Comment on attachment 292487 [details]
Patch

Attachment 292487 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2347408

New failing tests:
js/dom/module-not-found-error-event-with-src-and-import.html
js/dom/module-not-found-error-event-with-src.html
js/dom/module-will-fire-beforeload.html
http/tests/security/module-requires-javascript-mime-types.html
js/dom/module-not-found-error-event.html
http/tests/security/module-correct-mime-types.html
http/tests/security/module-no-mime-type.html
js/dom/module-load-same-module-from-different-entry-point.html
js/dom/module-load-event-with-src.html
js/dom/module-load-same-module-from-different-entry-point-dynamic.html
http/tests/security/module-incorrect-mime-types.html
http/tests/misc/module-script-async.html
js/dom/module-type-case-insensitive.html
js/dom/module-src-current-script.html
js/dom/module-inline-simple.html
js/dom/module-execution-order-mixed.html
js/dom/module-src-dynamic.html
js/dom/module-src-simple.html
js/dom/module-execution-error-inside-dependent-module-should-be-propagated-to-onerror.html
js/dom/module-inline-dynamic.html
Comment 124 Build Bot 2016-10-22 18:04:55 PDT
Created attachment 292524 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 125 Yusuke Suzuki 2016-10-28 00:04:19 PDT
Created attachment 293120 [details]
Patch
Comment 126 Yusuke Suzuki 2016-10-31 23:06:35 PDT
Created attachment 293531 [details]
Patch
Comment 127 Yusuke Suzuki 2016-11-01 15:33:18 PDT
Created attachment 293606 [details]
Patch
Comment 128 Yusuke Suzuki 2016-11-09 01:58:11 PST
Created attachment 294227 [details]
Patch

WIP
Comment 129 Yusuke Suzuki 2016-11-10 23:54:41 PST
Created attachment 294478 [details]
Patch

WIP
Comment 130 Yusuke Suzuki 2016-11-11 00:20:02 PST
Created attachment 294479 [details]
Patch
Comment 131 Build Bot 2016-11-11 01:43:47 PST
Comment on attachment 294479 [details]
Patch

Attachment 294479 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2496757

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-redirect-same-origin.html
Comment 132 Build Bot 2016-11-11 01:43:56 PST
Created attachment 294483 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 133 Yusuke Suzuki 2016-11-11 02:08:44 PST
Created attachment 294488 [details]
Patch
Comment 134 Ryosuke Niwa 2016-11-11 16:06:46 PST
Comment on attachment 294488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294488&action=review

Here's the first round o review comments.  I need a few more rounds of read through.

> Source/WebCore/ChangeLog:18
> +            1. ScriptModuleGraph
> +
> +                ScriptModuleGraph wraps the promise based JSModuleLoader pipeline and offers
> +                similar APIs to CachedScript. ScriptElement and PendingScript interact with
> +                ScriptModuleGraph when the script tag is the module tag instead of CachedScript.
> +                ScriptElement and PendingScript will receive the notification from
> +                ScriptModuleGraph by implementing ScriptModuleGraphClient.

I think it's very misleading to call this object a "graph".  A graph is a collection of nodes.  Here, what we have is a node in a graph.
How about CachedModuleScript or ModuleScriptVertex?

> Source/WebCore/bindings/js/CachedModuleScriptLoader.cpp:67
> +    // Note: If the content is already cached, this immeidately calls notifyFinished.

We don't normally prefix a comment with "Node:".

> Source/WebCore/bindings/js/CachedModuleScriptLoader.cpp:75
> +    if (!m_promise)
> +        return;

How could notifyFinished be called multiple times per CachedResourceClient!?

> Source/WebCore/bindings/js/CachedModuleScriptLoader.cpp:87
> +    // A CachedResourceHandle alone does not prevent the underlying CachedResource
> +    // from purging its data buffer. Until we finish using the backing buffer of the
> +    // cached script, we don't remove the client.

I don't understand what this comment is saying.
Are we trying to retain the cache? Or are we trying to clear the cache?

> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:63
> +    // For CachedResourceClient.

I don't think we need this comment given we're only inheriting from RefCounted and CachedResourceClient

> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:64
> +    void notifyFinished(CachedResource&) override;

Use final instead.

> Source/WebCore/bindings/js/ScriptController.cpp:371
> +    // NOTE: It is not guaranteed that either fulfillHandler or rejectHandler is eventually called.
> +    // For example, if the page load is canceled, the DeferredPromise used in the module loader pipeline will stop executing JS code.
> +    // Thus the promise returned from this function could remain unresolved.

Wouldn't that result in the leak of ScriptModuleGraph?

> Source/WebCore/bindings/js/ScriptController.cpp:373
> +    RefPtr<ScriptModuleGraph> protector(&moduleGraph);

Why don't we call this moduleGraph and the argument moduleGraphRef.
Alternatively, you can just modify the argument to be Ref<ScriptModuleGraph>.
Regardless, we should be using Ref here, not RefPtr.

> Source/WebCore/bindings/js/ScriptController.cpp:400
> +        else if (errorStatus == ErrorStatus::AlreadyReported) {
> +            protector->notifyErrored(LoadableScript::Error {
> +                LoadableScript::ErrorType::CachedScript,
> +                Nullopt
> +            });
> +        } else {

Why don't we just put the logic at where we check symbol->privateName() == moduleLoaderAlreadyReportedErrorSymbol
and then exit early there instead?

That is,
if (Symbol* symbol = ~) {
    if (ymbol->privateName() == moduleLoaderAlreadyReportedErrorSymbol) {
        ...
        return JSValue::encode(jsUndefined());
    }
    if (symbol->privateName() == moduleLoaderFetchingIsCanceledSymbol) {
        protector->notifyCanceled();
        return JSValue::encode(jsUndefined());
    }
}

If we did that, we don't even need the enum.

> Source/WebCore/bindings/js/ScriptModuleGraph.cpp:56
> +    Ref<Document> document(initiator.document());

This is yet another protector object?  Might be better call it documentProtector.

> Source/WebCore/bindings/js/ScriptModuleGraph.cpp:63
> +    Ref<Document> document(initiator.document());

Ditto.

> Source/WebCore/bindings/js/ScriptModuleGraph.cpp:91
> +    RefPtr<ScriptModuleGraph> protectedThis(this);

nit: protector as the variable name.

> Source/WebCore/bindings/js/ScriptModuleGraph.cpp:95
> +    Vector<ScriptModuleGraphClient*> vector;
> +    for (auto& pair : m_clients)
> +        vector.append(pair.key);

Can't we just do copyToVector(m_clients, clients) instead?
btw, we should call this variable clients instead of "vector".

> Source/WebCore/bindings/js/ScriptModuleGraph.cpp:103
> +    if (isLoaded()) {

We normally prefer an early exit.

> Source/WebCore/bindings/js/ScriptModuleGraph.cpp:104
> +        Ref<ScriptModuleGraph> protectedThis(*this);

Ditto about calling this protector.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:76
> +        promise->reject(TypeError, "Module specifier is not Symbol or String.");

Should we wrap these strings in ASCIILiteral?  You're doing it in some places but not everywhere.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:221
> +    else if (!MIMETypeRegistry::isSupportedJavaScriptMIMEType(cachedScript->response().mimeType())) {
> +        // https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-single-module-script

Does the list of mime types in isSupportedJavaScriptMIMEType match the ones in the spec?

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:247
> +            // The module loader pipeline is constructed as a chain of promises.
> +            // Rejecting a promise when some error occurs is important because the execution flow of
> +            // the promise chain is driven by "rejected" or "fulfilled" events.
> +            // If the promise is not rejected while error occurs, reject handler won't be executed
> +            // and all the subsequent promise chain will wait the result forever.
> +            //
> +            // As a result, even if the error is already reported to the inspector elsewhere,
> +            // it should be propagated in the pipeline. For example, the error of loading
> +            // CachedResource is already reported to the inspector by the loader. But we still need
> +            // to reject the promise to propagate this error to the script element.
> +            //
> +            // At that time, we don't want to report the same error twice. When we propagate the error
> +            // that is already reported to the inspector, we throw moduleLoaderAlreadyReportedErrorSymbol
> +            // symbol instead. By comparing the thrown error with this symbol, we can distinguish errors raised
> +            // when checking syntax of a module script from errors reported already.
> +            // In the reject handler of the promise, we only report a error that is not this symbol.

You might want to put this long version in the change log.  In the code, it's probably sufficient to say that
"Reject a promise to propagate the error back all the way through module promise chains to the script element"

> Source/WebCore/dom/Document.cpp:4792
> -    ASSERT(newCurrentScript);
> +    // HTMLScriptElement may be nullptr.

Since we also need to return nullptr when we're running the script inside a shadow tree,
we might want to do this check in Document::currentScript instead and keep this function unchanged.

> Source/WebCore/dom/LoadableScriptModuleGraph.h:35
> +class LoadableScriptModuleGraph final : public LoadableScript, private ScriptModuleGraphClient {

Again, this class name is misleading. This isn't really a graph. It's a node/vertex in a graph.

> Source/WebCore/dom/LoadableScriptModuleGraph.h:42
> +    Optional<Error> wasErrored() const override;

We might want to call this function error() if it returns Optional<Error> since wasError is a question.

> Source/WebCore/dom/ScriptElement.cpp:165
> -#if ENABLE(ES6_MODULES)
> +// #if ENABLE(ES6_MODULES)

What's up with this?

> Source/WebCore/dom/ScriptElement.cpp:226
> +    // We intentionally use the charset attribute even if the script is the module script.

Why is that?  We prefer why comments over what comments.

> Source/WebCore/dom/ScriptElement.cpp:243
> +    if ((scriptType == ScriptType::Classic && hasSourceAttribute() && deferAttributeValue() && m_parserInserted && !asyncAttributeValue()) || (scriptType == ScriptType::Module && m_parserInserted && !asyncAttributeValue())) {

This predicate is getting really long.  Can we put it into a helper function with a descriptive name?
Alternatively, since this is the first predicate we check, we can just declare a local boolean variable with a descriptive name.

> Source/WebCore/dom/ScriptElement.cpp:310
> +        bool didEventListenerDisconnectThisElement = !m_element.inDocument() || &m_element.document() != originalDocument.ptr();
> +        if (didEventListenerDisconnectThisElement)

Are these checks spec'ed somewhere?  Can we add a URL to reference that?
The ordering here is quite important as it's observable from scripts.

> Source/WebCore/dom/ScriptElement.cpp:328
> +    } else {

Why not early return here?

> Source/WebCore/dom/ScriptElement.cpp:412
> +    if (Frame* frame = document->frame()) {

Why not an early return instead?

> Source/WebCore/dom/ScriptElement.cpp:414
> +        // If the script is from an external file, or the script's type is "module",
> +        // then increment the ignore-destructive-writes counter of the script element's node document. Let neutralized doc be that Document.

I don't think we need this comment since the class name is pretty descriptive and there are a only few steps.

> Source/WebCore/dom/ScriptElement.cpp:417
> +        // Set the script element's node document's currentScript attribute to null.

Ditto.

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:150
> +        // We intentionally use the character attribute even if the given script tag loads the module script.

Ditto about explaining why instead of stating what.

> Source/WebCore/html/parser/HTMLResourcePreloader.h:39
> +    enum class ModuleScriptMode {
> +        Yes,
> +        No,
> +    };

I think it would be nicer to say ScriptType: Module/Classic.
Comment 135 Yusuke Suzuki 2016-11-12 00:18:41 PST
Comment on attachment 294488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294488&action=review

Thanks!

>> Source/WebCore/ChangeLog:18
>> +                ScriptModuleGraph by implementing ScriptModuleGraphClient.
> 
> I think it's very misleading to call this object a "graph".  A graph is a collection of nodes.  Here, what we have is a node in a graph.
> How about CachedModuleScript or ModuleScriptVertex?

Yup, that is named after the internal implementation (dependency graph of the scripts).
Actually, it manages the graph of CachedScripts(node) with dependency edges.
But it should not be exposed to WebCore side.
CachedModuleScript seems fine. It is consistent with CachedModuleScriptLoader. Changed.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.cpp:67
>> +    // Note: If the content is already cached, this immeidately calls notifyFinished.
> 
> We don't normally prefix a comment with "Node:".

Dropped.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.cpp:75
>> +        return;
> 
> How could notifyFinished be called multiple times per CachedResourceClient!?

Yeah, ok, it should not be called. This guard is meaningless. Dropped.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.cpp:87
>> +    // cached script, we don't remove the client.
> 
> I don't understand what this comment is saying.
> Are we trying to retain the cache? Or are we trying to clear the cache?

We are trying to retain the cache until we call m_client->notifyFinished.
To do so, removeClient(this) should not be called before calling `m_client->notifyFinished`.
Changed the comment.

    // A CachedResourceHandle alone does not prevent the underlying CachedResource
    // from purging its data buffer. To keep the underlying CachedResource during
    // processing, we remove the client after calling client's notifyFinished.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:63
>> +    // For CachedResourceClient.
> 
> I don't think we need this comment given we're only inheriting from RefCounted and CachedResourceClient

Dropped.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:64
>> +    void notifyFinished(CachedResource&) override;
> 
> Use final instead.

Nice, changed.

>> Source/WebCore/bindings/js/ScriptController.cpp:371
>> +    // Thus the promise returned from this function could remain unresolved.
> 
> Wouldn't that result in the leak of ScriptModuleGraph?

When the module cache is cleared (page is refreshed), they should be released.
Since module instances are cached (the same module key returns the same module instance), resource combined with the module may be retained while this page is live.
But once the page & context is destroyed, they should be collected by GC. (& GC will call destroy function, and it will destroy ScriptModuleGraph).

>> Source/WebCore/bindings/js/ScriptController.cpp:373
>> +    RefPtr<ScriptModuleGraph> protector(&moduleGraph);
> 
> Why don't we call this moduleGraph and the argument moduleGraphRef.
> Alternatively, you can just modify the argument to be Ref<ScriptModuleGraph>.
> Regardless, we should be using Ref here, not RefPtr.

It's because 2 functions reference this protector: fulfillHandler and rejectHandler.
But I think we can alleviate this situation by using C++14 generalized capture.
I'll change the argument to Ref<CachedModuleScript>, and use generalized capture like `[moduleScript = protector.copyRef()]`.

>> Source/WebCore/bindings/js/ScriptController.cpp:400
>> +        } else {
> 
> Why don't we just put the logic at where we check symbol->privateName() == moduleLoaderAlreadyReportedErrorSymbol
> and then exit early there instead?
> 
> That is,
> if (Symbol* symbol = ~) {
>     if (ymbol->privateName() == moduleLoaderAlreadyReportedErrorSymbol) {
>         ...
>         return JSValue::encode(jsUndefined());
>     }
>     if (symbol->privateName() == moduleLoaderFetchingIsCanceledSymbol) {
>         protector->notifyCanceled();
>         return JSValue::encode(jsUndefined());
>     }
> }
> 
> If we did that, we don't even need the enum.

It should be fine. Changed.

>> Source/WebCore/bindings/js/ScriptModuleGraph.cpp:56
>> +    Ref<Document> document(initiator.document());
> 
> This is yet another protector object?  Might be better call it documentProtector.

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleGraph.cpp:63
>> +    Ref<Document> document(initiator.document());
> 
> Ditto.

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleGraph.cpp:95
>> +        vector.append(pair.key);
> 
> Can't we just do copyToVector(m_clients, clients) instead?
> btw, we should call this variable clients instead of "vector".

Nice, copyToVector function works. Fixed.

>> Source/WebCore/bindings/js/ScriptModuleGraph.cpp:103
>> +    if (isLoaded()) {
> 
> We normally prefer an early exit.

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleGraph.cpp:104
>> +        Ref<ScriptModuleGraph> protectedThis(*this);
> 
> Ditto about calling this protector.

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:76
>> +        promise->reject(TypeError, "Module specifier is not Symbol or String.");
> 
> Should we wrap these strings in ASCIILiteral?  You're doing it in some places but not everywhere.

Oops, right. We should do that. Changed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:221
>> +        // https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-single-module-script
> 
> Does the list of mime types in isSupportedJavaScriptMIMEType match the ones in the spec?

Yeah, completely the same. :)

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:247
>> +            // In the reject handler of the promise, we only report a error that is not this symbol.
> 
> You might want to put this long version in the change log.  In the code, it's probably sufficient to say that
> "Reject a promise to propagate the error back all the way through module promise chains to the script element"

OK, I've moved the above comment to the ChangeLog. And use this short version.

>> Source/WebCore/dom/Document.cpp:4792
>> +    // HTMLScriptElement may be nullptr.
> 
> Since we also need to return nullptr when we're running the script inside a shadow tree,
> we might want to do this check in Document::currentScript instead and keep this function unchanged.

You mean that we should check ScriptType in Document::currentScript() side, correct?

>> Source/WebCore/dom/LoadableScriptModuleGraph.h:35
>> +class LoadableScriptModuleGraph final : public LoadableScript, private ScriptModuleGraphClient {
> 
> Again, this class name is misleading. This isn't really a graph. It's a node/vertex in a graph.

Changed it to LoadableModuleScript.

>> Source/WebCore/dom/LoadableScriptModuleGraph.h:42
>> +    Optional<Error> wasErrored() const override;
> 
> We might want to call this function error() if it returns Optional<Error> since wasError is a question.

OK, changed. We also need to change wasErrored() functions in LoadableClassicScript, PendingScript etc. Done.

>> Source/WebCore/dom/ScriptElement.cpp:165
>> +// #if ENABLE(ES6_MODULES)
> 
> What's up with this?

This is just commented out to check this patch on EWS.
When landing it, we should revert this change. And we should remove # of TestExpectations.

>> Source/WebCore/dom/ScriptElement.cpp:226
>> +    // We intentionally use the charset attribute even if the script is the module script.
> 
> Why is that?  We prefer why comments over what comments.

Add comment that is noted in ChangeLog.

According to the spec, the module tag ignores the "charset" attribute as the same to the worker's
importScript. But WebKit supports the "charset" for importScript intentionally. So to be consistent,
even for the module tags, we handle the "charset" attribute.

>> Source/WebCore/dom/ScriptElement.cpp:243
>> +    if ((scriptType == ScriptType::Classic && hasSourceAttribute() && deferAttributeValue() && m_parserInserted && !asyncAttributeValue()) || (scriptType == ScriptType::Module && m_parserInserted && !asyncAttributeValue())) {
> 
> This predicate is getting really long.  Can we put it into a helper function with a descriptive name?
> Alternatively, since this is the first predicate we check, we can just declare a local boolean variable with a descriptive name.

Changed. BTW, the above condition is strictly aligned to the spec.
https://html.spec.whatwg.org/multipage/scripting.html#script-processing-model
Named it as isParserInsertedDeferredScript.

>> Source/WebCore/dom/ScriptElement.cpp:310
>> +        if (didEventListenerDisconnectThisElement)
> 
> Are these checks spec'ed somewhere?  Can we add a URL to reference that?
> The ordering here is quite important as it's observable from scripts.

No. The before load event for the script tag is not specified in whatwg HTML5 spec. This is the extension in WebKit.
It is originally implemented for the classic script. And the module script is just using the completely same semantics.

BTW, it seems that blink dropped before load event support.

>> Source/WebCore/dom/ScriptElement.cpp:328
>> +    } else {
> 
> Why not early return here?

Fixed.

>> Source/WebCore/dom/ScriptElement.cpp:412
>> +    if (Frame* frame = document->frame()) {
> 
> Why not an early return instead?

Fixed.

>> Source/WebCore/dom/ScriptElement.cpp:414
>> +        // then increment the ignore-destructive-writes counter of the script element's node document. Let neutralized doc be that Document.
> 
> I don't think we need this comment since the class name is pretty descriptive and there are a only few steps.

OK, dropped.

>> Source/WebCore/dom/ScriptElement.cpp:417
>> +        // Set the script element's node document's currentScript attribute to null.
> 
> Ditto.

Dropped.

>> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:150
>> +        // We intentionally use the character attribute even if the given script tag loads the module script.
> 
> Ditto about explaining why instead of stating what.

Add the same comment.

According to the spec, the module tag ignores the "charset" attribute as the same to the worker's
importScript. But WebKit supports the "charset" for importScript intentionally. So to be consistent,
even for the module tags, we handle the "charset" attribute.

>> Source/WebCore/html/parser/HTMLResourcePreloader.h:39
>> +    };
> 
> I think it would be nicer to say ScriptType: Module/Classic.

Changed.
Comment 136 Yusuke Suzuki 2016-11-12 00:21:18 PST
Comment on attachment 294488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294488&action=review

>> Source/WebCore/bindings/js/ScriptModuleGraph.cpp:91
>> +    RefPtr<ScriptModuleGraph> protectedThis(this);
> 
> nit: protector as the variable name.

Oh, but according to the webkit-check-style script, in the above case, we should name it as `protectedThis`.

>>> Source/WebCore/bindings/js/ScriptModuleGraph.cpp:104
>>> +        Ref<ScriptModuleGraph> protectedThis(*this);
>> 
>> Ditto about calling this protector.
> 
> Fixed.

But according to the webkit-check-style script, in the above case, we should name it as `protectedThis`.
Comment 137 Yusuke Suzuki 2016-11-12 00:38:07 PST
Created attachment 294609 [details]
Patch
Comment 138 Yusuke Suzuki 2016-11-12 01:09:03 PST
Comment on attachment 294488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294488&action=review

>>> Source/WebCore/bindings/js/ScriptController.cpp:373
>>> +    RefPtr<ScriptModuleGraph> protector(&moduleGraph);
>> 
>> Why don't we call this moduleGraph and the argument moduleGraphRef.
>> Alternatively, you can just modify the argument to be Ref<ScriptModuleGraph>.
>> Regardless, we should be using Ref here, not RefPtr.
> 
> It's because 2 functions reference this protector: fulfillHandler and rejectHandler.
> But I think we can alleviate this situation by using C++14 generalized capture.
> I'll change the argument to Ref<CachedModuleScript>, and use generalized capture like `[moduleScript = protector.copyRef()]`.

Hm, the above one does not work. It causes compile error. It seems there are no good way to share Ref<> in 2 lambdas. Ref<> is not copyable.
Even if you write like, [moduleScript = protector.copyRef()]` it causes compile error since it causes copy.
And I also ensured that we cannot compile like `[moduleScript = WTFMove(protector.copyRef())]`.
Reverted it to RefPtr<>.
Comment 139 Yusuke Suzuki 2016-11-12 01:12:24 PST
Created attachment 294610 [details]
Patch
Comment 140 Yusuke Suzuki 2016-11-12 01:38:20 PST
Created attachment 294611 [details]
Patch
Comment 141 Yusuke Suzuki 2016-11-12 01:39:06 PST
OK, the review comments are addressed. Ready for second review.
Comment 142 Ryosuke Niwa 2016-11-12 16:20:19 PST
Comment on attachment 294488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294488&action=review

> Source/WebCore/dom/CurrentScriptIncrementer.h:44
>              m_document.pushCurrentScript(&downcast<HTMLScriptElement>(element));

Actually, on my second thought, what we should do is to update the condition the line above here
which already checks whether element in a shadow tree or not to check if it's classic script not.
There's already a bug here that we should be setting currentScript to nullptr
when the element in a shadow tree so let me fix that one first.
Comment 143 Ryosuke Niwa 2016-11-12 16:58:27 PST
(In reply to comment #142)
> Comment on attachment 294488 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294488&action=review
> 
> > Source/WebCore/dom/CurrentScriptIncrementer.h:44
> >              m_document.pushCurrentScript(&downcast<HTMLScriptElement>(element));
> 
> Actually, on my second thought, what we should do is to update the condition
> the line above here
> which already checks whether element in a shadow tree or not to check if
> it's classic script not.
> There's already a bug here that we should be setting currentScript to nullptr
> when the element in a shadow tree so let me fix that one first.

Uploaded a patch to fix this on https://bugs.webkit.org/show_bug.cgi?id=164693.
Comment 144 Ryosuke Niwa 2016-11-13 01:03:04 PST
It looks like this patch failed on all EWSes?
Comment 145 Yusuke Suzuki 2016-11-13 01:04:57 PST
(In reply to comment #144)
> It looks like this patch failed on all EWSes?

Oops, it's related to super simple compile error.
Soon, (maybe, in 10mins), I'll upload the patch with your current script change.
Comment 146 Yusuke Suzuki 2016-11-13 01:17:47 PST
Created attachment 294662 [details]
Patch
Comment 147 WebKit Commit Bot 2016-11-13 01:21:20 PST
Attachment 294662 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScriptController.cpp:376:  'moduleScript' is incorrectly named. It should be named 'protector' or 'protectedModuleScriptRef'.  [readability/naming/protected] [4]
Total errors found: 1 in 159 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 148 Darin Adler 2016-11-14 09:57:49 PST
Comment on attachment 294662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294662&action=review

Started reviewing, but only got half way through the patch before I had to leave.

> Source/WebCore/bindings/js/CachedModuleScript.cpp:56
> +    Ref<Document> documentProtector(initiator.document());

I think protectedDocument would be a slightly better name. I am a bit confused about why protecting the document if valuable, though. The only thing we do with the document is call Frame on it. Why protection here? Should this protection be done at a different level?

> Source/WebCore/bindings/js/CachedModuleScript.cpp:58
> +    if (Frame* frame = documentProtector->frame())
> +        frame->script().loadModuleScript(*this, rootURL.string(), initiator);

Is silently doing nothing the right thing to do in a document without a frame?

> Source/WebCore/bindings/js/CachedModuleScript.cpp:65
> +    Ref<Document> documentProtector(initiator.document());
> +    if (Frame* frame = documentProtector->frame())
> +        frame->script().loadModuleScript(*this, sourceCode, initiator);

Ditto.

> Source/WebCore/bindings/js/CachedModuleScript.cpp:77
> +    if (!m_error)
> +        m_error = WTFMove(error);

When can we get notified about multiple errors?

> Source/WebCore/bindings/js/CachedModuleScript.cpp:91
> +    RefPtr<CachedModuleScript> protectedThis(this);

Should be Ref, not RefPtr. What is the rationale for protection here?

> Source/WebCore/bindings/js/CachedModuleScript.cpp:101
> +    m_clients.add(&client);

Assert m_clients does not contain client?

> Source/WebCore/bindings/js/CachedModuleScript.cpp:105
> +    Ref<CachedModuleScript> protectedThis(*this);
> +    client.notifyFinished(*this);

What is the rationale for protection here?

> Source/WebCore/bindings/js/CachedModuleScript.cpp:110
> +    m_clients.remove(&client);

Assert m_clients contains client?

> Source/WebCore/bindings/js/CachedModuleScript.h:31
> +#include <wtf/Noncopyable.h>

No need for this include?

> Source/WebCore/bindings/js/CachedModuleScript.h:32
> +#include <wtf/WeakPtr.h>

No need for this include?

> Source/WebCore/bindings/js/CachedModuleScript.h:38
> +class Element;
> +class ScriptSourceCode;
> +class CachedModuleScriptClient;

Please sort in alphabetical order.

> Source/WebCore/bindings/js/CachedModuleScript.h:46
> +    void notifyLoaded(UniquedStringImpl* moduleKey);

Can this be null? If not, take a reference?

> Source/WebCore/bindings/js/CachedModuleScript.h:47
> +    void notifyErrored(LoadableScript::Error&&);

I have never been a big fan of the "notifyXXX" naming scheme, but this one in particular is not grammatical. Maybe notifyErrorOccured or notifyLoadFailed.

If we are going to stick with the notify prefix, then I suggest: notifyLoadCompleted, notifyLoadFailed, notifyLoadWasCanceled.

> Source/WebCore/bindings/js/CachedModuleScript.h:50
> +    Optional<LoadableScript::Error> error() const { return m_error; }

const Optional& return type?

> Source/WebCore/bindings/js/CachedModuleScript.h:76
> +    bool m_canceled { false };
> +    bool m_loaded { false };

m_wasCanceled?
m_isLoaded?

> Source/WebCore/bindings/js/CachedModuleScriptLoader.cpp:67
> +    // If the content is already cached, this immeidately calls notifyFinished.

Typo: "immediately" is spelled wrong.

> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:30
> +#include "JSDOMPromise.h"

I don’t think this include is needed. Forward declaration of DeferredPromise should suffice.

> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:31
> +#include "ScriptElement.h"

I don’t think this include is needed. There is a forward declaration below.

> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:32
> +#include "URL.h"

This include is not needed; forward declaration of URL should suffice.

> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:33
> +#include <runtime/JSInternalPromiseDeferred.h>

I don’t think this include is needed. Forward declaration of DeferredPromise should suffice.

> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:34
> +#include <wtf/Noncopyable.h>

This include is not needed.

> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:39
> +class Document;

Not used.

> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:40
> +class JSDOMGlobalObject;

Not used.

> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:41
> +class ScriptElement;

Only needed if we don’t remove ScriptElement.h above.

> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:42
> +class CachedModuleScriptLoaderClient;

Please sort alphabetically.

> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:48
> +    ~CachedModuleScriptLoader();

In our coding style we include virtual here.

> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:67
> +    CachedResourceHandle<CachedScript> m_cachedScript { };

Is the { } needed? I don’t think that does anything different from the default, but I may be mistaken.

> Source/WebCore/bindings/js/JSDOMBinding.cpp:159
> +String retrieveErrorMessage(ExecState* exec, VM& vm, JSValue exceptionValue, CatchScope& catchScope)

Please use ExecState& state in new functions, unless there is a good reason not to.

> Source/WebCore/bindings/js/JSDOMBinding.cpp:162
> +    if (ExceptionBase* exceptionBase = toExceptionBase(exceptionValue))

auto?

> Source/WebCore/bindings/js/JSDOMBinding.cpp:163
> +        errorMessage = exceptionBase->toString();

I suggest a return here, and use of early return instead of if/else with a local variable.

> Source/WebCore/bindings/js/JSDOMBinding.cpp:167
> +        if (ErrorInstance* error = jsDynamicDowncast<ErrorInstance*>(exceptionValue))

auto?

> Source/WebCore/bindings/js/JSDOMBinding.cpp:170
> +            errorMessage = exceptionValue.toString(exec)->value(exec);

toWTFString?

> Source/WebCore/bindings/js/JSDOMBinding.h:185
> +String retrieveErrorMessage(JSC::ExecState*, JSC::VM&, JSC::JSValue exceptionValue, JSC::CatchScope&);

Not sure we need the word "value" in "exceptionValue".

> Source/WebCore/bindings/js/ScriptController.cpp:94
> +    , m_moduleLoaderAlreadyReportedErrorSymbol()
> +    , m_moduleLoaderFetchingIsCanceledSymbol()

Why is this needed?

> Source/WebCore/bindings/js/ScriptController.cpp:193
> +    JSDOMWindowShell* shell = windowShell(world);

Please use a reference in new code unless there is a reason not to.

    auto& shell = *windowShell(world);

> Source/WebCore/bindings/js/ScriptController.cpp:194
> +    ExecState* exec = shell->window()->globalExec();

Please use ExecState& state in new code unless there is a reason not to.

> Source/WebCore/bindings/js/ScriptController.cpp:208
> +    JSDOMWindowShell* shell = windowShell(world);

Please use a reference in new code unless there is a reason not to.

> Source/WebCore/bindings/js/ScriptController.cpp:209
> +    ExecState* exec = shell->window()->globalExec();

Please use ExecState& state in new code unless there is a reason not to.

> Source/WebCore/bindings/js/ScriptController.cpp:224
> +    JSDOMWindowShell* shell = windowShell(world);
> +    ExecState* exec = shell->window()->globalExec();

Ditto.

> Source/WebCore/bindings/js/ScriptController.cpp:226
> +    Ref<Frame> protector(m_frame);

Unclear why a protector is helpful here. I am really concerned about adding protectors when it’s not clear exactly what they are needed for.

> Source/WebCore/bindings/js/ScriptController.cpp:231
> +        // FIXME: Give a chance to dump the stack trace if the "corssorigin" attribute allows.

Typo: "corssorigin".

> Source/WebCore/bindings/js/ScriptController.cpp:247
> +    const SourceCode& jsSourceCode = moduleRecord->sourceCode();

auto&

> Source/WebCore/bindings/js/ScriptController.cpp:250
> +    JSDOMWindowShell* shell = windowShell(world);
> +    ExecState* exec = shell->window()->globalExec();

auto&, references, state instead of exec

> Source/WebCore/bindings/js/ScriptController.cpp:251
> +    const String* savedSourceURL = m_sourceURL;

auto*; also, save/restore is not great, but I suppose we don’t have a good alternative

> Source/WebCore/bindings/js/ScriptController.cpp:254
> +    Ref<Frame> protector(m_frame);

rationale for protector

> Source/WebCore/bindings/js/ScriptController.cpp:256
> +    InspectorInstrumentationCookie cookie = InspectorInstrumentation::willEvaluateScript(m_frame, sourceURL, jsSourceCode.firstLine());

auto

> Source/WebCore/bindings/js/ScriptController.cpp:258
> +    JSValue returnValue = moduleRecord->evaluate(exec);

auto

> Source/WebCore/bindings/js/ScriptController.cpp:350
> +static Identifier jsValueToModuleKey(ExecState* exec, JSValue value)

ExecState& state please

> Source/WebCore/bindings/js/ScriptController.cpp:364
> +void ScriptController::setupModuleScriptHandlers(CachedModuleScript& moduleScriptRef, JSInternalPromise* promise, DOMWrapperWorld& world)

Can the promise argument be a reference?

> Source/WebCore/bindings/js/ScriptController.cpp:367
> +    JSDOMWindowShell* shell = windowShell(world);
> +    ExecState* exec = shell->window()->globalExec();

auto, state

> Source/WebCore/bindings/js/ScriptController.cpp:374
> +    PrivateName moduleLoaderAlreadyReportedErrorSymbol = m_moduleLoaderAlreadyReportedErrorSymbol;
> +    PrivateName moduleLoaderFetchingIsCanceledSymbol = m_moduleLoaderFetchingIsCanceledSymbol;

auto?

> Source/WebCore/bindings/js/ScriptController.cpp:378
> +    JSFunction* fulfillHandler = JSNativeStdFunction::create(exec->vm(), shell->window(), 1, String(), [moduleScript](ExecState* exec) {

auto

> Source/WebCore/bindings/js/ScriptController.cpp:384
> +    JSFunction* rejectHandler = JSNativeStdFunction::create(exec->vm(), shell->window(), 1, String(), [moduleScript, moduleLoaderAlreadyReportedErrorSymbol, moduleLoaderFetchingIsCanceledSymbol](ExecState* exec) {

auto

> Source/WebCore/bindings/js/ScriptController.cpp:386
> +        if (Symbol* symbol = jsDynamicCast<Symbol*>(error)) {

auto

> Source/WebCore/bindings/js/ScriptController.cpp:679
> +    Ref<Frame> protector(m_frame); // Script execution can destroy the frame, and thus the ScriptController.

Even this comment does not make it clear why the protector is a good idea. What‘s the harm in the ScriptController being destroyed? The harm comes in if code uses the script controller after destroyed, and the code below does not do that. So the protector belongs somewhere else I think. This is a real concern long term about our code structure.

> Source/WebCore/bindings/js/ScriptController.h:48
>  class JSGlobalObject;
> +class JSModuleRecord;
> +class JSInternalPromise;
>  class ExecState;

Alphabetical order please.

> Source/WebCore/bindings/js/ScriptController.h:64
>  class Frame;
>  class HTMLDocument;
>  class HTMLPlugInElement;
> +class CachedModuleScript;
>  class ScriptSourceCode;
>  class SecurityOrigin;
>  class Widget;

Alphabetical order please.

> Source/WebCore/bindings/js/ScriptController.h:129
> +    JSC::JSValue evaluateModule(const URL&, JSC::JSModuleRecord*, DOMWrapperWorld&);
> +    JSC::JSValue evaluateModule(const URL&, JSC::JSModuleRecord*);

References for JSModuleRecord&?

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:61
> +    JSDOMGlobalObject& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(jsGlobalObject);

auto&

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:63
> +    Ref<DeferredPromise> promise = DeferredPromise::create(globalObject, jsPromise);

auto

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:133
> +    JSDOMGlobalObject& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(jsGlobalObject);

auto&

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:135
> +    Ref<DeferredPromise> deferred = DeferredPromise::create(globalObject, jsPromise);

auto

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:159
> +        RefPtr<CachedModuleScriptLoader> loader = CachedModuleScriptLoader::create(*this, deferred.get());

auto

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:209
> +    CachedScript* cachedScript = loader.cachedScript();

auto*

> Source/WebCore/bindings/js/ScriptModuleLoader.h:66
> +    HashSet<RefPtr<CachedModuleScriptLoader>> m_loaders;

Ref instead of RefPtr?

> Source/WebCore/dom/LoadableModuleScript.h:37
> +    ~LoadableModuleScript();

WebKit coding style says virtual here

> Source/WebCore/dom/LoadableModuleScript.h:43
> +    bool isLoaded() const override;
> +    Optional<Error> error() const override;
> +    bool wasCanceled() const override;

final instead of override; can some of these be private too, if they are not called non-polymorphically?

> Source/WebCore/dom/LoadableModuleScript.h:48
> +    void execute(ScriptElement&) override;

Ditto.

> Source/WebCore/dom/ScriptElement.cpp:165
> -#if ENABLE(ES6_MODULES)
> +// #if ENABLE(ES6_MODULES)

???

> Source/WebCore/dom/ScriptElement.cpp:170
> -#endif
> +// #endif

???
Comment 149 Yusuke Suzuki 2016-11-14 13:27:50 PST
Comment on attachment 294662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294662&action=review

Quick reply. Still WIP. Soon, I'll reply to the all review comments. And update the patch.

>> Source/WebCore/bindings/js/CachedModuleScript.cpp:56
>> +    Ref<Document> documentProtector(initiator.document());
> 
> I think protectedDocument would be a slightly better name. I am a bit confused about why protecting the document if valuable, though. The only thing we do with the document is call Frame on it. Why protection here? Should this protection be done at a different level?

This behavior is aligned to the ScriptElement's executeScript function.
But this function is not changed from 2011.
From this time, so much code is changed.
And now, document() always returns Document&.

So I think this becomes meaningless. We can just use `initiator.document().frame()`.
Changed this code. And ScriptElement::executeScript is also changed.

>> Source/WebCore/bindings/js/CachedModuleScript.cpp:58
>> +        frame->script().loadModuleScript(*this, rootURL.string(), initiator);
> 
> Is silently doing nothing the right thing to do in a document without a frame?

This behavior is aligned to ScriptElement. ScriptElement gives up loading / executing when a document does not have a frame.

>> Source/WebCore/bindings/js/CachedModuleScript.cpp:65
>> +        frame->script().loadModuleScript(*this, sourceCode, initiator);
> 
> Ditto.

Ditto.

>> Source/WebCore/bindings/js/CachedModuleScript.cpp:77
>> +        m_error = WTFMove(error);
> 
> When can we get notified about multiple errors?

No. We just use `m_error = WTFMove(error)`.

>> Source/WebCore/bindings/js/CachedModuleScript.cpp:91
>> +    RefPtr<CachedModuleScript> protectedThis(this);
> 
> Should be Ref, not RefPtr. What is the rationale for protection here?

During `client->notifyFinished()`, CachedModuleScript's owner can drop this. So this is necessary.

>> Source/WebCore/bindings/js/CachedModuleScript.cpp:101
>> +    m_clients.add(&client);
> 
> Assert m_clients does not contain client?

We use HashCountedSet to allow clients to register themselves multiple times.

>> Source/WebCore/bindings/js/CachedModuleScript.cpp:105
>> +    client.notifyFinished(*this);
> 
> What is the rationale for protection here?

During `client.notifyFinished(*this)`, the owner can drop the ref to this `CachedModuleScript`.
Until `client.notifyFinished(*this)` is done, this protectedThis ensures that the given argument `CachedModuleScript&` is live.

>> Source/WebCore/bindings/js/CachedModuleScript.cpp:110
>> +    m_clients.remove(&client);
> 
> Assert m_clients contains client?

We use HashCountedSet to allow clients to register themselves multiple times.

>> Source/WebCore/bindings/js/CachedModuleScript.h:31
>> +#include <wtf/Noncopyable.h>
> 
> No need for this include?

Dropped.

>> Source/WebCore/bindings/js/CachedModuleScript.h:32
>> +#include <wtf/WeakPtr.h>
> 
> No need for this include?

Dropped.

>> Source/WebCore/bindings/js/CachedModuleScript.h:38
>> +class CachedModuleScriptClient;
> 
> Please sort in alphabetical order.

Done.

>> Source/WebCore/bindings/js/CachedModuleScript.h:46
>> +    void notifyLoaded(UniquedStringImpl* moduleKey);
> 
> Can this be null? If not, take a reference?

Fixed.

>> Source/WebCore/bindings/js/CachedModuleScript.h:47
>> +    void notifyErrored(LoadableScript::Error&&);
> 
> I have never been a big fan of the "notifyXXX" naming scheme, but this one in particular is not grammatical. Maybe notifyErrorOccured or notifyLoadFailed.
> 
> If we are going to stick with the notify prefix, then I suggest: notifyLoadCompleted, notifyLoadFailed, notifyLoadWasCanceled.

Your suggestion sounds super nice to me. Fixed.

>> Source/WebCore/bindings/js/CachedModuleScript.h:50
>> +    Optional<LoadableScript::Error> error() const { return m_error; }
> 
> const Optional& return type?

Yeah, done.

>> Source/WebCore/bindings/js/CachedModuleScript.h:76
>> +    bool m_loaded { false };
> 
> m_wasCanceled?
> m_isLoaded?

It sounds nice. Fixed.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.cpp:67
>> +    // If the content is already cached, this immeidately calls notifyFinished.
> 
> Typo: "immediately" is spelled wrong.

Oops, fixed.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:30
>> +#include "JSDOMPromise.h"
> 
> I don’t think this include is needed. Forward declaration of DeferredPromise should suffice.

Right. Dropped.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:31
>> +#include "ScriptElement.h"
> 
> I don’t think this include is needed. There is a forward declaration below.

Right. Fixed.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:32
>> +#include "URL.h"
> 
> This include is not needed; forward declaration of URL should suffice.

Right.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:33
>> +#include <runtime/JSInternalPromiseDeferred.h>
> 
> I don’t think this include is needed. Forward declaration of DeferredPromise should suffice.

Dropped.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:34
>> +#include <wtf/Noncopyable.h>
> 
> This include is not needed.

Yeah, dropped.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:39
>> +class Document;
> 
> Not used.

Dropped.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:40
>> +class JSDOMGlobalObject;
> 
> Not used.

Dropped.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:41
>> +class ScriptElement;
> 
> Only needed if we don’t remove ScriptElement.h above.

Yeah, leave this. And remove ScriptElement.h include.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:42
>> +class CachedModuleScriptLoaderClient;
> 
> Please sort alphabetically.

Done.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:48
>> +    ~CachedModuleScriptLoader();
> 
> In our coding style we include virtual here.

Oops, fixed.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:67
>> +    CachedResourceHandle<CachedScript> m_cachedScript { };
> 
> Is the { } needed? I don’t think that does anything different from the default, but I may be mistaken.

Not necessary. Thanks, dropped.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:159
>> +String retrieveErrorMessage(ExecState* exec, VM& vm, JSValue exceptionValue, CatchScope& catchScope)
> 
> Please use ExecState& state in new functions, unless there is a good reason not to.

Yeah, done.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:162
>> +    if (ExceptionBase* exceptionBase = toExceptionBase(exceptionValue))
> 
> auto?

Should be. Changed.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:163
>> +        errorMessage = exceptionBase->toString();
> 
> I suggest a return here, and use of early return instead of if/else with a local variable.

Nice! Yeah, we should do that. Fixed.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:167
>> +        if (ErrorInstance* error = jsDynamicDowncast<ErrorInstance*>(exceptionValue))
> 
> auto?

Fixed.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:170
>> +            errorMessage = exceptionValue.toString(exec)->value(exec);
> 
> toWTFString?

Yeah, it should be nice. Fixed.

>> Source/WebCore/bindings/js/JSDOMBinding.h:185
>> +String retrieveErrorMessage(JSC::ExecState*, JSC::VM&, JSC::JSValue exceptionValue, JSC::CatchScope&);
> 
> Not sure we need the word "value" in "exceptionValue".

Fixed.

>> Source/WebCore/bindings/js/ScriptController.cpp:94
>> +    , m_moduleLoaderFetchingIsCanceledSymbol()
> 
> Why is this needed?

This is noted in ChangeLog. Pasting.

 147        The module loader pipeline is constructed as a chain of promises.
 148        Rejecting a promise when some error occurs is important because the execution flow of
 149        the promise chain is driven by "rejected" or "fulfilled" events.
 150        If the promise is not rejected while error occurs, reject handler won't be executed
 151        and all the subsequent promise chain will wait the result forever.
 152        As a result, even if the error is already reported to the inspector elsewhere,
 153        it should be propagated in the pipeline. For example, the error of loading
 154        CachedResource is already reported to the inspector by the loader. But we still need
 155        to reject the promise to propagate this error to the script element.
 156        At that time, we don't want to report the same error twice. When we propagate the error
 157        that is already reported to the inspector, we throw moduleLoaderAlreadyReportedErrorSymbol
 158        symbol instead. By comparing the thrown error with this symbol, we can distinguish errors raised
 159        when checking syntax of a module script from errors reported already.
 160        In the reject handler of the promise, we only report a error that is not this symbol.

>> Source/WebCore/dom/ScriptElement.cpp:165
>> +// #if ENABLE(ES6_MODULES)
> 
> ???

To make EWS work, this patch temporary comments out this ifdef.
When landing this patch, this comment out should be reverted. And TestExpectations' [ Skip ] should be reverted too.

>> Source/WebCore/dom/ScriptElement.cpp:170
>> +// #endif
> 
> ???

Ditto.
Comment 150 Yusuke Suzuki 2016-11-14 15:46:57 PST
Comment on attachment 294662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294662&action=review

Thanks. OK. I've replied all the review comments. I'll upload the updated patch once I ensured that can be built.

>> Source/WebCore/bindings/js/ScriptController.cpp:193
>> +    JSDOMWindowShell* shell = windowShell(world);
> 
> Please use a reference in new code unless there is a reason not to.
> 
>     auto& shell = *windowShell(world);

Fixed.

>> Source/WebCore/bindings/js/ScriptController.cpp:194
>> +    ExecState* exec = shell->window()->globalExec();
> 
> Please use ExecState& state in new code unless there is a reason not to.

Fixed.

>> Source/WebCore/bindings/js/ScriptController.cpp:208
>> +    JSDOMWindowShell* shell = windowShell(world);
> 
> Please use a reference in new code unless there is a reason not to.

Fixed.

>> Source/WebCore/bindings/js/ScriptController.cpp:209
>> +    ExecState* exec = shell->window()->globalExec();
> 
> Please use ExecState& state in new code unless there is a reason not to.

Fixed.

>> Source/WebCore/bindings/js/ScriptController.cpp:224
>> +    ExecState* exec = shell->window()->globalExec();
> 
> Ditto.

Fixed.

>> Source/WebCore/bindings/js/ScriptController.cpp:226
>> +    Ref<Frame> protector(m_frame);
> 
> Unclear why a protector is helpful here. I am really concerned about adding protectors when it’s not clear exactly what they are needed for.

This is the same to ScriptController::executeScript. Script execution can destroy the frame.

>> Source/WebCore/bindings/js/ScriptController.cpp:231
>> +        // FIXME: Give a chance to dump the stack trace if the "corssorigin" attribute allows.
> 
> Typo: "corssorigin".

Oops, fixed. And add bugzilla URL here.

>> Source/WebCore/bindings/js/ScriptController.cpp:247
>> +    const SourceCode& jsSourceCode = moduleRecord->sourceCode();
> 
> auto&

Fixed.

>> Source/WebCore/bindings/js/ScriptController.cpp:250
>> +    ExecState* exec = shell->window()->globalExec();
> 
> auto&, references, state instead of exec

Fixed.

>> Source/WebCore/bindings/js/ScriptController.cpp:251
>> +    const String* savedSourceURL = m_sourceURL;
> 
> auto*; also, save/restore is not great, but I suppose we don’t have a good alternative

Currently, we change it `const auto* String` thing, and leave save / restore.
In JSC, we have some helper like SetForScope<>. I'll move this to WTF in the subsequent patch.

>> Source/WebCore/bindings/js/ScriptController.cpp:254
>> +    Ref<Frame> protector(m_frame);
> 
> rationale for protector

According to the ScriptController::executeScript, script evaluation can destroy the frame.
We would like to keep it until calling InspectorInstrumentation::didEvaluateScript.

>> Source/WebCore/bindings/js/ScriptController.cpp:256
>> +    InspectorInstrumentationCookie cookie = InspectorInstrumentation::willEvaluateScript(m_frame, sourceURL, jsSourceCode.firstLine());
> 
> auto

Fixed. BTW, these things are the same in non module evaluation code.
I'll upload the subsequent patch that will change like this after this patch.

>> Source/WebCore/bindings/js/ScriptController.cpp:258
>> +    JSValue returnValue = moduleRecord->evaluate(exec);
> 
> auto

Fixed.

>> Source/WebCore/bindings/js/ScriptController.cpp:364
>> +void ScriptController::setupModuleScriptHandlers(CachedModuleScript& moduleScriptRef, JSInternalPromise* promise, DOMWrapperWorld& world)
> 
> Can the promise argument be a reference?

Yes. This dereference should be done at JSC <-> WebCore border. So dereferencing this in JSMainThreadExecState.h. And passing the reference.

>> Source/WebCore/bindings/js/ScriptController.cpp:367
>> +    ExecState* exec = shell->window()->globalExec();
> 
> auto, state

Fixed.

>> Source/WebCore/bindings/js/ScriptController.cpp:374
>> +    PrivateName moduleLoaderFetchingIsCanceledSymbol = m_moduleLoaderFetchingIsCanceledSymbol;
> 
> auto?

No, we would like to copy this PrivateName into the lambda.

>> Source/WebCore/bindings/js/ScriptController.cpp:378
>> +    JSFunction* fulfillHandler = JSNativeStdFunction::create(exec->vm(), shell->window(), 1, String(), [moduleScript](ExecState* exec) {
> 
> auto

Fixed.

>> Source/WebCore/bindings/js/ScriptController.cpp:384
>> +    JSFunction* rejectHandler = JSNativeStdFunction::create(exec->vm(), shell->window(), 1, String(), [moduleScript, moduleLoaderAlreadyReportedErrorSymbol, moduleLoaderFetchingIsCanceledSymbol](ExecState* exec) {
> 
> auto

Fixed.

>> Source/WebCore/bindings/js/ScriptController.cpp:386
>> +        if (Symbol* symbol = jsDynamicCast<Symbol*>(error)) {
> 
> auto

Fixed.

>> Source/WebCore/bindings/js/ScriptController.cpp:679
>> +    Ref<Frame> protector(m_frame); // Script execution can destroy the frame, and thus the ScriptController.
> 
> Even this comment does not make it clear why the protector is a good idea. What‘s the harm in the ScriptController being destroyed? The harm comes in if code uses the script controller after destroyed, and the code below does not do that. So the protector belongs somewhere else I think. This is a real concern long term about our code structure.

Hm, right. So I think keeping protector for module script evaluation is fine in the meantime.
And in the other patch, we should drop these protectors and place appropriate protectors instead.

>> Source/WebCore/bindings/js/ScriptController.h:48
>>  class ExecState;
> 
> Alphabetical order please.

Fixed.

>> Source/WebCore/bindings/js/ScriptController.h:64
>>  class Widget;
> 
> Alphabetical order please.

Fixed.

>> Source/WebCore/bindings/js/ScriptController.h:129
>> +    JSC::JSValue evaluateModule(const URL&, JSC::JSModuleRecord*);
> 
> References for JSModuleRecord&?

We can do that thing by dereferencing it in WebCore ScriptModuleLoader.cpp. Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:61
>> +    JSDOMGlobalObject& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(jsGlobalObject);
> 
> auto&

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:63
>> +    Ref<DeferredPromise> promise = DeferredPromise::create(globalObject, jsPromise);
> 
> auto

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:133
>> +    JSDOMGlobalObject& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(jsGlobalObject);
> 
> auto&

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:135
>> +    Ref<DeferredPromise> deferred = DeferredPromise::create(globalObject, jsPromise);
> 
> auto

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:159
>> +        RefPtr<CachedModuleScriptLoader> loader = CachedModuleScriptLoader::create(*this, deferred.get());
> 
> auto

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:209
>> +    CachedScript* cachedScript = loader.cachedScript();
> 
> auto*

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.h:66
>> +    HashSet<RefPtr<CachedModuleScriptLoader>> m_loaders;
> 
> Ref instead of RefPtr?

Code becomes a bit weird (Ref<> cannot be copied, so in some place, we will use `copyRef` if we would like to continue using the original Ref<>). But we can.

Ref<> loader = ...;
m_loaders.add(loader.copyRef());
loader->...

Changed.

>> Source/WebCore/dom/LoadableModuleScript.h:37
>> +    ~LoadableModuleScript();
> 
> WebKit coding style says virtual here

Fixed.

>> Source/WebCore/dom/LoadableModuleScript.h:43
>> +    bool wasCanceled() const override;
> 
> final instead of override; can some of these be private too, if they are not called non-polymorphically?

Fixed. Currently they are polymorphically called in ScriptElement.cpp.

>> Source/WebCore/dom/LoadableModuleScript.h:48
>> +    void execute(ScriptElement&) override;
> 
> Ditto.

Fixed.
Comment 151 Yusuke Suzuki 2016-11-14 16:05:33 PST
Comment on attachment 294662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294662&action=review

>>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:30
>>> +#include "JSDOMPromise.h"
>> 
>> I don’t think this include is needed. Forward declaration of DeferredPromise should suffice.
> 
> Right. Dropped.

Ah, no. Unfortunately, RefPtr<DeferredPromise> requires complete class declaration since it will instantiate `->ref()` and `->deref()` calls.
Comment 152 Yusuke Suzuki 2016-11-14 17:34:23 PST
Created attachment 294786 [details]
Patch
Comment 153 WebKit Commit Bot 2016-11-14 17:39:12 PST
Attachment 294786 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScriptController.cpp:377:  'moduleScript' is incorrectly named. It should be named 'protector' or 'protectedModuleScriptRef'.  [readability/naming/protected] [4]
Total errors found: 1 in 160 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 154 Darin Adler 2016-11-14 17:59:48 PST
Comment on attachment 294662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294662&action=review

Thanks for considering all my comments! I have a few follow-up thoughts.

>>> Source/WebCore/bindings/js/CachedModuleScript.cpp:91
>>> +    RefPtr<CachedModuleScript> protectedThis(this);
>> 
>> Should be Ref, not RefPtr. What is the rationale for protection here?
> 
> During `client->notifyFinished()`, CachedModuleScript's owner can drop this. So this is necessary.

Oh, I see, it’s needed because we pass *this to the other notifyFinished functions.

>>> Source/WebCore/bindings/js/CachedModuleScript.cpp:105
>>> +    client.notifyFinished(*this);
>> 
>> What is the rationale for protection here?
> 
> During `client.notifyFinished(*this)`, the owner can drop the ref to this `CachedModuleScript`.
> Until `client.notifyFinished(*this)` is done, this protectedThis ensures that the given argument `CachedModuleScript&` is live.

I guess you are saying we want to protect "this" because we passed it in.

>>>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:30
>>>> +#include "JSDOMPromise.h"
>>> 
>>> I don’t think this include is needed. Forward declaration of DeferredPromise should suffice.
>> 
>> Right. Dropped.
> 
> Ah, no. Unfortunately, RefPtr<DeferredPromise> requires complete class declaration since it will instantiate `->ref()` and `->deref()` calls.

Not super important but: That would only be when compiling constructors, destructors, and other code that makes use of the data member. I would expect that we could include JSPromise.h in .cpp files and not in this header. I wonder what specific case drives the need for this?

>>> Source/WebCore/bindings/js/ScriptController.cpp:94
>>> +    , m_moduleLoaderFetchingIsCanceledSymbol()
>> 
>> Why is this needed?
> 
> This is noted in ChangeLog. Pasting.
> 
>  147        The module loader pipeline is constructed as a chain of promises.
>  148        Rejecting a promise when some error occurs is important because the execution flow of
>  149        the promise chain is driven by "rejected" or "fulfilled" events.
>  150        If the promise is not rejected while error occurs, reject handler won't be executed
>  151        and all the subsequent promise chain will wait the result forever.
>  152        As a result, even if the error is already reported to the inspector elsewhere,
>  153        it should be propagated in the pipeline. For example, the error of loading
>  154        CachedResource is already reported to the inspector by the loader. But we still need
>  155        to reject the promise to propagate this error to the script element.
>  156        At that time, we don't want to report the same error twice. When we propagate the error
>  157        that is already reported to the inspector, we throw moduleLoaderAlreadyReportedErrorSymbol
>  158        symbol instead. By comparing the thrown error with this symbol, we can distinguish errors raised
>  159        when checking syntax of a module script from errors reported already.
>  160        In the reject handler of the promise, we only report a error that is not this symbol.

I was asking a much more narrow question. Why do we need to explicitly specify the initialization of these data members in the constructor. Wouldn’t the same thing happen if we didn’t write those two lines of code?

>>> Source/WebCore/bindings/js/ScriptController.cpp:226
>>> +    Ref<Frame> protector(m_frame);
>> 
>> Unclear why a protector is helpful here. I am really concerned about adding protectors when it’s not clear exactly what they are needed for.
> 
> This is the same to ScriptController::executeScript. Script execution can destroy the frame.

OK, but why is it a problem that the frame was destroyed at that point? Presumably once the script is executed we don’t use the ScriptController or the Frame before returning from the function. It must be some code after the script is executed but before this function returns. I don’t see what code that is, which benefits from the protection.

>>> Source/WebCore/bindings/js/ScriptController.cpp:251
>>> +    const String* savedSourceURL = m_sourceURL;
>> 
>> auto*; also, save/restore is not great, but I suppose we don’t have a good alternative
> 
> Currently, we change it `const auto* String` thing, and leave save / restore.
> In JSC, we have some helper like SetForScope<>. I'll move this to WTF in the subsequent patch.

I think we have multiple helpers like that; another one is called TemporaryChange, I think.

>>> Source/WebCore/bindings/js/ScriptController.cpp:254
>>> +    Ref<Frame> protector(m_frame);
>> 
>> rationale for protector
> 
> According to the ScriptController::executeScript, script evaluation can destroy the frame.
> We would like to keep it until calling InspectorInstrumentation::didEvaluateScript.

OK, makes sense.
Comment 155 Yusuke Suzuki 2016-11-14 20:33:08 PST
Comment on attachment 294662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294662&action=review

>>>> Source/WebCore/bindings/js/CachedModuleScript.cpp:105
>>>> +    client.notifyFinished(*this);
>>> 
>>> What is the rationale for protection here?
>> 
>> During `client.notifyFinished(*this)`, the owner can drop the ref to this `CachedModuleScript`.
>> Until `client.notifyFinished(*this)` is done, this protectedThis ensures that the given argument `CachedModuleScript&` is live.
> 
> I guess you are saying we want to protect "this" because we passed it in.

Yup, right.

>>>> Source/WebCore/bindings/js/ScriptController.cpp:94
>>>> +    , m_moduleLoaderFetchingIsCanceledSymbol()
>>> 
>>> Why is this needed?
>> 
>> This is noted in ChangeLog. Pasting.
>> 
>>  147        The module loader pipeline is constructed as a chain of promises.
>>  148        Rejecting a promise when some error occurs is important because the execution flow of
>>  149        the promise chain is driven by "rejected" or "fulfilled" events.
>>  150        If the promise is not rejected while error occurs, reject handler won't be executed
>>  151        and all the subsequent promise chain will wait the result forever.
>>  152        As a result, even if the error is already reported to the inspector elsewhere,
>>  153        it should be propagated in the pipeline. For example, the error of loading
>>  154        CachedResource is already reported to the inspector by the loader. But we still need
>>  155        to reject the promise to propagate this error to the script element.
>>  156        At that time, we don't want to report the same error twice. When we propagate the error
>>  157        that is already reported to the inspector, we throw moduleLoaderAlreadyReportedErrorSymbol
>>  158        symbol instead. By comparing the thrown error with this symbol, we can distinguish errors raised
>>  159        when checking syntax of a module script from errors reported already.
>>  160        In the reject handler of the promise, we only report a error that is not this symbol.
> 
> I was asking a much more narrow question. Why do we need to explicitly specify the initialization of these data members in the constructor. Wouldn’t the same thing happen if we didn’t write those two lines of code?

Aha! I see. Yeah, default constructor is enough. Dropped.

>>>> Source/WebCore/bindings/js/ScriptController.cpp:226
>>>> +    Ref<Frame> protector(m_frame);
>>> 
>>> Unclear why a protector is helpful here. I am really concerned about adding protectors when it’s not clear exactly what they are needed for.
>> 
>> This is the same to ScriptController::executeScript. Script execution can destroy the frame.
> 
> OK, but why is it a problem that the frame was destroyed at that point? Presumably once the script is executed we don’t use the ScriptController or the Frame before returning from the function. It must be some code after the script is executed but before this function returns. I don’t see what code that is, which benefits from the protection.

Yeah, right. As you mentioned in ScriptController::executeScript, this Ref<> protection may be used for some weird thing.
Like, there are some missing protector, and instead, it guards against the crash.

I think keeping protector for module script evaluation is fine in the meantime.
This bug should be fixed in the other patch and at that time, both classic scripts and module scripts should be fixed at once.
I've filed it. https://bugs.webkit.org/show_bug.cgi?id=164763
And add FIXME comments.

>>>> Source/WebCore/bindings/js/ScriptController.cpp:251
>>>> +    const String* savedSourceURL = m_sourceURL;
>>> 
>>> auto*; also, save/restore is not great, but I suppose we don’t have a good alternative
>> 
>> Currently, we change it `const auto* String` thing, and leave save / restore.
>> In JSC, we have some helper like SetForScope<>. I'll move this to WTF in the subsequent patch.
> 
> I think we have multiple helpers like that; another one is called TemporaryChange, I think.

Yeah! That's completely the same. Use TemporaryChange<const String*> here.

>>> Source/WebCore/bindings/js/ScriptController.cpp:374
>>> +    PrivateName moduleLoaderFetchingIsCanceledSymbol = m_moduleLoaderFetchingIsCanceledSymbol;
>> 
>> auto?
> 
> No, we would like to copy this PrivateName into the lambda.

Ah, oops. I misread it as `auto&`. Use auto here.
Comment 156 Yusuke Suzuki 2016-11-14 20:37:04 PST
Created attachment 294802 [details]
Patch
Comment 157 WebKit Commit Bot 2016-11-14 20:42:16 PST
Attachment 294802 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScriptController.cpp:375:  'moduleScript' is incorrectly named. It should be named 'protector' or 'protectedModuleScriptRef'.  [readability/naming/protected] [4]
Total errors found: 1 in 160 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 158 Yusuke Suzuki 2016-11-14 21:21:08 PST
Created attachment 294805 [details]
Patch
Comment 159 Yusuke Suzuki 2016-11-14 21:23:02 PST
Comment on attachment 294805 [details]
Patch

Oops, failed.
Comment 160 WebKit Commit Bot 2016-11-14 21:25:18 PST
Attachment 294805 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScriptController.cpp:376:  'moduleScript' is incorrectly named. It should be named 'protector' or 'protectedModuleScriptRef'.  [readability/naming/protected] [4]
Total errors found: 1 in 143 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 161 Yusuke Suzuki 2016-11-14 21:28:46 PST
Created attachment 294806 [details]
Patch
Comment 162 WebKit Commit Bot 2016-11-14 21:32:39 PST
Attachment 294806 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScriptController.cpp:376:  'moduleScript' is incorrectly named. It should be named 'protector' or 'protectedModuleScriptRef'.  [readability/naming/protected] [4]
Total errors found: 1 in 160 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 163 Build Bot 2016-11-14 22:36:04 PST
Comment on attachment 294806 [details]
Patch

Attachment 294806 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2518032

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-blocked.html
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-invalidnonce.html
Comment 164 Build Bot 2016-11-14 22:36:13 PST
Created attachment 294813 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 165 Build Bot 2016-11-14 22:43:33 PST
Comment on attachment 294806 [details]
Patch

Attachment 294806 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2518063

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-blocked.html
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-invalidnonce.html
Comment 166 Build Bot 2016-11-14 22:43:42 PST
Created attachment 294815 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 167 Build Bot 2016-11-14 22:46:58 PST
Comment on attachment 294806 [details]
Patch

Attachment 294806 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2518035

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-blocked.html
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-invalidnonce.html
Comment 168 Build Bot 2016-11-14 22:47:07 PST
Created attachment 294817 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 169 Yusuke Suzuki 2016-11-14 22:52:36 PST
Created attachment 294818 [details]
Patch
Comment 170 WebKit Commit Bot 2016-11-14 22:57:13 PST
Attachment 294818 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScriptController.cpp:376:  'moduleScript' is incorrectly named. It should be named 'protector' or 'protectedModuleScriptRef'.  [readability/naming/protected] [4]
Total errors found: 1 in 160 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 171 Build Bot 2016-11-15 00:08:03 PST
Comment on attachment 294818 [details]
Patch

Attachment 294818 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2518409

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-blocked.html
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-invalidnonce.html
Comment 172 Build Bot 2016-11-15 00:08:12 PST
Created attachment 294823 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 173 Build Bot 2016-11-15 00:17:11 PST
Comment on attachment 294818 [details]
Patch

Attachment 294818 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2518411

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-blocked.html
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-invalidnonce.html
Comment 174 Build Bot 2016-11-15 00:17:20 PST
Created attachment 294824 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 175 Build Bot 2016-11-15 00:36:17 PST
Comment on attachment 294818 [details]
Patch

Attachment 294818 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2518517

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-blocked.html
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-invalidnonce.html
Comment 176 Build Bot 2016-11-15 00:36:26 PST
Created attachment 294826 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 177 Build Bot 2016-11-15 00:41:36 PST
Comment on attachment 294818 [details]
Patch

Attachment 294818 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2518503

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-blocked.html
http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-invalidnonce.html
Comment 178 Build Bot 2016-11-15 00:41:44 PST
Created attachment 294827 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 179 Yusuke Suzuki 2016-11-15 01:13:48 PST
Created attachment 294829 [details]
Patch
Comment 180 WebKit Commit Bot 2016-11-15 01:17:19 PST
Attachment 294829 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScriptController.cpp:376:  'moduleScript' is incorrectly named. It should be named 'protector' or 'protectedModuleScriptRef'.  [readability/naming/protected] [4]
Total errors found: 1 in 160 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 181 Yusuke Suzuki 2016-11-15 01:48:10 PST
Comment on attachment 294662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294662&action=review

>>>>> Source/WebCore/bindings/js/CachedModuleScriptLoader.h:30
>>>>> +#include "JSDOMPromise.h"
>>>> 
>>>> I don’t think this include is needed. Forward declaration of DeferredPromise should suffice.
>>> 
>>> Right. Dropped.
>> 
>> Ah, no. Unfortunately, RefPtr<DeferredPromise> requires complete class declaration since it will instantiate `->ref()` and `->deref()` calls.
> 
> Not super important but: That would only be when compiling constructors, destructors, and other code that makes use of the data member. I would expect that we could include JSPromise.h in .cpp files and not in this header. I wonder what specific case drives the need for this?

Oops, it was wrong. I need to include `<wtf/RefPtr.h>`. Fixed.
Comment 182 Yusuke Suzuki 2016-11-15 01:50:13 PST
Created attachment 294830 [details]
Patch
Comment 183 WebKit Commit Bot 2016-11-15 01:55:11 PST
Attachment 294830 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScriptController.cpp:376:  'moduleScript' is incorrectly named. It should be named 'protector' or 'protectedModuleScriptRef'.  [readability/naming/protected] [4]
Total errors found: 1 in 160 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 184 Yusuke Suzuki 2016-11-15 02:04:14 PST
The patch is ready.
Comment 185 Ryosuke Niwa 2016-11-15 18:57:22 PST
Comment on attachment 294830 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294830&action=review

> Source/WebCore/bindings/js/CachedModuleScriptClient.h:32
> +class CachedModuleScriptClient {

I'm not saying that we should do it in this patch but we should consider
using std::function / lambda in the lieu of these client classes with one function.

> Source/WebCore/bindings/js/CachedModuleScriptLoader.cpp:69
> +    // If the content is already cached, this immediately calls notifyFinished.
> +    m_cachedScript->addClient(*this);

What is the significant of this comment?
Why do we care that it may synchronously call notifyFinished?

> Source/WebCore/bindings/js/CachedModuleScriptLoader.cpp:85
> +    // from purging its data buffer. To keep the underlying CachedResource during
> +    // processing, we remove the client after calling client's notifyFinished.

It's not clear to me why removing the client keeps the data buffer.
Or are you saying that we can't do this before calling notifyFinished?
i.e. the data buffer may now be purged?
If so, a better way of phrasing this would be something like
"Remove the client after calling notifyFinished to keep
the data buffer in CachedResource alive while notifyFinished processes the resource."

> Source/WebCore/bindings/js/ScriptController.cpp:210
> +    setupModuleScriptHandlers(moduleScript, JSMainThreadExecState::loadModule(state, sourceCode.jsSourceCode(), toJS(&state, shell.window(), &element)), world);

There's a lot of nesting going on here. Can we split some of it to a separate line?

> Source/WebCore/bindings/js/ScriptController.cpp:362
> +enum class ErrorStatus {
> +    AlreadyReported,
> +    FetchingCanceled,
> +    ErrorThrown,
> +};

We don't need this enum anymore do we?

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:100
> +    bool isRootModule = importerModuleKey.isSymbol() || importerModuleKey.isUndefined();

Can this logic be extracted as a separate static local helper function?

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:150
> +        deferred->reject(TypeError, ASCIILiteral("Module key is an invalid URL."));

I think it's more natural to say "module key is not a valid URL" for an error message.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:155
> +    auto* scriptElement = toScriptElementIfPossible(&JSC::jsCast<JSElement*>(initiator)->wrapped());

Should we just call jsCast<HTMLScriptElement*>?
Or does SVG script element also support module?

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:209
> +    auto* cachedScript = loader.cachedScript();

Why not a reference? Also, why is it guaranteed that cachedScript is not null?

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:226
> +            "Refused to execute module script from '", cachedScript->url().stringCenterEllipsizedToLength(),
> +            "' because its MIME type ('", cachedScript->response().mimeType(), "') is not executable."));

"refused to execute" is quite a departure from other kinds of error messages we have.
How about simply "~ is not a valid JavaScript MIME type".

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:230
> +    if (auto* frame = m_document.frame()) {

We can just exit early when frame is null since that's not a formal flow of control.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:234
> +        } else if (cachedScript->wasCanceled())

Why not an early return instead of else iff for the same reason?

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:235
> +            promise->reject(frame->script().moduleLoaderFetchingIsCanceledSymbol());

Ditto to add an early exit instead.

> Source/WebCore/bindings/js/ScriptModuleLoader.h:50
> +WTF_MAKE_NONCOPYABLE(ScriptModuleLoader); WTF_MAKE_FAST_ALLOCATED;

I think we indent these.

> Source/WebCore/dom/ScriptElement.cpp:246
> +    bool isParserInsertedDeferredScript = (scriptType == ScriptType::Classic && hasSourceAttribute() && deferAttributeValue() && m_parserInserted && !asyncAttributeValue()) || (scriptType == ScriptType::Module && m_parserInserted && !asyncAttributeValue());

It seems like this can make a use of a line break in the middle.

> Source/WebCore/dom/ScriptElement.cpp:250
> +    } else if (scriptType == ScriptType::Classic && hasSourceAttribute() && m_parserInserted && !asyncAttributeValue()) {

It seems like we keep checking scriptType == ScriptType::Classic && hasSourceAttribute() multiple types.
How about declaring a local variable like isClassicExternalScript?

> Source/WebCore/dom/ScriptElement.cpp:253
> +    } else if ((scriptType == ScriptType::Classic && hasSourceAttribute() && !asyncAttributeValue() && !m_forceAsync) || (scriptType == ScriptType::Module && !asyncAttributeValue() && !m_forceAsync)) {

It looks like both classic & module scripts want to check !asyncAttributeValue() && !m_forceAsync.
I think it's cleaner to put that outside ||.
e.g. (isClassicExternalScript || scriptType == ScriptType::Module) && !asyncAttributeValue() && !m_forceAsync

Also, it might be worth adding a comment (better yet an assertion) here
or above all these if-else that inline module would be processed within requestModuleScript.
It's not great that our implementation is further away from the spec text:
https://html.spec.whatwg.org/#prepare-a-script

We should probably refactor at some point to make it more aligned with the spec'ed text given the complexity.

Alternatively, we can split into two functions and use this if-else for module as well.
I'm inclined to say that's probably more preferrable because that'll align our code more with the spec text.

> Source/WebCore/dom/ScriptElement.cpp:258
> +    } else if (hasSourceAttribute() || scriptType == ScriptType::Module) {
>          ASSERT(m_loadableScript);

So this is an async case. Can we assert that asyncAttributeValue() is true here?

> Source/WebCore/dom/ScriptElement.cpp:286
> +        String nonceAttribute = m_element.attributeWithoutSynchronization(HTMLNames::nonceAttr);
> +        String crossOriginMode = m_element.attributeWithoutSynchronization(HTMLNames::crossoriginAttr);
> +        auto request = requestScriptWithCache(m_element.document().completeURL(sourceURL), ScriptType::Classic, nonceAttribute, crossOriginMode);

Can we set crossOriginMode to "omit" as spec'ed in step 15 of https://html.spec.whatwg.org/#prepare-a-script
instead of changing it in CachedResourceRequest::setAsPotentiallyCrossOrigin?
That'll better match the spec and we don't have to make CachedResourceRequest aware of script type we're fetching.

> Source/WebCore/dom/ScriptElement.cpp:395
>              return;

Should we rename this to executeClassicScript for clarity?

> Source/WebCore/dom/ScriptElement.cpp:408
> +    // Create a script from the script element node, using the script
> +    // block's source and the script block's type.
> +    // Note: This is where the script is compiled and actually executed.

I don't think we really need this comment.
We call a function called "evaluate" right beneath it.

> Source/WebCore/html/parser/CSSPreloadScanner.cpp:206
> -            m_requests->append(std::make_unique<PreloadRequest>("css", url, baseElementURL, CachedResource::CSSStyleSheet, String()));
> +            m_requests->append(std::make_unique<PreloadRequest>("css", url, baseElementURL, CachedResource::CSSStyleSheet, String(), PreloadRequest::ScriptType::Classic));

!? Does why CSS preloader specifying a script type?
It doesn't load or run any scripts, right?

> Source/WebCore/html/parser/HTMLResourcePreloader.h:39
> +    enum class ScriptType {
> +        Classic,
> +        Module,
> +    };

Maybe we should declare this in a separate header like ScriptType.h and share it with ScriptElement.
It's redundant to have two enums.
Although I could see us wanting to add more types. e.g. ClassicScript, ModuleScript, CSS.
Yet another alternative is to split CachedResource::Script to CachedResource::ClassicScript and CachedResource::ModuleScript.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:85
> -void CachedResourceRequest::setAsPotentiallyCrossOrigin(const String& mode, Document& document)
> +void CachedResourceRequest::setAsPotentiallyCrossOriginImpl(const String& mode, Document& document, CrossOriginSettingContext crossOriginSettingContext)

We normally suffix these functions with "Internal" instead. e.g. setAsPotentiallyCrossOriginInternal here.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:98
> +        // https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-single-module-script
> +        m_options.mode = FetchOptions::Mode::Cors;
> +        m_options.credentials = FetchOptions::Credentials::Omit;
> +        m_options.allowCredentials = DoNotAllowStoredCredentials;

These defaults appear to come from:
https://html.spec.whatwg.org/#prepare-a-script
I think we should do this in prepareScript to better match the spec instead.

> LayoutTests/ChangeLog:66
> +        * js/dom/module-and-dom-content-loaded-expected.txt: Added.

We probably want to another directly inside dom or js since you've got so many tests here.

> LayoutTests/http/tests/misc/module-script-async.html:17
> +    if (!window.status_)
> +        window.status_ = '';
> +    window.status_ += ' ' + msg + ' ';

Can we use a more regular name like window.log and use an array instead of concatenating scripts?

> LayoutTests/http/tests/misc/module-script-async.html:32
> +    var expectedA = " async  external  inline  DOMContentLoaded  slowAsync ";
> +    var expectedB = " external  async  inline  DOMContentLoaded  slowAsync ";
> +    var expectedC = " external  inline  async  DOMContentLoaded  slowAsync ";
> +    var expectedD = " external  inline  DOMContentLoaded  async  slowAsync ";
> +    var results = "PASS";
> +    if (window.status_ != expectedA && window.status_ != expectedB && window.status_ != expectedC && window.status_ != expectedD)
> +        results = "FAIL: Expected one of '" + expectedA + "' || '" + expectedB + "' || '" + expectedC + "' || '" + expectedD + "', Actual='" + window.status_ + "'";

It appears to me that a better way of doing this would be something like
const expectedOrderings = [['async', 'external', 'inline', 'DOMContentLoaded', 'slowAsync'], ...]
if (!expectedOrderings.find((expected) => JSON.stringify(expected) == JSON.stringify(window.log)))
  results = 'FAIL: ...';

> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-and-scripthash.html:24
> +        <script type="module" nonce="nonceynonce">
> +            alert('PASS (3/3)');
> +            done("PASS");
> +        </script>

I think we should move one of these passes to the end so that we know for sure FAIL cases did run instead of the test finishing prematurely.

> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-basic-blocked.html:6
> +        <script nonce="noncynonce">
> +        if (window.testRunner) {

Maybe indent the script once more to be consistent?

> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-basic-blocked.html:32
> +        </script>

It seems like we should add a test case for using eval statement & calling eval function
in inline scripts and external scripts and make sure they're blocked.

> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-ignore-unsafeinline.html:4
> +        <meta http-equiv="Content-Security-Policy" content="script-src 'nonce-noncynonce' 'nonce-noncy+/=nonce' 'unsafe-inline'">

I guess unsafe-inline doesn't allow inline module scripts?
Where is that spec'ed?
Could you mention that in the change log?

> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-ignore-unsafeinline.html:26
> +        <script type="module">
> +            alert('FAIL (1/1)');
> +        </script>

Again, I think it's better to have a FAIL case not appear at the very end.

> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-invalidnonce.html:22
> +  <p>
> +      None of these scripts should execute, as all the nonces are invalid.

Inconsistent indentation.

> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-redirect-same-origin.html:9
> +        <script nonce="noncynonce">
> +        if (window.testRunner) {

Nit: inconsistent indentation.

> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-redirect-same-origin.html:17
> +            var preloaded = internals.isPreloaded("../resources/redir.php?url=http://127.0.0.1:8000/security/contentSecurityPolicy/resources/alert-pass.js");
> +            document.querySelector("#preloaded").innerText = preloaded ? "YES" : "NO";

Shouldn't this always be NO? If so, why don't we just assert with a comment clarifying
what we should do when we change the behavior of our preloader instead?
If that's going to change, that seems to make this test flaky, which needs to be avoided.

> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-redirect.html:17
> +            var preloaded = internals.isPreloaded("../resources/redir.php?url=http://localhost:8000/security/contentSecurityPolicy/resources/alert-pass.js");
> +            document.querySelector("#preloaded").innerText = preloaded ? "YES" : "NO";

Ditto.

> LayoutTests/http/tests/security/contentSecurityPolicy/resources/multiple-iframe-module-test.js:38
> +    iframe.src = baseURL + "resources/echo-module-script-src.pl?" +
> +                 "experimental=" + (experimental ? "true" : "false") +
> +                 "&should_run=" + encodeURIComponent(current[0]) +
> +                 "&csp=" + policy + "&q=" + scriptToLoad;

Nit: + should be at the beginning of each line and these should not be aligned against =.
(Same rule as C++).

> LayoutTests/http/tests/security/contentSecurityPolicy/resources/multiple-iframe-module-test.js:40
> +    if (current[3] !== undefined)
> +      iframe.src += "&nonce=" + encodeURIComponent(current[3]);

Wrong indentation.

Instead of building strings like this, can we just create an array of key-value pair and join them together at the end?
e.g.
const queries = {};
queries['experimental'] = (!!experimental).toString();
Object.getPropertyNames(queries).map((key) => key + '=' + queries[key]).join('&');

> LayoutTests/http/tests/security/module-correct-mime-types.html:8
> +description('Test module rejects scripts with correct mime types.');

Did you mean to say module scripts with correct mime types *run*? They're certainly not rejected here.

> LayoutTests/http/tests/security/module-correct-mime-types.html:52
> +    debug('Module rejects scripts with correct mime types.');

Ditto.

> LayoutTests/http/tests/security/module-crossorigin-loads-same-origin.html:10
> +function loaded() {
> +    document.querySelector("pre").innerHTML = "PASS";

This checks we've fired load script but not quite whether script had run or not.
Can we check the value of "secretness" defined in localScript.js to make sure the script had run?
You may need to modify localScript.js to export some symbol, etc... though...

> LayoutTests/http/tests/security/module-no-mime-type.html:19
> +function done()
> +{
> +    debug('Module rejects a script with no mime type.');
> +    finishJSTest();
> +}

Is it possible to check that the script actually didn't run via a global state, etc...?

> LayoutTests/http/tests/security/module-requires-javascript-mime-types.html:8
> +description('Test module rejects scripts with no mime type.');

How is this test different from module-no-mime-type.html?

> LayoutTests/js/dom/module-and-dom-content-loaded.html:21
> +window.addEventListener('DOMContentLoaded', function ()
> +{
> +    finishJSTest();
> +}, false);

Can we add a event listener in a classic inline script to make sure DOMContentLoaded is not fired until the module script had run?
Also, can we assert that document.readyState is not "loading" until modules had ran?

Otherwise, this test would only timeout when the semantics of type=module is violated,
which isn't a great way to signal that the semantics is actually violated.

> LayoutTests/js/dom/module-and-window-load.html:22
> +<script type="module">
> +window.onload = function ()
> +{
> +    finishJSTest();
> +}
> +</script>

Ditto. I think we should change some state in the module script and have a separate classic script
which attaches a load event handler which checks that the module had ran.

> LayoutTests/js/dom/module-async-and-window-load.html:18
> +window.onload = function ()

Ditto.

> LayoutTests/js/dom/module-execution-order-mixed.html:23
> +shouldBe("count++", "6");

Could you add another test where classical scripts and module scripts are mixed?

> LayoutTests/js/dom/module-incorrect-relative-specifier.html:15
> +    debug(`${current}`);

Can we push "this" into a set and verify at the end that all module script elements are in the set?

> LayoutTests/js/dom/module-load-event-with-src.html:14
> +<script src="../../resources/js-test-post.js"></script>
> +<script type="module" onload="finishJSTest()" src="script-tests/module-load-event-with-src.js"></script>

Can we also check some global state for making sure the script had run instead of relying the expected result to contain some text?
When this starts failing, some people might think it's okay to just rebaseline the test if glanced it quickly.
Whereas if it's explicitly checked via shouldBe, people are a lot more likely to realize that something is broken.

> LayoutTests/js/dom/module-load-event.html:16
> +<script type="module" onload="finishJSTest()">
> +debug('Executing an inlined module.');
> +</script>

Ditto about changing the global state instead of just emitting text.

> LayoutTests/js/dom/module-load-same-module-from-different-entry-point-dynamic.html:20
> +element.type = "module";
> +element.innerText = "script-tests/module-load-same-module-from-different-entry-point.js";

Ditto about checking the global state explicitly.

> LayoutTests/js/dom/module-load-same-module-from-different-entry-point.html:17
> +debug('Executing the module.');

Ditto about change the global state instead of just emitting text.

> LayoutTests/js/dom/module-not-found-error-event-with-src.html:16
> +function onError()
> +{
> +    successfullyParsed = true;
> +    finishJSTest()
> +}

Instead of just finishing script we should explicitly check that the error event had fired
by e.g. adding another module script that loads and checking this condition in that module script
or on its load event handler.

> LayoutTests/js/dom/module-not-found-error-event.html:19
> +<script type="module" onerror="onError()">

Ditto about more explicitly checking the error being dispatched.
Comment 186 Yusuke Suzuki 2016-11-16 02:14:17 PST
Comment on attachment 294830 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294830&action=review

Thanks!!

>> Source/WebCore/bindings/js/CachedModuleScriptClient.h:32
>> +class CachedModuleScriptClient {
> 
> I'm not saying that we should do it in this patch but we should consider
> using std::function / lambda in the lieu of these client classes with one function.

It's interesting clean up!

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.cpp:69
>> +    m_cachedScript->addClient(*this);
> 
> What is the significant of this comment?
> Why do we care that it may synchronously call notifyFinished?

It means that setting m_cachedScript should be done before this. notifyFinished requires that m_cachedScript is not null.

>> Source/WebCore/bindings/js/CachedModuleScriptLoader.cpp:85
>> +    // processing, we remove the client after calling client's notifyFinished.
> 
> It's not clear to me why removing the client keeps the data buffer.
> Or are you saying that we can't do this before calling notifyFinished?
> i.e. the data buffer may now be purged?
> If so, a better way of phrasing this would be something like
> "Remove the client after calling notifyFinished to keep
> the data buffer in CachedResource alive while notifyFinished processes the resource."

Right. Changed.

>> Source/WebCore/bindings/js/ScriptController.cpp:210
>> +    setupModuleScriptHandlers(moduleScript, JSMainThreadExecState::loadModule(state, sourceCode.jsSourceCode(), toJS(&state, shell.window(), &element)), world);
> 
> There's a lot of nesting going on here. Can we split some of it to a separate line?

Yeah, I've split it, like,

auto& promise = JSMainThreadExecState::loadModule(state, sourceCode.jsSourceCode(), toJS(&state, shell.window(), &element));
setupModuleScriptHandlers(moduleScript, promise, world);

>> Source/WebCore/bindings/js/ScriptController.cpp:362
>> +};
> 
> We don't need this enum anymore do we?

Ah, right! Dropped.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:100
>> +    bool isRootModule = importerModuleKey.isSymbol() || importerModuleKey.isUndefined();
> 
> Can this logic be extracted as a separate static local helper function?

Extracted it as `bool isRootModule(JSC::JSValue)`.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:150
>> +        deferred->reject(TypeError, ASCIILiteral("Module key is an invalid URL."));
> 
> I think it's more natural to say "module key is not a valid URL" for an error message.

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:155
>> +    auto* scriptElement = toScriptElementIfPossible(&JSC::jsCast<JSElement*>(initiator)->wrapped());
> 
> Should we just call jsCast<HTMLScriptElement*>?
> Or does SVG script element also support module?

Right now, we implemented it in ScriptElement. So SVG script element should support it.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:209
>> +    auto* cachedScript = loader.cachedScript();
> 
> Why not a reference? Also, why is it guaranteed that cachedScript is not null?

notifyFinished is called only when cachedScript is non null.
It never becomes nullptr. Taking reference instead.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:226
>> +            "' because its MIME type ('", cachedScript->response().mimeType(), "') is not executable."));
> 
> "refused to execute" is quite a departure from other kinds of error messages we have.
> How about simply "~ is not a valid JavaScript MIME type".

Looks good. Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:230
>> +    if (auto* frame = m_document.frame()) {
> 
> We can just exit early when frame is null since that's not a formal flow of control.

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:234
>> +        } else if (cachedScript->wasCanceled())
> 
> Why not an early return instead of else iff for the same reason?

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:235
>> +            promise->reject(frame->script().moduleLoaderFetchingIsCanceledSymbol());
> 
> Ditto to add an early exit instead.

Fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.h:50
>> +WTF_MAKE_NONCOPYABLE(ScriptModuleLoader); WTF_MAKE_FAST_ALLOCATED;
> 
> I think we indent these.

Fixed.

>> Source/WebCore/dom/ScriptElement.cpp:246
>> +    bool isParserInsertedDeferredScript = (scriptType == ScriptType::Classic && hasSourceAttribute() && deferAttributeValue() && m_parserInserted && !asyncAttributeValue()) || (scriptType == ScriptType::Module && m_parserInserted && !asyncAttributeValue());
> 
> It seems like this can make a use of a line break in the middle.

Inserted a line break.

>> Source/WebCore/dom/ScriptElement.cpp:250
>> +    } else if (scriptType == ScriptType::Classic && hasSourceAttribute() && m_parserInserted && !asyncAttributeValue()) {
> 
> It seems like we keep checking scriptType == ScriptType::Classic && hasSourceAttribute() multiple types.
> How about declaring a local variable like isClassicExternalScript?

Sounds nice. Fixed.

>> Source/WebCore/dom/ScriptElement.cpp:253
>> +    } else if ((scriptType == ScriptType::Classic && hasSourceAttribute() && !asyncAttributeValue() && !m_forceAsync) || (scriptType == ScriptType::Module && !asyncAttributeValue() && !m_forceAsync)) {
> 
> It looks like both classic & module scripts want to check !asyncAttributeValue() && !m_forceAsync.
> I think it's cleaner to put that outside ||.
> e.g. (isClassicExternalScript || scriptType == ScriptType::Module) && !asyncAttributeValue() && !m_forceAsync
> 
> Also, it might be worth adding a comment (better yet an assertion) here
> or above all these if-else that inline module would be processed within requestModuleScript.
> It's not great that our implementation is further away from the spec text:
> https://html.spec.whatwg.org/#prepare-a-script
> 
> We should probably refactor at some point to make it more aligned with the spec'ed text given the complexity.
> 
> Alternatively, we can split into two functions and use this if-else for module as well.
> I'm inclined to say that's probably more preferrable because that'll align our code more with the spec text.

The above fix is done. And I added the comment here.

>> Source/WebCore/dom/ScriptElement.cpp:258
>>          ASSERT(m_loadableScript);
> 
> So this is an async case. Can we assert that asyncAttributeValue() is true here?

Inserted, ASSERT(asyncAttributeValue() || m_forceAsync);.

>> Source/WebCore/dom/ScriptElement.cpp:286
>> +        auto request = requestScriptWithCache(m_element.document().completeURL(sourceURL), ScriptType::Classic, nonceAttribute, crossOriginMode);
> 
> Can we set crossOriginMode to "omit" as spec'ed in step 15 of https://html.spec.whatwg.org/#prepare-a-script
> instead of changing it in CachedResourceRequest::setAsPotentiallyCrossOrigin?
> That'll better match the spec and we don't have to make CachedResourceRequest aware of script type we're fetching.

Fixed.

>> Source/WebCore/dom/ScriptElement.cpp:395
>>              return;
> 
> Should we rename this to executeClassicScript for clarity?

Currently, XML script element is not handled.
Once it is done, we will rename this function.

>> Source/WebCore/dom/ScriptElement.cpp:408
>> +    // Note: This is where the script is compiled and actually executed.
> 
> I don't think we really need this comment.
> We call a function called "evaluate" right beneath it.

OK, dropped. Maybe this is ancient comment :)

>> Source/WebCore/html/parser/CSSPreloadScanner.cpp:206
>> +            m_requests->append(std::make_unique<PreloadRequest>("css", url, baseElementURL, CachedResource::CSSStyleSheet, String(), PreloadRequest::ScriptType::Classic));
> 
> !? Does why CSS preloader specifying a script type?
> It doesn't load or run any scripts, right?

Yes, this is because PreloadRequest always needs all the fields right now. That's why I selected PreloadRequest::ModuleScript::{Yes, No} previously.
At that time, we can pass PreloadRequest::ModuleScript::No.

Maybe, it is better for now, reverted.
Once the refactoring noted below is landed, this should be fixed.

>> Source/WebCore/html/parser/HTMLResourcePreloader.h:39
>> +    };
> 
> Maybe we should declare this in a separate header like ScriptType.h and share it with ScriptElement.
> It's redundant to have two enums.
> Although I could see us wanting to add more types. e.g. ClassicScript, ModuleScript, CSS.
> Yet another alternative is to split CachedResource::Script to CachedResource::ClassicScript and CachedResource::ModuleScript.

Right. It should be better. In the meantime, I reverted this ScriptType to ModuleScript { Yes, No }.
The above refactoring will be done in the separate patch.

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:85
>> +void CachedResourceRequest::setAsPotentiallyCrossOriginImpl(const String& mode, Document& document, CrossOriginSettingContext crossOriginSettingContext)
> 
> We normally suffix these functions with "Internal" instead. e.g. setAsPotentiallyCrossOriginInternal here.

Now, setAsPotentiallyCrossOriginImpl is dropped since crossOriginMode is changed in the caller side.

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:98
>> +        m_options.allowCredentials = DoNotAllowStoredCredentials;
> 
> These defaults appear to come from:
> https://html.spec.whatwg.org/#prepare-a-script
> I think we should do this in prepareScript to better match the spec instead.

Instead, we set "omit" to crossOriginMode. So we can drop this if clause and the else side is extended to accept "omit".
And we cannot move the above things to ScriptElement since it is also used from the preloader.

>> LayoutTests/ChangeLog:66
>> +        * js/dom/module-and-dom-content-loaded-expected.txt: Added.
> 
> We probably want to another directly inside dom or js since you've got so many tests here.

Nice. Moved to js/dom/module.

>> LayoutTests/http/tests/misc/module-script-async.html:17
>> +    window.status_ += ' ' + msg + ' ';
> 
> Can we use a more regular name like window.log and use an array instead of concatenating scripts?

Fixed.

>> LayoutTests/http/tests/misc/module-script-async.html:32
>> +        results = "FAIL: Expected one of '" + expectedA + "' || '" + expectedB + "' || '" + expectedC + "' || '" + expectedD + "', Actual='" + window.status_ + "'";
> 
> It appears to me that a better way of doing this would be something like
> const expectedOrderings = [['async', 'external', 'inline', 'DOMContentLoaded', 'slowAsync'], ...]
> if (!expectedOrderings.find((expected) => JSON.stringify(expected) == JSON.stringify(window.log)))
>   results = 'FAIL: ...';

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-and-scripthash.html:24
>> +        </script>
> 
> I think we should move one of these passes to the end so that we know for sure FAIL cases did run instead of the test finishing prematurely.

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-basic-blocked.html:6
>> +        if (window.testRunner) {
> 
> Maybe indent the script once more to be consistent?

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-basic-blocked.html:32
>> +        </script>
> 
> It seems like we should add a test case for using eval statement & calling eval function
> in inline scripts and external scripts and make sure they're blocked.

Added.

>> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-ignore-unsafeinline.html:4
>> +        <meta http-equiv="Content-Security-Policy" content="script-src 'nonce-noncynonce' 'nonce-noncy+/=nonce' 'unsafe-inline'">
> 
> I guess unsafe-inline doesn't allow inline module scripts?
> Where is that spec'ed?
> Could you mention that in the change log?

https://html.spec.whatwg.org/multipage/scripting.html#prepare-a-script Step 11, "If the script element does not have a src content attribute, and the Should element's inline behavior be blocked by Content Security Policy? algorithm returns "Blocked" when executed upon the script element, "script", and the script element's child text content, then abort these steps. The script is not executed. [CSP]"

So, if the script tag does not have src (it should include inline module scripts), CSP unsafe-inline should be applied.
Noted.

>> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-ignore-unsafeinline.html:26
>> +        </script>
> 
> Again, I think it's better to have a FAIL case not appear at the very end.

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-invalidnonce.html:22
>> +      None of these scripts should execute, as all the nonces are invalid.
> 
> Inconsistent indentation.

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-redirect-same-origin.html:9
>> +        if (window.testRunner) {
> 
> Nit: inconsistent indentation.

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-redirect-same-origin.html:17
>> +            document.querySelector("#preloaded").innerText = preloaded ? "YES" : "NO";
> 
> Shouldn't this always be NO? If so, why don't we just assert with a comment clarifying
> what we should do when we change the behavior of our preloader instead?
> If that's going to change, that seems to make this test flaky, which needs to be avoided.

OK, dropped.

>> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/module-scriptnonce-redirect.html:17
>> +            document.querySelector("#preloaded").innerText = preloaded ? "YES" : "NO";
> 
> Ditto.

Dropped.

>> LayoutTests/http/tests/security/contentSecurityPolicy/resources/multiple-iframe-module-test.js:40
>> +      iframe.src += "&nonce=" + encodeURIComponent(current[3]);
> 
> Wrong indentation.
> 
> Instead of building strings like this, can we just create an array of key-value pair and join them together at the end?
> e.g.
> const queries = {};
> queries['experimental'] = (!!experimental).toString();
> Object.getPropertyNames(queries).map((key) => key + '=' + queries[key]).join('&');

OK, fixed like this.

>> LayoutTests/http/tests/security/module-correct-mime-types.html:8
>> +description('Test module rejects scripts with correct mime types.');
> 
> Did you mean to say module scripts with correct mime types *run*? They're certainly not rejected here.

Oops, right. Fixed.

>> LayoutTests/http/tests/security/module-correct-mime-types.html:52
>> +    debug('Module rejects scripts with correct mime types.');
> 
> Ditto.

Fixed.

>> LayoutTests/http/tests/security/module-crossorigin-loads-same-origin.html:10
>> +    document.querySelector("pre").innerHTML = "PASS";
> 
> This checks we've fired load script but not quite whether script had run or not.
> Can we check the value of "secretness" defined in localScript.js to make sure the script had run?
> You may need to modify localScript.js to export some symbol, etc... though...

Fixed.

>> LayoutTests/http/tests/security/module-no-mime-type.html:19
>> +}
> 
> Is it possible to check that the script actually didn't run via a global state, etc...?

Fixed.

>> LayoutTests/http/tests/security/module-requires-javascript-mime-types.html:8
>> +description('Test module rejects scripts with no mime type.');
> 
> How is this test different from module-no-mime-type.html?

Oops, dropped.

>> LayoutTests/js/dom/module-and-dom-content-loaded.html:21
>> +}, false);
> 
> Can we add a event listener in a classic inline script to make sure DOMContentLoaded is not fired until the module script had run?
> Also, can we assert that document.readyState is not "loading" until modules had ran?
> 
> Otherwise, this test would only timeout when the semantics of type=module is violated,
> which isn't a great way to signal that the semantics is actually violated.

Fixed.

>> LayoutTests/js/dom/module-and-window-load.html:22
>> +</script>
> 
> Ditto. I think we should change some state in the module script and have a separate classic script
> which attaches a load event handler which checks that the module had ran.

Fixed.

>> LayoutTests/js/dom/module-async-and-window-load.html:18
>> +window.onload = function ()
> 
> Ditto.

Fixed.

>> LayoutTests/js/dom/module-execution-order-mixed.html:23
>> +shouldBe("count++", "6");
> 
> Could you add another test where classical scripts and module scripts are mixed?

Added.

>> LayoutTests/js/dom/module-incorrect-relative-specifier.html:15
>> +    debug(`${current}`);
> 
> Can we push "this" into a set and verify at the end that all module script elements are in the set?

Fixed.

>> LayoutTests/js/dom/module-load-event-with-src.html:14
>> +<script type="module" onload="finishJSTest()" src="script-tests/module-load-event-with-src.js"></script>
> 
> Can we also check some global state for making sure the script had run instead of relying the expected result to contain some text?
> When this starts failing, some people might think it's okay to just rebaseline the test if glanced it quickly.
> Whereas if it's explicitly checked via shouldBe, people are a lot more likely to realize that something is broken.

Fixed.

>> LayoutTests/js/dom/module-load-event.html:16
>> +</script>
> 
> Ditto about changing the global state instead of just emitting text.

Fixed.

>> LayoutTests/js/dom/module-load-same-module-from-different-entry-point-dynamic.html:20
>> +element.innerText = "script-tests/module-load-same-module-from-different-entry-point.js";
> 
> Ditto about checking the global state explicitly.

Fixed.

>> LayoutTests/js/dom/module-load-same-module-from-different-entry-point.html:17
>> +debug('Executing the module.');
> 
> Ditto about change the global state instead of just emitting text.

Fixed.

>> LayoutTests/js/dom/module-not-found-error-event-with-src.html:16
>> +}
> 
> Instead of just finishing script we should explicitly check that the error event had fired
> by e.g. adding another module script that loads and checking this condition in that module script
> or on its load event handler.

Fixed.

>> LayoutTests/js/dom/module-not-found-error-event.html:19
>> +<script type="module" onerror="onError()">
> 
> Ditto about more explicitly checking the error being dispatched.

Fixed.
Comment 187 Yusuke Suzuki 2016-11-16 02:45:11 PST
Created attachment 294928 [details]
Checking EWS
Comment 188 Yusuke Suzuki 2016-11-16 02:49:41 PST
Created attachment 294929 [details]
Patch for landing
Comment 189 WebKit Commit Bot 2016-11-16 02:51:12 PST
Attachment 294928 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScriptController.cpp:372:  'moduleScript' is incorrectly named. It should be named 'protector' or 'protectedModuleScriptRef'.  [readability/naming/protected] [4]
Total errors found: 1 in 169 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 190 WebKit Commit Bot 2016-11-16 02:54:53 PST
Attachment 294929 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScriptController.cpp:372:  'moduleScript' is incorrectly named. It should be named 'protector' or 'protectedModuleScriptRef'.  [readability/naming/protected] [4]
Total errors found: 1 in 169 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 191 Yusuke Suzuki 2016-11-16 03:39:59 PST
Committed r208788: <http://trac.webkit.org/changeset/208788>