Implement FetchEvent.clientId and clients.get(id)

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bkelly, Assigned: Ehsan)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

See:

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

We should do this sooner, rather than later, but its not a v1 blocker.
Assignee

Updated

4 years ago
Assignee: nobody → ehsan
Assignee

Comment 1

4 years ago
For top-level navigations, we need to compute the client ID when we decide to
intercept the document load, and we need to make sure the document that will
be created later will end up using that same ID.
Attachment #8692210 - Flags: review?(josh)
Assignee

Comment 2

4 years ago
Attachment #8692211 - Flags: review?(josh)
Assignee

Comment 3

4 years ago
I'll try to finish Clients.get() tomorrow...
Comment on attachment 8692210 [details] [diff] [review]
Part 1: Save a client ID for top-level navigations on the docshell and assign it as the document ID when we start loading the document

Review of attachment 8692210 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +3748,5 @@
>    if (controllingRegistration) {
>      StopControllingADocument(controllingRegistration);
>    }
>  
> +  StartControllingADocument(aWorkerRegistration, aDocument, NS_LITERAL_STRING(""));

It might be nice to assert that aDocument's id is not empty here.
Attachment #8692210 - Flags: review?(josh) → review+
Comment on attachment 8692211 [details] [diff] [review]
Part 2: Implement FetchEvent.clientId

Review of attachment 8692211 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +3335,5 @@
>    if (aIsSubresourceLoad) {
>      MOZ_ASSERT(aDoc);
>      serviceWorker = GetActiveWorkerInfoForDocument(aDoc);
>      loadGroup = aDoc->GetDocumentLoadGroup();
> +    nsresult rv = aDoc->GetId(documentId);

Would asserting that the document id matches the passed one here make sense? If not, could we rename the argument to better indicate how it differs from the passed document's id?
Attachment #8692211 - Flags: review?(josh) → review+
Comment on attachment 8692210 [details] [diff] [review]
Part 1: Save a client ID for top-level navigations on the docshell and assign it as the document ID when we start loading the document

Review of attachment 8692210 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +3748,5 @@
>    if (controllingRegistration) {
>      StopControllingADocument(controllingRegistration);
>    }
>  
> +  StartControllingADocument(aWorkerRegistration, aDocument, NS_LITERAL_STRING(""));

Having spent time contemplating this request (at Ehsan's prompting), let's rename Document::GetId to GetOrCreateId instead.
Assignee

Comment 7

4 years ago
Attachment #8693117 - Flags: review?(josh)
Assignee

Comment 8

4 years ago
Comment on attachment 8692211 [details] [diff] [review]
Part 2: Implement FetchEvent.clientId

Review of attachment 8692211 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +3335,5 @@
>    if (aIsSubresourceLoad) {
>      MOZ_ASSERT(aDoc);
>      serviceWorker = GetActiveWorkerInfoForDocument(aDoc);
>      loadGroup = aDoc->GetDocumentLoadGroup();
> +    nsresult rv = aDoc->GetId(documentId);

I'll rename the argument to aDocumentIdForTopLevelNavigation.  Hope that's better!
Comment on attachment 8693117 [details] [diff] [review]
Part 3: Implement Clients.get()

Review of attachment 8693117 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerClients.cpp
@@ +85,5 @@
> +      Promise* promise = mPromiseProxy->WorkerPromise();
> +      MOZ_ASSERT(promise);
> +
> +      if (mRv.Failed()) {
> +        promise->MaybeReject(mRv.StealNSResult());

Transferring ErrorResult values between threads isn't allowed, since they could be JS exceptions.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/clients-get.https.html
@@ +23,5 @@
> +          clientIds.push(frame1.contentDocument.body.textContent);
> +          frame1.focus();
> +          return with_iframe(scope + '#2');
> +        })
> +      .then(function(frame2) {

Can we have a test for an insecure client as well? Open an http:// page that has an <img> pointing at a controlled resource or something?

@@ +29,5 @@
> +          var channel = new MessageChannel();
> +          channel.port1.onmessage = t.step_func(on_message);
> +          frame2.contentWindow.navigator.serviceWorker.controller.postMessage(
> +              {port:channel.port2, clientIds:clientIds,
> +               message: 'get_client_ids'}, [channel.port2]);

Why the transferable object as well as the port member for the message?

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/clients-get-worker.js
@@ +1,2 @@
> +self.onfetch = function(e) {
> +  if (e.request.url.indexOf('?ignore') == -1) {

When is this used?
Attachment #8693117 - Flags: review?(josh)
Assignee

Comment 10

4 years ago
(In reply to Josh Matthews [:jdm] from comment #9)
> ::: dom/workers/ServiceWorkerClients.cpp
> @@ +85,5 @@
> > +      Promise* promise = mPromiseProxy->WorkerPromise();
> > +      MOZ_ASSERT(promise);
> > +
> > +      if (mRv.Failed()) {
> > +        promise->MaybeReject(mRv.StealNSResult());
> 
> Transferring ErrorResult values between threads isn't allowed, since they
> could be JS exceptions.

I'll pass the raw nsresult.

> :::
> testing/web-platform/mozilla/tests/service-workers/service-worker/clients-
> get.https.html
> @@ +23,5 @@
> > +          clientIds.push(frame1.contentDocument.body.textContent);
> > +          frame1.focus();
> > +          return with_iframe(scope + '#2');
> > +        })
> > +      .then(function(frame2) {
> 
> Can we have a test for an insecure client as well? Open an http:// page that
> has an <img> pointing at a controlled resource or something?

There seems to be no way to observe a non-secure client ID from within a service worker <https://github.com/slightlyoff/ServiceWorker/issues/791>

> @@ +29,5 @@
> > +          var channel = new MessageChannel();
> > +          channel.port1.onmessage = t.step_func(on_message);
> > +          frame2.contentWindow.navigator.serviceWorker.controller.postMessage(
> > +              {port:channel.port2, clientIds:clientIds,
> > +               message: 'get_client_ids'}, [channel.port2]);
> 
> Why the transferable object as well as the port member for the message?

That's how postMessage() works, you need to pass in the list of stuff to be structure cloned, presumably so that the API doesn't implicitly neuter stuff that the author didn't ask for to be neutered.

> :::
> testing/web-platform/mozilla/tests/service-workers/service-worker/resources/
> clients-get-worker.js
> @@ +1,2 @@
> > +self.onfetch = function(e) {
> > +  if (e.request.url.indexOf('?ignore') == -1) {
> 
> When is this used?

In another test which I didn't submit!
Assignee

Comment 11

4 years ago
Attachment #8694971 - Flags: review?(josh)
Assignee

Updated

4 years ago
Attachment #8693117 - Attachment is obsolete: true
Attachment #8694971 - Flags: review?(josh) → review+
Depends on: 1236933
You need to log in before you can comment on or make changes to this bug.