1057933 - ServiceWorkerRegistrations should use scope and not window to obtain correct worker

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

ServiceWorkerRegistration attempts to get a worker for the window it is running in, when it should be using it's scope.

// index.html
navigator.serviceWorker.register('foo.js', { scope: '/someotherscope/' }).then(function(swr) {
  swr.active/waiting/installing should be non-null because someotherscope does indeed have foo.js as it's worker.
});

Currently they are null because swr uses index.html's window to [[MatchScope]] on the list of registrations.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: ServiceWorkerRegistrations should use scope and not window to obtain workers

I'm not sure if the security check is required, since all the APIs to obtain a ServiceWorkerRegistration already perform those.

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8478089 [details] [diff] [review]
ServiceWorkerRegistrations should use scope and not window to obtain workers

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +1795,3 @@
>  {
> +  AssertIsOnMainThread();
> +  nsCOMPtr<nsIDocument> doc = GetEntryDocument();

I want to have the opinion of somebody else on this. I don't think it's safe enough to have a service that uses the stack to retrieve the document. It seems fragile. Can we have another approach? bz, feedback?

@@ +1800,5 @@
> +  nsCString scope = NS_ConvertUTF16toUTF8(aScope);
> +
> +  ///////////////////////////////////////////
> +  // Security check
> +  nsCOMPtr<nsIURI> scopeURI;

the indentation is a bit strange. Move nsCOMPtr<nsIURI> after the comment.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8478089 [details] [diff] [review]
ServiceWorkerRegistrations should use scope and not window to obtain workers

Why is this looking at the entry document's principal, instead of, say, the subject principal?  And for the window the entry window instead of the incumbent one or the current realm?

Can someone please point me to the spec for the behavior here?

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Comment on attachment 8478089 [details] [diff] [review]
> ServiceWorkerRegistrations should use scope and not window to obtain workers
> 
> Why is this looking at the entry document's principal, instead of, say, the
> subject principal?  And for the window the entry window instead of the
> incumbent one or the current realm?
> 
> Can someone please point me to the spec for the behavior here?

This part isn't in the spec. What I wanted to do was make sure that the page that calls serviceworkerRegistration.active (see bug description) is allowed to load the scope the registration is for, before handing it a ServiceWorker.

But similar other functions like ServiceWorkerManager::(Un)Register use EntryGlobal() as defined by their specs (entry settings object's API base URL) [1]. Is that correct?

[1]: http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#register-algorithm

Comment 5

[Boris Zbarsky [:bzbarsky]]

Using the entry global for base URLs makes sense in that it matches how other web APIs work for base URLs, though it's a pretty messed up behavior.  Pretty much any other use of entry globals is almost certainly wrong.

> What I wanted to do was make sure that the page that calls
> serviceworkerRegistration.active 

That would be the incumbent global.

Let me illustrate the difference between the two.

Say a user clicks a link in window A, which calls a function in window B, which then gets .active on a ServiceWorkerRegistration object from window C.

In this situation, the entry global is A, the incumbent global is B, the current realm is C.  Using A for security checks is a bit weird, imo.  Even using it for base URIs is weird moderately weird, unless A is the thing that passes a URL to B that we're trying to resolve relative to a base URI....  :(

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: interdiff

Just uses owner of the registration instead of mucking with entry document.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Andrea, please go ahead and land this if it is satisfactory (with interdiff).

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8478089 [details] [diff] [review]
ServiceWorkerRegistrations should use scope and not window to obtain workers

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

I think you can get rid of:

already_AddRefed<ServiceWorkerRegistrationInfo>
ServiceWorkerManager::GetServiceWorkerRegistrationInfo(nsPIDOMWindow* aWindow)

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d98477ddc9

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a5d98477ddc9