Closed
Bug 1222464
Opened 9 years ago
Closed 9 years ago
Implement FetchEvent.clientId and clients.get(id)
Categories
(Core :: DOM: Service Workers, defect)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: bkelly, Assigned: ehsan.akhgari)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(3 files, 1 obsolete file)
11.45 KB,
patch
|
jdm
:
review+
|
Details | Diff | Splinter Review |
20.50 KB,
patch
|
jdm
:
review+
|
Details | Diff | Splinter Review |
24.94 KB,
patch
|
jdm
:
review+
|
Details | Diff | Splinter Review |
See:
https://github.com/slightlyoff/ServiceWorker/issues/723
We should do this sooner, rather than later, but its not a v1 blocker.
Reporter | ||
Updated•9 years ago
|
Blocks: ServiceWorkers-compat
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ehsan
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.
Attachment #8692210 -
Flags: review?(josh)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8692211 -
Flags: review?(josh)
Assignee | ||
Comment 3•9 years ago
|
||
I'll try to finish Clients.get() tomorrow...
Updated•9 years ago
|
Keywords: dev-doc-needed
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.
Attachment #8692210 -
Flags: review?(josh) → review+
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?
Attachment #8692211 -
Flags: review?(josh) → review+
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.
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8693117 -
Flags: review?(josh)
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!
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?
Attachment #8693117 -
Flags: review?(josh)
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!
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8694971 -
Flags: review?(josh)
Assignee | ||
Updated•9 years ago
|
Attachment #8693117 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8694971 -
Flags: review?(josh) → review+
Comment 12•9 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•