756648 - Implement "cookie jars" for apps

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect, P1)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

See description in bug 756644.

I don't know the cookie code well enough to know how to fix this off the top of my head. Basically we need to add the app-identifier to the key when reading/writing cookies. The app-identifier should be available from the globalwindow, but I'm not sure that we can get to that 100% of the time?

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Jason: Any updates or ETA here?

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Is there any overlap between this bug and 770691? This bug is to enable per-site 3rd party cookie blocking (https://wiki.mozilla.org/Privacy/Features/Per-Site_Third-Party_Cookie_Setting).

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not really no.

Comment 4

[Jason Duell]

> updates or ETA here?

Working on the code while I hammer out the right IDL changes needed (bug 777620g).

Don't have exact ETA--hoping for a couple days?

Comment 5

[Jason Duell]

So a major design decision here is whether to put all cookies for all apps into a single big, Firefox OS-wide DBState (and SQLite database), or have a separate DBState and SQLite database file for each B2G AppId.

Most apps in B2G will only have a handful of cookies so it seems like serious overkill to create a new SQLite database for each one of them.   But the notable exception is B2G browsers (i.e. apps that have a mozbrowser element, like the Firefox app in B2G).  These are full-fledged browsers and will store lots of cookies, and it's a waste of memory to have Firefox OS keep all the cookies around in RAM for a browser app that the user no longer uses but hasn't uninstalled, for instance.  (To get a sense of the size involved here: my various desktop firefox profiles have cookie databases of around 1-1.5 MB each:  the theoretical max is 3K cookies * 4096 max size each = 11 MB, but most cookies are much smaller than 4K.  So we're probably not talking about more than single-digit MBs of wasted space in the OS).

A nice solution in the long run would be to only create separate cookie databases for apps with mozbrowser elements: but to do that we'd need a way to tell at install time (ie when we get the nsIObserverService notification that an app is getting installed) that it will use mozbrowser.  I doubt we'll have that easily available (and in the presence of app upgrades, apps might change into mozbrowsers).

I think perhaps the solution here is to start off with one big cookie database, and perhaps in future followup work add logic that splits out apps into their own databases if they've stored more than some threshold number of cookies and/or arent' being used often enough to merit keeping their cookies in storage.  

Thanks for listening to me think out loud--comments welcome :)

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I definitely think that we should stick all cookies in the same database.

I'm not at all worried about accumulating too much data. Databases don't slow down a lot as the amount of data grows. And all the built-in apps will be packaged apps which won't use cookies at all.

It could definitely end up being a problem if the user installs a lot of non-packaged apps, which use a lot of cookies, but I think it's premature to worry about scaling performance there at this point.

Comment 7

[Andreas Gal :gal]

How are we doing here?

Comment 8

[Jason Duell]

I'm very close to posting the patches.

Comment 9

[Lucas Adamski [:ladamski]]

Ping, anything you need to make this happen?

Comment 10

[Jason Duell]

I think this is actually LOE:S for the remaining work, but getting reviews may take a while.

Comment 11

[Jason Duell]

Attachment: part1, v1: change sqlite schema to support cookie namespaces

This adds 'appId' and 'inBrowserElement' fields to the sqlite database.  Handles case of reverting temporarily to earlier browser versions.

Nothing actually uses the fields yet in this patch.

It is possible that we could do without these schema changes, if we swizzled in the appid/inBrowser data into the 'basedomain' field of the database.  But that approach seemed error-prone and less clean than this to me.

Comment 12

[Jason Duell]

Attachment: part2, v1: support cookie namespaces

Main part of patch.  Actually start using the appId/inBrowser fields in the cookie lookup key.  

Currently uses {appId=0, browser=false} for most of the IDL pathways (i.e. the default namespace, used by desktop Firefox and, in B2G, parent process request). The previous patch converts existing sqlite cookies to use those values, so no cookie loss in Desktop :)

The only IDLS we use from child processes for now are {Get|Set}CookieString{FromHttp}, and in those cases we grab the appID/inBrowser from the channel's nsILoadContext.  e10s patch is coming next.

For now we don't need any IDL changes to make all this work.  For private browsing (and possibly for nsICookieManager{2} access in child) we'll make IDL changes, but that's bug 722850 and bug 777620.

Comment 13

[Jason Duell]

Attachment: part3, v1: e10s support

send LoadContext info across IPDL so we've got appID/inBrowser.   Uses UNKNOWN appID if request is lacking a LoadContext.  More security coming in bug 782542.

Comment 14

[Jason Duell]

Attachment: part4, v1: minor SerializedLoadContent fix

SerializedLoadContext::SerializedLoadContext already handles null/missing LoadContext, but wasn't handling null channels.

Comment 15

[Jason Duell]

Attachment: part5, v1: xpcshell test

Test that channels with different AppIds/inBrowserElements are stored in separate namespaces.

Comment 16

[Jason Duell]

Note: these patches don't support namespaces for private browsing in B2G (and thus e10s) yet.  PB still relies on the global, "in PB everywhere or not" behavior.  We're not planning to support PB in v1 of B2G, so that's future work.  (the main thing we'll need is a notification for when PB mode for an app is over, so we can purge its PB cookie storage).

Comment 17

[Josh Matthews [:jdm]]

Comment on attachment 652595 [details] [diff] [review]
part2, v1: support cookie namespaces

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

::: netwerk/cookie/nsCookieService.cpp
@@ +57,5 @@
> +// TODO: When we figure out what the API will look like for nsICookieManager{2}
> +// on content processes (see bug 777620), change to use the appropriate app
> +// namespace.  For now those IDLs aren't supported on child processes.
> +#define DEFAULT_APP_KEY(name, baseDomain) \
> +        nsCookieKey name(baseDomain, NECKO_NO_APP_ID, false)

Is there some way to make this macro |#define DEFAULT_APP_KEY(basedomain) nsCookieKey(baseDomain, NECKO_NO_APP_ID, false)|? I would find it much easier to read declarations like |nsCookieKey key = DEFAULT_APP_KEY(basedomain)|, or even just |nsCookieKey key(DEFUALT_APP_KEY)|.

@@ +2330,5 @@
>      if (NS_FAILED(rv))
>        continue;
>  
> +    // pre-existing cookies have appId=0, inBrowser=false
> +    nsCookieKey key(baseDomain, 0, false);

This should probably use DEFAULT_APP_KEY

@@ +2359,3 @@
>      }
>      else {
> +      DEFAULT_APP_KEY(key, baseDomain);

Why is this necessary?

@@ +2448,5 @@
>      COOKIE_LOGFAILURE(GET_COOKIE, aHostURI, nullptr, "invalid host/path from URI");
>      return;
>    }
>  
> +  nsCookieKey key(baseDomain, aAppId, aInBrowserElement);

Want to move this down to where it's first needed?

Comment 18

[Josh Matthews [:jdm]]

The e10s patch looks fine to me. Let me know if you want my official stamp on it.

Comment 19

[Josh Matthews [:jdm]]

Comment on attachment 652599 [details] [diff] [review]
part5, v1: xpcshell test

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

::: netwerk/test/unit/head_channels.js
@@ +189,5 @@
> +/**
> + * Class that implements nsILoadContext.  Use it as callbacks for channel when
> + * test needs it.
> + */
> +function LoadContextCallback(appId, inBrowserElement, isPrivate, isContent) {

Could we pass a named params object instead? That would be reading tests much clearer.

@@ +205,5 @@
> +  },
> +  QueryInterface: function(iid) {
> +    if (iid == Ci.nsILoadContext ||
> +               Ci.interfaces.nsIInterfaceRequestor ||
> +               Ci.interfaces.nsISupports) {

Those |interfaces| don't look right.

@@ +208,5 @@
> +               Ci.interfaces.nsIInterfaceRequestor ||
> +               Ci.interfaces.nsISupports) {
> +        return this;
> +    }
> +    throw Cr.results.NS_ERROR_NO_INTERFACE;

That results doesn't look right.

::: netwerk/test/unit/test_cookiejars.js
@@ +57,5 @@
> +    // all cookies set: switch to checking them
> +    i = 0;
> +    checkCookie();
> +  } else {
> +    do_check_eq("setNextCookie:i=" + i, "setNextCookie:i=" + i); 

What's the point of this tautological check?

@@ +91,5 @@
> +    }
> +  }
> +  // If we get here we're good.
> +  do_check_eq("Saw only cookie '" + expectedCookie + "'",
> +              "Saw only cookie '" + expectedCookie + "'");

I'm not thrilled with this either.

Comment 20

[Josh Matthews [:jdm]]

Comment on attachment 652596 [details] [diff] [review]
part3, v1: e10s support

I would feel comfortable upgrading this f+ to an r+.

Comment 21

[Josh Matthews [:jdm]]

Comment on attachment 652599 [details] [diff] [review]
part5, v1: xpcshell test

I would feel comfortable upgrading this f+ to an r+.

Comment 22

[Josh Matthews [:jdm]]

Comment on attachment 652590 [details] [diff] [review]
part1, v1: change sqlite schema to support cookie namespaces

This is my first time reading through the cookie DB code, but it jives with my experience modifying the download manager DB code.

Comment 23

[Josh Matthews [:jdm]]

Comment on attachment 652595 [details] [diff] [review]
part2, v1: support cookie namespaces

I've read parts of this code before, and the changes included look correct to me.

Comment 24

[Jason Duell]

Attachment: part2, v2: support cookie namespaces

fix jdm's nits, and forward his +f

Comment 25

[Jason Duell]

Attachment: part2, v3: support cookie namespaces

Missed one of jdm's nits.

Comment 26

[Jason Duell]

Attachment: part5, v2: xpcshell test

updated per jdm's comments

Comment 27

[Jason Duell]

Attachment: part1, v2: change sqlite schema to support cookie namespaces

unbitrotting all patches (NSPR int types -> stdint.h changes).

Reassigning reviews to biesi, forwarding +f from jdm

Comment 28

[Jason Duell]

Attachment: part2, v4: support cookie namespaces

Comment 29

[Jason Duell]

Attachment: part3, v2: e10s support

jdm: slight tweak to this patch:  my half-hearted effort to build in some B2G security here failed: setting NECKO_UNKNOWN_APP_ID breaks tests, as cookies are usually set on the parent (with NECKO_NO_APP_ID), and so tests in e10s mode would fail when they tried to find them with NECKO_UNKNOWN_APP_ID.  So for now just using NECKO_NO_APP_ID for e10s channels w/o any loadContext.   When I lock down the IPDL protocol I'm hoping to eliminate that use case entirely.

This is the interdiff from the last version of the patch:

--- a/netwerk/cookie/CookieServiceParent.cpp
+++ b/netwerk/cookie/CookieServiceParent.cpp
@@ -83,18 +83,18 @@ CookieServiceParent::RecvSetCookieString

 void
 CookieServiceParent::GetAppInfoFromLoadContext(
                         const IPC::SerializedLoadContext &aLoadContext,
                         PRUint32& aAppId,
                         bool& aIsInBrowserElement)
 {
   // TODO: bug 782542: what to do when we get null loadContext?  For now assume
-  // UNKNOWN_APP (effectively a throwaway junk namespace).
-  aAppId = NECKO_UNKNOWN_APP_ID;
+  // NECKO_NO_APP_ID.
+  aAppId = NECKO_NO_APP_ID;
   aIsInBrowserElement = false;

Comment 30

[Jason Duell]

Attachment: part5, v3: xpcshell tests

Try is green except for a failure in a workers test, which is bug 784508.

  https://tbpl.mozilla.org/?tree=Try&rev=98df63292a82

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 654397 [details] [diff] [review]
part2, v4: support cookie namespaces

+++ b/netwerk/base/public/nsNetUtil.h
+// Note: UNKNOWN also equals PR_UINT32_MAX
+#define NECKO_UNKNOWN_APP_ID 4294967295

so... why not use PR_UINT32_MAX here?

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 654400 [details] [diff] [review]
part5, v3: xpcshell tests

rs=me

Comment 33

[Jason Duell]

Attachment: part2, v5: support cookie namespaces

unbitrotted

Comment 34

[Jason Duell]

Attachment: part3, v3: e10s support

unbitrotted

Comment 35

[Jason Duell]

Attachment: part4, v2: minor SerializedLoadContent fix

bitrot

Comment 36

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/156bdf05dd97

Comment 37

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/156bdf05dd97