Closed Bug 722845 Opened 12 years ago Closed 12 years ago

Necko channel infrastructure for nextGen private browsing

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox15 --- disabled
firefox16 --- disabled
firefox17 --- disabled

People

(Reporter: jdm, Assigned: jdm)

References

Details

Attachments

(7 files, 21 obsolete files)

26.28 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
24.60 KB, patch
Details | Diff | Splinter Review
14.79 KB, patch
Details | Diff | Splinter Review
28.04 KB, patch
michal
: review+
Details | Diff | Splinter Review
10.45 KB, patch
michal
: review+
Details | Diff | Splinter Review
2.42 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
3.10 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
The global private browsing service is going away. Accordingly, decisions about things like caching and keep-alive should be based on the private browsing flag of the related docshell, if it exists.
Blocks: 722859
Attachment #593350 - Attachment is obsolete: true
You can't GetCallBack an nsIDocShell, in general.  Most especially not for external resource loads.
Would it make sense to put the "using private browsing" information on nsILoadContext?  You need to either do that or get the window from nsILoadContext and get the docshell from there.
Attachment #593349 - Attachment is obsolete: true
Attachment #593352 - Attachment is obsolete: true
Comment on attachment 593576 [details] [diff] [review]
Part 1: Add private browsing information to HTTP channels.

Things to consider about this patch:
- the goal is to get rid of uses of the global private browsing state
- that state is used by the cache service to decide when to clear the only cache that exists (and by the http handler to close persistent connections)
- I've mitigated the cache problem by creating a separate cache session that private browsing connections use, so no cache clearing is required for either mode
- my persistent connections solution is almost certainly not good enough yet - I inhibit kieepalive connections if it's a private connection. However, presumably this means that non-PB connections could make use of PB ones, which doesn't sound good. This may need a redesign of how connections are stored.
Attachment #593576 - Flags: review?(jduell.mcbugs)
Comment on attachment 593579 [details] [diff] [review]
Part 2: Generalize channel private browsing information to a PB consumer interface.

This provides a much nicer interface for consumers with access to a request object. Thoughts?
Attachment #593579 - Flags: feedback?(jduell.mcbugs)
Blocks: 722849
Patrick may also be able to help with how this affects connection management.

I think there may be quite a few things that have only implicit knowledge of private browsing, which only work correctly now because we shut down and then restart the networking layer during the private browsing transition. For example, I think PSM's SSL session cache works that way.
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Josh Matthews [:jdm] from comment #8)
> - I inhibit kieepalive connections if it's a private connection. However,
> presumably this means that non-PB connections could make use of PB ones,
> which doesn't sound good. This may need a redesign of how connections are
> stored.

Why would this be a problem?
(In reply to Brian Smith (:bsmith) from comment #10)
> Patrick may also be able to help with how this affects connection management.
> 
> I think there may be quite a few things that have only implicit knowledge of
> private browsing, which only work correctly now because we shut down and
> then restart the networking layer during the private browsing transition.
> For example, I think PSM's SSL session cache works that way.

No, we stopped doing that in bug 496335.

I don't understand the first part of your comment though.
Comment on attachment 593576 [details] [diff] [review]
Part 1: Add private browsing information to HTTP channels.

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

I believe that this patch should break some tests, as in the xpcshell test for necko support of PB we don't have docshells at all.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1686,5 @@
> +{
> +  nsCOMPtr<nsILoadContext> loadContext;
> +  GetCallback(loadContext);
> +  if (!loadContext) {
> +    //XXXjdm In my experience, this occurs with safebrowsing lookups. What should we do here?

We should behave as if we're not in PB mode.  IOW, you should just remove this comment and the warning.
Comment on attachment 593579 [details] [diff] [review]
Part 2: Generalize channel private browsing information to a PB consumer interface.

I like this idea.
> I believe that this patch should break some tests, as in the xpcshell test 
> for necko support of PB we don't have docshells at all.

Yes, we need another patch that fixes these tests (test_bug248970_cache.js and test_bug248970_cookie.js).

I believe we should be able to just create a JS object that QIs to nsILoadContext and implements GetUsePrivateBrowsing(), and set it as the channels' notificationCallbacks, and this should work fine in xpcshell.  We don't need full docshells (and maybe not even a full implementation of nsILoadContext).
I want some feedback from Patrick/Michal/Nick/Honza (and anyone who knows cookies) for this.  It touches a lot of stuff that others may know better than I do.

The basic issue here is we're trying to move private browsing from a global setting to one that can be set per-window (or maybe per-tab: the granularity is mostly moot for necko).  This mean we've got to switch from checking a global variable ("are we in PB?") that we've been keeping updated by listening to the private brower service's notifications, to a scheme where each channel knows whether or not it's part of PB, and passes that down to wherever it's needed.

The basic approach here (get the PB setting from the DocShell: under e10s, pass that info to the chrome process channel) seems right.

What I'm wondering about is:

1) connection management: right now the patch simply disables NS_HTTP_ALLOW_KEEPALIVE for PB channels.  That's going to hit performance for PB sessions.   I'm guessing we could instead set some sort of isPB flag per connection, so we know to use it only for private browsing.  Patrick or Honza should be able to answer that more definitively.  (We might be able to make this a followup bug to keep this bug manageable, but I'd hate to slow down existing PB mode if we can avoid it).

2) Caching:  The patch disables disk caching for PB channels.  That's good, but you've left in the cache's own checks to the global private browsing service.  You'll need to get rid of that logic.  Also, there's the end-of-session issue (see #4).

3) Cookies: We also have calls to the PB global service in the cookie code.  Right now it doesn't look like the cookie service is set up to handle simultaneous PB/regular sessions--it's switching a global mDBState variable to/from the regular/private database when it sees notifications that PB is turned on and off.  We'll need a way to pass in whether we're using PB on a per-request basis.  Most of the time the cookie code could get it from the channel itself (using the nsIPrivateBrowsingConsumer interface you're adding), but it looks like 3rd party cookies are requested by omitting the channel parameter (see nsICookieService::getCookieString), so I suspect you'll need to add a PB param to the IDL.  I don't know disruptive changing the cookie IDL would be for plugins, etc.   (This whole issue probably deserves it's own bug, and the infrastructure you're adding should be able to land w/o it).  Finally, as with caching, there's the end-of-session issue.

4) End-of-session.  So in the absence of PB on/off notifications from the global PB service, how do we know when to nuke a PB session's cache and cookies?  (and ideally also shut down any persistent connections, should we support that).  We need some notification when the last window/tab/whatever is closed to indicate that PB session is done.  Also, I assume the model here is that if a user opens multiple windows that are PB, they all share a single PB state, as opposed to each window having it's own PB session (we could support the latter if needed: I'm not sure I see a strong use case for it though).   Should we leave in the existing PB global service, and keep it around just to issue end-of-session notifications?

thanks for taking this on, jdm--it'll be great to have per-window private browsing.

Nits on the actual patches to follow.
(In reply to Jason Duell (:jduell) from comment #16)
> 2) Caching:  The patch disables disk caching for PB channels.  That's good,
> but you've left in the cache's own checks to the global private browsing
> service.  You'll need to get rid of that logic.  Also, there's the
> end-of-session issue (see #4).

See bug 722849.
 
> 4) End-of-session.  So in the absence of PB on/off notifications from the
> global PB service, how do we know when to nuke a PB session's cache and
> cookies?  (and ideally also shut down any persistent connections, should we
> support that).  We need some notification when the last window/tab/whatever
> is closed to indicate that PB session is done.  Also, I assume the model
> here is that if a user opens multiple windows that are PB, they all share a
> single PB state, as opposed to each window having it's own PB session (we
> could support the latter if needed: I'm not sure I see a strong use case for
> it though).   Should we leave in the existing PB global service, and keep it
> around just to issue end-of-session notifications?

I've got a patch that sends an internal (ie. undocumented for addons) notification when the count of private docshells drops to 0. We can have components like the cache, download manager, etc. watching this notification only and clear the private data they're storing.
(In reply to Jason Duell (:jduell) from comment #16)

> 1) connection management: right now the patch simply disables
> NS_HTTP_ALLOW_KEEPALIVE for PB channels.

right - that's way too big of a performance hit. SSL would be a nightmare.

you might be able to just clean the whole persistent connection pool when the private-tab count jdm describes goes to 0.

While that is a little less effective than what we do now (allowing some correlation server side by use of the tcp connection), it satisfies the "leave-no-trace-on-the-computer" guarantee of PB as it has been described to me. but maybe PB really means more than that; I don't know. (though if it did you'd think we'd require SSL or something).

alternatively you could add the PB bool to nsHttpConnectionInfo, which would prevent sharing between PB and non-PB channels. you could then use the drop-to-zero notification to close out any nsconnectionentrys .. right now those structures are non-persistent, but I'd like to change that, so having a bool right in there to mark them PB would be useful.
(In reply to Patrick McManus [:mcmanus] from comment #18)
> While that is a little less effective than what we do now (allowing some
> correlation server side by use of the tcp connection), it satisfies the
> "leave-no-trace-on-the-computer" guarantee of PB as it has been described to
> me. but maybe PB really means more than that; I don't know. (though if it
> did you'd think we'd require SSL or something).

I'm still not sure if we need to worry about persistent connections.  It's not like that they would automatically use the previously sent cookies or something, right?
Comment on attachment 593579 [details] [diff] [review]
Part 2: Generalize channel private browsing information to a PB consumer interface.

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

Is there any reason we can't just add a 'usingPrivateBrowsing' attribute to nsIChannel, and have the docshell set it (except under e10s, where the ParentChannel can set it in RecvAsyncOpen)?  This would 1) keep all docshell stuff out of necko, and follow our usual idiom of having docshell set properties on a channel, rather then channels interrogating docshells (the less we use channel Callbacks--a fundamentally mismatched model for multi-process--the better).  And it would 2) get rid of the whole concept of 'override'. 3) Less IDLs to keep track of.  The downside is that you'll need to make sure you find all sites where the docshell creates a channel, but that shouldn't be too many.  Let's do that unless there's a reason not to that I'm missing.
Attachment #593579 - Flags: feedback?(jduell.mcbugs) → feedback-
(In reply to Ehsan Akhgari [:ehsan] from comment #19)

> I'm still not sure if we need to worry about persistent connections.  It's
> not like that they would automatically use the previously sent cookies or
> something, right?

I'm with you. cookies and connections are technically independent.

however a server operator could reasonably infer that 2 transactions on the same connection are from the same person (and transfer the cookie information herself) - that would be the right conclusion most of the time. What I was trying to say was this seems irrelevant to me because as I understand PB it isn't about what the server or network observers can learn- it is about state leftover in the browser/computer after PB exits. I'm not a domain expert, but if I'm right it would seem you could just leave persistent connections on - or clean them up crudely when PB count goes to 0 for bonus points. (nsHttpConnectionMgr::ClosePersistentConnections() will do that)
> and have the docshell set it

We need this for all subresource channels, no?  Including ones not created by the docshell (font loader, css loader, image loader, etc, etc, etc).

We could maybe set this on the _loadgroup_, but even then there are problems for external resource documents, which have separate loadgroups.
From Firefox help page for PB:

   http://support.mozilla.org/en-US/kb/Private-Browsing

"Private Browsing allows you to browse the Internet without saving any information about which sites and pages you’ve visited...

Warning: Private Browsing doesn't make you anonymous on the Internet. Your Internet service provider, employer, or the sites themselves can still track what pages you visit."

Following that description, I don't think we need to flush persistent connections.  (Ehsan: no, we wouldn't send previously sent cookies: just reuse the TCP pipe), nor disable NS_HTTP_ALLOW_KEEPALIVE.  It's unlikely that users will reuse the same connection in most cases (usually they're browsing different sites in PB mode), but we're not violating any promises if we use a connection for both PB/non-PB channels.
Comment on attachment 593579 [details] [diff] [review]
Part 2: Generalize channel private browsing information to a PB consumer interface.

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

bz makes some good points.  So then this is a splendid way to proceed :)
Attachment #593579 - Flags: feedback- → review+
Comment on attachment 593576 [details] [diff] [review]
Part 1: Add private browsing information to HTTP channels.


So looking some more at the cache code, I think you can leave in the existing logic and still land this infrastructure patch.  Once we have your non-public end-of-session notification, we can change the cache to evict only PB entries when a session ends, not the entire cache (see nsCacheService::OnEnterExitPrivateBrowsing).  That can all be followup work.  (file a bug).

So this is actually pretty much ready to go AFAICT, with minor changes.

>+HttpBaseChannel::UsingPrivateBrowsing()
>+{
>+  nsCOMPtr<nsILoadContext> loadContext;
>+  GetCallback(loadContext);
>+  if (!loadContext) {
>+    //XXXjdm In my experience, this occurs with safebrowsing lookups. What should we do here?
>+    NS_WARNING("No loadContext present; assuming no private browsing");
>+    return false;

I'm inclined to go with Ehsan on this--no warning.  

>+  }
>+  bool pb;
>+  loadContext->GetUsePrivateBrowsing(&pb);
>+  return !!pb;

Why !!pb instead of just 'pb'?  It's already a bool.

>diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp
>@@ -60,16 +60,17 @@
> #include "nsDNSPrefetch.h"
> #include "nsChannelClassifier.h"
> #include "nsIRedirectResultListener.h"
>+#include "nsIDocShell.h"

I don't see why you need nsIDocShell.h here, unless I'm missing something.  

 
>+    // Private browsing capabilities can only be determined in content
>+    // processes, so when networking occurs these values are used in
>+    // lieu of UsingPrivateBrowsing().
>+    bool mOverridePrivateBrowsing;
>+    bool mUsingPrivateBrowsing;

To save space we might as well make these bitfields: see mRequestTimeInitialized and friends. 


>diff --git a/netwerk/protocol/http/nsHttpHandler.cpp b/netwerk/protocol/http/nsHttpHandler.cpp

> 
>-    const char *sessionName = "HTTP";
>+    const char *sessionNames[][3] = {

Should sessionNames be static?  I suppose a decent C++ compiler ought to avoid extra initialization each time we enter function (and it's not much anyway, of course).


>+        { "HTTP", "HTTP-memory-only", "HTTP-offline" },
>+        { "HTTP-PB", "HTTP-memory-only-PB", "HTTP-offline-PB" }

If we want to support multiple PB sessions at a time (one PB context per window, or whatever) we could append some sort of session ID onto the PB strings before passing them to the cache.  Doesn't sound like we're going in that direction.

>     }
>-    else if (strcmp(topic, NS_PRIVATE_BROWSING_SWITCH_TOPIC) == 0) {
>-        if (NS_LITERAL_STRING(NS_PRIVATE_BROWSING_ENTER).Equals(data))
>-            mInPrivateBrowsingMode = PRIVATE_BROWSING_ON;
>-        else if (NS_LITERAL_STRING(NS_PRIVATE_BROWSING_LEAVE).Equals(data))
>-            mInPrivateBrowsingMode = PRIVATE_BROWSING_OFF;
>-        if (mConnMgr)
>-            mConnMgr->ClosePersistentConnections();
>-    }

So if we switch to use your new, non-public notification to indicate that the PB session is ended, and we implement PB-flagged HTTP connections, we could get a little benefit from closing only PB persistent connections (statistically they're likely to not get reused again).  Given the amount of work you're doing to get per-window PB working, I think that can safely wait for another day.  File an enhancement bug.  Meanwhile you can rip this code out.

>+ if (UsingPrivateBrowsing()) {
>+   mCaps &= ~NS_HTTP_ALLOW_KEEPALIVE;
>+ }

As discussed I think we should just scrap this and allow reuse.
Attachment #593576 - Flags: review?(jduell.mcbugs) → review+
Summary: Determine long-lived nature of HTTP channels based on related docshell instead of global Private Browsing state → Necko channel infrastructure for nextGen private browsing
Since this is a privacy feature, doesn't it make more sense to fail safe and assume private browsing mode, unless we can get the load context and the load context tells us that private browsing mode is NOT in effect?
(In reply to Patrick McManus [:mcmanus] from comment #21)
> (In reply to Ehsan Akhgari [:ehsan] from comment #19)
> 
> > I'm still not sure if we need to worry about persistent connections.  It's
> > not like that they would automatically use the previously sent cookies or
> > something, right?
> 
> I'm with you. cookies and connections are technically independent.
> 
> however a server operator could reasonably infer that 2 transactions on the
> same connection are from the same person (and transfer the cookie
> information herself) - that would be the right conclusion most of the time.
> What I was trying to say was this seems irrelevant to me because as I
> understand PB it isn't about what the server or network observers can learn-
> it is about state leftover in the browser/computer after PB exits. I'm not a
> domain expert, but if I'm right it would seem you could just leave
> persistent connections on - or clean them up crudely when PB count goes to 0
> for bonus points. (nsHttpConnectionMgr::ClosePersistentConnections() will do
> that)

Ah that's a deal breaker.  While PB is all about local privacy, when designing it originally we tried to make it hard for a server to tell whether you're in PB mode or not.  I think this might be a regression in that sense from this patch then.  :/
(In reply to Brian Smith (:bsmith) from comment #26)
> Since this is a privacy feature, doesn't it make more sense to fail safe and
> assume private browsing mode, unless we can get the load context and the
> load context tells us that private browsing mode is NOT in effect?

Under what circumstances we won't have a load context exactly?  The answer to this question can help us decide what the default case should be.
(In reply to Ehsan Akhgari [:ehsan] from comment #28)
> (In reply to Brian Smith (:bsmith) from comment #26)
> > Since this is a privacy feature, doesn't it make more sense to fail safe and
> > assume private browsing mode, unless we can get the load context and the
> > load context tells us that private browsing mode is NOT in effect?
> 
> Under what circumstances we won't have a load context exactly?  The answer
> to this question can help us decide what the default case should be.

Safebrowsing channels are the only ones I recall seeing in mochitests so far.
The load context will be missing any time a load is not associated with a docshell.

In practice, that means that any place that manually creates channels and then doesn't add them to a loadgroup before opening them will have no load context.  This could obviously be any C++ or extension code.

However, by definition such loads are not associated with any window.  Any loads web content can trigger will always be in a loadgroup (so the stop button works) and hence will have a load context.
Depends on: 725210
> Ah that's a deal breaker.  While PB is all about local privacy, when 
> designing it originally we tried to make it hard for a server to tell 
> whether you're in PB mode or not.  I think this might be a regression 
> in that sense from this patch then.  :/

I don't see it as violating the promise we make to users for PB, but I'm no privacy czar, so perhaps we should run it by one.

If this is in fact important, than we should implement the "isPB flag per connection" mentioned in comment 16.  That could be a followup.  It might also be something Patrick or Honza or someone else on the necko team could write (though I shouldn't volunteer them :) since jdm may be overloaded with the rest of the PBnextGen work.

The "channel is private by default" sounds nice, but I wouldn't want it if it means we wind up having private channels when the user has never turned on private browsing.
Attachment #593579 - Attachment is obsolete: true
Assignee: nobody → josh
Tests are good! They help you realize that you forgot the pretty important addition to NS_IMPL_ISUPPORTS for your new interface!
Attachment #595448 - Flags: review?(michal.novotny)
Reusing a keep-alive connection for PB channels introduces a way for servers to identify you by tracking previously sent cookies or auth headers on that connection.  Also when reusing an ssl client authenticated connection you directly expose your identity.  Having PB connections is a precondition then.

Image loads must be updated since they share a single channel missing (naturally) both a load group and a load context.  Assuming PB in that case will break image loads, assuming non-PB will break PB.

Is it necessary to throw away the whole memory cache when leaving the last PB docshell?  Do you plan to evict only the special PB sessions you are using?

We must be very careful on what all loads are somehow bound to a window and not belonging to the load context (i.e. PB state unknown).  Favicon loads, for instance, coming to my mind as the case; not 100% sure here, but worth to check.
BTW, we have the 'anonymous' flag in the connection hashing key.  We can just mCaps |= NS_HTTP_LOAD_ANONYMOUS; in nsHttpChannel when PB mode is enforced.  Currently we set this flag in nsHttpChannel::SetupTransaction.
(In reply to Boris Zbarsky (:bz) from comment #30)
> The load context will be missing any time a load is not associated with a
> docshell.
> 
> In practice, that means that any place that manually creates channels and
> then doesn't add them to a loadgroup before opening them will have no load
> context.  This could obviously be any C++ or extension code.
> 
> However, by definition such loads are not associated with any window.  Any
> loads web content can trigger will always be in a loadgroup (so the stop
> button works) and hence will have a load context.

I'm assuming that loads triggered from XHR are also associated with the loadgroup of the window requesting them.  If this is the case, we're fine and we can default to non-PB mode if we don't have a load group.
> I'm assuming that loads triggered from XHR are also associated with the loadgroup of the
> window requesting them. 

Yes, they are.
(In reply to Jason Duell (:jduell) from comment #31)
> > Ah that's a deal breaker.  While PB is all about local privacy, when 
> > designing it originally we tried to make it hard for a server to tell 
> > whether you're in PB mode or not.  I think this might be a regression 
> > in that sense from this patch then.  :/
> 
> I don't see it as violating the promise we make to users for PB, but I'm no
> privacy czar, so perhaps we should run it by one.

Trust me, we want this.  Without this, a website can simply tell whether the user has switched away from PB mode if they receive a request on a channel without their identifier cookie, followed by another request on the same channel with their identifier cookie.

> If this is in fact important, than we should implement the "isPB flag per
> connection" mentioned in comment 16.  That could be a followup.  It might
> also be something Patrick or Honza or someone else on the necko team could
> write (though I shouldn't volunteer them :) since jdm may be overloaded with
> the rest of the PBnextGen work.

I don't want us to defer this to a follow-up since this is a regression from how PB works today.

> The "channel is private by default" sounds nice, but I wouldn't want it if
> it means we wind up having private channels when the user has never turned
> on private browsing.

Agreed, we don't want to do that.
(In reply to Honza Bambas (:mayhemer) from comment #35)
> Reusing a keep-alive connection for PB channels introduces a way for servers
> to identify you by tracking previously sent cookies or auth headers on that
> connection.  Also when reusing an ssl client authenticated connection you
> directly expose your identity.  Having PB connections is a precondition then.

Agreed.

> Image loads must be updated since they share a single channel missing
> (naturally) both a load group and a load context.  Assuming PB in that case
> will break image loads, assuming non-PB will break PB.

You mean we don't associate the channels used to load images on a page with the load context of its window?  That sounds broken!

> Is it necessary to throw away the whole memory cache when leaving the last
> PB docshell?  Do you plan to evict only the special PB sessions you are
> using?

Is there any reason to keep it around when all of the PB docshells have been destroyed?

> We must be very careful on what all loads are somehow bound to a window and
> not belonging to the load context (i.e. PB state unknown).  Favicon loads,
> for instance, coming to my mind as the case; not 100% sure here, but worth
> to check.

Yes, we should check that, maybe bz knows?

(In reply to Honza Bambas (:mayhemer) from comment #36)
> BTW, we have the 'anonymous' flag in the connection hashing key.  We can
> just mCaps |= NS_HTTP_LOAD_ANONYMOUS; in nsHttpChannel when PB mode is
> enforced.  Currently we set this flag in nsHttpChannel::SetupTransaction.

What is the exact semantics of the anonymous flag?  I think that it avoids cookies from being sent, right?  In that case we can't really use it since we do want to support cookies in PB mode.
(In reply to Ehsan Akhgari [:ehsan] from comment #40)
> What is the exact semantics of the anonymous flag?  I think that it avoids
> cookies from being sent, right?  In that case we can't really use it since
> we do want to support cookies in PB mode.

Ah, connection anonymity is not driven by NS_HTTP_LOAD_ANONYMOUS flag (that is btw something else then LOAD_ANONYMOUS flag).

We do the following in SetupTransaction:

  mConnectionInfo->SetAnonymous((mLoadFlags & LOAD_ANONYMOUS) != 0);

We may change this to

  mConnectionInfo->SetAnonymous((mLoadFlags & LOAD_ANONYMOUS) != 0 || UsingPrivateBrowsing());

and we are done.
> You mean we don't associate the channels used to load images on a page with
> the load context of its window?  That sounds broken!

Well, the whole idea of the image cache is to allow images to be shared between pages...
(In reply to Ehsan Akhgari [:ehsan] from comment #39)
> > The "channel is private by default" sounds nice, but I wouldn't want it if
> > it means we wind up having private channels when the user has never turned
> > on private browsing.
> 
> Agreed, we don't want to do that.

Then how do we handle the case where an addon is doing HTTP requests related to a particular document? For example, let's say I have an addon that shows a sidebar of "people tweeting about this page." That will load things like http://myaddon.com/tweetsFor?url=http://example.org. We have to make sure that the load of http://myaddon.com/tweetsFor?url=http://example.org respects private browsing mode (e.g. bypasses the disk cache), even if the addon wasn't explicitly done anything for private browsing mode, because it leaks the fact that you visited http://example.org.
(In reply to Josh Matthews [:jdm] from comment #29)
> Safebrowsing channels are the only ones I recall seeing in mochitests so far.

Don't we have to ensure that Safebrowsing channels also obey private browsing mode? For example, if one of the Safebrowsing requests were cached in the disk cache, then couldn't you tell that the user visited a particular URL by calculating the safe browsing hash for that URL, and then checking the disk cache to see of a request with that hash was made to the safe browsing server? (I don't know much about safe browsing; maybe there is some feature of safe browsing that makes it a non-issue. But, if so, then we should make sure we're not putting safe browsing content in the disk cache at all, because AFAICT any mechanism that would make this a non-issue would also mean that the memory cache would be sufficient for safe browsing.)
Comment on attachment 595448 [details] [diff] [review]
Part 3: Remove cache service use of private browsing service, and replace it with a single observer notification.

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

::: netwerk/cache/nsCacheService.cpp
@@ +2660,5 @@
> +nsCacheService::LeavePrivateBrowsing()
> +{
> +    nsCacheServiceAutoLock lock;
> +
> +    gService->DoomActiveEntries();

Michal: do all active entries need to be doomed, or is there a way we can doom just the PB ones?  We know for instance that any entries involving the disk cache are not PB (the 1st patch makes sure we store only in memory for PB).  We could also pass in a sessionID to this function if that helps (see my next comment).

@@ +2664,5 @@
> +    gService->DoomActiveEntries();
> +
> +    if (gService->mMemoryDevice) {
> +        // clear memory cache
> +        gService->mMemoryDevice->EvictEntries(nsnull);

jdm: so you went to the effort in the other patch to have nsHttpHandler create separate cache profiles ("HTTP-PB", "HTTP-memory-only-PB", "HTTP-offline-PB") for PB.  I assume it would be better to evict only those entries, rather than the whole memory cache.  So perhaps we should have nsHttpHandler listen to "last-pb-context-exited" instead of this class, and have it call LeavePrivateBrowsing with a new, optional sessionID parameter.  And it should probably keep track of which types of PB sessions have actually been opened, so it doesn't call for all three of ("HTTP-PB", "HTTP-memory-only-PB", "HTTP-offline-PB") if that's not needed. (AFAICT we'll never use 'HTTP-PB' since we set STORE_IN_MEMORY for PB, right?  Stick in an assert that we never use it, and give the string a name like "HTTP-PB-UNUSED".)  I'm not sure if we'll pass the offline string--probably?  I'll ask Honza.
Honza:

What's our story for how the Application cache currently interacts with private browsing?  Will it be affected by the nextGen PB logic?  I don't see any references to nsIPrivateBrowsingService in the code.  Do we flush (or omit storing) stuff into the Appcache for PB, or does the appcache not even know about PB (and is that OK?).
(In reply to Jason Duell (:jduell) from comment #46)
> Honza:
> 
> What's our story for how the Application cache currently interacts with
> private browsing?  Will it be affected by the nextGen PB logic?  I don't see
> any references to nsIPrivateBrowsingService in the code.  Do we flush (or
> omit storing) stuff into the Appcache for PB, or does the appcache not even
> know about PB (and is that OK?).

From the first look it seems like we don't block writes to app cache in PB at all, either first downloads or updates.  It might be a bug :)

We should allow app cache to work, but load from and store in memory, initially blank.

There is also the per-host offline-permission flag.  We should not expose it in PB mode, and if set (by user) in PB mode, then discard it after leaving PB mode.

App cache is using its own device implementation and it is strictly disk cache.  So, we may try to use a memory cache device for store, best a new instance dedicated to app cache.

Note: app cache is not using the HTTP-offline session (or what the name is) at all, that's a leftover.  The session name is the URL of the app cache manifest that uniquely identifies the cache.

I think this deserves its own bug.
jdm: It looks like at present we only create appcache/STORE_OFFLINE sessions directly via nsICacheService rather than via nsHttpHandler:GetCacheSession, so I guess "HTTP-offline-PB" will never get used either.   I guess add in asserts that we don't ever see any flag other than STORE_IN_MEMORY in GetCacheSession's storagePolicy param when inPrivateBrowsing is passed, and flush only the "HTTP-memory-only-PB" clientID from nsHttpHandler.

One other possible option:  we could add a STORE_PRIVATE flag to nsCacheStoragePolicy, and set it for PB sessions.  Then the cache itself could keep track of which sessions are private and flush them in LeavePrivateBrowsing().  That's a nice clean design (rather than forcing cache clients to keep track of and flush PB sessions). But right now nsCacheStoragePolicy is not set up as a set of flags, but a mutually exclusive set of enums, so maybe it's more work than it's worth. 

Speaking of cache clients, you're also going to need to have FTPChannel keep track of PB (only store in memory, flush when leaving), and also WYCIWYG channels.  Which I guess starts to be an argument for some sort of STORE_PRIVATE flag approach, so the cache can keep track of flushing them (or we could just flush the whole memory cache, as your patch does now, but that still seems suboptimal).
> I think this deserves its own bug.

Filed bug 725792 for appcache work.
(In reply to Jason Duell (:jduell) from comment #42)
> > You mean we don't associate the channels used to load images on a page with
> > the load context of its window?  That sounds broken!
> 
> Well, the whole idea of the image cache is to allow images to be shared
> between pages...

Right, but if an image is in the image cache, we don't need to make a network request for it, so that won't be a problem.  But we can still tell which docshell has requested for an image to be loaded, right?
(In reply to Brian Smith (:bsmith) from comment #44)
> (In reply to Josh Matthews [:jdm] from comment #29)
> > Safebrowsing channels are the only ones I recall seeing in mochitests so far.
> 
> Don't we have to ensure that Safebrowsing channels also obey private
> browsing mode? For example, if one of the Safebrowsing requests were cached
> in the disk cache, then couldn't you tell that the user visited a particular
> URL by calculating the safe browsing hash for that URL, and then checking
> the disk cache to see of a request with that hash was made to the safe
> browsing server? (I don't know much about safe browsing; maybe there is some
> feature of safe browsing that makes it a non-issue. But, if so, then we
> should make sure we're not putting safe browsing content in the disk cache
> at all, because AFAICT any mechanism that would make this a non-issue would
> also mean that the memory cache would be sufficient for safe browsing.)

Maybe, I don't know much about safe browsing either.  But the current behavior is for those requests to be made outside of the PB mode, so I don't want us to change that here.
(In reply to Jason Duell (:jduell) from comment #48)
> jdm: It looks like at present we only create appcache/STORE_OFFLINE sessions
> directly via nsICacheService rather than via nsHttpHandler:GetCacheSession,
> so I guess "HTTP-offline-PB" will never get used either.   

HTTP-offline is unused, so I think HTTP-offline-PB won't be either.

> One other possible option:  we could add a STORE_PRIVATE flag 

STORE_PRIVATE may be a good option here, but I don't know the cache stuff 100% well.

I would also suggest to have a smaller total limit for the 'private' memory cache.  We may have a lot of stuff in the memory cache very soon and exhaust process's RAM, so we should evict more aggressively or even using a different algorithm.  Also (an old topic) throwing entries related to a tab (anonymous) that had not been accessed a longer time would be a good policy here.
> I would also suggest to have a smaller total limit for the 'private' memory cache.

On the other hand, the memory cache is all that private browsing gets to use, so we might just want to resign ourselves to the fact that RAM cache entries from before private browsing are likely to be evicted by the time PB ends.  I don't see any particular harm in that.
(In reply to Jason Duell (:jduell) from comment #53)
> > I would also suggest to have a smaller total limit for the 'private' memory cache.
> 
> On the other hand, the memory cache is all that private browsing gets to
> use, so we might just want to resign ourselves to the fact that RAM cache
> entries from before private browsing are likely to be evicted by the time PB
> ends.  I don't see any particular harm in that.

Not sure what you mean.  My concern is that we may easily get OOM when in PB and also normally browsing.  There is already a bug about huge memory consumption in PB mode.
ah, bug 713203.  Well, sure, we need to be enforcing the regular RAM cache limit for PB just as for non-PB.   Let's keep that discussion in that bug.  I don't think it affects this one (other than that we need to be sure we're enforcing a single total RAM limit for both PB/non-PB once we support using both at the same time: but I assume we'll get that for free--the cache limits are per cache, not per session IIUC).
Comment on attachment 595448 [details] [diff] [review]
Part 3: Remove cache service use of private browsing service, and replace it with a single observer notification.

(In reply to Jason Duell (:jduell) from comment #45)
> @@ +2660,5 @@
> > +nsCacheService::LeavePrivateBrowsing()
> > +{
> > +    nsCacheServiceAutoLock lock;
> > +
> > +    gService->DoomActiveEntries();
> 
> Michal: do all active entries need to be doomed, or is there a way we can
> doom just the PB ones?

In fact we need to doom only those active entries with the specific sessionID that are not bound to any device since the bound entries are doomed by EvictEntries() method of the device. But I think that this behavior is wrong and that unbound entries should be doomed by nsCacheSession::EvictEntries() too.


> @@ +2664,5 @@
> > +    gService->DoomActiveEntries();
> > +
> > +    if (gService->mMemoryDevice) {
> > +        // clear memory cache
> > +        gService->mMemoryDevice->EvictEntries(nsnull);
> 
> jdm: so you went to the effort in the other patch to have nsHttpHandler
> create separate cache profiles ("HTTP-PB", "HTTP-memory-only-PB",
> "HTTP-offline-PB") for PB.  I assume it would be better to evict only those
> entries, rather than the whole memory cache.  So perhaps we should have
> nsHttpHandler listen to "last-pb-context-exited" instead of this class, and
> have it call LeavePrivateBrowsing with a new, optional sessionID parameter. 

This seems to me as a reasonable approach. With the fix mentioned above it would be sufficient just to call EvictEntries() on the appropriate session.
Attachment #595448 - Flags: review?(michal.novotny) → review-
michal,

what do you think of having a STORE_PRIVATE flag (see comment 48)?  Then we wouldn't need to have separate HTTP/FTP/Wyciwyg code that listens for the end-of-PB notification and flushes each protocol's private cache separately.
Comment on attachment 593576 [details] [diff] [review]
Part 1: Add private browsing information to HTTP channels.

clearing +r since there's a bunch of stuff that needs to happen to the patch (marking HTTP conections as private and shutting them down at end-of-PB; FTP and Wyciwyg PB sessions;  flushing the PB cache sessions, or passing STORE_PRIVATE is that's the way we go; and the nits in comment 25).

There's also the image/addons/safebrowsing load issues, which I don't think we've figured out the design for yet.
Attachment #593576 - Flags: review+ → review-
(In reply to Jason Duell (:jduell) from comment #57)
> what do you think of having a STORE_PRIVATE flag (see comment 48)?  Then we
> wouldn't need to have separate HTTP/FTP/Wyciwyg code that listens for the
> end-of-PB notification and flushes each protocol's private cache separately.

The question is if we want to allow to store PB sessions on disk sometime in future. We keep nsCacheStoragePolicy only in memory, so we would need to store it in the cache map (i.e. change a cache version) to be able to use the disk cache for PB.
(In reply to Ehsan Akhgari [:ehsan] from comment #50)
> Right, but if an image is in the image cache, we don't need to make a
> network request for it, so that won't be a problem.  But we can still tell
> which docshell has requested for an image to be loaded, right?

Imagine this:

- A non-PB tab (TAB1) loading www.example.com with ref to <img href="www.foo.com/image.jpg">

- A PB tab loading (TAB2) www.mozilla.org with ref to the same <img href="www.foo.com/image.jpg">

- Internally the instances will be connected like:

  imgRequestProxy 
  (in TAB1 loadGroup) --\
                         --- imgRequest --- nsHttpChannel
  imgRequestProxy     --/                   (in a fake loadGroup) 
  (in TAB2 loadGroup)


In other words, nsHttpChannel is shared by two imgRequestProxy channels from two different pages (tabs).  So, nsHttpChannel has no idea if it has to do PB or non-PB load.

The code that assigns imgRequestProxy to imgRequest has to change.  The mapping must also include the 'anonymous' flagging and we have to assign a proper (artificial) nsILoadContext to the underlying channel to query for PB state.
> The question is if we want to allow to store PB sessions on disk sometime in future.

Yes, I think we'll eventually want to store PB sessions in a separate disk cache that we can DeleteDir (or otherwise efficiently destroy w/o doing a linear eviction scan through the entire (non-PB and PB combined) disk cache.

The question of whether to use a STORE_PRIVATE flag seems a different issue to me, though--that's just about whether cache clients need to manually destroy their PB sessions at end-of-PB, or whether the disk cache will do it for them.  I still like the latter option if possible.
(In reply to Honza Bambas (:mayhemer) from comment #35)
> We must be very careful on what all loads are somehow bound to a window and
> not belonging to the load context (i.e. PB state unknown).  Favicon loads,
> for instance, coming to my mind as the case; not 100% sure here, but worth
> to check.

This is a good point. http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/AsyncFaviconHelpers.cpp#632 shows that we just create a channel with no special context; that will need fixing.
Attachment #595448 - Attachment is obsolete: true
I'm moving the imgRequest concerns from comment 60 to bug 722861.
Attachment #611079 - Attachment is obsolete: true
Blocks: 741059
Blocks: 722979
Blocks: 722861
Attachment #611137 - Attachment is obsolete: true
I made the anonymity change that Honza suggested, but I'm unclear if that prevents connection reuse like the proposed changes to nsHttpConnectionInfo. Honza, care to comment?
Attachment #614367 - Flags: review?(jduell.mcbugs)
Attachment #593576 - Attachment is obsolete: true
Attachment #595450 - Attachment is obsolete: true
Attachment #611062 - Attachment is obsolete: true
Attachment #614370 - Flags: review?(jduell.mcbugs)
Attachment #614351 - Attachment is obsolete: true
(In reply to Josh Matthews [:jdm] from comment #69)
> Created attachment 614367 [details] [diff] [review]
> Part 1: Add private browsing information to HTTP channels.
> 
> but I'm unclear if that
> prevents connection reuse like the proposed changes to nsHttpConnectionInfo.

I'm not sure I follow/remember, can you lead me to bugs/comments please?
I believe comment 18 is the clearest suggestion for dealing with reusable connections.
With actual passing tests this time.
Attachment #614377 - Flags: review?(michal.novotny)
Attachment #614369 - Attachment is obsolete: true
Attachment #614369 - Flags: review?(michal.novotny)
(In reply to Josh Matthews [:jdm] from comment #74)
> I believe comment 18 is the clearest suggestion for dealing with reusable
> connections.

Ah, yes.  We may use ANON connections for PB docshells and also for ANON xhr from "normal" docshell.  So that way we would mix PB and non-PB with a potential to let this be detected by the server.

I have to think of this a bit, as Patrick said, we may need another marking for connections like 'this connection pool is just PB'.
Comment on attachment 614354 [details] [diff] [review]
Part 0: Add STORE_PRIVATE flag to indicate transient cache entries for private browsing.

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

So you've added STORE_PRIVATE as a separate mode (which essentially means "STORE_MEMORY + PB").  That interface assumes we're only storing to memory when using PB (so it will work now, I think, unless app cache entries need to be marked PB/not-PB, which is unclear--Honza?).

Long run it would be nice to have PB entries stored on disk too (it doesn't look like so much work to create a separate session for them?), so I'd be happier with having STORE_PRIVATE be a flag that can be combined with the other modes.  That would require us changing the APIs of lots of functions to pass in the extra parameter, though.

::: netwerk/cache/nsCacheService.cpp
@@ +1409,5 @@
>      }
>  
>      if (storagePolicy == nsICache::STORE_ANYWHERE ||
> +        storagePolicy == nsICache::STORE_IN_MEMORY ||
> +        storagePolicy == nsICache::STORE_PRIVATE) {

Seems like it would make sense to write an inline function (similar to nsCacheEntry.h's "IsAllowedInMemory") that takes storagePolicy and returns a bool, so you don't have to repeat this over and over (it's also more readable).
Comment on attachment 614367 [details] [diff] [review]
Part 1: Add private browsing information to HTTP channels.

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

IFF we're OK with having STORE_PRIVATE mean "store in memory only for PB", this looks like +r, with the one open question being Honza's issue, so I'll pass r? onto him.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +646,5 @@
>      if (mTimingEnabled)
>          mCaps |= NS_HTTP_TIMING_ENABLED;
>  
> +    mConnectionInfo->SetAnonymous((mLoadFlags & LOAD_ANONYMOUS) != 0 ||
> +                                 UsingPrivateBrowsing());

So this is the part that Honza wants to think about (comment 76) since it may mix up XHR and PB requests.  We'll need to figure that out before I can +r this.
Attachment #614367 - Flags: review?(jduell.mcbugs)
Attachment #614367 - Flags: review?(honzab.moz)
Attachment #614367 - Flags: review+
Comment on attachment 614368 [details] [diff] [review]
Part 2: Generalize channel private browsing information to a PB consumer interface.

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

::: netwerk/base/src/PrivateBrowsingConsumer.h
@@ +69,5 @@
> + protected:
> +  bool UsingPrivateBrowsing()
> +  {
> +    nsCOMPtr<nsILoadContext> loadContext;
> +    NS_QueryNotificationCallbacks(mSelf, loadContext);

Dumb C++ question:  since the classes that use this logic inherit from PrivateBrowsingConsumer, can't you omit mSelf and just QI(this)?
Attachment #614368 - Flags: review?(jduell.mcbugs) → review+
So we chatted about this a bit on IRC today.  Jason, if given bug 725792 comment 2, there are still points you're not clear about for the reviews, please let us know.  Thanks!  :-)
Comment on attachment 614370 [details] [diff] [review]
Part 4: Add PB information to wyciwyg channels.

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

So this contains some non-wyciwyg stuff, and I'm confused by the IDL change.  Otherwise looks fine.  I'll wait for jdm to comment on IDL change before +r

::: netwerk/protocol/ftp/FTPChannelChild.cpp
@@ +193,5 @@
>    if (mLoadGroup)
>      mLoadGroup->AddRequest(this, nsnull);
>  
>    SendAsyncOpen(nsBaseChannel::URI(), mStartPos, mEntityID,
> +                IPC::InputStream(mUploadStream), UsePrivateBrowsing());

Looks like you already did this in Part 2

::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp
@@ +2258,3 @@
>      nsCOMPtr<nsICacheSession> session;
> +    cache->CreateSession(sessionName,
> +                         policy,

belongs in Part 2 patch?

Assuming we land all these patches at same time, this isn't a big deal (i.e don't re-cut new patches just for this).

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1074,5 @@
>                  IPC::InputStream(mUploadStream), mUploadStreamHasHeaders,
>                  mPriority, mRedirectionLimit, mAllowPipelining,
>                  mForceAllowThirdPartyCookie, mSendResumeAt,
>                  mStartPos, mEntityID, mChooseApplicationCache, 
> +                appCacheClientId, mAllowSpdy, UsePrivateBrowsing());

Looks like this belongs in the HTTP patch.

::: netwerk/protocol/wyciwyg/PWyciwygChannel.ipdl
@@ +54,5 @@
>  
>    Init(URI uri);
>    AsyncOpen(URI      originalURI,
> +            PRUint32 loadFlags,
> +            bool     usingPrivateBrowsing);

Hmm, you're modifying AsyncOpen signature here, but I don't see any change in the WyciwygChannel.cpp.  Instead you set mPrivate in WriteToCacheEntry.   Don't see how this patch would compile w/o change to AsyncOpen.
Attachment #614370 - Flags: review?(jduell.mcbugs)
So discussing w/Ehsan on IRC, he made the point that we don't want to store PB data on disk.  So perhaps we don't need to have STORE_PRIVATE just be another member of the enum.   Really comes down to whether it will play nicely with appcache.
(In reply to Jason Duell (:jduell) from comment #81)

The important thing to note in this patch (which really should just be rolled into part 2) is that I renamed nsPrivateBrowsingConsumer::UsingPrivateBrowsing to nsPrivateBrowsingConsumer::UsingPrivateBrowsingInternal, but I changed all the call sites to use nsIPrivateBrowsing::UsePrivateBrowsing. I did this because I caught a problem where I was using the incorrect function in the chrome process which wouldn't check the override flag.

> Comment on attachment 614370 [details] [diff] [review]
> Part 4: Add PB information to wyciwyg channels.
> 
> Review of attachment 614370 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So this contains some non-wyciwyg stuff, and I'm confused by the IDL change.
> Otherwise looks fine.  I'll wait for jdm to comment on IDL change before +r

Do you mean IPDL, since there is no IDL change?

> ::: netwerk/protocol/ftp/FTPChannelChild.cpp
> @@ +193,5 @@
> >    if (mLoadGroup)
> >      mLoadGroup->AddRequest(this, nsnull);
> >  
> >    SendAsyncOpen(nsBaseChannel::URI(), mStartPos, mEntityID,
> > +                IPC::InputStream(mUploadStream), UsePrivateBrowsing());
> 
> Looks like you already did this in Part 2

Nope, see above.

> ::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp
> @@ +2258,3 @@
> >      nsCOMPtr<nsICacheSession> session;
> > +    cache->CreateSession(sessionName,
> > +                         policy,
> 
> belongs in Part 2 patch?
> 
> Assuming we land all these patches at same time, this isn't a big deal (i.e
> don't re-cut new patches just for this).
> 
> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +1074,5 @@
> >                  IPC::InputStream(mUploadStream), mUploadStreamHasHeaders,
> >                  mPriority, mRedirectionLimit, mAllowPipelining,
> >                  mForceAllowThirdPartyCookie, mSendResumeAt,
> >                  mStartPos, mEntityID, mChooseApplicationCache, 
> > +                appCacheClientId, mAllowSpdy, UsePrivateBrowsing());
> 
> Looks like this belongs in the HTTP patch.
> 
> ::: netwerk/protocol/wyciwyg/PWyciwygChannel.ipdl
> @@ +54,5 @@
> >  
> >    Init(URI uri);
> >    AsyncOpen(URI      originalURI,
> > +            PRUint32 loadFlags,
> > +            bool     usingPrivateBrowsing);
> 
> Hmm, you're modifying AsyncOpen signature here, but I don't see any change
> in the WyciwygChannel.cpp.  Instead you set mPrivate in WriteToCacheEntry.  
> Don't see how this patch would compile w/o change to AsyncOpen.

Please note: IPDL, not IDL, so all SendAsyncOpen and RecvAsyncOpen calls are updated.
Comment on attachment 614370 [details] [diff] [review]
Part 4: Add PB information to wyciwyg channels.

> IPDL, not IDL

ah, right.  OK.
Attachment #614370 - Flags: review+
Comment on attachment 614367 [details] [diff] [review]
Part 1: Add private browsing information to HTTP channels.

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

Since some work needs to be done, I'm r-'ing.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +243,5 @@
>        }
>      }
>    }
>  
> +  httpChan->OverridePrivateBrowsing(usingPrivateBrowsing);

HttpChannelParentListener is assigned as callbacks of the real channel.  Just let it implement nsILoadContext and pass the PB info through it.  It refers HttpChannelParent.

This is an idea, please double check it before implementing it ! :)

But is elegant, gets rid of OverridePrivateBrowsing(), and may give us some future advantages too.  Consult with boris and jason too.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +139,5 @@
>      , mCustomConditionalRequest(false)
>      , mFallingBack(false)
>      , mWaitingForRedirectCallback(false)
>      , mRequestTimeInitialized(false)
> +    , mOverridePrivateBrowsing(false)

initialize mUsingPrivateBrowsing as well please.

@@ +646,5 @@
>      if (mTimingEnabled)
>          mCaps |= NS_HTTP_TIMING_ENABLED;
>  
> +    mConnectionInfo->SetAnonymous((mLoadFlags & LOAD_ANONYMOUS) != 0 ||
> +                                 UsingPrivateBrowsing());

If I'm right, then we need another dimension here:

for "normal" context: anon and non-anon connections
for "private" context: a separate set of anon and non-anon connections

So, connection info has to add another char to the key, probably "P" this time, set through SetPrivate().  Just copy how Set/GetAnonymous works.  Check all places where it is used.

@@ +3149,5 @@
>  
> +    if (UsingPrivateBrowsing()) {
> +        rv = mCacheEntry->SetStoragePolicy(nsICache::STORE_PRIVATE);
> +        if (NS_FAILED(rv)) return rv;
> +    }

if (UsingPrivateBrowsing()) {
}
else if (mLoadFlags...) {
}

?

@@ +5376,5 @@
>      nsCacheStoragePolicy policy = nsICache::STORE_ANYWHERE;
>      if (mLoadFlags & INHIBIT_PERSISTENT_CACHING)
>          policy = nsICache::STORE_IN_MEMORY;
> +    if (UsingPrivateBrowsing())
> +        policy = nsICache::STORE_PRIVATE;

Same here...

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +481,3 @@
>          break;
> +    case nsICache::STORE_PRIVATE:
> +        sessionName = sessionNames[1][1];

Why using an array then?  Just literals will do here.
Attachment #614367 - Flags: review?(honzab.moz) → review-
> HttpChannelParentListener is assigned as callbacks of the real channel. 
> Just let it implement nsILoadContext and pass the PB info through it.

Well, it's not so "elegant": we'd have to fudge the rest of the nsILoadContext IDL (we can return NULL for the windows: would we have to send appType/isContent from child just to have it available?).  But I do agree that the basic concept of having the Parent classes tell the channels about PB is cleaner than adding the nsIPrivateBrowsingConsumer.idl.  Not sure what's actually better overall once the details are hammered out.
I find the use of nsIPrivateBrowsingConsumer much more pleasant in the multitude of dependent bugs than the prospect of getting the notification callbacks and obtaining the load context from them.
Comment on attachment 614354 [details] [diff] [review]
Part 0: Add STORE_PRIVATE flag to indicate transient cache entries for private browsing.

>      const nsCacheStoragePolicy STORE_ANYWHERE        = 0;
>      const nsCacheStoragePolicy STORE_IN_MEMORY       = 1;
>      const nsCacheStoragePolicy STORE_ON_DISK         = 2;
>      const nsCacheStoragePolicy STORE_ON_DISK_AS_FILE = 3;
>      const nsCacheStoragePolicy STORE_OFFLINE         = 4;
> +    const nsCacheStoragePolicy STORE_PRIVATE         = 5;

I'm not sure that we should store this kind of information in StoragePolicy. From the point of storage selection is STORE_PRIVATE the same as STORE_IN_MEMORY. The fact that the entry should be evicted at some point should be IMO stored as some flag. Also STORE_PRIVATE, as such, does not guarantee any privacy. This is ensured by using a cache session with a different clientID.
Attachment #614354 - Flags: review?(michal.novotny)
(In reply to Jason Duell (:jduell) from comment #86)
> > HttpChannelParentListener is assigned as callbacks of the real channel. 
> > Just let it implement nsILoadContext and pass the PB info through it.
> 
> Well, it's not so "elegant": we'd have to fudge the rest of the
> nsILoadContext IDL (we can return NULL for the windows: would we have to
> send appType/isContent from child just to have it available?).  But I do
> agree that the basic concept of having the Parent classes tell the channels
> about PB is cleaner than adding the nsIPrivateBrowsingConsumer.idl.  Not
> sure what's actually better overall once the details are hammered out.

Understand.  What about:

nsIPrivateBrowsingConsumer {
  readonly attribute bool isUsingPrivateBrowsing;
}

nsILoadContext : nsIPrivateBrowsingConsumer {
  void usePrivateBrowsing(bool);
}

and then let nsHttpChannel (and others) demand just nsIPrivateBrowsingConsumer instead of the whole nsILoadContext.

But that is just a suggestion.
Try run for 76032f21344d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=76032f21344d
Results (out of 15 total builds):
    exception: 13
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-76032f21344d
Try run for 6daeeb52048f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6daeeb52048f
Results (out of 132 total builds):
    exception: 15
    success: 40
    warnings: 35
    failure: 42
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-6daeeb52048f
Attachment #618101 - Flags: review?(michal.novotny)
Attachment #614367 - Attachment is obsolete: true
Attachment #614368 - Attachment is obsolete: true
Attachment #614377 - Attachment is obsolete: true
Attachment #614377 - Flags: review?(michal.novotny)
Attachment #614370 - Attachment is obsolete: true
Attachment #614354 - Attachment is obsolete: true
I will note that there is a functionality loss with regards to the cache changes as these patches currently exist - the transition code used to disable the offline cache when entering PB mode, and we no longer do that.
Whoops, inverted a condition in RemoveActiveEntry.
Attachment #618108 - Flags: review?(michal.novotny)
Attachment #618105 - Attachment is obsolete: true
Attachment #618105 - Flags: review?(michal.novotny)
Try run for 2884e045fbe4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2884e045fbe4
Results (out of 129 total builds):
    success: 81
    warnings: 47
    other: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-2884e045fbe4
Blocks: 748890
Comment on attachment 618102 [details] [diff] [review]
Part 1: Add private browsing information to HTTP channels.

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2278,5 @@
>                                   getter_AddRefs(session));
>          NS_ENSURE_SUCCESS(rv, rv);
>  
> +        rv = session->SetIsPrivate(UsingPrivateBrowsing());
> +        NS_ENSURE_SUCCESS(rv, rv);

What is this for?  We just read.

@@ +5415,5 @@
> +}
> +
> +void
> +nsHttpChannel::OverridePrivateBrowsing(bool usingPrivateBrowsing)
> +{

Hmmm.. we may want to add a check this is not called after AsyncOpen.  At least an assertion.

::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
@@ +98,2 @@
>      clone->SetAnonymous(mHashKey.CharAt(2) == 'A');
> +    clone->SetPrivate(mHashKey.CharAt(3) == 'P');

I'd rather see GetAnonymous() and GetPrivate() here.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1047,5 @@
>      PipelineFeedbackInfo(ci, RedCorruptedContent, nsnull, 0);
> +
> +    ci = ci->Clone();
> +    ci->SetPrivate(false);
> +    PipelineFeedbackInfo(ci, RedCorruptedContent, nsnull, 0);

Just to prevent future mistakes and make the code more readable, could you please add ci->SetXXX(true/false) explicitly for both Private and Anonymous on all places?


However!  As I think of this more (may be to paranoid), we may want to separate this for private and non-private.  I.e. when the error is detected on a non-private connection, we shouldn't disable pipelining on private connections.  It could be used to detect private browsing easily just by a corrupted image in a content page.

But this might be an edge case, so probably for a follow up, if it even makes sense it self :)  On the other hand, PB tabs are in a role of a completely separate browser, IMO...
Attachment #618102 - Flags: review?(honzab.moz) → review+
Comment on attachment 618101 [details] [diff] [review]
Part 0: Add private flag for cache entries/sessions/requests.

> -                 getter_AddRefs(session));
> +                  getter_AddRefs(session));

Remove this change.


>      bool IsValid()           { return (mFlags & eValidMask) != 0; }
>      bool IsInvalid()         { return (mFlags & eValidMask) == 0; }
>      bool IsInUse()           { return IsBinding() ||
>                                          !(PR_CLIST_IS_EMPTY(&mRequestQ) &&
>                                            PR_CLIST_IS_EMPTY(&mDescriptorQ)); }
>      bool IsNotInUse()        { return !IsInUse(); }
> +    bool IsPrivate()         { return !!(mFlags & ePrivateMask); }

Please use the common style in this file, i.e. (mFlags & ePrivateMask) != 0


> +    bool   IsPrivate() { return !!(mInfo & ePrivateMask); }

The same as above.


>      enum CacheRequestInfo {
> -        eStoragePolicyMask         = 0x000000FF,
> +        eStoragePolicyMask         = 0x0000007F,
> +        ePrivateMask               = 0x00000080,

Don't change eStoragePolicyMask and use some unused bit (e.g. 0x200). BTW this would make nsCacheRequest::eStoragePolicyMask different from nsCacheEntry::eStoragePolicyMask.


>  nsCacheSession::nsCacheSession(const char *         clientID,
>                                 nsCacheStoragePolicy storagePolicy,
> -                               bool                 streamBased)
> +                               bool                 streamBased,
> +                               bool                 isPrivate)
>      : mClientID(clientID),
>        mInfo(0)

isPrivate is always false so there is no need to have it as an argument.


>          entry = new nsCacheEntry(request->mKey,
>                                   request->IsStreamBased(),
> +                                 request->IsPrivate(),
>                                   request->StoragePolicy());

This is the only place where isPrivate could be set to true. So my suggestion is to remove isPrivate argument from nsCacheEntry::nsCacheEntry() and nsCacheEntry::Create() and assume that it is always false. And here in nsCacheService::ActivateEntry() do

if (request->IsPrivate())
  entry->MarkPrivate();


Also we need to ensure that private entry cannot be bound to disk device in nsCacheService::EnsureEntryHasDevice().
Attachment #618101 - Flags: review?(michal.novotny) → review-
>However!  As I think of this more (may be to paranoid), we may want to separate this 
>for private and non-private.  I.e. when the error is detected on a non-private 
>connection, we shouldn't disable pipelining on private connections.  It could be 
>used to detect private browsing easily just by a corrupted image in a content page.
>
>But this might be an edge case, so probably for a follow up, if it even makes sense 
>it self :)  On the other hand, PB tabs are in a role of a completely separate 
>browser, IMO...

I looked into this, and it should probably be a followup. We don't have any contextual information for the url at present, and the object that dispatches the notification is imgRequest.
Attachment #618101 - Attachment is obsolete: true
Try run for b20a80c5e70e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b20a80c5e70e
Results (out of 132 total builds):
    exception: 6
    success: 105
    warnings: 21
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-b20a80c5e70e
Attachment #620062 - Flags: review?(michal.novotny) → review+
Attachment #618108 - Flags: review?(michal.novotny) → review+
The test fails reliably on all platforms without these changes, due to our nondeterministic garbage collection routines.
Attachment #621462 - Flags: review?(ehsan)
This patch is an attempt to restore the status quo of disabling the offline cache for PB-enabled windows. Please tell me if it's even remotely right?
Attachment #621465 - Flags: review?(honzab.moz)
Comment on attachment 621462 [details] [diff] [review]
Part 1.5: Make PB docshell notification test more deterministic in the face of uncertain GC behaviour.

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

r=me with the below comment.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_lastpbcontextexited.js
@@ +46,5 @@
>          is(aTopic, "last-pb-context-exited", "Correct topic should be dispatched");
> +        is(expected, true, "notification not expected yet");
> +        Services.obs.removeObserver(observer, "last-pb-context-exited", false);
> +        gPrefService.clearUserPref("browser.privatebrowsing.keep_current_session");
> +        finish();

Clearing this pref during this notification might have weird implications in the future (in case we choose to keep supporting this pref!).  Please move these two lines to an executeSoon callback.
Attachment #621462 - Flags: review?(ehsan) → review+
Comment on attachment 621465 [details] [diff] [review]
Part 5: Disable offline cache entries for private channels.

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

I don't think this patch is a good solution.

What is the actual goal of this patch?  As I see it:

- we may want to disallow offline cache reading when in PB mode just to be safe from user profile tracking (and therefor enclosure of being in PB mode) e.g. via unique fallback URIs ; I don't see another reason to that, though
- we definitely want to prevent writes to offline cache, i.e. performing cache updates, those are however triggered from nsContentSink that's well connected with the docshell, so it's close the information about being in PB mode

nsContentSink is the class that has to prevent offline cache update scheduling ; with this patch we will run the update mechanism, but just won't write anything to the offline cache, we'll only do no-op reads that would also expose being in PB mode after the main page reload (it wouldn't be cached)

Updates are coalesced.  When offline cache update is first scheduled by a PB docshell and then by a non-PB docshell, the operation of the update is void since the channels read PB state from the PB docshell only.


I think the patch should:
- (optionally) prevent read from offline cache, the change in nsDocShell.cpp is enough to achieve that
- prevent scheduling of updates from PB only docshells ; best it should behave like there would be no manifest attribute - put that change to nsContentSink::ProcessOfflineManifest

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2293,5 @@
>  
>      nsCOMPtr<nsICacheSession> session;
>  
>      // If we have an application cache, we check it first.
> +    if (mApplicationCache && !UsingPrivateBrowsing()) {

You already ensure there is no mApplicationCache by return false from nsDocShell::ShouldCheckAppCache when mInPrivateBrowsing.
Attachment #621465 - Flags: review?(honzab.moz)
The goal of this patch was to emulate the existing behaviour that I removed in the cache service patch, namely that we disable the offline cache while in PB mode and re-enable it upon exiting. Thanks for the suggestions.
Blocks: 722990
Is this better?
Attachment #623840 - Flags: review?(honzab.moz)
Is this better?
Attachment #623842 - Flags: review?(honzab.moz)
Attachment #623842 - Attachment is obsolete: true
Attachment #623842 - Flags: review?(honzab.moz)
Comment on attachment 623840 [details] [diff] [review]
Part 5: Disable offline cache entries for private channels.

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2497,5 @@
>          return NS_OK;
>  
> +    // We don't support the offline cache in private browsing mode.
> +    if (UsingPrivateBrowsing())
> +        return NS_OK;

This will never return |true| in this situation.  There are no callbacks implementing nsILoadContext on offline cache update channels.

Remove these lines then.
Attachment #623840 - Flags: review?(honzab.moz) → review+
Attachment #621465 - Attachment is obsolete: true
\o/
Target Milestone: --- → mozilla15
No longer blocks: 722990
Depends on: 758852
Blocks: 775119
Depends on: pbchannelfail
No longer blocks: 741059
Depends on: 741059
Backed out from Aurora and Beta as well.
Depends on: 802885
You need to log in before you can comment on or make changes to this bug.