537156 - [e10s] Implement cookies

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Benjamin Smedberg]

When we remote networking to the other process, we'll need special support for cookies. document.cookie at least needs to be able to read and write cookies for the current domain.

Comment 1

[[e-mail]]

-> me

I'll take a look at this once jduell lands his HttpChannel patch.

Comment 2

[[e-mail]]

Attachment: patch v1

Like so. Pretty simple; we maintain a singleton nsCookieService in the parent, just as with the single-process case. The nsICookieService singleton getter returns either the nsCookieService singleton in the parent, or a CookieServiceChild singleton in the child. The child interacts with the parent synchronously. A little rejiggering of internal nsCookieService methods was required to make serialization simple. (Since we can't send an nsIChannel across the wire and have the docshell hierarchy look sane, we get what we need from it in the child first. This isn't quite optimal, but it won't break anything important. See comments in patch for details.)

This only implements nsICookieService, which is necessary and sufficient for Gecko use. nsICookieManager{2} will simply be unavailable in the child, so any extensions expecting that to work will fail. I'm fine with that for now, since serializing cookie objects will be a lot more work.

This will get document.cookie working and most of what HttpChannel needs. Hooking that up I leave to a later exercise, since it requires a lot more work in figuring out the interaction model.

Eventually we might want to have child processes maintain a cache of interesting cookies, but as bsmedberg pointed out we have to be careful not to introduce races.

This includes a couple of simple tests; I'm reworking all the cookie tests on mozilla-central at the moment so more extensive testing can wait until that's done.

Comment 3

[Benjamin Smedberg]

I don't think the child network code will need access to cookies at all, since it should be the parent's responsibility to set up the channel after it is opened.

Comment 4

[[e-mail]]

I hope so! For the simple case of "here's a URI, open a channel", things will work. But if we want Get/SetRequestHeader f.e. to work in the child, and whatever is using that expects to be able to fiddle with cookies... RESO LATER :)

Comment 5

[[e-mail]]

Oh, we'll need to additionally implement nsIHttpChannelInternal::SetCookie at least, to get META HTTP-EQUIV Set-Cookie working. That bit will be trivial, can do in a separate patch.

Comment 6

[[e-mail]]

Comment on attachment 426762 [details] [diff] [review]
patch v1

bsmedberg said I should kick this over to bz and jduell for review.

bz -- looking for SR on the IPDL API bits; PCookieService.ipdl and PNecko.ipdl.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 426762 [details] [diff] [review]
patch v1

ipdl looks fine

Comment 8

[[e-mail]]

Comment on attachment 426762 [details] [diff] [review]
patch v1

Kicking to sdwilsh.

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> This only implements nsICookieService, which is necessary and sufficient for
> Gecko use. nsICookieManager{2} will simply be unavailable in the child, so any
> extensions expecting that to work will fail. I'm fine with that for now, since
> serializing cookie objects will be a lot more work.
I don't think we'll ever have a need for nsICookieManager[2] in the content process.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 426762 [details] [diff] [review]
patch v1

For more detailed comments with context, please see http://reviews.visophyte.org/r/426762/.

on file: extensions/cookie/nsCookiePermission.cpp line 190
>   // Lazily initialize ourselves
>   if (!mPermMgr && !Init())
>     return NS_ERROR_UNEXPECTED;
> 

So I'm not a huge fan of dong this like this.  Can you make an inlined
isInitiaized method that does these checks and call it instead?  Also,
probably want to wrap it in NS_UNLIKELY since it's not the common path
(although hopefully the compiler would know this).


on file: netwerk/cookie/src/CookieServiceChild.h line 1
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

vim modeline too please


on file: netwerk/cookie/src/CookieServiceChild.cpp line 132
>   NS_ENSURE_ARG(aCookieString);

nit: NS_ENSURE_ARG_POINTER


on file: netwerk/cookie/src/CookieServiceChild.cpp line 134
>   *aCookieString = nsnull;

nit: I'm 98% we can just use NULL, so please do so.


on file: netwerk/cookie/src/CookieServiceChild.cpp line 158
>   NS_ENSURE_ARG(aHostURI);
>   NS_ENSURE_ARG(aCookieString);

Don't you need to check the server time here too?


on file: netwerk/cookie/src/CookieServiceParent.cpp line 79
>   NS_NewURI(getter_AddRefs(hostURI),
>             aHostSpec, aHostCharset.get(),
>             nsnull, mIOService);
>   NS_NewURI(getter_AddRefs(originatingURI),
>             aOriginatingSpec, aOriginatingCharset.get(),
>             nsnull, mIOService);

Oh gosh this can't be fast :(


on file: netwerk/cookie/src/CookieServiceParent.cpp line 88
>   mCookieService->GetCookieInternal(hostURI, originatingURI,
>                                     aFromHttp, *aResult);
>   return true;

Should this be:
return !!NS_SUCCEEDED(mCookieService...)?


on file: netwerk/cookie/src/CookieServiceParent.cpp line 117
>   mCookieService->SetCookieStringInternal(hostURI, originatingURI,
>                                           aCookieString, aServerTime,
>                                           aFromHttp);
>   return true;

ditto here


r=sdwilsh with those changes.  You should get a necko peer to review the necko bits.

Comment 11

[[e-mail]]

(In reply to comment #10)
> So I'm not a huge fan of dong this like this.  Can you make an inlined
> isInitiaized method that does these checks and call it instead?  Also,
> probably want to wrap it in NS_UNLIKELY since it's not the common path
> (although hopefully the compiler would know this).

Sure thing.

> on file: netwerk/cookie/src/CookieServiceChild.cpp line 134
> >   *aCookieString = nsnull;
> 
> nit: I'm 98% we can just use NULL, so please do so.

They are different on some platforms. But not for this case, so sure. And the difference is actually a bug in nsnull, not NULL. :)

> on file: netwerk/cookie/src/CookieServiceChild.cpp line 158
> Don't you need to check the server time here too?

Nope, it's legal to be null.

> on file: netwerk/cookie/src/CookieServiceParent.cpp line 79
> >   NS_NewURI(getter_AddRefs(hostURI),
> >             aHostSpec, aHostCharset.get(),
> >             nsnull, mIOService);
> >   NS_NewURI(getter_AddRefs(originatingURI),
> >             aOriginatingSpec, aOriginatingCharset.get(),
> >             nsnull, mIOService);
> 
> Oh gosh this can't be fast :(

Yeah. :( We need a better way to deserialize URIs where we know which implementation should be used. Which means that information needs to be serialized somehow. Later, I hope...

> on file: netwerk/cookie/src/CookieServiceParent.cpp line 88
> Should this be:
> return !!NS_SUCCEEDED(mCookieService...)?

These Get/Set methods can't and shouldn't fail, so it's fine. If that changes, then yeah.

> r=sdwilsh with those changes.  You should get a necko peer to review the necko
> bits.

Will do.

Comment 12

[[e-mail]]

Comment on attachment 426762 [details] [diff] [review]
patch v1

jduell, can you please review the netwerk/ipc/Necko{Child,Parent}.cpp bits? (It's just a few lines.)

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> These Get/Set methods can't and shouldn't fail, so it's fine. If that changes,
> then yeah.
Then let's add an assertion that the method succeeded to make sure please.

Comment 14

[Jason Duell]

Comment on attachment 426762 [details] [diff] [review]
patch v1

Necko bits look fine.

>diff --git a/netwerk/cookie/src/CookieServiceChild.cpp b/netwerk/cookie/src/CookieServiceChild.cpp
>
>+CookieServiceChild*
>+CookieServiceChild::GetSingleton()
>+
>+  gCookieService = new CookieServiceChild();
>+  if (gCookieService) {
>+    NS_ADDREF(gCookieService);
>+    if (!gCookieService->Init())
>+      NS_RELEASE(gCookieService);
>+  }

Don't need the if(gCookieService), now that we've got infallible malloc?


>diff --git a/netwerk/cookie/src/PCookieService.ipdl b/netwerk/cookie/src/PCookieService.ipdl
>+  sync SetCookieString(nsCString hostSpec,
>+                       nsCString hostCharset,
>+                       nsCString originatingSpec,
>+                       nsCString originatingCharset,
>+                       nsCString cookieString,
>+                       nsCString serverTime,
>+                       bool fromHttp);

For efficiency, can we make SetCookieString async?  The only error check I see in CookieServiceParent::RecvGetCookieString is to check if child omitted HostURI--can't that check be done on child, and then this can be async?

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> For efficiency, can we make SetCookieString async?  The only error check I see
> in CookieServiceParent::RecvGetCookieString is to check if child omitted
> HostURI--can't that check be done on child, and then this can be async?
+1 on this

Comment 16

[[e-mail]]

(In reply to comment #14)

> For efficiency, can we make SetCookieString async?  The only error check I see
> in CookieServiceParent::RecvGetCookieString is to check if child omitted
> HostURI--can't that check be done on child, and then this can be async?

I don't *think* SetCookieString can race (which was the original reason for making both calls sync). It's different to GetCookieString in this regard.

So, I'll make it async, and if we discover a race we can change it.

Comment 17

[[e-mail]]

Comment on attachment 426762 [details] [diff] [review]
patch v1

http://hg.mozilla.org/projects/electrolysis/rev/9f69e891fb08