In services/sync/Weave.js there is a call to createCodebasePrincipal that needs to be aware of originAttributes. > services/sync/Weave.js > line 189 > let ssm = Cc["@mozilla.org/scriptsecuritymanager;1"] > .getService(Ci.nsIScriptSecurityManager); > let principal = ssm.createCodebasePrincipal(uri, {}); I'm not sure if we want to use the default user context or if it is more complicated than that. If we're sync'ing cookies, we want isolation but if we're sync'ing bookmarks maybe we don't?

IIUC, that code is simply trying to make a new channel to gain access to files in the profile directory for about:sync-log - it's not used to actual syncing at all. I don't know enough about channels, but I suspect that code was cargo-culted and will have an easy fix - all we need is for about:sync-log to work.

(In reply to Mark Hammond [:markh] from comment #1) > IIUC, that code is simply trying to make a new channel to gain access to > files in the profile directory for about:sync-log What are the files used for? What does about:sync-log have on it (I don't see anything when I access it on my browser). My guess is that a default origin attributes could be used here, but a little more info would help to confirm that.

(In reply to Tanvi Vyas [:tanvi] from comment #2) > What are the files used for? What does about:sync-log have on it (I don't > see anything when I access it on my browser). It has a typical "index" view of a directory in your profile, containing log files written by Sync via Log.jsm. It should appear on any profile where Sync is configured. > My guess is that a default origin attributes could be used here, but a > little more info would help to confirm that. I'd guess that too, although there always seems to be funkiness with file:// URIs.

this makes sure the origin attributes used to create the principal for the channel that accesses the sync log files is set to the default user context. we don't want any user context isolation for this (ATM).

Comment on attachment 8709215 [details] [diff] [review] Bug_1233904.patch rs+=me if all you want is "gee, I dunno what this does but the people in bug 1218479 seem smart". But I'll bounce this to markh in case he wants to say more than he already has. :)

Comment on attachment 8709215 [details] [diff] [review] Bug_1233904.patch Review of attachment 8709215 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me.

This is a rebased and updated patch to match the changes in Bug 1229222. This cannot land until 1229222 does.

BTW, Weave/Sync is going to need a much more thorough audit to make sure that in all cases, the principals it uses all have the default user context id origin attribute set. We have decided that we are not going to isolate bookmarks, usernames/passwords, etc based on user context id in the first version of the feature. That means sync needs to always be working in the default user context id. I am especially concerned with the sync tabs part of this feature. We need to make sure that the serialization of tabs includes the full origin attributes so that upon restoration, it can recreate tabs in contexts.

This patch cleans up the previous patch to use the new API defined in bug 1229222. The goal here is to make the about:sync-log page to use the default user context id.

Comment on attachment 8715012 [details] [diff] [review] Bug_1233904.patch Review of attachment 8715012 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/sync/Weave.js @@ +191,3 @@ > let ssm = Cc["@mozilla.org/scriptsecuritymanager;1"] > .getService(Ci.nsIScriptSecurityManager); > + let attrs = ChromeUtils.setDefaultUserContextId(aLoadInfo.originAttributes); Why can't this just do: let attrs = aLoadInfo.originAttributes; attrs.userContextId = 0; ?

(In reply to Jonas Sicking (:sicking) from comment #10) > Comment on attachment 8715012 [details] [diff] [review] > Bug_1233904.patch > > Review of attachment 8715012 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: services/sync/Weave.js > @@ +191,3 @@ > > let ssm = Cc["@mozilla.org/scriptsecuritymanager;1"] > > .getService(Ci.nsIScriptSecurityManager); > > + let attrs = ChromeUtils.setDefaultUserContextId(aLoadInfo.originAttributes); > > Why can't this just do: > > let attrs = aLoadInfo.originAttributes; > attrs.userContextId = 0; > > ? Again, we don't want to have manual assignments. I originally submitted some patches like that to Bobby and he rejected it for maintenance reasons. I think he was right, plus there's the JS -> C++ -> JS transition that forces all members to be defined and initialized with default values.

updated for new API.

Comment on attachment 8722051 [details] [diff] [review] Bug_1233904.patch Review of attachment 8722051 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/sync/Weave.js @@ +191,4 @@ > let ssm = Cc["@mozilla.org/scriptsecuritymanager;1"] > .getService(Ci.nsIScriptSecurityManager); > + let attrs = ChromeUtils.originAttributesFromDict(aLoadInfo.originAttributes); > + attrs.userContextId = 0; Change this to assert that userContextId is already 0. If it isn't, something very fishy is going on.

removed assignment per review feedback.

Comment on attachment 8722088 [details] [diff] [review] Bug_1233904.patch Review of attachment 8722088 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/sync/Weave.js @@ +191,4 @@ > let ssm = Cc["@mozilla.org/scriptsecuritymanager;1"] > .getService(Ci.nsIScriptSecurityManager); > + let attrs = ChromeUtils.originAttributesFromDict(aLoadInfo.originAttributes); > + let principal = ssm.createCodebasePrincipal(uri, attrs); No need to call originAttributesFromDict here. Both because aLoadInfo.oA will already return a real OA, and because createCodebasePrincipal would accept a dict. So just call ssm.createCodebasePrincipal(uri, aLoadInfo.oA)

already r+ by :sicking. I'm incorporating the last change he requested.

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=664839308e4b

lot's of reds in windows 7. let's try this one more time with newer code: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee1f410e061c

looks good, ready to land.

+r by :sicking, updating the commit message to be more verbose.