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 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.

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 on attachment 593359 [details] [diff] [review] Check the private browsing status of channels when checking cookie permissions. dwitte has agreed to review these.

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

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.

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.

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).

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 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 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 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 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.

(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. :-)

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.

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 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 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 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 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 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 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 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 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.

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 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.

unint32_t aAppId, > +bool aInBrowserElement, > +bool aIsPrivate) These three args could probably be consolidated into an nsILoadContext arg.

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.

> 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 :)

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.

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.

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

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

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.

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

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

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

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