See discussion starting here: https://bugzilla.mozilla.org/show_bug.cgi?id=570867#c11

need to investigate

This sounds related to bug 1043256, is it fixed now?

Moving to M3 milestone. These bugs affect e10s dogfooding but Brad and Gavin did not think they were M2 blockers.

This was actually fixed by bug 558624. We still waste some time calling SetCookie in the child, though. That's an easy fix.

I left the call to SetCookie because it notifies observers about the cookie (via the http-on-response-set-cookie) notification. I wasn't sure by looking at who uses it whether we need to send the notification in the child or not. I'm leaving it in for now.

Jason, any ETA on Blake's review request? :)

Comment on attachment 8507255 [details] [diff] [review] patch v1 Review of attachment 8507255 [details] [diff] [review]: ----------------------------------------------------------------- So I'm not sure if we can simply make SetCookie() a no-op like this in the child. We still have a couple places where we're explicitly calling it--so we should either remove those call sites, or if they're doing something important, do a different fix here: 1) https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#472 2) https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1072 At this point we only call SetCookie in one place in nsHttpChannel: https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1355 Honza, do you know if that call makes both #1 and #2 redundant to call in the child? #1 looks OK, but I'm less sure about the redirect case. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +1408,5 @@ > > + bool useCookieService = > + (XRE_GetProcessType() == GeckoProcessType_Default); > + nsresult rv = NS_OK; > + if (useCookieService) { Nit: we could just use "if (!IsNeckoChild())" here.

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #7) > Comment on attachment 8507255 [details] [diff] [review] > patch v1 > > Review of attachment 8507255 [details] [diff] [review]: > ----------------------------------------------------------------- > > So I'm not sure if we can simply make SetCookie() a no-op like this in the > child. We still have a couple places where we're explicitly calling it--so > we should either remove those call sites, or if they're doing something > important, do a different fix here: > > 1) > https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/ > HttpChannelChild.cpp#472 > 2) > https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/ > HttpChannelChild.cpp#1072 > > At this point we only call SetCookie in one place in nsHttpChannel: > > > https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/ > nsHttpChannel.cpp#1355 > > Honza, do you know if that call makes both #1 and #2 redundant to call in > the child? #1 looks OK, but I'm less sure about the redirect case. Both are absolutely redundant. As I understand, SetCookie on HttpChannelChild only ends up sending PCookieService.SetCookieString that again sets the same (and maybe even older!) string on the parent. http://hg.mozilla.org/mozilla-central/annotate/2209a8de5d23/netwerk/cookie/CookieServiceParent.cpp#l104 Both can definitely go away. OTOH, I kinda don't follow title of this bug - what is it we actually need here? Auth is fully handled on the parent. Dialogs are popped on the parent. Cookies and auth cache is handled on the parent. Child only receives OnStart/etc after the auth cycle is done (succeeded or failed). If there is a Cookie request header set on the child prior AsyncOpen we send it up in the request head serialization. On the parent it merges with what has been stored in the cookie service. And child then gets the request headers in RecvOnStartRequest. So we do update (Cookie and Set-Cookies is gonna be there as well). The title should update IMO.

Comment on attachment 8507255 [details] [diff] [review] patch v1 Review of attachment 8507255 [details] [diff] [review]: ----------------------------------------------------------------- Please just remove calls to SetCookie from HttpChannelChild. Do we still need this? http://hg.mozilla.org/mozilla-central/annotate/2209a8de5d23/netwerk/cookie/PCookieService.ipdl#l98

> Do we still need this? > http://hg.mozilla.org/mozilla-central/annotate/2209a8de5d23/netwerk/cookie/PCookieService.ipdl#l98 I assume we still need this to support setting cookies from JS in the child. i.e the nsHTMLDocument::SetCookie() codepath.

According info from Jason taking this bug.

Comment on attachment 8573394 [details] [diff] [review] v2 Review of attachment 8573394 [details] [diff] [review]: ----------------------------------------------------------------- My only question: do we have enough cookie test coverage to notice if this breaks anything?

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #13) > Comment on attachment 8573394 [details] [diff] [review] > v2 > > Review of attachment 8573394 [details] [diff] [review]: > ----------------------------------------------------------------- > > My only question: do we have enough cookie test coverage to notice if this > breaks anything? I'm not maintaining cookies, so I'm not the right one to ask. Maybe ehsan?