+++ This bug was initially created as a clone of Bug #870179 +++ Before bug 870179 was fixed, if you opened a private window, logged in to a site in that window, and then used BackgroundPageThumbs to capture a thumbnail of that site, your logged-in page was shown in the thumbnail. That's exactly what we're trying to avoid. It happened because, in an effort to visit sites anonymously, BackgroundPageThumbs uses a xul:browser with a PB load context, same as the browser in the private window, and the design of PB is such that there's only a single shared pool of PB state for all PB windows. Bug 870179's fix prevents thumbnail captures when any private windows are open, but that's not a good solution. Thumbnail captures should happen anonymously, regardless of whatever else the user is doing. Ehsan, do you have any thoughts on a better fix? Should we not be using PB at all? Should we add support for one-off anonymous page loads?

Needinfoing... please see comment 0.

I'm *shocked* to learn that we've been using PB to do this. PB is not synonymous to anonymous browsing. What is it exactly that you're trying to achieve?

Uh oh. We're trying to load pages "anonymously" in the browser that we use to capture thumbnails in this new thumbnail service. If the user is logged in to a site in a browser window (private or not) and we capture a thumbnail of that site, the thumbnail should not reveal any logged-in state. Instead it should show the site as if the user had visited it without being logged in. We're not actually using this new service yet.

(In reply to comment #3) > Uh oh. We're trying to load pages "anonymously" in the browser that we use to > capture thumbnails in this new thumbnail service. If the user is logged in to > a site in a browser window (private or not) and we capture a thumbnail of that > site, the thumbnail should not reveal any logged-in state. Instead it should > show the site as if the user had visited it without being logged in. Just to confirm my understanding, are you *only* talking about cookies? IOW, if there was a magic flag which would let you capture the thumbnail without cookies being sent to the website, would that suffice? > We're not actually using this new service yet. Thank god! :-)

Hmm, I think that would suffice...

(In reply to comment #5) > Hmm, I think that would suffice... And we're doing this stuff from an isolated content process, right? If yes, then it seems to me that the right fix would be to extend nsHttpHandler::GetProtocolFlags and friends to return URI_FORBIDS_COOKIE_ACCESS when run in the special content process that we use for this purpose. That ensures that the channels created in that process will never attempt to access the cookie store.

Well, we're using a content process. :-) Just a remote browser, so there's nothing that makes it more or less isolated or special.

(In reply to comment #7) > Well, we're using a content process. :-) Just a remote browser, so there's > nothing that makes it more or less isolated or special. Then I don't know of an easy solution, unfortunately. I suggest you ask on dev.platform...

Couldn't we just, say, annotate the xul:browser or its docshell to do what you suggest and forward that annotation wherever necessary? I mean if we only need some mechanism to signal that URI_FORBIDS_COOKIE_ACCESS should be used, there are probably other ways besides sniffing for a special child process, no? I'm up for (my) patching whatever needs to be patched in core to accomplish this. :-)

(In reply to comment #9) > Couldn't we just, say, annotate the xul:browser or its docshell to do what you > suggest and forward that annotation wherever necessary? I mean if we only need > some mechanism to signal that URI_FORBIDS_COOKIE_ACCESS should be used, there > are probably other ways besides sniffing for a special child process, no? The HTTP handler doesn't know about the docshell that has initiated the channel load, as far as I know, which is why doing what you suggest may not be easy.

Wait, hold up - we want more than just cookie isolation. We also want to: a) avoid persisting any new state as a result of these loads (affecting history, etc.) b) avoid _using_ any saved state to inform the load, which isn't just limited to cookies AFAIK, private browsing mostly serves this purpose. I agree that it's not quite exactly what we want (and leads to issues like this one), but it's pretty close. Do you have specific concerns about our use of it for this purpose, other than the issue described in this bug?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #11) > Wait, hold up - we want more than just cookie isolation. We also want to: > a) avoid persisting any new state as a result of these loads (affecting > history, etc.) > b) avoid _using_ any saved state to inform the load, which isn't just > limited to cookies That does sound like functionality that PB provides. > AFAIK, private browsing mostly serves this purpose. I agree that it's not > quite exactly what we want (and leads to issues like this one), but it's > pretty close. Do you have specific concerns about our use of it for this > purpose, other than the issue described in this bug? I don't have specific concerns, but that is mostly because I have not spent a lot of time thinking about this, and we have definitely not been thinking about this use case when designing how PB should work. It's mostly fear of the unknown. Does that make sense to you?

Ehsan, assuming you're now more receptive to using private browsing here, do you have any thoughts on avoiding the problem mentioned in comment 0? Is there nothing we can do short of redesigning PB to support something like per-window sandboxed PB?

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #12) > I don't have specific concerns, but that is mostly because I have not spent > a lot of time thinking about this, and we have definitely not been thinking > about this use case when designing how PB should work. It's mostly fear of > the unknown. Does that make sense to you? Of course - some skepticism is warranted :) I think there aren't any "easy" solutions here - we're trying to do something novel and Gecko just doesn't have any existing tools for handling our use case. An attractive-sounding solution would be a way to launch a child process that uses a null profile, or something like that, but e10s wasn't designed to support that (there are many inter-dependencies between a child process and its parent). We probably need wider input on the best approach overall here. It may be useful to describe the problem overall, build a list of requirements (comment 11 is a start), explain our current approach and its difficulties, and then post to dev.platform to see if anyone else has ideas or suggestions for alternate solutions.

(In reply to comment #13) > Ehsan, assuming you're now more receptive to using private browsing here, do > you have any thoughts on avoiding the problem mentioned in comment 0? Is there > nothing we can do short of redesigning PB to support something like per-window > sandboxed PB? There is no easy way to make PB work with what you want for cookies. A solution for that needs to be implemented regardless.

(In reply to comment #14) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #12) > > I don't have specific concerns, but that is mostly because I have not spent > > a lot of time thinking about this, and we have definitely not been thinking > > about this use case when designing how PB should work. It's mostly fear of > > the unknown. Does that make sense to you? > > Of course - some skepticism is warranted :) > > I think there aren't any "easy" solutions here - we're trying to do something > novel and Gecko just doesn't have any existing tools for handling our use case. > An attractive-sounding solution would be a way to launch a child process that > uses a null profile, or something like that, but e10s wasn't designed to > support that (there are many inter-dependencies between a child process and its > parent). > > We probably need wider input on the best approach overall here. It may be > useful to describe the problem overall, build a list of requirements (comment > 11 is a start), explain our current approach and its difficulties, and then > post to dev.platform to see if anyone else has ideas or suggestions for > alternate solutions. To be honest, I have not thought a lot about what the implications of using PB for this purpose are going to be. Figuring that out requires some proper thought, and of course knowing exactly what our requirements are. I'm afraid I'm not going to have enough time to think this through properly in the next few weeks. :( But it's a great idea to start figuring out what the requirements are, and get feedback from other people. Just to make sure we're on the same page, this won't get enabled by default in the mean time, right?

I came up with these three requirements. Does anyone have any comments before I post to dev.platform? (a) Requests must be stateless. Requests must not include any information derived from previous requests. e.g., requests must not include cookies. (b) Requests must leave no trace. Requests must not be stored in whole or part, and no information about or derived from requests must be stored. e.g., the request must not be recorded in the user's history. (c) Responses must leave no trace. Responses must not be stored in whole or part, and no information about or derived from responses must be stored. e.g., cookies returned by the response must be ignored, and the response must not be recorded in the user's history. They're intentionally broad. Like, they don't enumerate all the components in Firefox that are affected and state what they must do to accommodate us. One, I don't think we know what all the components are, and that's what gathering wider feedback will hopefully tell us. And two, we should explain what we need rather than trying to prejudge how to do it. There's a kind of second tier of requirements that's not necessarily related to privacy or logged-in or -out status. I'm thinking of things like: * requests should not block the main thread (e.g., we're using a remote browser) * ignore HTTP basic auth * assume there's no human looking at the browser in which these requests are made -- maybe there's no browser at all What do people think about those kinds of things?

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #16) > Just to make sure we're on the same page, > this won't get enabled by default in the mean time, right? We do want to enable it soon (bug 870100) get some testing feedback on whether the service works in general. This bug doesn't seem like a blocker to doing that to me - do you disagree?

(In reply to Drew Willcoxon :adw from comment #17) > There's a kind of second tier of requirements that's not necessarily related > to privacy or logged-in or -out status. I'm thinking of things like: > > * requests should not block the main thread (e.g., we're using a remote > browser) > * ignore HTTP basic auth > * assume there's no human looking at the browser in which these requests are > made -- maybe there's no browser at all > > What do people think about those kinds of things? I think you should mention the "don't block the main thread" requirement. The second two can probably be grouped together as "this process needs to happen in the background and not be visible to the user" requirement. Overall that's a good summary of the requirements on the service in general, I think. When posting to dev.platform, I think it would also be good to add a quick high-level summary of our current solution (and its issues).

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #18) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #16) > > Just to make sure we're on the same page, > > this won't get enabled by default in the mean time, right? > > We do want to enable it soon (bug 870100) get some testing feedback on > whether the service works in general. This bug doesn't seem like a blocker > to doing that to me - do you disagree? Comment 0 seems to suggest that the current code works contrary to what has been intended. If that's fine with you, who am I to disagree? :-) Responding to comment 17, this seems like a good agenda for a multi-person-year long project! Do we _really_ need all of these guarantees? It seems like PB + cookies disabled gives you most of what you want. I seriously recommend against turning this into a full fledged PB-like mode, that will require an enormous amount of effort.

Bug 902439 will make us use LOAD_ANONYMOUS, and bug 908962 will investigate using other load flags to disable caching and history in the thumbnail browser. We think that these two bugs combined may allow us to stop using private browsing mode.

This patch depends on bug 909218 (although it's not marked as dependency while that bug is still in flux). It just sets the docShell.isAnonymous flag, so doesn't arrange for other flags we might desire (LOAD_BYPASS_CACHE etc), but it does seem to work as desired - all tests pass, which include tests that no cookies are sent, etc. Obviously the private-browsing specific tests have been changed to ensure the thumbnail *is* captured. The biggest part of the patch is a test-only content script that adds a progress listener to watch every request made, and check that LOAD_ANONYMOUS is actually set on the request. Arguably this is testing bug 909218 more than this bug, but it still seems worthwhile to me (especially as bug 909218 is unlikely to land with tests)

Comment on attachment 796496 [details] [diff] [review] 0002-Bug-875986-use-custom-docShell-flags-instead-of-priv.patch Review of attachment 796496 [details] [diff] [review]: ----------------------------------------------------------------- This suddenly becomes very easy if we have a docshell.isAnonymous flag. :-) But are we also going to end up with docshell.bypassCache, docshell.bypassHistory, etc.? Does this patch not fix bug 902439 (and does it belong there)? (In reply to Mark Hammond (:markh) from comment #22) > Arguably this is testing bug 909218 more than this bug, but it still seems > worthwhile to me (especially as bug 909218 is unlikely to land with tests) Bug 909218 ought to land with a test I would think. It's the right thing to do, plus you wouldn't have to worry about it here. ::: toolkit/components/thumbnails/test/browser_thumbnails_background.js @@ +63,5 @@ > + let window = subject.defaultView; > + if (window.location.href == "chrome://global/content/mozilla.xhtml") { > + let loadHandler = function(event) { > + window.removeEventListener("DOMContentLoaded", loadHandler); > + // add a mutation observer to notice when our <browser> is added. ... and removed. This is fairly complex. If I were writing this I'd probably make BackgroundPageThumbs broadcast observer service notifications when the browser is created and destroyed. But it's probably a relatively unimportant implementation detail. @@ +116,5 @@ > + let messageListener = function(msg) { > + mm.removeMessageListener("test:pong", messageListener); > + doCheck(); > + }.bind(this); > + mm.addMessageListener("test:pong", messageListener); Do you know whether the remote process is killed when the browser is removed from its parent, or when its refcount becomes 0? If the former, there's a race here, since the browser may be removed while we're waiting on the pong, right? The mutation listener should doCheck() if the browser is removed while this message listener exists.

Comment on attachment 796496 [details] [diff] [review] 0002-Bug-875986-use-custom-docShell-flags-instead-of-priv.patch I didn't look at the details very closely, Drew can provide better feedback there than I can. But I like the idea (perhaps obviously! :) and comment 22 sounds fine.

(In reply to Drew Willcoxon :adw from comment #23) > This suddenly becomes very easy if we have a docshell.isAnonymous > flag. :-) But are we also going to end up with docshell.bypassCache, >docshell.bypassHistory, etc.? A good question I've no idea to the answer of :) There is some discussion in that bug on that very question. > Does this patch not fix bug 902439 (and does it belong there)? It only 1/2 fixes it (well - probably more like 8/10 fixes it). Bug 910207 is responsible for other cases when the auth prompt is shown (which is why I'm doing this patch in this bug - it "fixes" this bug but only "helps" 910207) > Bug 909218 ought to land with a test I would think. It's the right thing to > do, plus you wouldn't have to worry about it here. Yeah, I guess I should investigate that (I wasn't sure how much test infra exists around docShells). Assuming we can do that, do you then think I should nuke most test changes in this patch? > This is fairly complex. If I were writing this I'd probably make > BackgroundPageThumbs broadcast observer service notifications when the > browser is created and destroyed. But it's probably a relatively > unimportant implementation detail. Yeah, it's a bit messy, but I'm still in my camp of preferring to avoid runtime overhead if it just means more complex tests. > Do you know whether the remote process is killed when the browser is removed > from its parent, or when its refcount becomes 0? If the former, there's a > race here, since the browser may be removed while we're waiting on the pong, > right? The mutation listener should doCheck() if the browser is removed > while this message listener exists. I'm not sure TBH - I'm actually not sure how we interact with "unexpected" termination of the child process - it would be useful to have a way to know for sure the child is dead - we could use it here as well as ensure BackgroundPageThumbs itself doesn't end up too upset if that happens.

Very similar to the last patch, although we now include the flags LOAD_ANONYMOUS, LOAD_BYPASS_CACHE, INHIBIT_CACHING and LOAD_FLAGS_BYPASS_HISTORY. Also, as bug 909218 did land with tests, most of the new code in the test suite has been removed.

A new test - this test attempts to set a cookie during the capture, then checks it wasn't actually saved. This didn't demonstrate any problems (ie, it worked as expected) but I figure it's a worthwhile addition.

Comment on attachment 800618 [details] [diff] [review] 0001-Bug-875986-part-1-use-custom-docShell-flags-instead-.patch Review of attachment 800618 [details] [diff] [review]: ----------------------------------------------------------------- Very nice! ::: toolkit/components/thumbnails/test/Makefile.in @@ +17,5 @@ > head.js \ > background_red.html \ > background_red_scroll.html \ > background_red_redirect.sjs \ > + background_test_content_helper.js \ This line is added in this patch and then removed in the tests patch, and the referenced file isn't in either. Probably a remnant from an older patch, but just want to clear that up.

Comment on attachment 800619 [details] [diff] [review] 0002-Bug-875986-part-2-additional-test-that-no-cookies-ar.patch Review of attachment 800619 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: toolkit/components/thumbnails/test/browser_thumbnails_background.js @@ +248,5 @@ > + function noCookiesStored() { > + let url = testPageURL({ setRedCookie: true }); > + yield capture(url); > + let file = fileForURL(url); > + ok(file.exists(), "Thumbnail file should exist after capture."); Please test that the file doesn't exist before the capture, and remove it after the capture.

Thanks - fixed that strange Makefile.in non-change. https://hg.mozilla.org/integration/fx-team/rev/b152bdd937eb https://hg.mozilla.org/integration/fx-team/rev/6bee20c2e0c6

(In reply to Drew Willcoxon :adw from comment #29) > Comment on attachment 800619 [details] [diff] [review] > 0002-Bug-875986-part-2-additional-test-that-no-cookies-ar.patch > > Review of attachment 800619 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks! > > ::: toolkit/components/thumbnails/test/browser_thumbnails_background.js > @@ +248,5 @@ > > + function noCookiesStored() { > > + let url = testPageURL({ setRedCookie: true }); > > + yield capture(url); > > + let file = fileForURL(url); > > + ok(file.exists(), "Thumbnail file should exist after capture."); > > Please test that the file doesn't exist before the capture, and remove it > after the capture. Oops, I screwed up this part - so 2 more pushes to fix it :( https://hg.mozilla.org/integration/fx-team/rev/5fa36cd1823d https://hg.mozilla.org/integration/fx-team/rev/81f13346eb7c

https://hg.mozilla.org/mozilla-central/rev/b152bdd937eb https://hg.mozilla.org/mozilla-central/rev/6bee20c2e0c6 https://hg.mozilla.org/mozilla-central/rev/5fa36cd1823d https://hg.mozilla.org/mozilla-central/rev/81f13346eb7c

Do we really want LOAD_BYPASS_CACHE? Is the reasoning is that the cache might include some sensitive information? I don't know how likely that is to occur in practice, and not letting these requests get served by the cache seems unfortunate. I guess it's probably not a big deal either way.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #33) > Do we really want LOAD_BYPASS_CACHE? Is the reasoning is that the cache > might include some sensitive information? I don't know how likely that is to > occur in practice, and not letting these requests get served by the cache > seems unfortunate. I guess it's probably not a big deal either way. The thinking there was that Tarak specifically called out "don't do disk io" as something we should aim for. I'm inclined to think that without that consideration, allowing the cache to satisfy the request would be fine.