Ben Kelly [:bkelly, not reviewing] Reporter
	Description • 9 years ago

See:

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

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

	(no longer active) Assignee
	Comment 1 • 9 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.

	(no longer active) Assignee
	Comment 3 • 9 years ago

I'll try to finish Clients.get() tomorrow...

	Josh Matthews [:jdm]
	Comment 4 • 9 years ago

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.

	Josh Matthews [:jdm]
	Comment 5 • 9 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);

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?

	Josh Matthews [:jdm]
	Comment 6 • 9 years ago

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.

	(no longer active) Assignee
	Comment 8 • 9 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!

	Josh Matthews [:jdm]
	Comment 9 • 9 years ago

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?

	(no longer active) Assignee
	Comment 10 • 9 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!

	Pulsebot
	Comment 12 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c72962b1f5fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/45f2e8cc696a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d93d213d982

	Carsten Book [:Tomcat]
	Comment 13 • 9 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

	Chris Mills [:cmills]
	Comment 14 • 9 years ago

New features documented:

https://developer.mozilla.org/en-US/docs/Web/API/Clients/get
https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent/clientId

I've also added a note about this in the release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/45

Let me know if these read ok. Thanks!