Closed Bug 777620 Opened 12 years ago Closed 8 years ago

Changes to cookie IDLs to allow/require nsILoadContext information when needed.

Categories

(Core :: Networking: Cookies, defect)

Product:
Core
Component:
Networking: Cookies
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: jduell.mcbugs, Assigned: bugzilla)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-would-take])

Attachments

(2 files)

Proposed API changes
11.98 KB, patch
jdm
: review-
jduell.mcbugs
: feedback+
Details | Diff | Splinter Review
Bug 777620 - Support access to private prowsing cookies in nsICookieManager
58 bytes, text/x-review-board-request
bugzilla
: review+
Details 
For both per-tab private browsing (bug 722850)  and B2G application "cookie jars" (bug 756648) we need to implement separate simultaneous cookie databases, i.e. have separate cookie namespaces (aka "Cookie jars").  

When nsICookieService:GetCookieString, etc, are passed an nsIChannel we can get appId/isInBrowserElement and/or private browsing mode from the necko channel's callbacks (which now implement nsILoadContext on both parent/child in e10s).  But this isn't good enough for these reasons:

1) We permit callers to pass null for the channel (with it meaning just that cookie is set as 3rd party) (The only caller in the tree that omits the channel is NPAPI: but I'm guessing there are addons that might omit it?) 

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

2) NPAPI doesn't have a channel to pass in, so we can't just change that call site to the existing IDL API. 

3) The functions in nsICookieManager need to have the AppId/isInBrowserElement/privateBrowsing info too, so they operate on the right cookie database.

I can see a bunch of solution spaces here, with tradeoffs, and want feedback from dwitte/biesi on which we should go for.

Solution 1: Backward compatibility, with minimal changes overall

I assume we'd like to keep nsICookie{Service|Manager}.idl, as is, for backward compatibility for addons.  This is easy in the AppId/isInBrowserElement case: we could just have the current functions use the default cookie jar (in firefox desktop we don't have appId/InBrowser-indexed cookie jars anyway: it's only for B2G, where we don't have addons and can make them use a new IDL when we do IMO).  This doesn't work for the private browsing case, however: if we continue to allow addons to pass a null channel, we can't tell whether they should be operating on private or non-private cookies.  Thoughts?  One option here is to start failing Set/GetCookie if the addon passes null for the channel.  This keeps the signatures the same but changes the behavior (any idea how many addons might pass null?  I'm hoping very few).  It also doesn't fix the nsICookieManager APIs, which have the same problem (a gross hack: we could simply ignore remove(), enumerate, etc if *any* window is in private browsing mode: this would make most addons that use them work most of the time). 

For code in the tree, we'd convert to use some new APIs:

1) void setCookieStringFromContext(in nsIURI aURI, in string aCookie, in nsILoadContext);
  
Instead of nsILoadContext, we could use nsIDocshell (which we can convert under the covers to an nsILoadContext, as long as we're on the child process).  Or we could split out the needed args explicitly (AppId, isInBrowserElement, usingPrivateBrowsing), but that seems verbose and makes it less flexible for future changes (which we could support by adding fields to nsILoadContext).

2) We need versions of nsICookieManager functions that also take a nsILoadContext/DocShell/etc.

Solution 2: backward compat, with new IDLs

Same as #1 but create entirely new IDLs for the cookie manager/service, and take opportunity to clean up the APIs a little (we can remove both the 'prompt' and 'firstURI' args from setCookieStringFromHttp, for instance), as well as insert language about checking the channel callbacks for cookie-jar Ids.  The existing nsICookieManager/Service IDLs would be deprecated and we'd encourage addons to switch to them, but still support with the kludges mentioned in #1.

3)  Remove existing IDLs and switch to new IDLs.

Harsh, but there are some pretty big warts with allowing the old IDLs to exist, at least for nextgen Private browsing.
Depends on: 775860, 770831, 775861
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
If nsILoadContext is an option, it's preferable since again some channels don't have a docshell per se but do have a loadcontext.
If a channel is passed in we'd get the nsILoadContext from it.  The functions that take nsILoadContext/nsIDocShell/raw parameters are only for plugins or addon contexts where we don't have a channel handy.
Adding some folks who may have a voice from the addons end of things.
Unfortunately, https://mxr.mozilla.org/addons/search?string=%28set|get%29cookiestring&regexp=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons shows a _lot_ of addons calling cookie service methods and passing a null channel :/
(In reply to comment #4)
> Unfortunately,
> https://mxr.mozilla.org/addons/search?string=%28set|get%29cookiestring&regexp=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons
> shows a _lot_ of addons calling cookie service methods and passing a null
> channel :/

We can probably make that return the non-private cookie string if available, without breaking assumptions from those call-sites.  But I'm not sure what needs to be done for cookie jars (perhaps this is not a concern for cookie jars if they're targetted for b2g?)
> perhaps this is not a concern for cookie jars if they're targetted for b2g?

exactly--we can punt on addons for B2G for now, and require a different cookie API for them when they're supported.

> We can probably make that return the non-private cookie string if available

Isn't this a privacy breach (my addon gives sites cookies from my non-private browsing session while I'm in PB)?   And what about setting cookies?

High-tech solutions that probably won't work:
- Is there some magic we could use to inspect the JS call stack to find out what context a call is from?
- could we stuff context info into per-thread data somewhere higher on the call stack, and/or put it in events, so we can get the context somehow?
- can we insert a gross hack into the JS/C interface that adds context args to get/setCookie?

Gross "fixes":
- failing old functions w/o context info while PB mode is on
- disabling (perhaps with UI to warn user) addons that use the functions when we go into PB mode.
(In reply to Jason Duell (:jduell) from comment #6)
> > perhaps this is not a concern for cookie jars if they're targetted for b2g?
> 
> exactly--we can punt on addons for B2G for now, and require a different
> cookie API for them when they're supported.

Great!

> > We can probably make that return the non-private cookie string if available
> 
> Isn't this a privacy breach (my addon gives sites cookies from my
> non-private browsing session while I'm in PB)?   And what about setting
> cookies?

Not really... The PB mode's concern is about not writing private data to the disk.  But yeah this solution is not perfect and doesn't handle setting cookies, which is actually the more important part.

> High-tech solutions that probably won't work:
> - Is there some magic we could use to inspect the JS call stack to find out
> what context a call is from?

Actually we might be able to get the JS context.  Boris, smaug: is that a thing that we can/should do?

> - could we stuff context info into per-thread data somewhere higher on the
> call stack, and/or put it in events, so we can get the context somehow?

That sounds very scary!

> - can we insert a gross hack into the JS/C interface that adds context args
> to get/setCookie?

I don't think that would be great...

> Gross "fixes":
> - failing old functions w/o context info while PB mode is on

We might resort to do this if we can get our hands on a JS context which somehow gives us a docshell or a load context...  Can add-ons run code without a JS context anyways?

> - disabling (perhaps with UI to warn user) addons that use the functions
> when we go into PB mode.

We don't want to go in that direction if possible.  If this really cannot be solved in any other way, we should just bite the bullet and break compatibility (but I really really don't want to do that here.)
I like (2), and I think we should do (3) one release train later (maybe two) so there is a transition period for add-ons.
chat on irc w/bz reveals the JS context won't give us docshell/loadContext, so I think breaking IDLs is the long-range plan for nextgen PB support :(
I don't think this needs to block the 1st rev of B2G cookie-jars.  Until we have addons in B2G we don't have to solve the issue (the http channel and document.cookie get/set cases give me a channel, so we'll always have the nsILoadContext).  And when we do allow addons in B2G, I'm not sure what the mechanism is going to be to provide an appId--it's likely not going to be an nsILoadContext/DocShell, because an addon that you get to via a menu isn't likely to have one to provide.  And I suspect we may want have shims to nsICookieService/Manager that store the AppID, and hand those out to B2G apps instead of the real cookieService, to prevent apps from masquerading as another app and reading/setting its cookies.

So I'm going to punt on this for now.  I'm afraid that leaves nextgen-PB out in the cold for the moment--sorry.  But I think we should probably take some time to look at how addons are using the APIs anyway.
No longer blocks: 756648
blocking-basecamp: ? → ---
blocking-kilimanjaro: ? → ---
Well, can we just add versions of the functions which take an nsILoadContext parameter for now for per-window PB and ignore the cookie jar problem for now?  Given what I suggested in comment 5, I think that will be all that is needed for per-window PB.
> can we just add versions of the functions which take an nsILoadContext

My understanding from talking to bz is that an addon will often not have a loadContext to pass in (if it's invoked from a chrome menu for instance), so it's not clear this is the fix.

Generally, I'm still not clear on the right use case for addon cookie operations: if they're some sort of "manage/sync/edit your cookie database" app, they almost definitely want to operate on the non-PB database,even if you invoke them while some/all of your tabs are in PB mode.  If they're anything that operates on cookies as they're coming in/out via HTTP, we really want them to use whatever PB mode the channel is using.  If they generally pass in the channel parameter, we're good, but we'll need to audit a bit to find out if some are omitting the channel arg yet are still altering channels' cookies, i.e. if they're leaking non-PB cookies into PB requests.
(In reply to comment #12)
> > can we just add versions of the functions which take an nsILoadContext
> 
> My understanding from talking to bz is that an addon will often not have a
> loadContext to pass in (if it's invoked from a chrome menu for instance), so
> it's not clear this is the fix.
> 
> Generally, I'm still not clear on the right use case for addon cookie
> operations: if they're some sort of "manage/sync/edit your cookie database"
> app, they almost definitely want to operate on the non-PB database,even if you
> invoke them while some/all of your tabs are in PB mode.  If they're anything
> that operates on cookies as they're coming in/out via HTTP, we really want them
> to use whatever PB mode the channel is using.  If they generally pass in the
> channel parameter, we're good, but we'll need to audit a bit to find out if
> some are omitting the channel arg yet are still altering channels' cookies,
> i.e. if they're leaking non-PB cookies into PB requests.


I took a look and most of them just pass null as the channel argument.

I think we can safely assume that any request without a corresponding nsILoadContext is solely going to access the non-PB database.  This is not fool proof, but our goal with regards to add-ons should be to get sane defaults.  If an add-on wishes to handle PB mode explicitly, they can do that the proper way.
(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> I think we can safely assume that any request without a corresponding
> nsILoadContext is solely going to access the non-PB database.  This is not
> fool proof, but our goal with regards to add-ons should be to get sane
> defaults.  If an add-on wishes to handle PB mode explicitly, they can do
> that the proper way.

I agree with this.
> I think we can safely assume that any request without a corresponding nsILoadContext is solely going to access the non-PB database.

Ah, in that case this should be easier.  For desktop addons I think it would be enough to provide versions of the APIs that provide an 'isPrivate' bool parameter.   Those might be enough for B2G too, in that we're moving to no longer trusting the nsILoadContext to provide the AppId (and instead getting it from IPDL), and we can pretty much assume IsInBrowserElement (when we have addons for B2G, we'll be letting them access the mozbrowser content--regular web content--but not the cookie space for the B2G firefox HTML app itself).
(In reply to comment #15)
> > I think we can safely assume that any request without a corresponding nsILoadContext is solely going to access the non-PB database.
> 
> Ah, in that case this should be easier.  For desktop addons I think it would be
> enough to provide versions of the APIs that provide an 'isPrivate' bool
> parameter.   Those might be enough for B2G too, in that we're moving to no
> longer trusting the nsILoadContext to provide the AppId (and instead getting it
> from IPDL), and we can pretty much assume IsInBrowserElement (when we have
> addons for B2G, we'll be letting them access the mozbrowser content--regular
> web content--but not the cookie space for the B2G firefox HTML app itself).

This sounds good to me.
Blocks: 823941
Am I correct in understanding that in FF19+ there is no way to read PB cookies because CookieManager.enumerator always returns the non-PB cookies?

As the developer of the CookieSwap addon, I must admit that in my add-on, the current API assumption could cause cookies from the non-PB DB to be leaked into the PB DB because I read cookies from CookieManager.enumerator (which is always the non-PB DB now) and write them using the CookieService.setCookieString (which allows me to direct them to the non-PB DB).
  
I'm actually going to have to disable my addon completely for PB windows because there is no way to read the PB cookies (as best I can tell) because CookieManager.enumerator doesn't have a PB/non-PB flag.

From the tone of the bug comments, it looks like you are leaning toward a new CookieManager interface to support PB/non-PB, right?  If you know what you want the new API for CookieManager to look like, I'd be willing to look into helping implement it.
Any add-on that checks or listens for cookie changes to see if the user is currently logged in to a web site or just logged in, is also broken as of FF 19 if the user enables "Never Remember History".

For example using the "cookie-changed" observer doesn't work at all now.  Will this be fixed once this change goes through?
Never mind about the "cookie-changed" observer.  I see there's now a "private-cookie-changed" observer.  That helps with new logins, but not to check for existing ones.
(In reply to cookieswap from comment #17)
> From the tone of the bug comments, it looks like you are leaning toward a
> new CookieManager interface to support PB/non-PB, right?  If you know what
> you want the new API for CookieManager to look like, I'd be willing to look
> into helping implement it.

Yes, that's what we're thinking of here, and thanks a lot for stepping up for this.

I will attach a patch with the IDL changes that I'm proposing to give you a sense of the API changes that we would like to support here.
Please let me know what you think about these changes, guys!
Attachment #723642 - Flags: review?(josh)
Attachment #723642 - Flags: review?(jduell.mcbugs)
Comment on attachment 723642 [details] [diff] [review]
Proposed API changes

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

::: netwerk/cookie/nsICookieManager.idl
@@ +24,2 @@
>     */
> +  void removeAll(in boolean aRemovePrivateOnly);

This name is now confusing, since neither invocation removes all cookies.

How about

/**
 * Called to remove cookies from the given cookie list.
 * param aPrivate True to use the private cookie database, false otherwise.
 */
removeCookies(in boolean aUsePrivate)

::: netwerk/cookie/nsICookieManager2.idl
@@ +8,5 @@
>  interface nsICookie2;
>  interface nsIFile;
>  
>  /** 
>   * Additions to the frozen nsICookieManager

nsICookieManager is no longer frozen, since this change touches it, right?

@@ +83,5 @@
>     *        the host string to search for, e.g. "google.com". this should consist
>     *        of only the host portion of a URI. see @add for a description of
>     *        acceptable host strings.
> +   * @param aIsPrivate
> +   *        Whether to count private cookies.

If aIsPrivate is true, is countCookiesFromHost intended to count both databases, or just the private one?

If it's the latter, maybe countPrivateCookiesFromHost and countPublic(?)CookiesFromHost are more accurate. Similar comments apply elsewhere.

::: netwerk/cookie/nsICookieService.idl
@@ +109,5 @@
>     *        provided, the cookies will be assumed third-party.)
> +   * @param aIsPrivate
> +   *        Whether we should look up the cookie in the private cookies database.
> +   *        Note that if a channel is provided, the privacy status of the
> +   *        channel overrides this argument.

nsIChannel doesn't seem to have an isPrivate attribute. What does a new developer need to know in order to figure out the privacy status of the channel?
I'm not 100% sure about this, but it looks like you can check the following:

channel.QueryInterface(Components.interfaces.nsIPrivateBrowsingChannel).isChannelPrivate
Comment on attachment 723642 [details] [diff] [review]
Proposed API changes

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

r- for the changes to the channel-related methods.

::: netwerk/cookie/nsICookieManager2.idl
@@ +56,5 @@
>             in boolean     aIsSecure,
>             in boolean     aIsHttpOnly,
>             in boolean     aIsSession,
> +           in int64_t     aExpiry,
> +           in boolean     aIsPrivate);

If we're breaking every caller by not making this optional, we also have the opportunity to either turn these booleans into bitflags or go all out and pass a dictionary. I don't know which choice is best at this point, or whether to just accept another death by thousand booleans. Thoughts?

::: netwerk/cookie/nsICookieService.idl
@@ +78,5 @@
>     *        provided, the cookies will be assumed third-party.)
> +   * @param aIsPrivate
> +   *        Whether we should look up the cookie in the private cookies database.
> +   *        Note that if a channel is provided, the privacy status of the
> +   *        channel overrides this argument.

This does not seem like a good design to me. We're already strongly suggesting that consumers pass in the needed context to get proper third-party behaviour already; I propose that we default to the public database if that context isn't present. Requiring a bogus value for (what should be) the common case is not appealing to me. Same argument goes for the rest of the changes in this file.
Attachment #723642 - Flags: review?(josh) → review-
Comment on attachment 723642 [details] [diff] [review]
Proposed API changes

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

I don't claim to know the internals of the code, but here are a few thoughts/ides from an outside's point of view:
-Would it be 'crazy talk' to consider keeping the interface identical (to maintain backwards compatibility) and just using a new class/contractID instance?  Like having:
var privCookieSvc = Components.classes["@mozilla.org/PRIVATEcookieService;1"]
                    .getService(Components.interfaces.nsICookieService);
var privateCcookieMgr = Components.classes["@mozilla.org/PRIVATEcookiemanager;1"]
                  .getService(Components.interfaces.nsICookieManager);
Comment on attachment 723642 [details] [diff] [review]
Proposed API changes

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

The current bool approach has limited Firefox to only ever having 2 cookie DBs (private and non-private).  This seems reasonable at the moment, but is it potentially architecturally limiting?  Has there been any discussion of supporting a different private cookie DB per window/tab?  I could see this being a really nice feature (like being logged in to a web site as a different user on each tab...this is a common request I get as part of my CookieSwap add-on).

Instead of passing bool around, does the nsIChannel uniquely identify the DB (although this is a challenge for code that doesn't have a channel context...like the built in Cookie Viewer).  To solve that, would it make sense to also have an enumeration of the cookieDBs and then a method to get the particular instance of the CookieManager/CookieService matching the DB instance.

Just throwing an idea out there.
(In reply to Monica Chew from comment #22)
> Comment on attachment 723642 [details] [diff] [review]
> Proposed API changes
> 
> Review of attachment 723642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cookie/nsICookieManager.idl
> @@ +24,2 @@
> >     */
> > +  void removeAll(in boolean aRemovePrivateOnly);
> 
> This name is now confusing, since neither invocation removes all cookies.
> 
> How about
> 
> /**
>  * Called to remove cookies from the given cookie list.
>  * param aPrivate True to use the private cookie database, false otherwise.
>  */
> removeCookies(in boolean aUsePrivate)

Hmm, that breaks more add-on code than absolutely necessary here.

> ::: netwerk/cookie/nsICookieManager2.idl
> @@ +8,5 @@
> >  interface nsICookie2;
> >  interface nsIFile;
> >  
> >  /** 
> >   * Additions to the frozen nsICookieManager
> 
> nsICookieManager is no longer frozen, since this change touches it, right?

Yeah, no interface is frozen any more...

> @@ +83,5 @@
> >     *        the host string to search for, e.g. "google.com". this should consist
> >     *        of only the host portion of a URI. see @add for a description of
> >     *        acceptable host strings.
> > +   * @param aIsPrivate
> > +   *        Whether to count private cookies.
> 
> If aIsPrivate is true, is countCookiesFromHost intended to count both
> databases, or just the private one?

Just the private one.

> If it's the latter, maybe countPrivateCookiesFromHost and
> countPublic(?)CookiesFromHost are more accurate. Similar comments apply
> elsewhere.

This is the boolean versus two function names question that we discussed somewhere (but apparently not this bug?!).  I'm personally not a fan of booleans, but in that discussion everybody else seemed to really like them, which is why I proposed these changes.

> ::: netwerk/cookie/nsICookieService.idl
> @@ +109,5 @@
> >     *        provided, the cookies will be assumed third-party.)
> > +   * @param aIsPrivate
> > +   *        Whether we should look up the cookie in the private cookies database.
> > +   *        Note that if a channel is provided, the privacy status of the
> > +   *        channel overrides this argument.
> 
> nsIChannel doesn't seem to have an isPrivate attribute. What does a new
> developer need to know in order to figure out the privacy status of the
> channel?

C++ code can call NS_UsePrivateBrowsing on it.  JS code can query nsIPrivateBrowsingChannel.
(In reply to Josh Matthews [:jdm] from comment #24)
> Comment on attachment 723642 [details] [diff] [review]
> Proposed API changes
> 
> Review of attachment 723642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for the changes to the channel-related methods.
> 
> ::: netwerk/cookie/nsICookieManager2.idl
> @@ +56,5 @@
> >             in boolean     aIsSecure,
> >             in boolean     aIsHttpOnly,
> >             in boolean     aIsSession,
> > +           in int64_t     aExpiry,
> > +           in boolean     aIsPrivate);
> 
> If we're breaking every caller by not making this optional, we also have the
> opportunity to either turn these booleans into bitflags or go all out and
> pass a dictionary. I don't know which choice is best at this point, or
> whether to just accept another death by thousand booleans. Thoughts?

Oh wait, I thought that xpconnect will automatically pass in false if you omit a trailing boolean arg.  If |optional| is the keyword to make that magic happen, just assume I used that everywhere.  :-)

And no, I don't think scope creeping this bug is a great idea.  ;-)  But please do file another bug for that.

> ::: netwerk/cookie/nsICookieService.idl
> @@ +78,5 @@
> >     *        provided, the cookies will be assumed third-party.)
> > +   * @param aIsPrivate
> > +   *        Whether we should look up the cookie in the private cookies database.
> > +   *        Note that if a channel is provided, the privacy status of the
> > +   *        channel overrides this argument.
> 
> This does not seem like a good design to me. We're already strongly
> suggesting that consumers pass in the needed context to get proper
> third-party behaviour already; I propose that we default to the public
> database if that context isn't present. Requiring a bogus value for (what
> should be) the common case is not appealing to me. Same argument goes for
> the rest of the changes in this file.

The thing is that if you're writing a cookie manager for example, you won't have the correct context/channel, since you don't necessarily have any private windows around, and even if you do, passing an arbitrary one seems sub-optimal.
(In reply to Steve Tine from comment #25)
> Comment on attachment 723642 [details] [diff] [review]
> Proposed API changes
> 
> Review of attachment 723642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't claim to know the internals of the code, but here are a few
> thoughts/ides from an outside's point of view:
> -Would it be 'crazy talk' to consider keeping the interface identical (to
> maintain backwards compatibility) and just using a new class/contractID
> instance?  Like having:
> var privCookieSvc = Components.classes["@mozilla.org/PRIVATEcookieService;1"]
>                     .getService(Components.interfaces.nsICookieService);
> var privateCcookieMgr =
> Components.classes["@mozilla.org/PRIVATEcookiemanager;1"]
>                   .getService(Components.interfaces.nsICookieManager);

That would be much harder to implement.  As I said before, I meant to maintain as much backwards compat as possible by just assuming false if these boolean arguments are not passed in.
(In reply to Steve Tine from comment #26)
> Comment on attachment 723642 [details] [diff] [review]
> Proposed API changes
> 
> Review of attachment 723642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The current bool approach has limited Firefox to only ever having 2 cookie
> DBs (private and non-private).  This seems reasonable at the moment, but is
> it potentially architecturally limiting?  Has there been any discussion of
> supporting a different private cookie DB per window/tab?  I could see this
> being a really nice feature (like being logged in to a web site as a
> different user on each tab...this is a common request I get as part of my
> CookieSwap add-on).
> 
> Instead of passing bool around, does the nsIChannel uniquely identify the DB
> (although this is a challenge for code that doesn't have a channel
> context...like the built in Cookie Viewer).  To solve that, would it make
> sense to also have an enumeration of the cookieDBs and then a method to get
> the particular instance of the CookieManager/CookieService matching the DB
> instance.
> 
> Just throwing an idea out there.

Yeah, there has been talk about implementing cookie jars, but I don't think that has been designed yet...  But this is a good point, it would definitely be nice if we could change the API here in a way that would cover cookie jars as well.  Suggestions welcome!
(In reply to :Ehsan Akhgari from comment #28)
> (In reply to Josh Matthews [:jdm] from comment #24)
> > ::: netwerk/cookie/nsICookieService.idl
> > @@ +78,5 @@
> > >     *        provided, the cookies will be assumed third-party.)
> > > +   * @param aIsPrivate
> > > +   *        Whether we should look up the cookie in the private cookies database.
> > > +   *        Note that if a channel is provided, the privacy status of the
> > > +   *        channel overrides this argument.
> > 
> > This does not seem like a good design to me. We're already strongly
> > suggesting that consumers pass in the needed context to get proper
> > third-party behaviour already; I propose that we default to the public
> > database if that context isn't present. Requiring a bogus value for (what
> > should be) the common case is not appealing to me. Same argument goes for
> > the rest of the changes in this file.
> 
> The thing is that if you're writing a cookie manager for example, you won't
> have the correct context/channel, since you don't necessarily have any
> private windows around, and even if you do, passing an arbitrary one seems
> sub-optimal.

nsICookieService is intended for manipulating the DB in response to page loads (http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsICookieService.idl#15). nsICookieManager is designed for general manipulation, and accordingly your interface changes make it possible to deal with the private/public separation as desired.
Comment on attachment 723642 [details] [diff] [review]
Proposed API changes

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

Does +sr mean anything any more?  If so this would seem like a patch that might need it.  Note that I'm neither an sr nor a cookie peer :)

> just assume I used [optional] everywhere.  :-)

With that fixed, I think the boolean approach is the way to go.

> there has been talk about implementing cookie jars, but I don't think that has
> been designed yet

We have cookie namespaces (aka "jars") on a per-app basis for multiprocess (aka electrolysis aka e10s) which is currently used only in Firefox OS, which currently doesn't support addons, so we've been able to kick the down the road the issue of how these IDLs will work in a multi-app environment.   But since namespaces correspond to "I'm a different app", trying to implement the "cookie DB per PB tab" idea would also mean each PB tab would have to be a separate app.  

> Would it be 'crazy talk' to consider keeping the interface identical (to
> maintain backwards compatibility) and just using a new class/contractID
> instance?

I don't think this is the right thing to do for PB (at least in its existing form: if we ever want separate per-tab DBs then sure).  But note that I suspect that we will wind up doing exactly this in Firefox OS if/when we provide the cookie management IDLs to apps (like the browser on Firefox OS).  I.e. we'll provide an implementation of the existing CookieManager IDLs that under the covers uses a fixed app ID, so that 1) the API doesn't change; 2) we securely isolate apps from being able to "manage" :) other apps' cookies.

> scope creeping this bug

Yeah, someday it would be nice to replace all the booleans with a flags arg or a dictionary.  While we're at it let's merge nsICookieManager2 in with nsICookieManager, and also remove all support for "Cookie2" type cookies, since RFC 6265 has obsoleted them.  :)

::: netwerk/cookie/nsICookieService.idl
@@ +78,5 @@
>     *        provided, the cookies will be assumed third-party.)
> +   * @param aIsPrivate
> +   *        Whether we should look up the cookie in the private cookies database.
> +   *        Note that if a channel is provided, the privacy status of the
> +   *        channel overrides this argument.

Dropping the isPrivate arg and requiring that you pass in a private channel to access the private DB sounds good in theory to me--but I don't know if there are call sites that are important where we don't/can't have a channel.  Sounds like jdm may be right about being able to use nsICookieManager for addons and other contexts that don't have a channel.
Attachment #723642 - Flags: review?(jduell.mcbugs) → feedback+
(In reply to :Ehsan Akhgari from comment #27)
> > nsICookieManager is no longer frozen, since this change touches it, right?
> 
> Yeah, no interface is frozen any more...

Can this comment be updated to say that we only separate nsICookieManager and nsICookieManager2 for backwards compatibility? I think it would reduce confusion.

> This is the boolean versus two function names question that we discussed
> somewhere (but apparently not this bug?!).  I'm personally not a fan of
> booleans, but in that discussion everybody else seemed to really like them,
> which is why I proposed these changes.

Can these comments be made clearer, then?
* @param aUsePrivateDatabase
* If true, counts cookies in the private database. If false, counts cookies in 
* non-private database.

Similar comments elsewhere.

> > nsIChannel doesn't seem to have an isPrivate attribute. What does a new
> > developer need to know in order to figure out the privacy status of the
> > channel?
> 
> C++ code can call NS_UsePrivateBrowsing on it.  JS code can query
> nsIPrivateBrowsingChannel.

Can this be in the comment? I don't fully understand jdm's comments, but agree that a design that supports two conflicting isPrivate values seems wrong.

Re: optional params and backwards compatibility, I guess if we wanted to support querying private, public, and both, we could use an enum with 0 being the public database if xpconnect supports automatically passing in 0 for trailing int args. However, this is just ugly in a different way, and I agree with Steve that a cleaner long term solution is to provide a cookie manager that only has access to what the caller wants.

Thanks,
Monica
(In reply to comment #33)
> (In reply to :Ehsan Akhgari from comment #27)
> > > nsICookieManager is no longer frozen, since this change touches it, right?
> > 
> > Yeah, no interface is frozen any more...
> 
> Can this comment be updated to say that we only separate nsICookieManager and
> nsICookieManager2 for backwards compatibility? I think it would reduce
> confusion.

Sure.  Note that I'm not planning to attach a new version of the patch as I did the previous patch only to lay out the API I had in mind, and I didn't even verify my IDL syntax.  :-)  But r=me if you just want to check in that comment change out of bound.

> > This is the boolean versus two function names question that we discussed
> > somewhere (but apparently not this bug?!).  I'm personally not a fan of
> > booleans, but in that discussion everybody else seemed to really like them,
> > which is why I proposed these changes.
> 
> Can these comments be made clearer, then?
> * @param aUsePrivateDatabase
> * If true, counts cookies in the private database. If false, counts cookies in 
> * non-private database.
> 
> Similar comments elsewhere.

Yeah these comments should probably be rewritten, I just quickly copy and pasted them around!

> > > nsIChannel doesn't seem to have an isPrivate attribute. What does a new
> > > developer need to know in order to figure out the privacy status of the
> > > channel?
> > 
> > C++ code can call NS_UsePrivateBrowsing on it.  JS code can query
> > nsIPrivateBrowsingChannel.
> 
> Can this be in the comment? I don't fully understand jdm's comments, but agree
> that a design that supports two conflicting isPrivate values seems wrong.

In which comments?  This is documented in both of the places that I mentioned IIRC.

> Re: optional params and backwards compatibility, I guess if we wanted to
> support querying private, public, and both, we could use an enum with 0 being
> the public database if xpconnect supports automatically passing in 0 for
> trailing int args. However, this is just ugly in a different way, and I agree
> with Steve that a cleaner long term solution is to provide a cookie manager
> that only has access to what the caller wants.

I still don't think that these changes are worth breaking backwards compat for...
Blocks: 864150
Whiteboard: [necko-would-take]
I am trying to implement support for private cookies in nsICookieManager for bug 1254221.

Forgive me if this point was already made, since I just skimmed the discussion.

If we add an optional boolean parameter aIsPrivate, and interpret aIsPrivate=false to mean that we use the non-private cookie database, this would change the meaning of existing callers, since the existing methods may use either the non-private or the private cookie database depending on what is the current state of nsICookieManager. I have found that at least nsCookiePermission would be broken by such a change, and I am not sure if there are others.

One solution could be to have aIsPrivate=false mean use the current cookie database and have aIsPrivate=true mean use the private cookie database. But that seems inconsistent.

Another solution could be to add an optional integer parameter instead where 0 = use the current database, 1 = use the non-private database and 2 = use the private database.

A third solution which I have attempted in bug 1254221 is to add a method to change the current database, so we can use all the existing methods of nsICookieManager as-is.
Huh, there are cases when it's possible to access the private cookie database through nsICookieManager right now? Is that when executing synchronous observer notifications for cookie-related operations or something?
I'd also really like to avoid introducing more "cookie jar" mechanisms. We already have one through OriginAttributes and one through private-browsing. My hope is to make private-browsing use OriginAttributes and thus reduce it to one. Not to add a third axis.
(In reply to Josh Matthews [:jdm] from comment #36)
> Huh, there are cases when it's possible to access the private cookie
> database through nsICookieManager right now? Is that when executing
> synchronous observer notifications for cookie-related operations or
> something?

This call can access private cookies:
https://dxr.mozilla.org/mozilla-central/rev/a66bf0a800f3d859b4bd2ceebc57b2e3fa6544d8/extensions/cookie/nsCookiePermission.cpp#224
Because it is invoked by the cookie manager here:
https://dxr.mozilla.org/mozilla-central/rev/a66bf0a800f3d859b4bd2ceebc57b2e3fa6544d8/netwerk/cookie/nsCookieService.cpp#3272

Observer notifications might be another case. I don't know.
I have put up a patch for the smallest and most backwards compatible change I could come up with. If you prefer one more like the one sketched out in the attachment from three years ago, I can try to implement that instead.

(In reply to Jonas Sicking (:sicking) from comment #37)
> I'd also really like to avoid introducing more "cookie jar" mechanisms. We
> already have one through OriginAttributes and one through private-browsing.
> My hope is to make private-browsing use OriginAttributes and thus reduce it
> to one. Not to add a third axis.

Has there been any talk about a third? What is that?
What can I do to help move this forward?
Mail Jonas directly?
Blocks: 1254221
Attachment #8737495 - Flags: review?(jonas) → review?(honzab.moz)
Redirected towards honza as a necko peer, I think... :)
Comment on attachment 8737495 [details]
Bug 777620 - Support access to private prowsing cookies in nsICookieManager

Jason is a better reviewer here.
Attachment #8737495 - Flags: review?(honzab.moz) → review?(jduell.mcbugs)
Comment on attachment 8737495 [details]
Bug 777620 - Support access to private prowsing cookies in nsICookieManager

https://reviewboard.mozilla.org/r/43991/#review47929

I like that this is a reasonable, minimal hack for what you're trying to do here.  AFAICT it shouldn't break anything.
Attachment #8737495 - Flags: review?(jduell.mcbugs) → review+
Jesper, is this good to check in now?
Assignee: nobody → bugzilla
Flags: needinfo?(bugzilla)
Yes. I didn't plan to ask for it to be checked in before Bug 1254221 and Bug 1272953 are ready, but I see no real reason why you should not check it in now.
Flags: needinfo?(bugzilla)
Keywords: checkin-needed
Keywords: checkin-needed
When I tried to rebase the patch for bug 1254221, it conflicted with bug 1267910. Not sure if I understand it yet, but maybe bug 1267910 and bug 1269361 added the same functionality?
Keywords: checkin-needed
Please set this to r+ in MozReview so it can autoland.
Flags: needinfo?(bugzilla)
Keywords: checkin-needed
Attachment #8737495 - Flags: review?(bugzilla)
Comment on attachment 8737495 [details]
Bug 777620 - Support access to private prowsing cookies in nsICookieManager

https://reviewboard.mozilla.org/r/43991/#review81076
Comment on attachment 8737495 [details]
Bug 777620 - Support access to private prowsing cookies in nsICookieManager

https://reviewboard.mozilla.org/r/43991/#review81078
Attachment #8737495 - Flags: review?(bugzilla) → review+
I don't know if I did the right thing. It feels quite odd.
Flags: needinfo?(bugzilla)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e6764b1be721
Support access to private prowsing cookies in nsICookieManager. r=jduell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e6764b1be721
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
See Also: → 1354229
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: