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.)
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 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 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 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.
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 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.
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
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
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
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
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
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 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 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 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 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 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 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 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 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 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().
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
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 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 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 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 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 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.
(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.
(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.
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 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 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 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 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.
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 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 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.
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.
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.
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.
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
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
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
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.
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
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
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
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
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 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.
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 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 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.
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.
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.
2015-09-10 01:24 PDT, Yusuke Suzuki
2015-09-10 21:03 PDT, Yusuke Suzuki
2015-09-14 15:21 PDT, Yusuke Suzuki
2015-09-15 03:00 PDT, Yusuke Suzuki
2015-09-18 17:36 PDT, Yusuke Suzuki
2015-09-18 20:23 PDT, Yusuke Suzuki
2015-09-18 21:16 PDT, Yusuke Suzuki
2015-09-21 11:56 PDT, Yusuke Suzuki
2015-09-23 14:18 PDT, Yusuke Suzuki
2015-09-23 15:35 PDT, Yusuke Suzuki
2015-09-23 17:31 PDT, Yusuke Suzuki
2015-09-23 17:41 PDT, Yusuke Suzuki
2015-09-23 22:01 PDT, Yusuke Suzuki
2015-09-24 11:49 PDT, Yusuke Suzuki
2015-09-24 13:22 PDT, Yusuke Suzuki
2015-09-24 13:46 PDT, Yusuke Suzuki
2015-09-24 13:49 PDT, Yusuke Suzuki
2015-09-25 15:06 PDT, Yusuke Suzuki
2016-08-20 00:59 PDT, Yusuke Suzuki
2016-08-20 23:01 PDT, Yusuke Suzuki
2016-08-20 23:17 PDT, Yusuke Suzuki
2016-08-25 21:01 PDT, Yusuke Suzuki
2016-08-26 11:18 PDT, Yusuke Suzuki
2016-08-26 15:50 PDT, Yusuke Suzuki
2016-08-26 17:07 PDT, Build Bot
2016-08-26 17:08 PDT, Build Bot
2016-08-26 17:27 PDT, Build Bot
2016-08-26 19:10 PDT, Build Bot
2016-08-28 02:10 PDT, Yusuke Suzuki
2016-08-28 03:23 PDT, Build Bot
2016-08-28 03:27 PDT, Build Bot
2016-08-28 03:37 PDT, Build Bot
2016-08-28 03:37 PDT, Build Bot
2016-08-28 23:05 PDT, Yusuke Suzuki
2016-08-29 00:25 PDT, Build Bot
2016-08-29 00:28 PDT, Build Bot
2016-08-29 00:30 PDT, Build Bot
2016-08-29 00:39 PDT, Build Bot
2016-08-29 10:10 PDT, Yusuke Suzuki
2016-08-29 10:35 PDT, Yusuke Suzuki
2016-08-29 11:38 PDT, Build Bot
2016-08-29 11:45 PDT, Build Bot
2016-08-29 11:49 PDT, Build Bot
2016-08-29 11:55 PDT, Build Bot
2016-08-29 14:27 PDT, Yusuke Suzuki
2016-08-30 21:04 PDT, Yusuke Suzuki
2016-08-31 01:14 PDT, Yusuke Suzuki
2016-08-31 14:52 PDT, Yusuke Suzuki
2016-08-31 16:34 PDT, Build Bot
2016-08-31 21:44 PDT, Yusuke Suzuki
2016-09-01 11:43 PDT, Yusuke Suzuki
2016-09-01 15:23 PDT, Yusuke Suzuki
2016-09-01 16:06 PDT, Yusuke Suzuki
2016-09-01 19:47 PDT, Yusuke Suzuki
2016-09-01 20:52 PDT, Yusuke Suzuki
2016-09-01 21:17 PDT, Yusuke Suzuki
2016-09-01 22:18 PDT, Yusuke Suzuki
2016-09-01 23:34 PDT, Build Bot
2016-09-01 23:38 PDT, Build Bot
2016-09-01 23:50 PDT, Yusuke Suzuki
2016-09-02 01:30 PDT, Build Bot
2016-09-02 09:21 PDT, Yusuke Suzuki
2016-09-02 11:07 PDT, Yusuke Suzuki
2016-09-02 17:18 PDT, Yusuke Suzuki
2016-09-02 18:34 PDT, Build Bot
2016-09-02 18:44 PDT, Build Bot
2016-09-02 19:09 PDT, Yusuke Suzuki
2016-09-06 15:07 PDT, Yusuke Suzuki
2016-09-06 15:18 PDT, Yusuke Suzuki
2016-09-06 16:29 PDT, Yusuke Suzuki
2016-09-08 22:44 PDT, Yusuke Suzuki
2016-09-09 17:55 PDT, Yusuke Suzuki
2016-10-14 13:17 PDT, Yusuke Suzuki
2016-10-22 00:51 PDT, Yusuke Suzuki
2016-10-22 18:04 PDT, Build Bot
2016-10-28 00:04 PDT, Yusuke Suzuki
2016-10-31 23:06 PDT, Yusuke Suzuki
2016-11-01 15:33 PDT, Yusuke Suzuki
2016-11-09 01:58 PST, Yusuke Suzuki
2016-11-10 23:54 PST, Yusuke Suzuki
2016-11-11 00:20 PST, Yusuke Suzuki
2016-11-11 01:43 PST, Build Bot
2016-11-11 02:08 PST, Yusuke Suzuki
2016-11-12 00:38 PST, Yusuke Suzuki
2016-11-12 01:12 PST, Yusuke Suzuki
2016-11-12 01:38 PST, Yusuke Suzuki
2016-11-13 01:17 PST, Yusuke Suzuki
2016-11-14 17:34 PST, Yusuke Suzuki
2016-11-14 20:37 PST, Yusuke Suzuki
2016-11-14 21:21 PST, Yusuke Suzuki
2016-11-14 21:28 PST, Yusuke Suzuki
2016-11-14 22:36 PST, Build Bot
2016-11-14 22:43 PST, Build Bot
2016-11-14 22:47 PST, Build Bot
2016-11-14 22:52 PST, Yusuke Suzuki
2016-11-15 00:08 PST, Build Bot
2016-11-15 00:17 PST, Build Bot
2016-11-15 00:36 PST, Build Bot
2016-11-15 00:41 PST, Build Bot
2016-11-15 01:13 PST, Yusuke Suzuki
2016-11-15 01:50 PST, Yusuke Suzuki
2016-11-16 02:45 PST, Yusuke Suzuki
2016-11-16 02:49 PST, Yusuke Suzuki