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

The phrase "delay the fetch interception" sounds concerning. Can explain more why the permissions are not present in the content process initiating the fetch?

MozReview-Commit-ID: 1HDS8zw6dpF

MozReview-Commit-ID: E6OTY6IDThb

(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 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.

(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.

(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.

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

MozReview-Commit-ID: E6OTY6IDThb

MozReview-Commit-ID: DOposXSoAgM

(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!

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

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.

Sounds good to me!

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?

I don't think it's worth anyone's time to do anything fancy here sadly. :-( Let's give up.

MozReview-Commit-ID: 1HDS8zw6dpF

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

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

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

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?

(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.

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

https://hg.mozilla.org/mozilla-central/rev/eb249c8b3e60 https://hg.mozilla.org/mozilla-central/rev/0334bfc886c8