Closed Bug 756648 Opened 8 years ago Closed 7 years ago

Implement "cookie jars" for apps

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla18
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: sicking, Assigned: jduell.mcbugs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [LOE:M] [WebAPI:P1] [qa-])

Attachments

(5 files, 10 obsolete files)

6.50 KB, patch
Biesinger
: review+
jduell.mcbugs
: feedback+
Details | Diff | Splinter Review
10.90 KB, patch
Biesinger
: review+
jduell.mcbugs
: feedback+
Details | Diff | Splinter Review
45.50 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
9.81 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
1.19 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
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?
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Assignee: nobody → jduell.mcbugs
Depends on: 775119
Depends on: 775861
Blocks: 722850
Depends on: 777620
Jason: Any updates or ETA here?
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).
> 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?
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 :)
OS: Mac OS X → All
Hardware: x86 → All
No longer depends on: 777620
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.
How are we doing here?
Priority: -- → P1
Blocks: 770691
I'm very close to posting the patches.
Ping, anything you need to make this happen?
I think this is actually LOE:S for the remaining work, but getting reviews may take a while.
Whiteboard: [LOE:M]
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.
Attachment #652590 - Flags: review?(sdwilsh)
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.
Attachment #652595 - Flags: review?(sdwilsh)
Attached patch part3, v1: e10s support (obsolete) — Splinter Review
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.
Attachment #652596 - Flags: review?(sdwilsh)
SerializedLoadContext::SerializedLoadContext already handles null/missing LoadContext, but wasn't handling null channels.
Attachment #652597 - Flags: review?(bugs)
Attached patch part5, v1: xpcshell test (obsolete) — Splinter Review
Test that channels with different AppIds/inBrowserElements are stored in separate namespaces.
Attachment #652599 - Flags: review?(sdwilsh)
Attachment #652597 - Flags: review?(bugs) → review+
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).
Blocks: 783407
Blocks: 783408
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?
The e10s patch looks fine to me. Let me know if you want my official stamp on it.
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 on attachment 652596 [details] [diff] [review]
part3, v1: e10s support

I would feel comfortable upgrading this f+ to an r+.
Attachment #652596 - Flags: feedback+
Comment on attachment 652599 [details] [diff] [review]
part5, v1: xpcshell test

I would feel comfortable upgrading this f+ to an r+.
Attachment #652599 - Flags: feedback+
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.
Attachment #652590 - Flags: feedback+
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.
Attachment #652595 - Flags: feedback+
fix jdm's nits, and forward his +f
Attachment #652595 - Attachment is obsolete: true
Attachment #652595 - Flags: review?(sdwilsh)
Attachment #652854 - Flags: feedback+
Attachment #652854 - Flags: review?(sdwilsh)
Missed one of jdm's nits.
Attachment #652856 - Flags: review?(sdwilsh)
Attachment #652856 - Flags: feedback+
Attachment #652854 - Attachment is obsolete: true
Attachment #652854 - Flags: review?(sdwilsh)
Attached patch part5, v2: xpcshell test (obsolete) — Splinter Review
updated per jdm's comments
Attachment #652599 - Attachment is obsolete: true
Attachment #652599 - Flags: review?(sdwilsh)
Attachment #652880 - Flags: review?(sdwilsh)
Attachment #652880 - Flags: feedback+
Depends on: 784508
unbitrotting all patches (NSPR int types -> stdint.h changes).

Reassigning reviews to biesi, forwarding +f from jdm
Attachment #652590 - Attachment is obsolete: true
Attachment #652590 - Flags: review?(sdwilsh)
Attachment #654395 - Flags: review?(cbiesinger)
Attachment #654395 - Flags: feedback+
Attachment #652856 - Attachment is obsolete: true
Attachment #652856 - Flags: review?(sdwilsh)
Attachment #654397 - Flags: review?(cbiesinger)
Attachment #654397 - Flags: feedback+
Attached patch part3, v2: e10s support (obsolete) — Splinter Review
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;
Attachment #654399 - Flags: review?(cbiesinger)
Attachment #654399 - Flags: feedback?(josh)
Attachment #652596 - Attachment is obsolete: true
Attachment #652596 - Flags: review?(sdwilsh)
Attachment #654395 - Flags: review?(cbiesinger) → review+
Try is green except for a failure in a workers test, which is bug 784508.

  https://tbpl.mozilla.org/?tree=Try&rev=98df63292a82
Attachment #652880 - Attachment is obsolete: true
Attachment #652880 - Flags: review?(sdwilsh)
Attachment #654400 - Flags: review?(cbiesinger)
Attachment #654400 - Flags: feedback+
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?
Attachment #654397 - Flags: review?(cbiesinger) → review+
Attachment #654399 - Attachment is patch: true
Attachment #654399 - Flags: review?(cbiesinger) → review+
Comment on attachment 654400 [details] [diff] [review]
part5, v3: xpcshell tests

rs=me
Attachment #654400 - Flags: review?(cbiesinger) → review+
Attachment #654399 - Flags: feedback?(josh) → feedback+
Whiteboard: [LOE:M] → [LOE:M] [WebAPI:P1]
No longer depends on: 786835
unbitrotted
Attachment #654397 - Attachment is obsolete: true
Attachment #662156 - Flags: review+
unbitrotted
Attachment #662157 - Flags: review+
Attachment #654399 - Attachment is obsolete: true
bitrot
Attachment #652597 - Attachment is obsolete: true
Attachment #662167 - Flags: review+
Blocks: 792421
https://hg.mozilla.org/mozilla-central/rev/156bdf05dd97
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Whiteboard: [LOE:M] [WebAPI:P1] → [LOE:M] [WebAPI:P1] [qa-]
You need to log in before you can comment on or make changes to this bug.