Skip to content

Handle asynchronous modules (top-level await). #1444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 11, 2021
Merged

Handle asynchronous modules (top-level await). #1444

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
12fbfaf
Handle asynchronous modules (top-level await).
Ms2ger Jun 24, 2019
d4db15b
Tentative fix
Ms2ger May 25, 2021
51e01b8
fixup! Tentative fix
Ms2ger May 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 51 additions & 9 deletions docs/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ spec: push; urlPrefix: https://w3c.github.io/push-api/
text: push; url: h-the-push-event

spec: ecma-262; urlPrefix: http://tc39.github.io/ecma262/
type: abstract-op
text: NormalCompletion; url: sec-normalcompletion
type: dfn
text: Assert; url: sec-algorithm-conventions
text: [[Call]]; url: sec-ecmascript-function-objects-call-thisargument-argumentslist
Expand All @@ -64,6 +66,8 @@ spec: ecma-262; urlPrefix: http://tc39.github.io/ecma262/
text: execution context; url: sec-execution-contexts
text: abrupt completion; url: sec-completion-record-specification-type
text: Completion; url: sec-completion-record-specification-type
text: Module Record; url: sec-abstract-module-records
text: Cyclic Module Record; url: sec-cyclic-module-records

spec: page-visibility; urlPrefix: https://www.w3.org/TR/page-visibility/
type: enum; text: VisibilityState; url: VisibilityState
Expand All @@ -83,6 +87,9 @@ spec: html; urlPrefix: https://html.spec.whatwg.org/multipage/
text: delay the load event; for: document; url: delay-the-load-event
urlPrefix: origin.html
text: creating a policy container from a fetch response
urlPrefix: webappapis.html
text: module map; url: module-map
text: resolve a module specifier; url: resolve-a-module-specifier

spec: fetch; urlPrefix: https://fetch.spec.whatwg.org/
type: dfn
Expand Down Expand Up @@ -2710,16 +2717,15 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
Note: The control does not break the loop in this step to continue with all the imported scripts to populate the cache.
1. Asynchronously complete these steps with |response|.

If the algorithm asynchronously completes with null, then:
When the algorithm asynchronously completes, continue the rest of these steps, with |script| being the asynchronous completion value.

1. Invoke [=Reject Job Promise=] with |job| and `TypeError`.

Note: This will do nothing if [=Reject Job Promise=] was previously invoked with "{{SecurityError}}" {{DOMException}}.
1. If |script| is null or [=Is Async Module=] with |script|'s [=script/record=], |script|'s [=script/base URL=], and « » is true, then:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A random idea: How about checking whether |evaluationPromise|.\[[PromiseState]] is "pending" instead of [=Is Async Module=] ... is true?

This makes the logic a little simpler (using only |evaluationPromise|.\[[PromiseState]], removing module graph traversal and module inspection), but this is not equivalent from the current condition (e.g. for modules with await undefined).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A goal of the design here was to not be "racy" in the sense that, both asynchronous behavior and errors are based on the module syntax, not whether it has already run or not, or whether it dynamically reached an await. This would make behavior more stable.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it makes sense > based on syntax.

1. Invoke [=Reject Job Promise=] with |job| and `TypeError`.

1. If |newestWorker| is null, then [=map/remove=] [=scope to registration map=][|scopeURL|, [=URL serializer|serialized=]].
1. Invoke <a>Finish Job</a> with |job| and abort these steps.
Note: This will do nothing if [=Reject Job Promise=] was previously invoked with "{{SecurityError}}" {{DOMException}}.

Else, continue the rest of these steps after the algorithm's asynchronous completion, with |script| being the asynchronous completion value.
1. If |newestWorker| is null, then [=map/remove=] [=scope to registration map=][|scopeURL|, [=URL serializer|serialized=]].
1. Invoke [=Finish Job=] with |job| and abort these steps.
1. If |hasUpdatedResources| is false, then:
1. Set |registration|'s [=service worker registration/update via cache mode=] to |job|'s [=job/update via cache mode=].
1. Invoke [=Resolve Job Promise=] with |job| and |registration|.
Expand Down Expand Up @@ -2940,8 +2946,17 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Create a new {{WorkerLocation}} object and associate it with |workerGlobalScope|.
1. If the <a>run CSP initialization for a global object</a> algorithm returns "<code>Blocked</code>" when executed upon |workerGlobalScope|, set |startFailed| to true and abort these steps.
1. If |serviceWorker| is an <a>active worker</a>, and there are any <a>tasks</a> queued in |serviceWorker|'s <a>containing service worker registration</a>'s [=service worker registration/task queues=], <a lt="queue a task">queue</a> them to |serviceWorker|'s <a>event loop</a>'s [=/task queues=] in the same order using their original <a>task sources</a>.
1. Let |evaluationStatus| be the result of <a lt="run a classic script">running the classic script</a> |script| if |script| is a <a>classic script</a>, otherwise, the result of <a lt="run a module script">running the module script</a> |script| if |script| is a [=module script=].
1. If |evaluationStatus|.\[[Value]] is empty, this means the script was not evaluated. Set |startFailed| to true and abort these steps.
1. Let |evaluationStatus| be null.
1. If |script| is a [=classic script=], then:
1. Set |evaluationStatus| to the result of [=run a classic script|running the classic script=] |script|.
1. If |evaluationStatus|.\[[Value]] is empty, this means the script was not evaluated. Set |startFailed| to true and abort these steps.
1. Otherwise, if |script| is a [=module script=], then:
1. Let |evaluationPromise| be the result of [=run a module script|running the module script=] |script|, with report errors set to false.
1. Assert: |evaluationPromise|.\[[PromiseState]] is not "pending".
1. If |evaluationPromise|.\[[PromiseState]] is "rejected":
1. Set |evaluationStatus| to [$ThrowCompletion$](|evaluationPromise|.\[[PromiseResult]]).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ThrowCompletion() does not become a link in the Preview. Is it possible to fix it?

1. Otherwise:
1. Set |evaluationStatus| to [$NormalCompletion$](undefined).
1. If the script was aborted by the [=Terminate Service Worker=] algorithm, set |startFailed| to true and abort these steps.
1. Set |serviceWorker|'s [=start status=] to |evaluationStatus|.
1. If |script|'s <a>has ever been evaluated flag</a> is unset, then:
Expand Down Expand Up @@ -3603,6 +3618,33 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

Note: When an exception is [=throw|thrown=], the implementation does undo (roll back) any changes made to the cache storage during the batch operation job.
</section>

<section algorithm>
<h3 id="is-async-module-algorithm"><dfn>Is Async Module</dfn></h3>

: Input
:: |record|, a [=Module Record=]
:: |moduleMap|, a [=/module map=]
:: |base|, a [=/URL=]
:: |seen|, a [=/set=] of [=/URLs=]
: Output
:: a boolean

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If |record| is not a [=Cyclic Module Record=]

Does this include the case of |record| == null?

1. If |record| is not a [=Cyclic Module Record=], then:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling to figure out what a "Cyclic Module Record" is. What's the difference between a Cyclic Module Record and a non-Cyclic Module Record? How does each relate to a module?

Copy link

@littledan littledan Jan 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JavaScript modules (Source Text Module Records) had a bunch of their functionality for traversing the module graph refactored up into a spec-internal superclass called Cyclic Module Records, so that WebAssembly modules could share the logic. Not all modules use this infrastructure, for example, JSON modules are leaves so they don't bother.

1. Return false.
1. If |record|.\[[Async]] is true, then:
1. Return true.
1. [=list/For each=] string |requested| of |record|.\[[RequestedModules]]:
1. Let |url| be the result of [=resolve a module specifier|resolving a module specifier=] given |base| and |requested|.
1. Assert: |url| is never failure, because [=resolve a module specifier|resolving a module specifier=] must have been previously successful with these same two arguments.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During implementation in Chrome we realized that linking failures means this assert can trip. If the module graph couldn't be successfully linked, we can't assume the entire module graph is accessible this way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really familiar with the module loading stuff. Any idea what spec change is needed here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't grok the module loading stuff enough myself to be confident with any suggested fix. I don't think resolving a module specifier itself can fail, but that a module graph could have failed to Link(). In which case a requested module's url may not map to any module record in the module map.

Is there an observable error timing currently for a "SW has a TLA" error vs a linking error? My in-flight CL in Chrome chooses to only check if a module graph has any TLAs in it if it successfully linked. If it failed to link, it doesn't check if it's async at all and let the linking error propagate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While reviewing this I was not sure if this comment was resolved. If I understand correctly, per the next comment thread, the assert failure was fixed in a later commit to this PR that added the following:

If |moduleMap|[|url|] does not have a [=script/record=], then return false.

1. If |seen| does not [=set/contain=] |url|, then:
1. [=set/Append=] |url| to |seen|.
1. If |moduleMap|[|url|] does not have a [=script/record=], then:
1. Return false.
1. If [=Is Async Module=] for |moduleMap|[|url|]'s [=script/record=], |moduleMap|, |base|, and |seen| is true, then:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec bug I was pointing out in #1444 (comment) is here, moduleMap[url] might not have a module record in case of link failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to fix this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we also want to bring the assertion back: Ms2ger@6290a2c

1. Return true.
1. Return false.
</section>
</section>

<section>
Expand Down