When loading a website in a container window that was opened/created while all the other windows have been closed under OSX, cookies are being loaded in the wrong container. It looks like there's several duplicate cookies, but one belongs to the default container and the other belongs to the container the user is currently using. So it looks like the website is perhaps being loaded two different times, once in the default container and once in the container that the user is currently using. * attached a video to illustrate the issue ** https://youtu.be/9DFfNxVcR6A STR: (under OSX) * open a brand new profile within m-c * enable containers via about:config -> privacy.userContext.enabled;true * close all the windows that are currently opened * file -> new container tab -> personal * load tumblr.com and check the cookie manager under about:preferences#privacy You should notice that some of the cookies look like they're duplicates and were loaded under the "None" (default) container which is incorrect. All the cookies should belong to the "Personal" container.

This isn't as bad as I initially reported in comment #0. It looks like cookies are being loaded from about:newtab, which is why I was seeing two sets of cookies. Tanvi, should FX be loading cookies from the about:newtab page? If this is the intended behaviour, than this is a nonissue. Either way, I don't think this is a container bug. Apologies for the red flag! STR: Video Example -> https://youtu.be/pIMiJuILYWQ * open a brand new installation of m-c * ensure that ALL history & cookies have been removed * close all windows that are currently opened * open new container via file -> new container tab -> personal * load facebook.com into this container * open about:preferences#privacy in the same container and view the cookies (they should all be listed as "Container: Personal") * now open a new tab which will be using the default container Once you open the second tab, you'll notice that the facebook.com tile will be completely white and will eventually appear after a second or two. I'm assuming about:newtab loads the website, hence the cookies with the default container tag in the cookie manager.

Okay, that sounds a lot better Kamil! Not a firedrill or regression. If the cookie leakage is due to about:newtab, then this is not a blocker for enabling containers. But it is still an issue for containers: 1) Load embarrasing-site.com in your Personal context 2) Open a new default tab. about:newtab loads embarrassing-site.com to get a screenshot and sets cookies for embarrasing-site.com in the default context, even though you never visited it in the default context. 3) Go to other-site.com in the default context. other-site.com embeds embarrasing-site.com. embarrasing-site.com loads and sees you already have cookies set. Hence, it knows that you have visited the page before. But the user hasn't visited this page before in the default context, only in their personal context. So now we are leaking data across containers. No good. We need to learn more about how about:newtab works and if cookies are necessary when capturing the screenshots. If they are, then we have to find ways to solve this data leakage issue. I can come up with many, but none are great. The right thing to do would be to segregate about:newtab by containers, so a new tab in the default context won't show you results from another container. But I believe this is very complicated to implement. When we store History, do we store a URI or a Principal? If a Principal, we can get the OriginAttributes and usercontextID from that. Also, per Kamil it appears that about:newtab uses the default context when loading these thumbnails even if it is not in a default context tab. So that might be an additional bug to handle here. Anyway, we need more information before we can decide how to proceed. Any volunteers to investigate this?

Hi Ed, Do we need cookies to be sent when loading about:newtab and grabbing screenshots? Can we strip cookies from the requests that get the screenshots? Also, do you have a pointer to the code that initiates these network requests? Thank you!

I agree, we shouldn't making those requests with cookies. I was under the impression that we weren't sending any cookies. The screenshots are generated through the PageThumbs codepath in the background. The background pagethumbs framescript loads the content with certain flags set: http://searchfox.org/mozilla-central/source/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js#19

I believe the new tab screenshots are captured here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/BackgroundPageThumbs.jsm It says the page is loaded anonymously. Not sure if that means cookies are supposed to be excluded?

In http://searchfox.org/mozilla-central/source/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js#19 , it looks like the LOAD_ANONYMOUS[1] flag is set and that should strip cookies. Do you know why cookies aren't being stripped. Or maybe there are multiple code paths and they all need to set a strip cookie flag? [1] http://searchfox.org/mozilla-central/source/netwerk/base/nsIRequest.idl#205

IIRC, the LOAD_ANONYMOUS stuff was added explicitly to avoid cookies - I also recall there are tests explicitly to check cookies are not used - but I'm afraid I've lost all context of this code over the last couple of years since I touched it - hopefully Drew knows the current state.

This might need a little more triage, but I'm confident this isn't caused by the "background" thumbnailer - tests like https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/test/browser_thumbnails_bg_no_cookies_sent.js and https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/test/browser_thumbnails_bg_no_cookies_stored.js check the cookie behaviour there. However, this is also the "foreground" thumbnailer that takes snapshots of certain pages as they appear during normal browsing. Given this requires the ContextualIdentity pref to reproduce, I'm going to mark it as blocking that bug so hopefully someone involved in that work can offer more info.

We need to triage this bug - someone to take a closer look at this and figure out what code we need to change.

You(In reply to Mark Hammond [:markh] from comment #8) > This might need a little more triage, but I'm confident this isn't caused by > the "background" thumbnailer - tests like > https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/ > test/browser_thumbnails_bg_no_cookies_sent.js and > https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/ > test/browser_thumbnails_bg_no_cookies_stored.js check the cookie behaviour > there. However, this is also the "foreground" thumbnailer that takes > snapshots of certain pages as they appear during normal browsing. > > Given this requires the ContextualIdentity pref to reproduce, I'm going to > mark it as blocking that bug so hopefully someone involved in that work can > offer more info. I can reproduce this bug even if turning off the container pref. STR: * open a new m-c profile * disable container by turning off pref privacy.userContext.enabled * ensure that ALL history & cookies have been removed * close all windows that are currently opened * load facebook.com * open about:preferences#privacy in the same container and delete all the cookies * now open a new tab Once I open a new tab again, the cookies of facebook.com will appear. Mark, Ed, if you can confirm this bug is actually independent from containers, we need someone who knows about:newtab better than us container folks to take this bug. Thanks.

(In reply to Jonathan Hao [:jhao] from comment #10) > Mark, Ed, if you can confirm this bug is actually independent from > containers, we need someone who knows about:newtab better than us container > folks to take this bug. Yeah, I can reproduce this, and it is the background thumbnailer. The LOAD_ANONYMOUS flag is indeed expected to protect us from this - removing that flag causes the browser_thumbnails_bg_no_cookiestests tests to fail. So I think this might end up similar to bug 909218, which tortuously brings us to https://groups.google.com/forum/#!msg/mozilla.dev.tech.network/X8t4Sob3l9U/l3oyhd1gwo8J. My *guess* is that something like iframes or xhr by the page isn't getting the LOAD_ANONYMOUS, but I'd have to dig deeper to know. What impact on containers does this actually have? It seems likely this bug has existed "forever" with background thumbnails, so it would be good to understand how/if this blocks containers from being unpref'd.

(In reply to Mark Hammond [:markh] from comment #11) > What impact on containers does this actually have? It seems likely this bug > has existed "forever" with background thumbnails, so it would be good to > understand how/if this blocks containers from being unpref'd. Ideally we don't want websites to connect two identities in different containers by the data it stored. In comment 2, Tanvi described a way to exploit this bug and leak information across containers. (In reply to Jonathan Hao [:jhao] from comment #10) > Mark, Ed, if you can confirm this bug is actually independent from > containers, we need someone who knows about:newtab better than us container > folks to take this bug. I can still take this bug. What I was trying to say is that it'd be great if you can help us find the cause and the right fix of this bug. But you're more than welcome to take it if you want. Thank you.

(In reply to Jonathan Hao [:jhao] from comment #12) > Ideally we don't want websites to connect two identities in different > containers by the data it stored. In comment 2, Tanvi described a way to > exploit this bug and leak information across containers. Yeah, not ideal ;) Although not *that* bad - a leak of the fact a site was anonymously visited. If we could demonstrate we *sent* cookies it would be far more urgent IMO. > I can still take this bug. What I was trying to say is that it'd be great if > you can help us find the cause and the right fix of this bug. But you're > more than welcome to take it if you want. Without significant (ie, days) more effort I doubt I could get much further - which is why I was trying to guess urgency. Please take it ;)

If I'm not mistaken, the flag LOAD_ANONYMOUS does not prevent setting cookies by document.cookies. In thumbnails_background.sjs, it only tries to set cookies by Set-Cookie header. That's why the test passes even though this bug exists. I modified the test a little to set cookies by document.cookies, and then the test fails as expected. We should somehow disable using document.cookies=. There's a flag mDisableCookieAccess in nsHTMLDocument, but I don't know if I can get the document in time before it can execute scripts. The load seems to happen here: http://searchfox.org/mozilla-central/source/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js#89

(In reply to Jonathan Hao [:jhao] from comment #14) > If I'm not mistaken, the flag LOAD_ANONYMOUS does not prevent setting > cookies by document.cookies. Yes, that makes sense! > We should somehow disable using document.cookies=. There's a flag > mDisableCookieAccess in nsHTMLDocument, but I don't know if I can get the > document in time before it can execute scripts. The load seems to happen > here: > http://searchfox.org/mozilla-central/source/toolkit/components/thumbnails/ > content/backgroundPageThumbsContent.js#89 I suspect that using the existing progress listener to fetch this._webNav.document (nominally a nsIDOMDocument according to nsIWebNavigation.idl) at the right time might be a path to the nsIHTMLDocument.disableCookieAccess() call which flips that flag. Might also be worth exploring how iframes work too.

I added a declaration the HTMLDocument.webidl so that I can call disableCookieAccess in background thumbnailer. Now I have to find the right place to call it.

(In reply to Jonathan Hao [:jhao] from comment #10) > I can reproduce this bug even if turning off the container pref. > > STR: > * open a new m-c profile > * disable container by turning off pref privacy.userContext.enabled > * ensure that ALL history & cookies have been removed > * close all windows that are currently opened > * load facebook.com > * open about:preferences#privacy in the same container and delete all the > cookies > * now open a new tab > > Once I open a new tab again, the cookies of facebook.com will appear. > Hi Jonathan, Here you are saying that about:newtab is causing new cookies to be created, correct? Not that the previously deleted cookies are somehow coming back?

(In reply to Mark Hammond [:markh] from comment #11) > What impact on containers does this actually have? It seems likely this bug > has existed "forever" with background thumbnails, so it would be good to > understand how/if this blocks containers from being unpref'd. The bug has existed forever, but the consequences have not. The bug allows recently visited sites to view and set cookies while the user is in about:newtab. about:newtab is disabled in private mode so there is no way this would leak data across reguarl and private mode. But with containers, the consequence of this bug is that a site in one container can learn about your browsing behavior in another container. Hence it is a Containers blocker. But it looks like Jonathan is making good progress towards fixing. Thanks Mark for your help and Jonathan for your patches!

This patch can disable document.cookies for background thumbnails. The tests that I modified in the other patch will pass now. Unfortunately, when I tested by the STR in comment 10, two (out of the origin 11) cookies remains. Mark has told me that he suspects iframe or xhr might set cookies, too.

I added an iframe in the response of sjs, and use document.cookies inside.

I disable the cookies of iframes if their parent document is also disabled. But, unfortunately again, when I tested the STR with tumblr, one cookie still remains (which means I got rid of the other one). Its cookie name is anon_id and is a POST request to https://www.tumblr.com/services/cslog I could see it from the browser console. It's not an xhr. When I examined who tried to set the cookie, it was HttpBaseChannel::SetCookie(), so it wasn't related to document.cookie. The LOAD_ANONYMOUS wasn't set. I have no idea what triggered that POST request, and why it doesn't have LOAD_ANONYMOUS set.

(In reply to Jonathan Hao [:jhao] from comment #22) > and why it doesn't have LOAD_ANONYMOUS set. Bug 909218 arranged for that flag to be on all requests in the "load group" - so I suspect this request isn't part of that, being a POST request after load.

This patch is an alternative approach Paul and Mark suggested to me. It uses a temporary container to capture the thumbnails and clear the data afterwards. I think this one is better than the previous one, but I'll keep that unobsolete in case we need it in the future. I'm not sure if it's OK to introduce temporary container this way. Baku, could you review it? Mark, could you review the changes related to background thumbnails? Thank you.

Comment on attachment 8766380 [details] [diff] [review] Capture thumbnails in a temporary container. Review of attachment 8766380 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/contextualidentity/ContextualIdentityService.jsm @@ +50,5 @@ > }, > > getUserContextLabel(userContextId) { > let identity = this.getIdentityFromId(userContextId); > + if (!identity) return "Temp"; IIUC this function is for things that will be presented to the user, and it seems a problem if we are actually hitting it with this patch - and even if we are, it's not clear that "Temp" is the right string to expose (ie, if in reality it is only to be used for diagnostics, the including the userContextId might be useful to the specific value can be tracked down. But if amarchesini is OK with that, I guess I am too. ::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm @@ +431,5 @@ > > PageThumbs._store(this.url, data.finalURL, data.imageData, true) > .then(done, done); > + > + Services.obs.notifyObservers(null, "clear-origin-data", I think we should move this above the _store call or into done (but I think the former is better - once we get here the capture browser is on about:blank so it is safe)

If history entries really do have the write usercontextid associated with them, then the thumbnail service can load with the right set of OriginAttributes. - https://bugzilla.mozilla.org/show_bug.cgi?id=1283320

(In reply to Tanvi Vyas[:tanvi] from comment #27) > If history entries really do have the write usercontextid associated with > them, then the thumbnail service can load with the right set of > OriginAttributes. - https://bugzilla.mozilla.org/show_bug.cgi?id=1283320 The thumbnail service will presumably also need to be changed to that it knows the usercontextid when storing and retrieving previously generated thumbnails (ie, it will need to be able to have multiple thumbnails per contextid for the same URL.)

Comment on attachment 8766380 [details] [diff] [review] Capture thumbnails in a temporary container. Review of attachment 8766380 [details] [diff] [review]: ----------------------------------------------------------------- Before reviewing this I have a question: Can we use private browsing mode for doing this instead?

Comment on attachment 8766380 [details] [diff] [review] Capture thumbnails in a temporary container. Review of attachment 8766380 [details] [diff] [review]: ----------------------------------------------------------------- 2 things: 1. Can we use private browsing instead? It seems more appropriated. 2. ContextualIdentityService must be in charge to create temporary UCI. What about if you introduce: var tmpUserContextId = ContextualIdentityService.createTemporaryUserContextId(); then.. in done: ContextualIdentityService.deleteTemporaryUserContextId(tmpUserContextId); Plus, what about if FF crashes in the meantime? We will keep this tmpUserContextId stored in the profile forever... this is bad. Really I prefer to have privateBrowsing instead. If we cannot, the right solution is to: 1. COntextualIdentityService must use a db to store the temp contextId. 2. when deleteTemporaryUserContextId is called, the tmp contextId is removed from that db. 3. On startup, ContextualIdentityService opens that db and if some temp contextId are still there, it must delete them. ::: browser/components/contextualidentity/ContextualIdentityService.jsm @@ +50,5 @@ > }, > > getUserContextLabel(userContextId) { > let identity = this.getIdentityFromId(userContextId); > + if (!identity) return "Temp"; this should return an empty string. ::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm @@ +35,5 @@ > XPCOMUtils.defineConstant(this, "TEL_CAPTURE_DONE_CRASHED", TEL_CAPTURE_DONE_CRASHED); > XPCOMUtils.defineConstant(this, "TEL_CAPTURE_DONE_BAD_URI", TEL_CAPTURE_DONE_BAD_URI); > > const global = this; > +const tmpUserContextId = 99999; The management of temporary contextId must be done by ContextualIdentityService. @@ +432,5 @@ > PageThumbs._store(this.url, data.finalURL, data.imageData, true) > .then(done, done); > + > + Services.obs.notifyObservers(null, "clear-origin-data", > + JSON.stringify({ userContextId: tmpUserContextId })); Move it into done().

(In reply to Andrea Marchesini (:baku) from comment #29) > Before reviewing this I have a question: Can we use private browsing mode > for doing this instead? That's where thumbnails started - we moved away from that in bug 875986 due to issues when there's an existing PB session (or, say, one starts during capture.) In some ways I see this patch as a move back in that direction, but to a private-private-browsing session :)

Mark, thanks for your comment. Good news is that soon-ish we could have OriginAttributes.privateBrowsingID with something different than 0 and 1. At that point we can use a custom privateBrowsing session just for the thumbnail generation.

Jonathan, ignore all my 'private browsing' thing in the review. But all the other comments are still valid. Thanks!

Thanks, baku. I forgot about the possibility of crashing. Since this temporary solution is getting more and more complex, I think we should wait for private browsing ID, or we should add userContext to history db and use the correct container instead of a temporary one.

I think it's not going to happen soon. Consider to use dom/base/IndexedDBHelper.jsm

Jonathan, another approach would be to introduce an internal-only userContextIds in ContextualIdentityService. What about if: ContextualIdentityService.getIdentities() returns all the 'public' identities, but we also have: ContextualIdentityService.getPrivateIdentity() that returns a internal-only, identity and you use this for the thumbnail browser.

Baku, please take another look. Thanks.

Comment on attachment 8769193 [details] [diff] [review] Capture thumbnails in an internal-only container. Review of attachment 8769193 [details] [diff] [review]: ----------------------------------------------------------------- I like it! But let's use the label to identify the private identity. ::: browser/components/contextualidentity/ContextualIdentityService.jsm @@ +44,5 @@ > + { userContextId: Math.pow(2, 31) - 1, > + public: false, > + icon: "", > + color: "", > + label: "userContextInternal.label", call it 'userContextIdInternal.thumbnail' @@ +54,5 @@ > getIdentities() { > + return this._identities.filter(info => info.public); > + }, > + > + getPrivateIdentity() { getPrivateIdentity(label) { check info.public == false and info.label == label ::: browser/locales/en-US/chrome/browser/browser.properties @@ +723,5 @@ > userContextBanking.label = Banking > userContextShopping.label = Shopping > userContextNone.label = No Container > > +userContextInternal.label = Private This should not be needed. Remove it. ::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm @@ +196,5 @@ > browser.setAttribute("remote", "true"); > browser.setAttribute("disableglobalhistory", "true"); > > + // Use the private container. > + let privateIdentity = ContextualIdentityService.getPrivateIdentity(); ContextualIdentityService.getPrivateIdentity("userContextIdInternal.thumbnail") @@ +426,5 @@ > } > } > + > + // Clear the data in the private container. > + dump("JONATHAN: clear\n"); remove this :)

Thank you, baku. The cookies in the internal container will exist for a short time before it's cleared. It can be seen in the cookie inspector. I'll open another bug to determine if we should allow that. Mark, here's a review request to you again because this patch has been quite different since your last review. Thank you.

Comment on attachment 8769256 [details] [diff] [review] Capture thumbnails in an internal-only container. Review of attachment 8769256 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: browser/components/contextualidentity/ContextualIdentityService.jsm @@ +66,5 @@ > getUserContextLabel(userContextId) { > let identity = this.getIdentityFromId(userContextId); > + if (identity.public) { > + return gBrowserBundle.GetStringFromName(identity.label); > + } else { nit: no "else" after a return

Comment on attachment 8769256 [details] [diff] [review] Capture thumbnails in an internal-only container. Review of attachment 8769256 [details] [diff] [review]: ----------------------------------------------------------------- lgtm as well.

Pushed by jhao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/df69a26957d3 Modify thumbnails tests to make sure cookies can't be set by document.cookies or xhr. r=markh https://hg.mozilla.org/integration/mozilla-inbound/rev/2262d40c51c6 Capture thumbnails in an internal-only container. r=baku,markh

https://hg.mozilla.org/mozilla-central/rev/df69a26957d3 https://hg.mozilla.org/mozilla-central/rev/2262d40c51c6

This bug leaks some data across containers, but doesn't break functionality or pages. So not necessary to uplift to 49 for test pilot for containers.

This bug has effectively caused us to ship containers as a platform level feature, and bug 1289001 suggests that this may not have been a good idea. Was shipping this a conscious decision?

This could be a potential reason for the higher crasher rates on Beta50, ever since 50 went to Beta cycle. I'd appreciate a prompt follow up here. If we need to land a backout, I will be happy to take it asap, I gtb 50.0b7 tomorrow noon PST.

(In reply to :Ehsan Akhgari from comment #46) > This bug has effectively caused us to ship containers as a platform level > feature, and bug 1289001 suggests that this may not have been a good idea. > Was shipping this a conscious decision? In bug 1309699, we have put a pref around the code that landed in this bug.