570867 - e10s http: add cookies to request in child, not parent

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[[e-mail]]

dougt is seeing an assert in HttpChannelParent, where AsyncOpen is trying to add cookies to the request. This is fail because the cookieservice requires a legit docshell heirarchy to determine whether the request is first or third party; this can only happen in the child.

Thus, the information needs to be pulled in the child, and added to the request header there -- which should then just automagically work, since the header gets serialized across.

Comment 2

[Benjamin Stover (:stechz)]

Attachment: Patch v1

First hack at this.  In nsHttpChannel, I had to know the difference between a remoted channel and otherwise, so I stole the (seemingly unused) mRemoteChannel flag to do my evil bidding.  This isn't ideal.  The right thing to do eventually is to have two different implementations (do you agree, Dan?).

Although this fixes the crash and sends cookies along with the request, it does not set cookies.  I think a follow-up bug for remoting SetCookieStringInternal is the best thing to do.

Comment 3

[Josh Matthews [:jdm]]

Note that mRemoteChannel is already used by nsHttpChannel::SetRemoteChannel which is called by HttpChannelParent.

Comment 4

[Josh Matthews [:jdm]]

Oh, right, you did see that.  Ignore that last comment.

Comment 5

[Jason Duell]

Comment on attachment 450442 [details] [diff] [review]
Patch v1

> I stole the (seemingly unused) mRemoteChannel flag to do my evil
> bidding.  This isn't ideal.  The right thing to do eventually
> is to have two different implementations

Why?  Seems like SetRemoteChannel is exactly what you need.

>+NS_IMETHODIMP
>+HttpChannelChild::SetRemoteChannel(PRBool aRemote)
>+{
>+  DROP_DEAD();
>+}

Don't change SetRemoteChannel into an IDL function; just keep it as a public function in nsHttpChannel only. We've already resolved that we don't care about trying to support third-party implementations of nsHttpChannel, so we are guaranteed that we've got an nsHttpChannel.  Just cast to it, like we do here:

>   nsHttpChannel *httpChan = static_cast<nsHttpChannel *>(mChannel.get());
>-  httpChan->SetRemoteChannel();
>+  httpChan->SetRemoteChannel(PR_TRUE);

You can keep the added bool flag--I see you need that for SetupReplacementChannel.

>+HttpBaseChannel::AddCookiesToRequest(const nsCString& userSetCookies)
>+{
>+    if (mLoadFlags & LOAD_ANONYMOUS) {
>+      return;
>+    }

Convert to 2-space indents.

>+
>+  // TODO: hook up cookie setting in child process
>+  // SetCookie(mResponseHead->PeekHeader(nsHttp::Set_Cookie));

Does this need to happen as part of this patch? I don't like adding in commented-out code, esp w/o a bug number.

>@@ -110,16 +110,17 @@ nsHttpChannel::nsHttpChannel()
>     , mCacheForOfflineUse(PR_FALSE)
>     , mCachingOpportunistically(PR_FALSE)
>     , mFallbackChannel(PR_FALSE)
>     , mInheritApplicationCache(PR_TRUE)
>     , mChooseApplicationCache(PR_FALSE)
>     , mLoadedFromApplicationCache(PR_FALSE)
>     , mTracingEnabled(PR_TRUE)
>     , mCustomConditionalRequest(PR_FALSE)
>+    , mRemoteChannel(PR_FALSE)

Oh, nice catch!  Thanks.

Everything else looks fine.

Comment 6

[Benjamin Stover (:stechz)]

> Does this need to happen as part of this patch? I don't like adding in
> commented-out code, esp w/o a bug number.

I thought it'd be better as a separate patch, since cookies will need to be sent back to the chrome process for saving in the cookie service (SetCookieStringInternal seemed liked a great place to do this).

I didn't put a bug # yet because I didn't know if there was one already.

Comment 7

[Benjamin Stover (:stechz)]

I see now that the cookie IPC work has already been done!  However, I'm still not able to log in to sites even though cookies are being set.

I have a sneaking suspicion this has to do with the redirect bug, but I want to verify AddCookiesToRequest is working as it should be first.

Comment 8

[Benjamin Stover (:stechz)]

Attachment: Patch v2

Review comments addressed.  Fixed a bug in AddCookiesToRequest so that cookies are sent over to the parent process correctly.  Cookies are now being used with HTTP requests!

Comment 9

[Benjamin Stover (:stechz)]

Attachment: The real patch v2

Sorry for spam, wrong attachment.

Comment 10

[Benjamin Stover (:stechz)]

Attachment: Patch

Apologies again!  I forgot to qref before export tip.  *gets coffee*

Comment 11

[[e-mail]]

Comment on attachment 450702 [details] [diff] [review]
Patch

>diff --git a/netwerk/protocol/http/HttpBaseChannel.cpp b/netwerk/protocol/http/HttpBaseChannel.cpp

>+void
>+HttpBaseChannel::AddCookiesToRequest(const nsCString& userSetCookies)

>+  // overwrite any existing cookie headers.  be sure to clear any
>+  // existing cookies if we have no cookies to set or if the cookie
>+  // service is unavailable.
>+  SetRequestHeader(nsCString(nsHttp::Cookie), cookie, PR_FALSE);

You can just directly set the header here, like nsHttpChannel did:

  mRequestHead.SetHeader(nsHttp::Cookie, cookie, PR_FALSE);

I assume you changed it for a reason, but I can't see why.

Convention has the inparam named 'aUserSetCookies' (in the .h too).

>diff --git a/netwerk/protocol/http/HttpChannelChild.cpp b/netwerk/protocol/http/HttpChannelChild.cpp

>@@ -114,16 +114,19 @@ HttpChannelChild::RecvOnStartRequest(con
>   if (!NS_SUCCEEDED(rv)) {

Gah. Can you drive-by this into NS_FAILED?

>@@ -297,17 +300,18 @@ HttpChannelChild::AsyncOpen(nsIStreamLis

>-  // FIXME bug 562587: need to dupe nsHttpChannel::AsyncOpen cookies logic 
>+  const char *cookieHeader = mRequestHead.PeekHeader(nsHttp::Cookie);
>+  AddCookiesToRequest(nsCString(cookieHeader));

nsDependentCString(cookieHeader) here.

>diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp

>@@ -843,19 +818,20 @@ nsHttpChannel::ProcessResponse()
> 
>     if (mTransaction->SSLConnectFailed() &&
>         !ShouldSSLProxyResponseContinue(httpStatus))
>         return ProcessFailedSSLConnect(httpStatus);
> 
>     // notify "http-on-examine-response" observers
>     gHttpHandler->OnExamineResponse(this);
> 
>-    // set cookies, if any exist; done after OnExamineResponse to allow those
>-    // observers to modify the cookie response headers

Keep this bit of the comment (push inside the 'if' block)?

>@@ -4119,23 +4098,24 @@ nsHttpChannel::AsyncOpen(nsIStreamListen

>+    if (!mRemoteChannel) {
>+      // For non-remote channels, we are responsible for cookies.
>+
>+      // Remember the cookie header that was set, if any
>+      const char *cookieHeader = mRequestHead.PeekHeader(nsHttp::Cookie);
>+      // fetch cookies, and add them to the request header
>+      AddCookiesToRequest(nsCString(cookieHeader));

nsDependentCString(cookieHeader) here.

>@@ -4932,17 +4912,20 @@ nsHttpChannel::DoAuthRetry(nsAHttpConnec

>     // fetch cookies, and add them to the request header.
>     // the server response could have included cookies that must be sent with
>     // this authentication attempt (bug 84794).
>-    AddCookiesToRequest();
>+    if (!mRemoteChannel) {
>+      // For non-remote channels, we are responsible for cookies.
>+      AddCookiesToRequest(nsCString());
>+    }

Could use EmptyCString() here.

But, this won't quite work. mUserSetCookieHeader is set in AsyncOpen() for a reason: calling AddCookiesToRequest() here will re-use that saved header. You're dropping that part. And we can't just not call AddCookiesToRequest() (i.e. reuse the existing cookie string the child gave us), because of the reason quoted in the comment.

I'm guessing that HttpChannelChild::RecvOnStartRequest would've been called prior to this, which would've set the cookies from the response. You could prove this by sticking some printfs around, to show that for auth retries, the call sequence is:

nsHttpChannel::OnStartRequest
HttpChannelChild::RecvOnStartRequest
HttpBaseChannel::SetCookie
nsHttpChannel::OnStopRequest
nsHttpChannel::DoAuthRetry

So you can solve this easily enough by having HttpChannelChild::RecvOnStartRequest, after it calls SetCookie, send an IPDL message across to the parent to say "update your cookie request header to this value". This will only be important for the auth retry case.

You could call it UpdateRequestHeader(atom, string, merge).

Does that make sense?

For the behavior of what used to be mUserSetCookieHeader, things are now pretty confusing. Does the child store it? The parent? Both? You should add it back in the parent, I think, to keep the behavior the same as it was. You could add it in the child easily enough too, I *think*. So the total request header would be the sum of both user set values.

Not going to require you to do it in the child, though. Can leave for later.

Want to look at this again.

Comment 12

[Benjamin Stover (:stechz)]

> You can just directly set the header here, like nsHttpChannel did:

mRequestHead isn't used in the child process--the tuple is used to send over the headers instead.  Using the method makes sure the cookies are properly set in both nsHttpChannel and Child.

> But, this won't quite work. mUserSetCookieHeader is set in AsyncOpen() for a
> reason: calling AddCookiesToRequest() here will re-use that saved header.
> You're dropping that part. And we can't just not call AddCookiesToRequest()
> (i.e. reuse the existing cookie string the child gave us), because of the
> reason quoted in the comment.

Ah!  I definitely missed that.

Comment 13

[Jason Duell]

Since all the PHttpChannel IPDL msgs are async, and the chrome nsHttpChannel's progress doesn't currently rely on any replies from the child process, there's currently no guarantee that RecvOnStartReq will have been called by the time nsHttpChannel::OnStopRequest is called (which is the only call to DoAuthRetry() I see).  For small HTTP requests it's actually quite likely that RecvOnStartReq won't have been called yet, as the local code's may run through OnStart/Data/Stop before the IPC msg is read by the child.

> So you can solve this easily enough by having
> HttpChannelChild::RecvOnStartRequest, after it calls 
> SetCookie, send an IPDL message across to the parent 
> to say "update your cookie request header to this value".

Unless we suspend the nsHttpChannel, this msg won't help the race condition.

For gross reasons that I've been hoping will be temporary, the download manager code (specifically the nsIEncodedChannel part--bug 567267) is going to require chrome to suspend the nsHttpChannel until we get an IPDL message back from the child indicating that OnStartRequest has been called.  If you really need to guarantee that RecvOnStartReq is called before DoAuthRetry, you have my temporary blessing to piggyback on this mechanism (for now, just add the IPDL msg as new--your code will be ready before the DL mgr stuff lands--and call it "StartRequestCompleted" instead of UpdateRequestHeader).

I'd really prefer that we not suspend the nsHttpChannel in the long run, though.  Is there any way we can resolve this issue without doing that?

Comment 14

[Jason Duell]

Oops, didn't look carefully enough at the code.

If we wind up calling DoAuthRetry, we haven't called OnStartRequest yet (on either the parent or child).  So there's no race--the child process definitely won't have processed the cookie yet.

So the solution, I assume, is to hook DoAuthRetry so that 1) if mRemoteChannel, it cause HttpChannelParent to send the cookie header to the child, where it can be added correctly; 2) have the child function sent a message saying the cookie has been added, and then 3) have HttpChannelParent call some new 'continueDoAuthRetry' method on the nsHttpChannel, which can then proceed as normal (i.e. re-request, with auth and cookie).  Does that make sense?  It adds a round-trip IPDL time to the logic, but only when HTTP auth has been entered, which is not the common case, obviously.

Comment 15

[Jason Duell]

P.S. Since the DoAuthRetry part of this bug is getting complex, and depends on bug 537782 to test, can we split that part off into a new bug, and just land the rest?  I'd like to stop crashing the browser at startup.  Which we could do with a stopgap hack to GetInterface, but it seems like we're just about ready with the rest of this patch.

Comment 16

[[e-mail]]

Sure, let's split the auth bit out. Ben, wanna file a bug on that?

We might have to do something similar with redirects, but there's a separate bug for that too. :)

Comment 17

[Jason Duell]

P.S.  If mRemoteChannel is set, we know for sure that mListener is a HttpChannelParent, so you can just cast it to that and call SendCookieToChild (or whatever you want to call it) directly.  Better than adding XPCOM interface functions, etc.

Comment 18

[Benjamin Stover (:stechz)]

Attachment: Review changes

Follow up bug here: https://bugzilla.mozilla.org/show_bug.cgi?id=572151

Dan: could you do one last pass please?

Comment 19

[[e-mail]]

Comment on attachment 451324 [details] [diff] [review]
Review changes

>+void
>+HttpBaseChannel::AddCookiesToRequest(const nsCString& userSetCookies)

>+  // overwrite any existing cookie headers.  be sure to clear any
>+  // existing cookies if we have no cookies to set or if the cookie
>+  // service is unavailable.
>+  SetRequestHeader(nsCString(nsHttp::Cookie), cookie, PR_FALSE);

nsDependentCString(nsHttp::Cookie)

>@@ -4119,23 +4100,25 @@ nsHttpChannel::AsyncOpen(nsIStreamListen

>-    // Remember the cookie header that was set, if any
>-    const char *cookieHeader = mRequestHead.PeekHeader(nsHttp::Cookie);
>-    if (cookieHeader)
>-        mUserSetCookieHeader = cookieHeader;
>-
>-    // fetch cookies, and add them to the request header
>-    AddCookiesToRequest();
>+    if (!mRemoteChannel) {
>+      // For non-remote channels, we are responsible for cookies.
>+
>+      // Remember the cookie header that was set, if any

This comment isn't really true anymore: we're not remembering it, just appending it.

I think you should revert this bit: for non-remote channels, keep mUserSetCookieHeader in the parent, and pass it into AddCookiesToRequest() (including in DoAuthRetry()). Forget about the child case for now, but can you file a bug on persisting cookie-related SetRequestHeader bits there?

Otherwise looks good. r=dwitte

Comment 20

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/projects/electrolysis/rev/b480c5f26a70