Implement FetchEvent "resulting" clientId attributes

RESOLVED FIXED in Firefox 65

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: bkelly, Assigned: perry, NeedInfo)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

Trunk
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox65 fixed)

Details

(Whiteboard: btpp-fixlater[wptsync upstream])

Attachments

(1 attachment, 1 obsolete attachment)

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
Whiteboard: btpp-fixlater
I'm going to implement this after I land the infrastructure in bug 1293277.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Depends on: 1293277
Summary: Implement FetchEvent "target" and "potential" clientId attributes → Implement FetchEvent "target" and "reserved" clientId attributes
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.
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.
Summary: Implement FetchEvent "target" and "reserved" clientId attributes → Implement FetchEvent "target" and "resulting" clientId attributes
See Also: → 1434913
Priority: -- → P3
Priority: P3 → P2
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.
Assignee: ben → nobody
Status: ASSIGNED → NEW
Summary: Implement FetchEvent "target" and "resulting" clientId attributes → Implement FetchEvent "resulting" clientId attributes
Assignee

Updated

10 months ago
Assignee: nobody → pjiang
Status: NEW → ASSIGNED
Assignee

Comment 5

9 months ago
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.
Attachment #9001647 - Attachment description: Bug 1434913 - Make FetchEvent.clientId non-nullable. → Bug 1264177 - Implement FetchEvent.resultingClientId
Attachment #9001647 - Attachment is obsolete: true
Status?
Flags: needinfo?(perry)
Assignee

Comment 8

7 months ago
Waiting on review, will check in with asuth.
Flags: needinfo?(perry)
Attachment #9007378 - Attachment description: Bug 1264177 - Implement FetchEvent.resultingClientId r?asuth → Bug 1264177 - Implement FetchEvent.resultingClientId r?edenchuang
Assignee

Updated

6 months ago
Keywords: checkin-needed
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
Flags: needinfo?(perry)
Assignee

Comment 10

6 months ago
Added mrbkap as a reviewer.
Flags: needinfo?(perry)
Assignee

Updated

6 months ago
Keywords: checkin-needed

Comment 11

6 months ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ad8b10cc0d6
Implement FetchEvent.resultingClientId r=edenchuang,mrbkap
Keywords: checkin-needed
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14022 for changes under testing/web-platform/tests
Whiteboard: btpp-fixlater → btpp-fixlater[wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging
Assignee

Comment 17

6 months ago
Believe I found the error, will submit to try to see.
Flags: needinfo?(perry)
Assignee

Updated

6 months ago
Keywords: checkin-needed

Comment 19

6 months ago
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c79d1544486a
Implement FetchEvent.resultingClientId r=edenchuang,mrbkap
Keywords: checkin-needed
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 21

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c79d1544486a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Upstream PR merged
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.
Flags: needinfo?(perry)
Assignee

Comment 24

5 months ago
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?
Flags: needinfo?(perry) → needinfo?(bugmail)
(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.
You need to log in before you can comment on or make changes to this bug.