Permissions aren't sent to the content process when intercepting a fetch request

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: mystor, Assigned: mystor)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

9.61 KB, patch
baku
: review+
Details | Diff | Splinter Review
5.12 KB, patch
catalinb
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 months ago
This patch adds the tools to delay the fetch interception for permissions to be sent down when necessary.

Comment 1

10 months ago
The phrase "delay the fetch interception" sounds concerning.  Can explain more why the permissions are not present in the content process initiating the fetch?
(Assignee)

Comment 2

10 months ago
Created attachment 8857180 [details] [diff] [review]
Part 1: Add tools to nsPermissionManager to await permissions becoming avaliable

MozReview-Commit-ID: 1HDS8zw6dpF
Attachment #8857180 - Flags: review?(amarchesini)
(Assignee)

Comment 3

10 months ago
Created attachment 8857181 [details] [diff] [review]
Part 2: Request permissions from the parent process when necessary before intercepting a fetch request

MozReview-Commit-ID: E6OTY6IDThb
Attachment #8857181 - Flags: review?(catalin.badea392)
(Assignee)

Comment 4

10 months ago
(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #1)
> The phrase "delay the fetch interception" sounds concerning.  Can explain
> more why the permissions are not present in the content process initiating
> the fetch?

Sure thing.

When a content process initiates a fetch, it may make a fetch into a scope which has not had its service worker loaded yet, or had any documents opened yet within that scope. This fetch would then trigger a service worker to start without permissions being available yet, which is problematic as we need permissions to run the service worker. (Consider for example the cookie permission which is required for any type of persistent storage).

Unfortunately right now we don't go to the parent process at any point between starting the fetch (which is our first sign of intent) and starting the service worker. Unless we opt to send permissions for all registered service workers to all processes at startup this requires that we delay starting the service worker for permissions to become available.

My understanding is that with the service worker rewrite we will begin intercepting fetches in the parent process instead of the child, meaning that we will be able to do this without overhead. I may be incorrect in this.

Comment 5

10 months ago
Comment on attachment 8857181 [details] [diff] [review]
Part 2: Request permissions from the parent process when necessary before intercepting a fetch request

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +2764,5 @@
> +  // for the service worker's principal in the content process.
> +  nsCOMPtr<nsIPrincipal> swPrincipal = serviceWorker->Principal();
> +  nsCOMPtr<nsIRunnable> permissionsRunnable = NS_NewRunnableFunction([swPrincipal, continueRunnable] () {
> +      nsCOMPtr<nsIPermissionManager> permMgr = services::GetPermissionManager();
> +      MOZ_ALWAYS_SUCCEEDS(permMgr->RunWhenPermissionsAvaliableFor(swPrincipal, continueRunnable));

Does this run sync if permissions are already available?  We don't want to introduce another main thread turn into every fetch.
(Assignee)

Comment 6

10 months ago
(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #5)
> Does this run sync if permissions are already available?  We don't want to
> introduce another main thread turn into every fetch.

Currently it introduces a main thread turn, but I can make it sync without any difficulty. I'll post an updated patch with that change.

Comment 7

10 months ago
(In reply to Michael Layzell [:mystor] from comment #4)
> Unfortunately right now we don't go to the parent process at any point
> between starting the fetch (which is our first sign of intent) and starting
> the service worker. Unless we opt to send permissions for all registered
> service workers to all processes at startup this requires that we delay
> starting the service worker for permissions to become available.

How much memory do these permissions use?  There aren't that many service worker registrations right now.  We actually do spam them to every content process on start.  It might make sense to take the permissions on to those messages.

> My understanding is that with the service worker rewrite we will begin
> intercepting fetches in the parent process instead of the child, meaning
> that we will be able to do this without overhead. I may be incorrect in this.

Sure, but we don't want to introduce performance regressions in the meantime.
(Assignee)

Comment 8

10 months ago
Created attachment 8857556 [details] [diff] [review]
Part 1: Add tools to nsPermissionManager to await permissions becoming avaliable

I've updated the patches - These new patches avoid requesting the permission when possible, instead choosing to send permisisons for service workers to the content process eagerly when they are registered.

This is not a win for security which is unfortunate. One of the reasons why we wanted to do this was so that we didn't have to send permission information to the content process unless that content process needed that information. Fortunately, asuth mentioned on IRC that there will be a change to service workers soon which will move interception into the parent process. Once that is done this eager permission transmission can be replaced.

These patches should allow the assertion fixes to land before the service worker changes.

MozReview-Commit-ID: 1HDS8zw6dpF
Attachment #8857556 - Flags: review?(amarchesini)
(Assignee)

Updated

10 months ago
Attachment #8857180 - Attachment is obsolete: true
Attachment #8857180 - Flags: review?(amarchesini)
(Assignee)

Updated

10 months ago
Attachment #8857181 - Attachment is obsolete: true
Attachment #8857181 - Flags: review?(catalin.badea392)
(Assignee)

Comment 9

10 months ago
Created attachment 8857557 [details] [diff] [review]
Part 2: Request permissions from the parent process when necessary before intercepting a fetch request

MozReview-Commit-ID: E6OTY6IDThb
Attachment #8857557 - Flags: review?(catalin.badea392)
(Assignee)

Comment 10

10 months ago
Created attachment 8857558 [details] [diff] [review]
Part 3: Eagerly transmit permissions to the content process for service worker registrations

MozReview-Commit-ID: DOposXSoAgM
Attachment #8857558 - Flags: review?(catalin.badea392)

Comment 11

10 months ago
(In reply to Michael Layzell [:mystor] from comment #8)
> I've updated the patches - These new patches avoid requesting the permission
> when possible, instead choosing to send permisisons for service workers to
> the content process eagerly when they are registered.

Thanks!
(Assignee)

Updated

10 months ago
Blocks: 1355899
(Assignee)

Comment 12

10 months ago
Created attachment 8858108 [details] [diff] [review]
Temporarially transmit the cookie permission to the content process eagerly for service workers

I _think_ (and am currently testing on try) that this should (temporarially) make most of my service worker woes go away in the smallest amount of code possible and allow me to land bug 1353179.

Basically if I remember correctly the only permission which we actually check when starting a service worker is the "cookie" permission to see if storage settings are overridden for the service worker's principal. This patch makes the cookie permission be sent to the content process eagerly.

For (mostly privacy) reasons we would prefer to send this permission to the content process only when the content process needs it, so we'd like to revert this change once the changes to service worker architecture allow for it.

MozReview-Commit-ID: 7fMea06LyMY
Attachment #8858108 - Flags: review?(ehsan)
(Assignee)

Updated

10 months ago
Attachment #8857556 - Attachment is obsolete: true
Attachment #8857557 - Attachment is obsolete: true
Attachment #8857558 - Attachment is obsolete: true
Attachment #8857556 - Flags: review?(amarchesini)
Attachment #8857557 - Flags: review?(catalin.badea392)
Attachment #8857558 - Flags: review?(catalin.badea392)
(Assignee)

Comment 13

10 months ago
Comment on attachment 8858108 [details] [diff] [review]
Temporarially transmit the cookie permission to the content process eagerly for service workers

I'm clearing the review on this patch as I think we should just wait for asuth's parent process fetch interception patch to land.

I have found another fetch interception related permissions issue (namely after intercepting a fetch request, the service worker produces a response stream without leaving the content process, and thus we don't have a chance to send down permissions for the document which was created either). I don't think there's a good way to fix that without a round-trip to the parent process, so I'm just going to wait until I don't have to do that.
Attachment #8858108 - Flags: review?(ehsan)

Comment 14

10 months ago
Sounds good to me!
(Assignee)

Comment 15

10 months ago
From bug 1231222 comment 80 it looks like asuth won't be removing the within-process code path for fetch interception entirely.

Unfortunately this means I can't just wait for it to land and for my problems with as-needed permissions + service workers to go away.

I'm not sure how to do this. Based on conversations I've had with bkelly, we can't add any delay into the pipeline which involves talking to the parent process. This means that the "cookie" permission needs to be available in the content process for, at the minimum, every service worker which could want to write to indexeddb etc. 

Secondly, we also need permissions for the document which is created as a result of the fetch interception. That response also doesn't go through the parent process and thus doesn't get a chance to have permissions for it sent down earlier, which means that we need all permissions for any possible service worker response in the content process when the fetch interception happens.

My only potential idea is that we hook in as early as possible into the ServiceWorker infrastructure and send the request to the parent process, and don't block on it arriving until as late as possible in the pipeline (for example, we could have the "cookie" permission already in the content process, and send a request for the response permission as soon as the fetch starts, hoping that the response will have arrived from the parent process before the service worker gives us an answer - if it hasn't we'd have to delay for a bit). I'm not sure if that would be a problem.

The other simple option is the "service workers are hard - let's give up" option, where we just send down all of the permissions for active service worker scopes at startup.

I really don't know what we can do here. Ehsan do you have any ideas?
Flags: needinfo?(ehsan)

Comment 16

10 months ago
I don't think it's worth anyone's time to do anything fancy here sadly.  :-(  Let's give up.
Flags: needinfo?(ehsan)
(Assignee)

Comment 17

10 months ago
Created attachment 8860209 [details] [diff] [review]
Part 1: Add tools to nsPermissionManager to await permissions becoming avaliable

MozReview-Commit-ID: 1HDS8zw6dpF
(Assignee)

Updated

10 months ago
Attachment #8858108 - Attachment is obsolete: true
(Assignee)

Comment 18

10 months ago
Created attachment 8860210 [details] [diff] [review]
Part 2: Eagerly transmit permissions to the content process for service worker registrations

This requires running code which checks whether or not the permissions have
arrived, potentially delaying a fetch request for a very short period of time if
the permissions are still in-flight.

MozReview-Commit-ID: E6OTY6IDThb
(Assignee)

Updated

10 months ago
Attachment #8860209 - Flags: review?(amarchesini)
(Assignee)

Comment 19

10 months ago
Created attachment 8860395 [details] [diff] [review]
Part 2: Eagerly transmit permissions to the content process for service worker registrations

This requires running code which checks whether or not the permissions have
arrived, potentially delaying a fetch request for a very short period of time if
the permissions are still in-flight.

MozReview-Commit-ID: E6OTY6IDThb
Attachment #8860395 - Flags: review?(catalin.badea392)
(Assignee)

Updated

10 months ago
Attachment #8860210 - Attachment is obsolete: true
Comment on attachment 8860209 [details] [diff] [review]
Part 1: Add tools to nsPermissionManager to await permissions becoming avaliable

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

lgtm
Attachment #8860209 - Flags: review?(amarchesini) → review+
Comment on attachment 8860395 [details] [diff] [review]
Part 2: Eagerly transmit permissions to the content process for service worker registrations

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

Could you please write a test for this so we don't break it as the SW infrastructure is being changed?
Attachment #8860395 - Flags: review?(catalin.badea392) → review+
(Assignee)

Comment 22

10 months ago
(In reply to Cătălin Badea (:catalinb) from comment #21)
> Comment on attachment 8860395 [details] [diff] [review]
> Part 2: Eagerly transmit permissions to the content process for service
> worker registrations
> 
> Review of attachment 8860395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could you please write a test for this so we don't break it as the SW
> infrastructure is being changed?

I'll add a test specifically for this in a follow-up. With the assertion in bug 1353179 infra already fails without this patch. I'd like to get that bug landed soon so I'm going to land it before I write a purpose-made patch.
(Assignee)

Updated

10 months ago
Blocks: 1360255

Comment 23

10 months ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb249c8b3e60
Part 1: Add tools to nsPermissionManager to await permissions becoming avaliable, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0334bfc886c8
Part 2: Eagerly transmit permissions to the content process for service worker registrations, r=catalinb

Comment 24

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eb249c8b3e60
https://hg.mozilla.org/mozilla-central/rev/0334bfc886c8
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.