Implement FetchEvent.clientId and clients.get(id)

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
3 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

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

Comment 8

3 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

3 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

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

Updated

3 years ago
Attachment #8693117 - Attachment is obsolete: true
Attachment #8694971 - Flags: review?(josh) → review+

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c72962b1f5fb
https://hg.mozilla.org/mozilla-central/rev/45f2e8cc696a
https://hg.mozilla.org/mozilla-central/rev/2d93d213d982
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Reporter)

Updated

3 years ago
Depends on: 1236933
You need to log in before you can comment on or make changes to this bug.