Re: [w3c/ServiceWorker] Improve handling of extend lifetime promises (#1049)

annevk requested changes on this pull request.



> @@ -1289,7 +1289,9 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
 
     An {{ExtendableEvent}} object has an associated <dfn for="ExtendableEvent">extend lifetime promises</dfn> (an array of <a>promises</a>). It is initially an empty array.
 
-    An {{ExtendableEvent}} object has an associated <dfn id="extensions-allowed-flag">extensions allowed flag</dfn>. It is initially set.
+    An {{ExtendableEvent}} object has an associated <dfn for="ExtendableEvent" id="extensions-allowed-flag">extensions allowed flag</dfn>. It is initially set.

It would be good to invert this somehow. Flags and booleans that default to false are much more intuitive.

>  
+          Note: If no lifetime extension promise has been added up to this point (i.e., at the end of the task that called the event handlers), the [=ExtendableEvent/extensions allowed flag=] is immediately unset. Calling {{ExtendableEvent/waitUntil()}} in subsequent asynchronous tasks will throw.

Why do you need both the flag and the count then?

>        The user agent *should not* <a lt="terminate service worker">terminate</a> the [=/service worker=] associated with |event|'s <a>relevant settings object</a>'s [=environment settings object/global object=] until |event|'s <a>extensions allowed flag</a> is unset. However, the user agent *may* impose a time limit to this lifetime extension.
   </section>
 
   <section algorithm>
+    <h3 id="handle-extend-lifetime-promise-algorithm"><dfn>Handle Extend Lifetime Promise</dfn></h3>
+
+      : Input
+      :: |event|, an {{ExtendableEvent}} object
+      :: |promise|, a [=promise=]
+      : Output
+      :: None
+
+      1. Wait until |promise| is settled [=in parallel=].
+      2. [=Queue a microtask=] to run the following substeps:

You cannot queue microtasks from something that runs in parallel. Needs to be a regular task.

>        The user agent *should not* <a lt="terminate service worker">terminate</a> the [=/service worker=] associated with |event|'s <a>relevant settings object</a>'s [=environment settings object/global object=] until |event|'s <a>extensions allowed flag</a> is unset. However, the user agent *may* impose a time limit to this lifetime extension.
   </section>
 
   <section algorithm>
+    <h3 id="handle-extend-lifetime-promise-algorithm"><dfn>Handle Extend Lifetime Promise</dfn></h3>
+
+      : Input
+      :: |event|, an {{ExtendableEvent}} object
+      :: |promise|, a [=promise=]
+      : Output
+      :: None
+
+      1. Wait until |promise| is settled [=in parallel=].
+      2. [=Queue a microtask=] to run the following substeps:
+          1. Decrease |event|'s [=ExtendableEvent/pending promises count=] by one.
+          1. For each |reaction| in |promise|.\[[PromiseFulfillReactions]]:

This seems extremely hacky and probably wrong. I don't think we should be poking at the guts of promises.

-- 
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/1049#pullrequestreview-15296014