[ES6 modules] Introduce ModuleScriptLoader

third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "core/loader/modulescript/ModuleScriptLoader.h"
+
+#include "core/dom/Modulator.h"
+#include "core/dom/ModuleScript.h"
+#include "core/loader/modulescript/ModuleScriptLoaderClient.h"
+#include "core/loader/modulescript/ModuleScriptLoaderRegistry.h"
+#include "platform/loader/fetch/ResourceFetcher.h"
+#include "platform/network/mime/MIMETypeRegistry.h"
+#include "platform/weborigin/SecurityPolicy.h"
+
+namespace blink {
+
+ModuleScriptLoader::ModuleScriptLoader(Modulator* modulator,
+                                       ModuleScriptLoaderRegistry* registry,
+                                       ModuleScriptLoaderClient* client)
+    : m_modulator(modulator), m_registry(registry), m_client(client) {
+  DCHECK(modulator);
+  DCHECK(registry);
+  DCHECK(client);
+}
+
+ModuleScriptLoader::~ModuleScriptLoader() {}
+
+#ifndef NDEBUG
+const char* ModuleScriptLoader::stateToString(ModuleScriptLoader::State state) {

[hiroshige, 2017/02/20 23:15:32]

Why aren't stateToString() and m_url used in this CL?

[kouhei (in TOK), 2017/02/21 01:26:51]

I used them in ::show(), which I accidentally dropped from this CL. re-added
show()

+  switch (state) {
+    case State::Initial:
+      return "Initial";
+    case State::Fetching:
+      return "Fetching";
+    case State::Finished:
+      return "Finished";
+    default:

[yhirano, 2017/02/17 04:59:15]

No default: is needed. Please add NOTRECHED() outside of the switch statement.

[kouhei (in TOK), 2017/02/21 01:26:51]

Done.

+      return "<unknown>";
+  }
+}
+#endif
+
+void ModuleScriptLoader::advanceState(ModuleScriptLoader::State newState) {
+  switch (m_state) {
+    case State::Initial:
+      DCHECK_EQ(newState, State::Fetching);
+      break;
+    case State::Fetching:
+      DCHECK_EQ(newState, State::Finished);
+      break;
+    case State::Finished:
+      NOTREACHED();
+      break;
+    default:

[hiroshige, 2017/02/20 23:15:32]

This |default| clause is also not needed.

[kouhei (in TOK), 2017/02/21 01:26:51]

Done.

+      NOTREACHED();
+  }
+
+  m_state = newState;
+
+  if (m_state == State::Finished) {
+    m_registry->releaseFinishedLoader(this);
+    m_client->notifyNewSingleModuleFinished(m_moduleScript);
+  }
+}
+
+void ModuleScriptLoader::fetch(const ModuleScriptFetchRequest& moduleRequest,

[hiroshige, 2017/02/20 23:15:32]

Could you add a comment with the link to the spec corresponding to the steps
below?
(e.g. Line 69 says "Step 4", but Step 4 of what?)

[kouhei (in TOK), 2017/02/21 01:26:51]

Done.

+                               ResourceFetcher* fetcher,
+                               ModuleGraphLevel level) {
+  // Step 4. Set moduleMap[url] to "fetching".
+  advanceState(State::Fetching);
+
+  // Step 5. Let request be a new request whose url is url, ...
+  ResourceRequest resourceRequest(moduleRequest.url());
+#ifndef NDEBUG
+  m_url = moduleRequest.url();
+#endif
+
+  // TODO(kouhei): handle "destination is destination,"
+  // ... type is "script", -> FetchResourceType is specified by
+  // ScriptResource::fetch mode is "cors", ...

[hiroshige, 2017/02/20 23:15:32]

Perhaps we need a line break or something between "fetch" and "mode"?

[kouhei (in TOK), 2017/02/21 01:26:51]

Done.

+  resourceRequest.setFetchRequestMode(WebURLRequest::FetchRequestModeCORS);

[yhirano, 2017/02/17 04:59:15]

+tyoshino@:

You may need to call FetchRequest::setCrossOriginAccessControl.

[kouhei (in TOK), 2017/02/21 01:26:51]

Done, but I'm not confident here.

[tyoshino (SeeGerritForStatus), 2017/02/21 03:47:17]

Yes. setCrossOriginAccessControl() is needed.

Now, in the latest patchset you have it, and then this setFetchRequestMode()
call and the following setFetchCredentialsMode() call will be overridden by
etCrossOriginAccessControl() below. For now, please remove these and just depend
on setCrossOriginAccessControl().

Maybe in some of the design docs, I suggested that the flag we should use is
FetchCredentialsMode. That's still true, but currently the loading logic doesn't
work without setAllowStoredCredentials() called appropriately.

[kouhei (in TOK), 2017/02/21 05:52:28]

Done.

+  // ... credentials mode is credentials mode, ...
+  resourceRequest.setFetchCredentialsMode(moduleRequest.credentialsMode());
+  // parser metadata is parser state,
+  ResourceLoaderOptions options;
+  options.parserDisposition = moduleRequest.parserState();
+  // referrer is referrer,
+  if (!moduleRequest.referrer().isNull()) {
+    resourceRequest.setHTTPReferrer(SecurityPolicy::generateReferrer(
+        m_modulator->referrerPolicy(), moduleRequest.url(),
+        moduleRequest.referrer()));
+  }
+  // and client is fetch client settings object. -> set by ResourceFetcher
+
+  // TODO(kouhei): what initiatorName should we set here? should we set

[hiroshige, 2017/02/20 23:15:32]

ScriptLoader sets the initiator to |m_element->localName()| for classic scripts,
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/d...
but I'm not sure whether this is really correct.

[kouhei (in TOK), 2017/02/21 01:26:50]

I'm not sure here too. If we are to align to the spec, we should set "" as
initiatorName:
https://fetch.spec.whatwg.org/#concept-request-initiator

However, we have a bunch of non-standard initiatorNames already:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/loade...

Inspector code seem to refer to some:
https://cs.chromium.org/search/?q=file:Inspector+initiatortype&type=cs

l闇tm. Changed to "" for now.

+  // "module"?
+  // ... cryptographic nonce metadata is cryptographic nonce, ...
+  FetchRequest fetchRequest(resourceRequest, "module", options);
+  fetchRequest.setContentSecurityPolicyNonce(moduleRequest.nonce());
+
+  // Module scripts are always async.
+  fetchRequest.setDefer(FetchRequest::LazyLoad);
+
+  // Step 6. If the caller specified custom steps to perform the fetch,
+  // perform them on request, setting the is top-level flag if the top-level
+  // module fetch flag is set. Return from this algorithm, and when the custom
+  // perform the fetch steps complete with response response, run the remaining
+  // steps.
+  // Otherwise, fetch request. Return from this algorithm, and run the remaining
+  // steps as part of the fetch's process response for the response response.
+  // TODO(ServiceWorker team): Perform the "custom steps" for module usage
+  // inside service worker.
+  (void)level;
+  // Otherwise, fetch request. Return from this algorithm, and run the remaining

[hiroshige, 2017/02/20 23:15:32]

Lines 109-110 and 114-115 are duplicated.

[kouhei (in TOK), 2017/02/21 01:26:50]

Done.

+  // steps as part of the fetch's process response for the response response.
+  // Note: response is always CORS-same-origin.
+  ScriptResource* resource = ScriptResource::fetch(fetchRequest, fetcher);

[yhirano, 2017/02/17 04:59:15]

DCHECK(resource)?

[kouhei (in TOK), 2017/02/21 01:26:51]

Done.

[hiroshige, 2017/02/21 23:50:01]

ScriptResource::fetch() can be return nullptr in general, and I doubt we can
assume |resource| always non-null.

[kouhei (in TOK), 2017/02/22 03:52:29]

You're right. We are only checking synchronous case.
I'll follow-up with async loading unit test.

[yhirano, 2017/02/22 08:02:57]

I'm not following. It sounds we need to care about the null case but there is no
code for the case.

[kouhei (in TOK), 2017/02/23 03:49:26]

Added test for this case/

+  setResource(resource);

[hiroshige, 2017/02/20 23:15:32]

notifyFinished() might be called synchronously here, when the request hits the
MemoryCache?

I think it would be good to make this setResource() always async
(not sure what is the best way to do that though), to match with the spec.

[kouhei (in TOK), 2017/02/21 01:26:50]

I think this is fine for the current ModuleScriptLoader clients (ModuleMap),
since that is triggering notifications async:
https://codereview.chromium.org/2555653002/diff/620001/third_party/WebKit/Sou...

[hiroshige, 2017/02/21 23:50:01]

Hmm, then we have to set |m_nonce| and |m_parserState| before setResource(),
because ModuleScriptLoader::notifyFinished() (which is still called
synchronously) references these members.

Also, could you add comments that states ModuleScriptLoader::fetch() can notify
ModuleScriptLoaderClient synchronously.
(And also "We can/should make it async"?)

[kouhei (in TOK), 2017/02/22 03:52:29]

Done.

[yhirano, 2017/02/22 08:02:57]

In such a case, I think we should not run |setResource(resource)|.

[kouhei (in TOK), 2017/02/23 03:49:26]

Done.

+
+  m_nonce = moduleRequest.nonce();
+  m_parserState = moduleRequest.parserState();
+  m_credentialsMode = moduleRequest.credentialsMode();
+}
+
+bool ModuleScriptLoader::wasModuleLoadSuccessful(Resource* resource) {
+  // Implements conditions in Step 7 of
+  // https://html.spec.whatwg.org/#fetch-a-single-module-script
+
+  // - response's type is "error"
+  if (resource->errorOccurred()) {
+    return false;
+  }
+
+  const auto& response = resource->response();
+  // - response's status is not an ok status
+  if (response.isHTTP() &&
+      (response.httpStatusCode() < 200 || response.httpStatusCode() >= 300)) {
+    return false;
+  }
+
+  // The result of extracting a MIME type from response's header list
+  // (ignoring parameters) is not a JavaScript MIME type
+  // Note: For historical reasons, fetching a classic script does not include
+  // MIME type checking. In contrast, module scripts will fail to load if they

[hiroshige, 2017/02/20 23:15:32]

Could you clarify that this refers to "response's" MIME type checking?
Because we check <script>'s language/type attributes (i.e. 
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/d...),
which also contains MIME type checking.

[kouhei (in TOK), 2017/02/21 01:26:51]

This is a copy of spec text per
https://html.spec.whatwg.org/#fetch-a-single-module-script Step 7.

[hiroshige, 2017/02/21 23:50:01]

Acknowledged.

+  // are not of a correct MIME type.
+  if (!MIMETypeRegistry::isSupportedJavaScriptMIMEType(response.mimeType()))
+    return false;
+
+  return true;
+}
+
+// ScriptResourceClient callback handler
+void ModuleScriptLoader::notifyFinished(Resource*) {
+  // Note: "conditions" referred in Step 7 is implemented in
+  // wasModuleLoadSuccessful().
+  // Step 7. If any of the following conditions are met, set moduleMap[url] to
+  // null, asynchronously complete this algorithm with null, and abort these
+  // steps.
+  if (!wasModuleLoadSuccessful(resource())) {
+    advanceState(State::Finished);
+    setResource(nullptr);

[yhirano, 2017/02/17 04:59:15]

Is it preferable to move this statement into advanceState?

[kouhei (in TOK), 2017/02/21 01:26:50]

Done.

+    return;
+  }
+
+  // Step 8. Let source text be the result of UTF-8 decoding response's body.
+  String sourceText = resource()->script();
+
+  // Step 9. Let module script be the result of creating a module script given
+  // source text, module map settings object, response's url, cryptographic
+  // nonce, parser state, and credentials mode.
+  m_moduleScript =
+      createModuleScript(sourceText, resource()->response().url(), m_modulator,
+                         m_nonce, m_parserState, m_credentialsMode);

[yhirano, 2017/02/17 04:59:15]

Can we use resource->resourceRequest().fetchCredentialsMode()?

[kouhei (in TOK), 2017/02/21 01:26:50]

Done.

+
+  advanceState(State::Finished);
+  setResource(nullptr);
+}
+
+// https://html.spec.whatwg.org/#creating-a-module-script
+ModuleScript* ModuleScriptLoader::createModuleScript(
+    const String& sourceText,
+    const KURL& url,
+    Modulator* modulator,
+    const String& nonce,
+    ParserDisposition parserState,
+    WebURLRequest::FetchCredentialsMode credentialsMode) {
+  // Step 1. Let script be a new module script that this algorithm will
+  // subsequently initialize.
+  // Step 2. Set script's settings object to the environment settings object
+  // provided.
+  // Note: "script's settings object" will be "modulator".
+
+  // Delegate to Modulator::compileModule to process Steps 3-6.
+  ScriptModule record = modulator->compileModule(sourceText, url.getString());

[hiroshige, 2017/02/20 23:15:32]

nit: should this local variable's name |result| instead of |record| to match
with the spec?

[kouhei (in TOK), 2017/02/21 01:26:50]

Done.

+  if (record.isNull())
+    return nullptr;

[hiroshige, 2017/02/20 23:15:32]

Isn't this a part of Step 6: "...return null, and abort these steps."?

[kouhei (in TOK), 2017/02/21 01:26:51]

Done.

+  // Step 7. Set script's module record to result.
+  // Step 8. Set script's base URL to the script base URL provided.
+  // Step 9. Set script's cryptographic nonce to the cryptographic nonce
+  // provided.
+  // Step 10. Set script's parser state to the parser state.
+  // Step 11. Set script's credentials mode to the credentials mode provided.
+  // Step 12. Return script.
+  return ModuleScript::create(record, url, nonce, parserState, credentialsMode);
+}
+
+DEFINE_TRACE(ModuleScriptLoader) {
+  visitor->trace(m_modulator);
+  visitor->trace(m_moduleScript);
+  visitor->trace(m_registry);
+  visitor->trace(m_client);
+  ResourceOwner<ScriptResource>::trace(visitor);
+}
+
+}  // namespace blink