1260915 - Write a test to make sure Service Worker handle userContextId correctly

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Dave Huseby [:huseby]]

In the file ServiceWorkerManager.cpp there are three calls to createCodebasePrincipal:

https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#2428
https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#3695
https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#3900

We need to analyze how the newly created principals are being used.  Are they used to isolate any browser state? If so, do we want to isolate on userContextId?

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I believe we serialize and deserialize all principal attributes.  If userContextId is included there, then it should already get propagated correctly.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Can someone summarize what this actually means for service workers?  I have no clue what semantics you are expecting SWM to implement here.

Comment 3

[Jonathan Hao (inactive) [:jhao]]

(In reply to Ben Kelly [:bkelly] from comment #2)
> Can someone summarize what this actually means for service workers?  I have
> no clue what semantics you are expecting SWM to implement here.

We only want to make sure that those principals created in ServiceWorkerManager are not used to separate browser states like cookies. If they aren't, there would be nothing to fix. If they are, we need to check if the origin attributes are passed correctly with userContextId's.

Comment 4

[Jonathan Hao (inactive) [:jhao]]

As far as I can tell, we'll get scopeKeys, essentially origins, from those principals, and use it to get registration info. So, it seems that under different user context, there will be different service workers. If a user opens a web page in context A, they may access it offline later in context A, but they may not access it offline in context B.

Hi Ben and Jonas,
Just to confirm. Is this the behavior that we want?

Thanks.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I guess so.  To be honest I don't really understand what you are trying to achieve with the user context stuff.

From my perspective service workers are based on origins.  If you are adding something to the principal such that two different pages no longer pass a principal equality test, then they should have separate service workers and separate storage.

Its important you make sure this works with QuotaManager since our storage layers are built on top of that.

Please don't make any assumptions about particular user contexts running in particular child processes.  That caused us all manner of trouble with the appId extension to principals.

Comment 6

[Jonathan Hao (inactive) [:jhao]]

Thanks, Ben.

I tried open https://mdn.github.io/sw-test/ in a couple of different user contexts. I checked about:serviceworkers, and they each have their own service worker as expected. Since it behaves the way we want, there's nothing to fix. We can close this bug if Jonas has no objections.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

OriginAttributes are very similar to separate profiles. You can create a profile for yourself, and a profile for your child. These profiles will have separate cookies, separate http cache, separate service-worker registrations, separate IndexedDB databases, separate Cache API storages, etc.

OriginAttributes work the same way, except that gecko is able to access all profiles without having to be restarted. So we can run one profile in one tab, and another profile in another tab.

If ServiceWorkers is written such that it always uses nsIPrincipals to do same-origin checks, and it always uses nsIPrincipal.origin when you need a serializable key (such as when writing to disk), then we should be good. If the serviceworker code ever looks at the scheme+host+port to do either of those, then we're in trouble.

And if you're ever reading back origins from disk (which were serialized using nsIPrincipal.origin), then you need to use the appropriate parsing function. Though I'm not sure which function that is actually.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Yea, service workers does all that already because we had to support appId correctly.

We do look at scheme in some places, but thats just to block things like file://, etc.

Comment 9

[Jonathan Hao (inactive) [:jhao]]

Thanks to Jonas and Ben. I'm closing this bug.

Comment 10

[Jonathan Hao (inactive) [:jhao]]

On second thought, I probably should write an automated test for it besides testing it manually.

Comment 11

[Jonathan Hao (inactive) [:jhao]]

Attachment: Write a test to make sure Service Worker handle userContextId correctly

This is a wip patch. It keeps failing, but I can't figure out why. If I ran this test with --keep-open=true and manually open about:serviceworkers, I can see three service workers, but by swm.getAllRegistrations() I always get 0 registration info.

Comment 12

[Jonathan Hao (inactive) [:jhao]]

Hi Ben, do you have any idea about what was wrong with my patch? I can't see why it didn't work since that's what about:serviceworkers do[1].

p.s. serviceworker.html[2] and worker.js[3] already exists in that test directory.

Thank you.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/content/aboutServiceWorkers.js#36
[2] https://dxr.mozilla.org/mozilla-central/source/dom/workers/test/serviceworkers/serviceworker.html
[3] https://dxr.mozilla.org/mozilla-central/source/dom/workers/test/serviceworkers/worker.js

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

The only thing I can think is that you are getting the SWM service instance too early.  Try moving it into the your function call right before you use swm.

Comment 14

[Jonathan Hao (inactive) [:jhao]]

Attachment: Write a test to make sure ServiceWorkerManager handles userContextId correctly

Thanks, Ben. That didn't solve the problem, but later, with Dimi's help, I realized that registration doesn't happen right away. I needed to add a listener.

May I ask you to be the reviewer? Thanks.

Comment 15

[Jonathan Hao (inactive) [:jhao]]

Dave, do we need someone from container side to review this patch?

Comment 16

[Jonathan Hao (inactive) [:jhao]]

Attachment: Write a test to make sure Service Worker handle userContextId correctly

Hi Tanvi, I move the test to the contextual identity folder as you suggested. I also added some cleanup code to ensure all service workers added in this test are unregistered. Do you want to take a look? Thanks.

Comment 17

[Jonathan Hao (inactive) [:jhao]]

Attachment: Write a test to make sure Service Worker handle userContextId correctly

I changed one line because the url in the previous patch still points to the service worker directory.

Comment 18

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8745920 [details] [diff] [review]
Write a test to make sure Service Worker handle userContextId correctly

I talked to Tanvi. Since Ben has already given r+, this patch is good to go. I'll push to try later.

Comment 19

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38cf5307e87f

Comment 20

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05008ade2ebc

Comment 21

[Carsten Book [:Tomcat]]

needs rebasing 

patching file browser/components/contextualidentity/test/browser/browser.ini
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file browser/components/contextualidentity/test/browser/browser.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory

Comment 22

[Jonathan Hao (inactive) [:jhao]]

Attachment: Write a test to make sure Service Worker handle userContextId correctly

Rebased. Thanks, Carsten.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/763dfabc9784

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/763dfabc9784