Check for Run Service Worker failure and disallow starting redundant workers. #1419

mfalken · 2019-06-06T08:46:50Z

This cleans up Run Service Worker to block until the worker is running or fails to start. If running, the Completion record is stashed in a new variable. Callsites are updated to check if the worker is running before proceeding. It also disallows starting a redundant worker. postMessage is dropped silently if starting the service worker failed.

Addresses #1146 and #1403.

Preview | Diff

mfalken · 2019-06-06T08:47:10Z

There were too many conflicts trying to patch to v1 so I'll give that a go later.

mfalken · 2019-06-07T04:15:51Z

Hm, I think it doesn't actually work to make Run Service Worker blocking because then Terminate Service Worker can never be entered while the worker is starting up.

It seems we have to make everything asynchronous or hand-wave that the user agent can abort starting the worker at any time. Or maybe keep it non-blocking and do something like:

Run the "Run Service Worker" algorithm.
In parallel:
1. Wait for service worker to be running or to have been aborted by the Terminate Service Worker algorithm.
2. If service worker is not running, return.
3. ... continue with dispatching the event etc.

mfalken · 2019-06-07T04:22:28Z

We also need to make Run Service Worker re-entrant :(

mfalken · 2019-06-07T05:16:09Z

Ah I may have fallen into a trap. The Terminate Service Worker algorithm probably should be specified to "run the following steps in parallel with the worker's main loop" so it runs on the worker's loop not the main algorithm loop that Run Service Worker is invoked on, and can be entered at any time.

mfalken · 2019-06-07T05:55:14Z

OK revised.

jakearchibald · 2019-06-07T06:10:15Z

https://infra.spec.whatwg.org/#algorithm-conditional-abort might be handy if you need to abort steps.

jakearchibald · 2019-06-07T10:01:28Z

docs/index.bs

+      <div class="note">
+        Note: This algorithm typically blocks until the service worker is [=running=]. Most callsites can check for success or failure by checking if the service worker is [=running=] after this algorithm returns.
+
+        This specification generally treats an [=abrupt completion=] due to an uncaught exception the same as a normal completion. That is, if the worker throws an exception during initial script evaluation, it is still considering running and can receive events. Some callsites distinguish between these scenarios by examining the [=start status=] of the [=running=] worker.


Would it be cleaner to return failure or the start status from this algorithm?

That means callers could use

If the result of running [=Run Service Worker=] with |someServiceWorker| is failure, then return.

Or:

If the result of running [=Run Service Worker=] with |someServiceWorker| is failure or an [=abrupt completion=], then….

Oh yes that would be much simpler... I'll do that.

jakearchibald · 2019-06-07T10:14:22Z

docs/index.bs

-      1. If |serviceWorker| is already running, this algorithm must have been invoked previously. If |callbackSteps| is provided, run them with the same value as the previous time, and abort these steps.
-      1. Create a separate parallel execution environment (i.e. a separate thread or process or equivalent construct), and run the following substeps in that context:
+      1. Let |startFailed| be false.
+      1. Create a separate parallel execution environment (i.e. a separate thread or process or equivalent construct), and run the following substeps in that context [=in parallel=]:


I'm not sure we need [=in parallel=] here. The steps are already running in "that context" which is a "separate parallel execution environment". Otherwise it could read like you're running steps in that context, then in parallel, which would take you off the JavaScript thread for that context.

May as well replace "substeps" with "steps".

Agreed, it's redundant, I remember thinking about this and somehow decided to put it in. I'll remove.

jakearchibald · 2019-06-07T10:17:05Z

docs/index.bs

-
-              Note: In addition to the usual possibilities of returning a value or failing due to an exception, this could be prematurely aborted by the <a>terminate service worker</a> algorithm.
-          1. If |callbackSteps| is provided, run them with |evaluationStatus| on the original thread that invoked this algorithm, while continuing in parallel with these steps.
+          1. If |evaluationStatus|.\[[Value]] is empty, this means the script was not evaluated. Set |startFailed| to true and abort the rest of these steps.


I think "abort these steps" is fine.

jakearchibald · 2019-06-07T10:19:03Z

docs/index.bs

@@ -2780,14 +2797,16 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
      :: None

      1. If |serviceWorker| is not running, abort these steps.


Link to your "running" definition?

I wasn't sure what to do here. My definition doesn't work because it will abort the steps if the worker is starting up and hasn't started its event loop. On the other hand, the "running" used in the existing spec isn't defined. I wonder if we can just delete this step? The HTML equivalent doesn't have it:
https://html.spec.whatwg.org/multipage/workers.html#terminate-a-worker

Or else say something like, "If the steps of the "Create a separate parallel execution environment" step in the Run Service Worker algorithm are not being executed, abort these steps". ?

Deleting the step seems best I think

jakearchibald · 2019-06-07T10:24:09Z

docs/index.bs

@@ -2857,8 +2876,9 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
          1. If |shouldSoftUpdate| is true, then [=in parallel=] run the [=Soft Update=] algorithm with |registration|.
          1. Return null.
      1. If |activeWorker|'s <a>state</a> is *activating*, wait for |activeWorker|'s <a>state</a> to become *activated*.
-      1. Invoke <a>Run Service Worker</a> algorithm with |activeWorker| as the argument.
-      1. <a>Queue a task</a> |task| to run the following substeps:
+      1. Run the [=Run Service Worker=] algorithm with |activeWorker|.


Do you prefer "Run the" to "Invoke the"? Happy to go with "Run the".

Ah, meant to bring this up earlier. There's some inconsistency around "Invoke" vs "Run" and also "with xx, yy, and zz" vs "passing xx, yy, and zz as arguments" and other variations. I planned to just always go with the shortest which is "Run the AA algorithm with xx, yy, and zz." But not sure if there is a style guideline here.

I guess with Run in the name of algorithm, it looks a bit awkward.

It probably doesn't matter much. In other specs I avoid it by making the link text an action, like "Let foo be the result of getting a service worker object…". But I guess we should keep stuff in the algorithm section more like a function.

"Run" or "invoke" seems fine here. No point trying to solve the inconsistencies as part of this PR.

jakearchibald · 2019-06-07T10:26:59Z

docs/index.bs

+      1. Run the [=Run Service Worker=] algorithm with |activeWorker|.
+      1. If |activeWorker| is not [=running=], then:
+          1. If |registration| is [=stale=], then [=in parallel=] run the [=Soft Update=] algorithm with |registration|.
+          2. Return.


Ohh good catch!

jakearchibald

Couple of nits/questions, but otherwise lgtm!

mfalken · 2019-06-07T16:08:06Z

Thanks, addressed all the comments I think. I'll look at backporting to v1 next time.

mfalken · 2019-06-10T01:25:09Z

Hi Jake, I made a couple changes since last time:

For postMessage, moved getting incumbentSettings and serializing (which possibly rethrows the exception) out of the parallel steps, since I don't think you can do those in parallel.
Backported to v1 which needed some work since the "callbackSteps" part of Run Service Worker which this replaces was never in v1 to begin with.

I don't see how to compile v1 so I'm not sure it works, but looking at https://www.w3.org/TR/service-workers-1 it hasn't been built since 2017 (?).

I'll go ahead and merge this but if you see something wrong we can fix it.

mfalken requested a review from jakearchibald June 6, 2019 08:46

jakearchibald reviewed Jun 7, 2019

View reviewed changes

jakearchibald approved these changes Jun 7, 2019

View reviewed changes

mfalken force-pushed the postmessage branch from f3ad407 to 3bfaf31 Compare June 7, 2019 15:36

mfalken and others added 4 commits June 10, 2019 09:32

run service worker

679fac5

fixes

61c0917

comments

26c9804

fix bad rebase

dae39ed

mfalken force-pushed the postmessage branch 2 times, most recently from 50f3a54 to dae39ed Compare June 10, 2019 00:52

mfalken added 3 commits June 10, 2019 09:58

can't throw in parallel

382d46c

port to v1

66f7452

note

f08c1aa

small fix

b729b47

mfalken merged commit 5059261 into w3c:master Jun 10, 2019

This was referenced Jun 10, 2019

"Unregister" operation may be interrupted by ServiceWorker#postMessage #1146

Closed

Missing task queuing #1403

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for Run Service Worker failure and disallow starting redundant workers. #1419

Check for Run Service Worker failure and disallow starting redundant workers. #1419

mfalken commented Jun 6, 2019 •

edited by pr-preview bot

Loading

mfalken commented Jun 6, 2019

mfalken commented Jun 7, 2019

mfalken commented Jun 7, 2019

mfalken commented Jun 7, 2019

mfalken commented Jun 7, 2019

jakearchibald commented Jun 7, 2019

jakearchibald Jun 7, 2019 •

edited

Loading

mfalken Jun 7, 2019

jakearchibald Jun 7, 2019

jakearchibald Jun 7, 2019

mfalken Jun 7, 2019

jakearchibald Jun 7, 2019

jakearchibald Jun 7, 2019

mfalken Jun 7, 2019

jakearchibald Jun 7, 2019

jakearchibald Jun 7, 2019

mfalken Jun 7, 2019

jakearchibald Jun 7, 2019 •

edited

Loading

jakearchibald Jun 7, 2019

jakearchibald left a comment

mfalken commented Jun 7, 2019

mfalken commented Jun 10, 2019

		@@ -2780,14 +2797,16 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
		:: None

		1. If \|serviceWorker\| is not running, abort these steps.

Check for Run Service Worker failure and disallow starting redundant workers. #1419

Check for Run Service Worker failure and disallow starting redundant workers. #1419

Conversation

mfalken commented Jun 6, 2019 • edited by pr-preview bot Loading

mfalken commented Jun 6, 2019

mfalken commented Jun 7, 2019

mfalken commented Jun 7, 2019

mfalken commented Jun 7, 2019

mfalken commented Jun 7, 2019

jakearchibald commented Jun 7, 2019

jakearchibald Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakearchibald Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakearchibald left a comment

Choose a reason for hiding this comment

mfalken commented Jun 7, 2019

mfalken commented Jun 10, 2019

mfalken commented Jun 6, 2019 •

edited by pr-preview bot

Loading

jakearchibald Jun 7, 2019 •

edited

Loading

jakearchibald Jun 7, 2019 •

edited

Loading