1264177 - Implement FetchEvent "resulting" clientId attributes

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

At the face-to-face meeting we decided to more explicitly expose client IDs for:

  * Client being replaced by the navigation
  * Client being created for navigation, which is currently about:blank

This is in addition to the ID for the Client that triggered the navigation.

Some of the details in terms of naming are still up in the air, but we decided the principles.  See:

  https://github.com/slightlyoff/ServiceWorker/issues/870

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I'm going to implement this after I land the infrastructure in bug 1293277.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Note to self: the spec indicates we should mark the reserved Client as being controlled by the service worker before the navigation fetch event.  I'm not sure that is observable to script, but if it is we should test for it.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

The spec has changed to use a "resulting" client ID instead of "reserved".  In addition, the spec does not allow Clients.get(resultingClientId) to resolve until the Client is execution ready.  We won't be exposing a Client object for clients prior to execution ready for now.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Underlying platform support for the "resulting" client ID is in place.  Basically its:

  Maybe<ClientInfo> resulting;
  resulting = loadInfo->GetInitialClientInfo();
  if (resulting.isNothing()) {
    resulting = loadInfo->GetReservedClientInfo();
  }

  if (result.isSome()) {
    fetchEvent->SetResultingClientId(resulting.ref().Id());
  }

The other part is making `clients.get(fetchEvent.resultingClientId)` delay resolving its promise until the client is execution ready or destroyed.  The place to add this behavior is in the ClientManagerService which is invoked here:

https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/dom/clients/manager/ClientManagerOpParent.cpp#71-76

You'll need to create a MozPromise on the ClientSourceParent that settles when the source is marked execution ready or its destructor runs.

I'm removing the "target" id from this bug as its probably a separate set of work to plumb that through.  (The spec also is changing to call it "replacing" id.)

Note, I believe the resultingClientId is a high priority for some large web properties and other browsers.

Comment 5

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1264177 - Implement FetchEvent.resultingClientId

Bug 1264177 - Implement FetchEvent.resultingClientId

Expose FetchEvent.resultingClientId on non-subresource, non-"report"-destination requests.
Delay Clients.get(FetchEvent.resultingClientId) resolution until the resulting client is execution ready.
Add WPTs to test for existence of resultingClientId and Clients.get promise resolution values.

Comment 6

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1264177 - Implement FetchEvent.resultingClientId r?edenchuang

Comment 7

[Marion Daly [:mdaly]]

Status?

Comment 8

[Perry Jiang [:perry] [no longer employee, use ni?]]

Waiting on review, will check in with asuth.

Comment 9

[Cosmin Sabou [:CosminS]]

Hi! Lando coomplains about the reviewer for this patch, please use one of the below reviewers in order to get the patch landed. 

hg error in cmd: hg push -r tip upstream: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 1 changesets with 13 changes to 13 files remote: remote: ******************************* ERROR ******************************* remote: Changeset 36c394080273 alters WebIDL file(s) without DOM peer review: remote: dom/webidl/FetchEvent.webidl remote: remote: Please, request review from either: remote: - Andrea Marchesini (:baku) remote: - Andrew McCreight (:mccr8) remote: - Ben Kelly (:bkelly) remote: - Blake Kaplan (:mrbkap) remote: - Bobby Holley (:bholley) remote: - Boris Zbarsky (:bz) remote: - Ehsan Akhgari (:ehsan) remote: - Henri Sivonen (:hsivonen) remote: - Kyle Machulis (:qdot) remote: - Nika Layzell (:mystor) remote: - Olli Pettay (:smaug) remote: - Peter Van der Beken (:peterv) remote: ********************************************************************* remote: remote: transaction abort! remote: rollback completed remote: pretxnchangegroup.mozhooks hook failed abort: push failed on remote

Comment 10

[Perry Jiang [:perry] [no longer employee, use ni?]]

Added mrbkap as a reviewer.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ad8b10cc0d6
Implement FetchEvent.resultingClientId r=edenchuang,mrbkap

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14022 for changes under testing/web-platform/tests

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 14

[Narcis Beleuzu [:NarcisB]]

Backed out for wpt failures on fetch-destination-no-load-event.https.html 

Backout link: https://hg.mozilla.org/integration/autoland/rev/8bb9edd7ca13ba1d4eceb5c72a182830e7209cbc
Push link: https://hg.mozilla.org/integration/autoland/rev/6ad8b10cc0d6951618ca814d87a28ac651f1f333
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=211278296&repo=autoland&lineNumber=12950

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 16

[Cosmin Sabou [:CosminS]]

Please take a look also at this failure: https://treeherder.mozilla.org/logviewer.html#?job_id=211277672&repo=autoland&lineNumber=2580

it turned perma after this bug landed: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2Cx64%2Casan%2Cmochitests%2Cwith%2Ce10s%2Ctest-linux64-asan%2Fopt-mochitest-e10s-5%2Cm-e10s%285%29&fromchange=6ad8b10cc0d6951618ca814d87a28ac651f1f333&tochange=8bb9edd7ca13ba1d4eceb5c72a182830e7209cbc&selectedJob=211277672

Comment 17

[Perry Jiang [:perry] [no longer employee, use ni?]]

Believe I found the error, will submit to try to see.

Comment 18

[Perry Jiang [:perry] [no longer employee, use ni?]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3e1f05b6d994175761953effc2db2c07ee08123

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c79d1544486a
Implement FetchEvent.resultingClientId r=edenchuang,mrbkap

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c79d1544486a

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 23

[Chris Mills [:cmills]]

Hi there,

I'm thinking about documenting the new properties mentioned here, but I need some clarification on what the ids represent, and what they are used for.

We already have clientId, which is the id of the Client that initiated the fetch. But now we have

resultingClintId — "the Client being replaced by the navigation." So, is this the ID of the Client that is navigated to during a navigation? So for example if you clicked a link on a page controlled by a SW, the ID would represent the page linked to?

replacedClientId — "the Client being created for navigation, which is currently about:blank." So does this represent a blank client that would be created as the first step in a navigation history? I'm not 100% sure about this.

And can you give me an obvious use for each of them?

Many thanks in advance, for any help you can give.

Comment 24

[Perry Jiang [:perry] [no longer employee, use ni?]]

Sorry for the very late reply. This is my understanding of the two properties (the naming is somewhat confusing IMO):

replacesClientId: ID of the client that is being replaced. For example, when navigating from page A to page B, replacesClientId is the ID of the client associated with page A. It can be the empty string when navigating from about:blank to another page, as the about:blank's client will be reused, rather than be replaced. Additionally, if the fetch isn't a navigation, replacesClientId will be the empty string[1]. This could be used to access/communicate with a client that will imminently be replaced, right before a navigation.

resultingClientId: ID of the client which will replace the navigated-away-from client. I think this is like the mirror of replaceClientId; this could be the client associated with page B in the above example. If the fetch request is NOT a non-subresource request (i.e. it is a subresource request) OR the request's destination is "report," resultingClientId will be the empty string[1].

[1]: https://w3c.github.io/ServiceWorker/#fetch-event-resultingclientid (see step 8 and 9)

Andrew, could you double check this/confirm?

Comment 25

[Chris Mills [:cmills]]

(In reply to Perry Jiang [:perry] from comment #24)
> Sorry for the very late reply. This is my understanding of the two
> properties (the naming is somewhat confusing IMO):
> 
> replacesClientId: ID of the client that is being replaced. For example, when
> navigating from page A to page B, replacesClientId is the ID of the client
> associated with page A. It can be the empty string when navigating from
> about:blank to another page, as the about:blank's client will be reused,
> rather than be replaced. Additionally, if the fetch isn't a navigation,
> replacesClientId will be the empty string[1]. This could be used to
> access/communicate with a client that will imminently be replaced, right
> before a navigation.
> 
> resultingClientId: ID of the client which will replace the
> navigated-away-from client. I think this is like the mirror of
> replaceClientId; this could be the client associated with page B in the
> above example. If the fetch request is NOT a non-subresource request (i.e.
> it is a subresource request) OR the request's destination is "report,"
> resultingClientId will be the empty string[1].
> 
> [1]: https://w3c.github.io/ServiceWorker/#fetch-event-resultingclientid (see
> step 8 and 9)
> 
> Andrew, could you double check this/confirm?

No problem; thank you for the information — this is just what I needed!

This is now documented, so if Andrew could that, it would be really cool ;-)

I've added pages to document the properties:
https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent
https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent/replacesClientId
https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent/resultingClientId

I've submitted a PR to update the compat data:
https://github.com/mdn/browser-compat-data/pull/3221

Finally I've added a note to the Fx65 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#APIs 

Thanks in advance for a review.