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.

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.

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

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

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

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

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

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

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

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

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

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

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.

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.

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.

Reflagged that last patch because it felt like some non trivial changes. https://tbpl.mozilla.org/?tree=Try&rev=3794e4c42912

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

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

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

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?

Verifying these asserts work ok on try: https://tbpl.mozilla.org/?tree=Try&rev=456e750532b6

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

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

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.

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

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

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.

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!

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

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

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