Re: [w3c/ServiceWorker] Handle asynchronous modules (top-level await). (#1444)

@mfalken commented on this pull request.

Sorry for the delay! I needed some time to understand this.

Can the commit description elaborate more on what the change is (the bug thread is very long). It sounds like this is implementing https://github.com/w3c/ServiceWorker/issues/1407#issuecomment-531673790, is that right?


> @@ -2873,8 +2879,19 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
           1. Set |workerGlobalScope|'s [=ServiceWorkerGlobalScope/force bypass cache for import scripts flag=] if |forceBypassCache| is true.
           1. Create a new {{WorkerLocation}} object and associate it with |workerGlobalScope|.
           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 |startFailed| to true and abort these steps.
+
+                      Note: This is intended to maintain compatibilty with failing synchronous modules before the introduction of asynchronous modules.

I don't understand this point, and it seems to be opposite to what @hiroshige-g is saying. Does throwing an error in the synchronous module case cause |startFailed| to be true?

(If we keep the note, there's a typo "compatibilty".)

> +  <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
+
+      1. If |record| is not a [=Cyclic Module Record=], then:
+          1. Return false.
+      1. If |record|.\[[Async]] is true, then:
+          1. Return true.
+      1. [=list/For each=] string |requested| of |record|.\[[RequestedModules]],

nit: use a colon instead of a comma at the end for consistency

> +      :: |moduleMap|, a [=/module map=]
+      :: |base|, a [=/URL=]
+      :: |seen|, a [=/set=] of [=/URLs=]
+      : Output
+      :: a boolean
+
+      1. If |record| is not a [=Cyclic Module Record=], then:
+          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.
+          1. If |seen| does not [=set/contain=] |url|, then:
+              1. [=set/Append=] |url| to |seen|.
+              1. If [=Is Async Module=] for |moduleMap|[|url|]'s [=script/record=], |moduleMap|, |base|, and |seen| is true, then

nit: colon at end

> @@ -2873,8 +2879,19 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
           1. Set |workerGlobalScope|'s [=ServiceWorkerGlobalScope/force bypass cache for import scripts flag=] if |forceBypassCache| is true.
           1. Create a new {{WorkerLocation}} object and associate it with |workerGlobalScope|.
           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 <a>classic script</a>, then:
+              1. Set |evaluationStatus| to the result of <a lt="run a classic script">running the classic script</a> |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 <a lt="run a module script">running the module script</a> |script|, with report errors set to false.
+              1. Assert: |evaluationPromise|.\[[PromiseState]] is not "pending".
+              1. If |evaluationPromise|.\[[PromiseState]] is "rejected":
+                  1. Set |startFailed| to true and abort these steps.

I agree with @hiroshige-g.  This is subtle, but throwing an error during service worker script evaluation doesn't count as a startup failure in the current spec. The service worker thread is running and will get events dispatched to it. Throwing an error only affects the Install algorithm; the service worker will subsequently be terminated and will not be installed. If we want to change this behavior, we should do so for classic scripts as well as module scripts.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/pull/1444#pullrequestreview-573968941