Skip to content
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

Clients.get: block on reserved clients. #1315

Closed
wants to merge 13 commits into from
Closed

Clients.get: block on reserved clients. #1315

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1f77b03
Building Spec: 9a6b738771d1e3d235b539bbdc896e88803573e8
mfalken May 29, 2018
95f857f
Clients.get: block on reserved clients.
mfalken May 29, 2018
af0da60
Rebuilding spec (#1314)
mfalken May 29, 2018
6477cd1
Building Spec: af0da60935ffd5e80db388709de0ffa157431ed3
May 29, 2018
46d6b65
Clients.get: block on reserved clients.
mfalken May 29, 2018
59374f0
ok
mfalken May 30, 2018
29c9a2e
ok
mfalken May 30, 2018
e90c0d8
Merge branch 'clients-get' of https://github.com/mattto/ServiceWorker…
mfalken May 30, 2018
0d5ba24
Change the affiliation of Jungkee Song
jungkees May 31, 2018
3a8e09c
Building Spec: e616460b2fae8a4d589de9a099a8c6dd739d89fe
May 31, 2018
ad1b2b3
Change affiliation of Jungkee Song in Nightly
jungkees May 31, 2018
7e994b5
Restore the reference to response's cache state
jungkees Jun 1, 2018
08ad67e
rebase-compile
mfalken Jun 7, 2018
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
5 changes: 5 additions & 0 deletions docs/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,11 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Run these substeps <a>in parallel</a>:
1. For each [=/service worker client=] |client| whose [=service worker client/origin=] is the <a lt="same origin">same</a> as the associated [=ServiceWorkerGlobalScope/service worker=]'s [=environment settings object/origin=]:
1. If |client|'s [=environment/id=] is not |id|, continue to the next iteration of the loop.
1. If |client|'s [=environment/execution ready flag=] is unset, then:
1. Wait for the [=/HTTP fetch=] steps using the [=/request=] that |client| is the associated [=request/reserved client=] of to finish.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a bit hand-wavey, since there isn't an explicit connection between a client & it's fetch.

Adding this connection would likely be really hard, since fetch doesn't really return an instance (this has been a problem elsewhere).

Feels like it would be easier to turn the execution ready flag into an enum, eg execution state, which could be "pending", "ready" or "failed". Then, this part of the spec would be explicit, as you'd be waiting for that enum to change to "failed" (in which case you'd reject and abort) or "ready" (in which case you'd continue).

Copy link
Member

Choose a reason for hiding this comment

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

I agree that waiting for a state change is probably better. At least, it matches what I've anticipated implementing.

Note, I'm not sure we need a tri-state enum, though. I think we can just get by with an execution ready boolean. If the browser fails to mark a client as execution ready I believe it should simply destroy the client fairly quickly. So we can just listen for "client goes away before being marked execution ready".

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh, if we have a standard way of describing "goes away", I agree that would fit nicely.

Copy link
Member

Choose a reason for hiding this comment

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

Its probably not clearly defined for windows in the html spec. I think defining when to set this error state would be equivalent to defining discarding the client on failure, though.

Copy link
Member

Choose a reason for hiding this comment

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

Note, we also already depend on client discarding in other places in the service worker spec. We activate waiting workers when the last client "goes away". If that last controlled client is a reserved client being navigated, then we have this same situation.

Not saying we shouldn't spec it better, but just highlighting that we already depend on this concept pretty heavily.

Copy link
Member Author

Choose a reason for hiding this comment

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

Strangely, "terminating" links to "service worker termination", which doesn't really make sense... service workers are not service worker clients.

I didn't know about reserved clients going away on redirect. I thought the reserved client gets set one time in the HTML spec here https://html.spec.whatwg.org/#process-a-navigate-fetch before continuing on to the Fetch spec. The Fetch spec follows the redirects here https://fetch.spec.whatwg.org/#http-redirect-fetch. So I thought there's just one time the reserved client gets set, at the start of the navigation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, it probably means to link to worker termination in the HTML spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think probably here "process a navigate response" https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-response we can add something like "discard reservedEnvironment" and have the service worker spec wait for that.

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, the reserved client is not discarded on all redirects. Redirecting cross-origin, though, must result in a new client being reserved with the new origin to avoid violating same-origin restrictions. So at that point the old reserved client can be discarded.

Copy link
Member Author

Choose a reason for hiding this comment

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

@wanderview Hum, that's not what I thought. I filed #1316

FYI: created whatwg/html#3723

1. If the response returned by the [=/HTTP fetch=] steps was a [=network error=], then:
1. Reject |promise| with a "{{NetworkError}}" {{DOMException}} and abort these steps.
1. Wait for |client|'s [=environment/execution ready flag=] to be set.
1. If |client| is an [=environment settings object=], then:
1. If |client| is not a <a>secure context</a>, reject |promise| with a "{{SecurityError}}" {{DOMException}} and abort these steps.
1. Else:
Expand Down
Loading