Closed
Bug 1057933
Opened 10 years ago
Closed 10 years ago
ServiceWorkerRegistrations should use scope and not window to obtain correct worker
Categories
(Core :: DOM: Workers, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: nsm, Assigned: nsm)
References
Details
Attachments
(2 files)
10.92 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
8.52 KB,
application/octet-stream
|
Details |
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.
Assignee | ||
Comment 1•10 years ago
|
||
I'm not sure if the security check is required, since all the APIs to obtain a ServiceWorkerRegistration already perform those.
Attachment #8478089 -
Flags: review?(amarchesini)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Comment 2•10 years ago
|
||
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.
Attachment #8478089 -
Flags: feedback?(bzbarsky)
Comment 3•10 years ago
|
||
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?
Attachment #8478089 -
Flags: feedback?(bzbarsky)
Assignee | ||
Comment 4•10 years ago
|
||
(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•10 years ago
|
||
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.... :(
Assignee | ||
Comment 6•10 years ago
|
||
Just uses owner of the registration instead of mucking with entry document.
Assignee | ||
Comment 7•10 years ago
|
||
Andrea, please go ahead and land this if it is satisfactory (with interdiff).
Comment 8•10 years ago
|
||
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)
Attachment #8478089 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d98477ddc9
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a5d98477ddc9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•