1107516 - set principal correctly for workers without parent document or load group

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently workers without a parent document or valid load group will get a null principal due to the code here:

  http://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp?from=ScriptLoader.cpp&case=true#120

This breaks ServiceWorkers because we're pretty much guaranteed to hit this case at the moment and we can't use QuotaManager based services (like Cache) without a real principal.

While we should ultimately fix SW's and shared workers to set a load group appropriately, it seems we should also not lose the principal here unnecessarily.

I'd like to set a load context here for the principal if one is not provided by the given load group.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I should also note that I got a head nod for this approach from Jonas in IRC.

I think the ServiceWorker load group needs to be added as part of bug 931249.  That implements the SW specific parts of the ScriptLoader to either pull from the network or read from the cache.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

And once all worker types are fixed to provide a load group we can assert here that a context is properly provided.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add factory method for create a LoadContext from an nsIPrincipal. (v0)

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Create a LoadContext if one is not provided in worker ScriptLoader. (v0)

This checks to see if an nsILoadContext is already available through the given load group.  If the context is missing, then it creates one for the given principal.

I did this for both the parent doc and principal-only cases.  Maybe it should only be done for the principal-only case.

https://tbpl.mozilla.org/?tree=Try&rev=8f325c6edf93

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I can also try to pursue the load group here if people think its better, but its less clear to me what needs to be done.

I also looked at what it would take to provide a load group and it seems a lot of plumbing would be required.  Maybe we would want to add a load group somewhere on LoadInfo or the WorkerPrivate which could then be passed in to the ChannelFromScriptURLMainThread()?  I assume we want all scripts loaded for a worker to share the same group.

The load group would just have default values with a LoadContext matching the principal?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8532051 [details] [diff] [review]
P1 Add factory method for create a LoadContext from an nsIPrincipal. (v0)

Why don't you just have a LoadContext ctor taking principal?

GetAppId and GetIsInBrowserElement should never return anything else but NS_OK,
so you could just assert the nsresult value.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8532052 [details] [diff] [review]
P2 Create a LoadContext if one is not provided in worker ScriptLoader. (v0)

Dropping review request.  I'm going to try to make the load groups work.  Sorry for the churn.

Comment 8

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

Comment on attachment 8532052 [details] [diff] [review]
P2 Create a LoadContext if one is not provided in worker ScriptLoader. (v0)

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

::: dom/workers/ScriptLoader.cpp
@@ +133,5 @@
>                         parentDoc,
>                         nsILoadInfo::SEC_NORMAL,
>                         nsIContentPolicy::TYPE_SCRIPT,
>                         loadGroup,
> +                       loadContext,

Don't set a loadcontext as callbacks. It shouldn't be needed. The existing code should be getting the context through the loadgroup.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add LoadContext constructor taking an nsIPrincipal. (v1)

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make sure all workers have an nsILoadGroup when loading scripts. (v1)

Flagging both Ben and Jonas per Jonas's suggestion.

This stores an nsILoadGroup in the WorkerPrivate LoadInfo.  If possible, it gets the load group from the document.  Otherwise it creates a new load group with a context based on the principal.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

https://tbpl.mozilla.org/?tree=Try&rev=1d8acc2983d7

Comment 12

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

Comment on attachment 8532340 [details] [diff] [review]
P2 Make sure all workers have an nsILoadGroup when loading scripts. (v1)

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

Looks good. Ben, feel free to have a look too if you feel that it's needed.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make sure all workers have an nsILoadGroup when loading scripts. (v2)

Sorry, I missed merging a fix back from my test area on maple.  There was a spot I was not using the new WorkerPrivate::GetLoadGroup() in ScriptLoader.

  https://tbpl.mozilla.org/?tree=Try&rev=14c42ed6e083

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

There also appears to be a case where the loadGroup is not set and triggering an assertion in test_csp.html.  Investigating.

Comment 15

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

Oh, and ideally when we create a sub-worker, we should use the loadgroup of the parent worker.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonas Sicking (:sicking) from comment #15)
> Oh, and ideally when we create a sub-worker, we should use the loadgroup of
> the parent worker.

Yep, that was the problem.  I have a fix I just need to clean up and upload as an attachment.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8532339 [details] [diff] [review]
P1 Add LoadContext constructor taking an nsIPrincipal. (v1)

Hmm, shouldn't mIsContent be true, or can one use service workers in chrome code?

(and issue perhaps not to be solved here, do we need to care about private browsing + service workers?)

r+ if you change mIsContent to true and explain private browsing case.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Let me see if I can get the private browsing and isContent flags propagated properly.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Ben, have you added the private browsing flag to WorkerPrivate::LoadInfo in order to check it for IDB on workers?

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add LoadContext constructor taking an nsIPrincipal and base context. (v2)

I updated the LoadContext constructor to take an optional base context.  If provided then the private browsing, content, and remote tab flags will be extracted from that base context.

We talked about this on IRC, but I wasn't sure if you wanted to re-review.  Flagging again to be safe.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add nsILoadGroup helpers to nsNetUtil.h. (v0)

Add some additional nsILoadGroup helpers to nsNetUtil.  I chose this location because there was an existing NS_NewLoadGroup() there.

NS_NewLoadGroup(result, principal, baseGroup) creates a new group with a LoadContext that matches the given principal and based on the context of base group (if present).

NS_LoadGroupMatchesPrincipal(group, principal) returns true if the context in the given load group matches the principal.  In this case "match" is defined as having the same appId and inBrowserElement flag.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make sure all workers have an nsILoadGroup when loading scripts. (v3)

Updated the worker patch to propagate the load group to child workers.  This is done by adding an argument to WorkerPrivate::SetPrincipal().

It also more proactively creates a new load group if the principal does not match the current load group.

Finally, when creating new load groups, the patch now uses the previous parent's load group as a base wherever possible.  This propagates the private browsing, context, and remote tab flags.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Reflagged that last patch because it felt like some non trivial changes.

https://tbpl.mozilla.org/?tree=Try&rev=3794e4c42912

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8532665 [details] [diff] [review]
P1 Add LoadContext constructor taking an nsIPrincipal and base context. (v2)

aBaseContext=nullptr

null before and after =

Comment 25

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

Comment on attachment 8532669 [details] [diff] [review]
P3 Make sure all workers have an nsILoadGroup when loading scripts. (v3)

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

::: dom/workers/ScriptLoader.cpp
@@ +339,3 @@
>      if (!principal) {
>        NS_ASSERTION(parentWorker, "Must have a principal!");
>        NS_ASSERTION(mIsWorkerScript, "Must have a principal for importScripts!");

Does this need to be done recursively? I.e. do you need to walk up the parent chain until you reach the top-level work in order to get the loadgroup/principal?

I'd think not, but wanted to check.

@@ +552,5 @@
>            return NS_ERROR_DOM_BAD_URI;
>          }
>        }
>  
> +      if (!NS_LoadGroupMatchesPrincipal(channelLoadGroup, channelPrincipal)) {

This looks wrong. How could these two ever not match? You just got both from the same channel so they should always match. Only exception involves resource://, and system principals, but I think in that case all appids will be NO_APP, so also should match fine.

Put it another way, if you changed this to just assert that they match, under what circumstances would that assertion trigger?

::: dom/workers/ServiceWorkerManager.cpp
@@ +2107,5 @@
> +  //  - private browsing = false
> +  //  - content = true
> +  //  - use remote tabs = false
> +  // Alternatively we could persist the original load group values and use
> +  // them here.

We definitely want to make sure that a SW installed during private browsing doesn't suddenly remain installed during non-private browsing. Or using non-private browsing cookies.

I don't know what the other two flags do, so I don't know if they matter. Would be worth checking with someone on the necko team.

@@ +2108,5 @@
> +  //  - content = true
> +  //  - use remote tabs = false
> +  // Alternatively we could persist the original load group values and use
> +  // them here.
> +  rv = NS_NewLoadGroup(getter_AddRefs(info.mLoadGroup), info.mPrincipal);

Seems like something like this is also needed when we create a new shared worker? Or will the code in WorkerPrivate::GetLoadInfo take care of that?

::: dom/workers/WorkerPrivate.cpp
@@ +4126,5 @@
>        loadInfo.mReportCSPViolations = false;
>      }
>  
> +    if (!NS_LoadGroupMatchesPrincipal(loadInfo.mLoadGroup,
> +                                      loadInfo.mPrincipal)) {

See comment above.

This should likely just be |if (!loadInfo.mLoadGroup)|, and then an assertion that the resulting principal/loadgroup pair match.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

> ::: dom/workers/ScriptLoader.cpp
> @@ +339,3 @@
> >      if (!principal) {
> >        NS_ASSERTION(parentWorker, "Must have a principal!");
> >        NS_ASSERTION(mIsWorkerScript, "Must have a principal for importScripts!");
> 
> Does this need to be done recursively? I.e. do you need to walk up the
> parent chain until you reach the top-level work in order to get the
> loadgroup/principal?
> 
> I'd think not, but wanted to check.

I believe we want the same load group for whatever principal is being used.  So we should only walk up the chain if we do the same for the principal.

This should not be necessary because I think this is just the case of loading a child worker for an existing worker.  After this patch, I believe all existing workers should have a load group and principal at this point.

> > +      if (!NS_LoadGroupMatchesPrincipal(channelLoadGroup, channelPrincipal)) {
> 
> This looks wrong. How could these two ever not match? You just got both from
> the same channel so they should always match. Only exception involves
> resource://, and system principals, but I think in that case all appids will
> be NO_APP, so also should match fine.
> 
> Put it another way, if you changed this to just assert that they match,
> under what circumstances would that assertion trigger?

This was a hedge on my part because I didn't fully understand how principals can change through a channel with redirects, etc.  Also, since the code was overriding with a system principal, I thought perhaps they could no longer match.

Since you're telling me this code should not cause a mismatch, I'll strengthen this constraint to an assert.

> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +2107,5 @@
> > +  //  - private browsing = false
> > +  //  - content = true
> > +  //  - use remote tabs = false
> > +  // Alternatively we could persist the original load group values and use
> > +  // them here.
> 
> We definitely want to make sure that a SW installed during private browsing
> doesn't suddenly remain installed during non-private browsing. Or using
> non-private browsing cookies.
> 
> I don't know what the other two flags do, so I don't know if they matter.
> Would be worth checking with someone on the necko team.

ServiceWorker serialization is being handled by Andrea in a separate bug.  He needs to include the private browsing info.

> > +  rv = NS_NewLoadGroup(getter_AddRefs(info.mLoadGroup), info.mPrincipal);
> 
> Seems like something like this is also needed when we create a new shared
> worker? Or will the code in WorkerPrivate::GetLoadInfo take care of that?

I believe this is taken care of automatically for SharedWorker because there we either have a window or a parent worker which is supported by GetLoadInfo().  Also, if this was needed then we would have orange in the try build for any SharedWorkers used.

> > +    if (!NS_LoadGroupMatchesPrincipal(loadInfo.mLoadGroup,
> > +                                      loadInfo.mPrincipal)) {
> 
> See comment above.
> 
> This should likely just be |if (!loadInfo.mLoadGroup)|, and then an
> assertion that the resulting principal/loadgroup pair match.

Will strengthen this as well.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Andrea, please see comment 26 about additional information that we need to serialize for a ServiceWorker.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

So, based on Jonas's comments, I'm basically removing the creation of a load group based on an original load group.  So I can remove the optional aBase arguments to LoadContext() and NS_NewLoadGroup().

I will leave any changes to allow updating the private browsing flag, etc in LoadContext until ServiceWorker is ready to actually use them.  For now we just use the defaults of content=true, private=false, remote tabs=false.

Any objections?

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Verifying these asserts work ok on try:

https://tbpl.mozilla.org/?tree=Try&rev=456e750532b6

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

> ::: dom/workers/WorkerPrivate.cpp
> @@ +4126,5 @@
> >        loadInfo.mReportCSPViolations = false;
> >      }
> >  
> > +    if (!NS_LoadGroupMatchesPrincipal(loadInfo.mLoadGroup,
> > +                                      loadInfo.mPrincipal)) {
> 
> See comment above.
> 
> This should likely just be |if (!loadInfo.mLoadGroup)|, and then an
> assertion that the resulting principal/loadgroup pair match.

So this definitely did not work.  Lots of orange in the try.  Seems to effect almost everything that uses workers...  Maybe I am grabbing the wrong load group to start?

Comment 31

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8532669 [details] [diff] [review]
P3 Make sure all workers have an nsILoadGroup when loading scripts. (v3)

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

This looks fine to me with sicking's comments addressed.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

The document principal and load group do not match.  Let me see if I can determine why...

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Ah, its a bug in my NS_LoadGroupMatchesPrincipal() call.  I was trying to QI the callbacks to nsILoadContext, when I should be using NS_QueryNotificationCallbacks() instead.

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add LoadContext constructor taking an nsIPrincipal. (v2)

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add nsILoadGroup helpers to nsNetUtil.h. (v1)

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make sure all workers have an nsILoadGroup when loading scripts. (v4)

https://tbpl.mozilla.org/?tree=Try&rev=3dd487f0bcf6

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Try was green.  Pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c3b2309fdf93
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ce51e5cd2a85
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5fe0df3298aa

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

B2G emulator mochitests 10 and 16 apparently disagreed with these patches. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7bc8853f1230

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4605161&repo=mozilla-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4605165&repo=mozilla-inbound

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Thanks Wes.  I will look at it.  In hindsight I should have run more than my T-style try since this patch effects b2g app IDs.

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

So these failures are due to data URI workers getting a null principal. On b2g, this effectively loses the appId.

I spoke to Jonas, though, and it appears this is ok.  If a worker has a null principal then the various permission checks will effectively prevent it from doing anything that would actually use the load group values.  I believe it can load another data URI worker, but not perform any network operations.

With that in mind, I intend to make NS_LoadGroupMatchPrincipal() return true if its passed a null principal.

If this try ends up green:

  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=572528ff9389

Then I plan to fold this try commit into P2 and re-push to mozilla-inbound:

  https://hg.mozilla.org/try/rev/b0b2ca3cbaee

Please let me know if there are any objections.  Thanks!

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add nsILoadGroup helpers to nsNetUtil.h. (v2)

Updated P2 patch.  The try looks green.  I got positive confirmation from Jonas that putting the check here looks good.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e29791c42832
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec85c7c9b5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/a880d7b38485

Comment 43

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/e29791c42832
https://hg.mozilla.org/mozilla-central/rev/6ec85c7c9b5f
https://hg.mozilla.org/mozilla-central/rev/a880d7b38485