722850 - Manipulate cookie DB based on per-request private browsing data

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Josh Matthews [:jdm]]

Since the global PB service is going away, we can't just store a current database to use based on the global PB status.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Query the private browsing status of channels used to manipulate cookies.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Check the private browsing status of channels when checking cookie permissions.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Query the private browsing status of channels used to manipulate cookies.

Comment 4

[Josh Matthews [:jdm]]

Comment on attachment 595452 [details] [diff] [review]
Query the private browsing status of channels used to manipulate cookies.

I don't recall who the main person for cookies is any longer.

Comment 5

[Jason Duell]

IANACMP (I am not a cookie module peer :) but here's an IRC chat about what I'm wondering about with this patch:

(Summary: how do we know whether private browsing is on if null is passed as the
channel to cookie functions?)

So we're changing private browsing (PB ) to be on a per-channel (actually per window, but channel keeps track of it) basis
This means keeping two databases around in the cookie service, instead of switching the "active" one between PB and regular
that's fine
The tricky part is that we can't ask a global PB service any more whether PB is on
<biesi> sounds great :)
If the cookie functions are passed a channel, they can query the channel to ask if its a PB channel
But the function in nsICookieSerice also allow null to be passed for channel (3rd party cookies, anything else?)
And I don't know how we're going to imply PB/non-PB in that case
Also looks like nsNPAPI also doesn't pass channel in for its cookie calls
biesi: I'm guessing we may need to add a "isPB" parameter to all the cookie IDL calls?
<biesi> jduell, yeah, or enforce a channel to be passed...

Comment 6

[Jason Duell]

Comment on attachment 593359 [details] [diff] [review]
Check the private browsing status of channels when checking cookie permissions.

dwitte has agreed to review these.

Comment 7

[Jason Duell]

Dan, see bug 722845 for the necko channel changes that allow you to get PB status (not landed yet).

Comment 8

[Josh Matthews [:jdm]]

Hmm, so the cookie permission patch won't work as written since the single caller (nsCookieService::SetCookieInternal) passes a null channel for reasons relating to prompts and the UI. There's a comment describing this and stating that the code will go away with bug 546746, which is completely unowned: http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#2598.

Comment 9

[(no longer active)]

Jason, seems like Dan will not have enough time for this review.  Can you please find somebody else to review this?  The patches here have been waiting for review way too long, and I'd like to move forward here.

Comment 10

[Jason Duell]

biesi/Shawn/Mike:

Cc-ing all of the other cookie peers: can any of you take these?  I definitely don't have cycles to learn the cookie code well enough to review this before the end of the month (and technically I'd need one of your blessings anyway).

Comment 11

[(no longer active)]

I have originally written the private browsing support for the cookie manager, and I think these patches mostly require somebody who's familiar with how private browsing should work, and also be sort of familiar with the cookie service.  I am a bit familiar with the latter as well (and I may even have reviewed some patches on this code before).  Do you think I would be an acceptable reviewer for these patches?  ("No" would be a perfectly acceptable answer here -- not trying to pressure you; just want to make sure that we have a clear path for pushing this forward.  :-)

Comment 12

[Jason Duell]

Comment on attachment 593359 [details] [diff] [review]
Check the private browsing status of channels when checking cookie permissions.

dwitte/biesi have signed off on ehsan reviewing these.

Comment 13

[(no longer active)]

Comment on attachment 593359 [details] [diff] [review]
Check the private browsing status of channels when checking cookie permissions.

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

::: extensions/cookie/nsCookiePermission.cpp
@@ +281,5 @@
>        // without asking, or if we are in private browsing mode, just
>        // accept the cookie and return
> +      bool inPrivateBrowsing = false;
> +      if (aChannel) {
> +        nsCOMPtr<nsIPrivateBrowsingConsumer> consumer = do_QueryInterface(aChannel);

Nit: you don't need to check aChannel and consumer separately.  do_QueryInterface can handle null arguments, so just wish for the best and don't check aChannel, and just check consumer once you're done QIing.

@@ +283,5 @@
> +      bool inPrivateBrowsing = false;
> +      if (aChannel) {
> +        nsCOMPtr<nsIPrivateBrowsingConsumer> consumer = do_QueryInterface(aChannel);
> +        if (consumer) {
> +          consumer->GetUsingPrivateBrowsing(&inPrivateBrowsing);

Nit: use UsePrivateBrowsing().

Comment 14

[(no longer active)]

Comment on attachment 595452 [details] [diff] [review]
Query the private browsing status of channels used to manipulate cookies.

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

The patch looks good, except for the cookie-changed notification below.  Minusing for that.

(And I liked the test very very much!)

::: netwerk/cookie/CookieServiceChild.cpp
@@ +144,5 @@
>  
> +  bool usingPB = false;
> +  nsCOMPtr<nsIPrivateBrowsingConsumer> consumer = do_QueryInterface(aChannel);
> +  if (consumer) {
> +    consumer->GetUsingPrivateBrowsing(&usingPB);

Nit: UsePrivateBrowsing()

@@ +180,5 @@
>  
> +  bool usingPB = false;
> +  nsCOMPtr<nsIPrivateBrowsingConsumer> consumer = do_QueryInterface(aChannel);
> +  if (consumer) {
> +    consumer->GetUsingPrivateBrowsing(&usingPB);

Ditto.

::: netwerk/cookie/nsCookieService.cpp
@@ +652,5 @@
>    mObserverService = mozilla::services::GetObserverService();
>    NS_ENSURE_STATE(mObserverService);
>    mObserverService->AddObserver(this, "profile-before-change", true);
>    mObserverService->AddObserver(this, "profile-do-change", true);
> +  mObserverService->AddObserver(this, "last-pb-context-destroyed", true);

You meant last-pb-context-exited, right?

@@ -1405,5 @@
> -      mPrivateDBState = NULL;
> -      mDBState = mDefaultDBState;
> -    }
> -
> -    NotifyChanged(nsnull, NS_LITERAL_STRING("reload").get());

Why did you remove the cookie-changed notification?  That seems wrong...

@@ +1408,5 @@
> +{
> +  bool usingPB = false;
> +  nsCOMPtr<nsIPrivateBrowsingConsumer> consumer = do_QueryInterface(aChannel);;
> +  if (consumer) {
> +    consumer->GetUsingPrivateBrowsing(&usingPB);

Nit: UsePrivateBrowsing()

@@ +1491,5 @@
>  {
>    NS_ASSERTION(aHostURI, "null host!");
>  
> +  AutoRestore<DBState*> savePrevDBState(mDBState);
> +  mDBState = aUsingPrivateBrowsing ? mPrivateDBState : mDefaultDBState;

Hmm, this is a bit hackish, I guess, but I can't think of a cleaner solution.  However, you need to adjust the comment above mDBState in nsCookieService.h to reflect the new way in which mDBState is being used.

Also, the mDefaultDBState name doesn't make any sense any more.  Could you please submit a follow-up patch to rename it to mNonPrivateDBState?  (Could be a separate/mentored bug)

::: netwerk/test/unit/test_bug248970_cookie.js
@@ +156,5 @@
> +  tests.push(function() {
> +    // Simulate all private browsing instances being closed
> +    var obsvc = Cc["@mozilla.org/observer-service;1"].
> +        getService(Ci.nsIObserverService);
> +    obsvc.notifyObservers(null, "last-pb-context-destroyed", null);

Nit: exited.

Comment 15

[Jason Duell]

Comment on attachment 593359 [details] [diff] [review]
Check the private browsing status of channels when checking cookie permissions.

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

::: extensions/cookie/nsCookiePermission.cpp
@@ +281,5 @@
>        // without asking, or if we are in private browsing mode, just
>        // accept the cookie and return
> +      bool inPrivateBrowsing = false;
> +      if (aChannel) {
> +        nsCOMPtr<nsIPrivateBrowsingConsumer> consumer = do_QueryInterface(aChannel);

The way to query channels for PB now is to call NS_UsePrivateBrowsing() on them (which queries channels' callbacks for nsILoadContext, which we now provide on child).   

Bigger issue: the cookie IDLs don't require you to pass a channel in, and some important call sites (like NPAPI) don't pass one, so withoug some other way to check this we'll be setting non-PB cookies from Flash, etc, which is not ok.

  http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.cpp#2736

I've got to solve the same issue for per-appId cookies (bug 756648).  I'll try to knock off PB mode at the same time.  So don't work on these patched for now.

Comment 16

[(no longer active)]

(In reply to comment #15)
> Comment on attachment 593359 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=593359
> Check the private browsing status of channels when checking cookie permissions.
> 
> Review of attachment 593359 [details] [diff] [review]:
>  --> (https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=722850&attachment=593359)
> -----------------------------------------------------------------
> 
> ::: extensions/cookie/nsCookiePermission.cpp
> @@ +281,5 @@
> >        // without asking, or if we are in private browsing mode, just
> >        // accept the cookie and return
> > +      bool inPrivateBrowsing = false;
> > +      if (aChannel) {
> > +        nsCOMPtr<nsIPrivateBrowsingConsumer> consumer = do_QueryInterface(aChannel);
> 
> The way to query channels for PB now is to call NS_UsePrivateBrowsing() on them
> (which queries channels' callbacks for nsILoadContext, which we now provide on
> child).   
> 
> Bigger issue: the cookie IDLs don't require you to pass a channel in, and some
> important call sites (like NPAPI) don't pass one, so withoug some other way to
> check this we'll be setting non-PB cookies from Flash, etc, which is not ok.
> 
>  
> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.cpp#2736
> 
> I've got to solve the same issue for per-appId cookies (bug 756648).  I'll try
> to knock off PB mode at the same time.  So don't work on these patched for now.

That should be solved with the plugin knowing which docshell it belongs to, which you asked about on dev.platform a few days ago, so I'm assuming that you're going to use that information here.  :-)

Comment 17

[Josh Matthews [:jdm]]

Attachment: Part 1: Add missing privacy-bit-valid serialization for load contexts. Loosen semantics for marking a channel as private when callbacks are present that do not provide a load context.

Jason and Ehsan, since the original IPC PB serialization code did not actually it work (as determined by tests added in following patches \o/), I need to loosen the restrictions of CanSetCallbacks, since in the parent process with e10s we always provide notification callbacks (HTTPChannelParentListener) and try to set the privacy status as well.

Comment 18

[Josh Matthews [:jdm]]

Attachment: Part 2: Query the private browsing status of channels used to manipulate cookies.

With regards to your coment about removing the change notification - external consumers no longer have access to the private cookie database, so there's no point to telling them when it's being cleared.

Comment 19

[Josh Matthews [:jdm]]

Attachment: Part 3: Make plugins provide the channel of the owning document when manipulating cookies.

Comment 20

[Josh Matthews [:jdm]]

Comment on attachment 663750 [details] [diff] [review]
Part 3: Make plugins provide the channel of the owning document when manipulating cookies.

Nevermind, I'm writing a test for this first.

Comment 21

[Josh Matthews [:jdm]]

Attachment: Part 4: Make plugins provide the channel of the owning document when manipulating cookies.

Comment 22

[Josh Matthews [:jdm]]

Attachment: Part 4: Make plugins provide the channel of the owning document when manipulating cookies.

Comment 23

[Josh Matthews [:jdm]]

Attachment: Part 4: Make plugins provide the channel of the owning document when manipulating cookies.

Comment 24

[Josh Matthews [:jdm]]

Attachment: Part 3: Check the private browsing status of channels when checking cookie permissions.

Comment 25

[(no longer active)]

Comment on attachment 663744 [details] [diff] [review]
Part 1: Add missing privacy-bit-valid serialization for load contexts. Loosen semantics for marking a channel as private when callbacks are present that do not provide a load context.

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

Hmm, well, this sort of steps over my patch here <https://bug788275.bugzilla.mozilla.org/attachment.cgi?id=661948> which is also waiting for Jason's review.  Can you please rebase this on top of that patch?

Comment 26

[Josh Matthews [:jdm]]

Comment on attachment 663749 [details] [diff] [review]
Part 2: Query the private browsing status of channels used to manipulate cookies.

Jason, this will need some other input besides Ehsan (not necessarily you), as we established that he is not totally familiar with the ins and outs of the code.

Comment 27

[Josh Matthews [:jdm]]

Comment on attachment 663761 [details] [diff] [review]
Part 3: Check the private browsing status of channels when checking cookie permissions.

This, too, should have some other input besides Ehsan.

Comment 28

[Jason Duell]

Comment on attachment 663749 [details] [diff] [review]
Part 2: Query the private browsing status of channels used to manipulate cookies.

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

We should get a cookie peer to review this--mconnor, you recently said you had cycles for reviews.  I'll take review if you want to delegate it, and I can also look over it before you review if you want.

Comment 29

[Jason Duell]

Comment on attachment 663761 [details] [diff] [review]
Part 3: Check the private browsing status of channels when checking cookie permissions.

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

ditto

Comment 30

[(no longer active)]

Comment on attachment 663749 [details] [diff] [review]
Part 2: Query the private browsing status of channels used to manipulate cookies.

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

::: netwerk/cookie/CookieServiceParent.cpp
@@ +12,5 @@
>  using namespace mozilla::ipc;
>  
> +static void
> +GetAppInfoFromLoadContext(const IPC::SerializedLoadContext &aLoadContext,
> +                          uint32_t& aAppId,

Nit: please be consistent about the location of ampersand.

::: netwerk/cookie/nsCookieService.cpp
@@ +1547,5 @@
>      return;
>    }
>  
> +  AutoRestore<DBState*> savePrevDBState(mDBState);
> +  mDBState = aIsPrivate ? mPrivateDBState : mDefaultDBState;

Hmm, I really really hate this pattern, but I can't think of a better way other than passing a DBState* all around the place, which sucks even more.

Please document the heck out of these semantics in the header.  ;-)

Comment 31

[Mike Connor [:mconnor]]

Comment on attachment 663749 [details] [diff] [review]
Part 2: Query the private browsing status of channels used to manipulate cookies.

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

::: netwerk/cookie/nsCookieService.cpp
@@ +1537,5 @@
>                                           const nsCString    &aServerTime,
>                                           bool                aFromHttp,
>                                           uint32_t            aAppId,
> +                                         bool                aInBrowserElement,
> +                                         bool                aIsPrivate)

At some point, we're going to end up with death by a thousand arguments.  Now's not the time to draw the line, but... soon.

Comment 32

[(not currently active) Ted Mielczarek]

Lots of boolean arguments aren't great API design. If you're just using flags here you could pass a bitmask instead. Alternately you could setup a bunch of one-off enums, but that's a lot more work.

Comment 33

[Mike Connor [:mconnor]]

Comment on attachment 663761 [details] [diff] [review]
Part 3: Check the private browsing status of channels when checking cookie permissions.

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

::: extensions/cookie/nsCookiePermission.cpp
@@ +209,5 @@
>      *aResult = false;
>      break;
>  
>    case nsICookiePermission::ACCESS_ALLOW_FIRST_PARTY_ONLY:
> +    mThirdPartyUtil->IsThirdPartyChannel(nullptr, aURI, &isThirdParty);

I flagged this as looking like it breaks this call.  jdm points out that the caller doesn't pass the channel, which means this never works... so we're going to fix it.  From IRC

<jdm> mconnor: how about I leave the channel, remove the new boolean, pass a real channel in and remove the code that tries to get a domwindow from the channel?

r=me with that change made.

Comment 34

[Jason Duell]

  unint32_t            aAppId,
> +bool                aInBrowserElement,
> +bool                aIsPrivate)

These three args could probably be consolidated into an nsILoadContext arg.

Comment 35

[Josh Matthews [:jdm]]

Attachment: Part 1: Add missing privacy-bit-valid serialization for load contexts.

Comment 36

[Josh Matthews [:jdm]]

Attachment: Part 5: Make setting a cookie internals take a channel object to allow for proper third-party checks.

Comment 37

[Jason Duell]

Comment on attachment 675693 [details] [diff] [review]
Part 5: Make setting a cookie internals take a channel object to allow for proper third-party checks.

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

I don't think this patch provides prpper 3rd party checks.  It doesn't hook the channel passed to the isForeign logic, but more fundamentally, it uses the parent-side nsHttpChannel, which is never going to have the window object that we need for propert 3rd party checks.  I think we need the re-architecture I describe in bug 805616 comment 7 to get proper 3rd party checks in e10s.

Comment 38

[Jason Duell]

> I don't think this patch provides prpper 3rd party checks... we need for propert 

And my approaches to spelling "proper" also need some work :)

Comment 39

[Josh Matthews [:jdm]]

Attachment: Part 5: Make setting a cookie internals take a channel object to allow for proper third-party checks.

With the patch from bug 805616 applied, this patch now ensures that cookies set from channels in the parent process will have proper third-party status, as will cookies set from channels in the child process.

Comment 40

[Josh Matthews [:jdm]]

Hum. I think my part 5 patch will break setting document.cookie in content processes, since we don't hold on to the load context in the parent after OnStopRequest. I guess I need to keep sending the serialized load context.

Comment 41

[Josh Matthews [:jdm]]

Attachment: Part 5: Make setting a cookie internals take a channel object to allow for proper third-party checks.

Sigh. The last patch had some nice cleanup in it that isn't possible any longer.

Comment 42

[(no longer active)]

Please re-enable this test <https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f86ee61600#l1.33> when you land the patches here.  Thanks!

Comment 43

[Jason Duell]

Comment on attachment 676685 [details] [diff] [review]
Part 5: Make setting a cookie internals take a channel object to allow for proper third-party checks.

As discussed on IRC, taking this would step over bug 782542, which needs to be backported for B2G.  We don't want to backport all this to aurora, and I don't want to write two different patches for 782542.  So let's land the rest of this patch now (it won't break anything--e10s PB won't be complete but B2G doesn't use PB yet) and split this patch into a different bug that depends on bug 782542.

Comment 44

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d907ac7bf669
https://hg.mozilla.org/integration/mozilla-inbound/rev/320c1567e431
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a9fbd15cad4
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a0fe388a24b

Comment 45

[Ed Morley [:emorley]]

Backed out for xpcshell & browser-chrome failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7a0fe388a24b

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f467d800bee

Comment 46

[Josh Matthews [:jdm]]

Failures unfailed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/238384fdf0df
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/97d1006e5ba5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c68a517ff3db
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/07e5d2b0d49c

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/238384fdf0df
https://hg.mozilla.org/mozilla-central/rev/97d1006e5ba5
https://hg.mozilla.org/mozilla-central/rev/c68a517ff3db
https://hg.mozilla.org/mozilla-central/rev/07e5d2b0d49c