1363311 - [e10s] SetCookie in a multipart content doesn't do anything

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Dragana Damjanovic [:dragana]]

This is assertion only in debug build in child process at line:
https://hg.mozilla.org/mozilla-central/annotate/tip/netwerk/streamconv/converters/nsMultiMixedConv.cpp#l1032
(thisis in nsMultiMixedConv::ProcessHeader())

I tried to do a simple search in bugzilla and it fails every time.

It tries to set cookie that is not implemented in child process:
CookieServiceChild::SetCookieStringFromHttp

https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/CookieServiceChild.cpp#227

Comment 1

[Honza Bambas (:mayhemer)]

Do you have a page you can repro with by hand?

Comment 2

[Dragana Damjanovic [:dragana]]

This is happening only for a debug build.

Go to https://bugzilla.mozilla.org/query.cgi?format=specific
and enter anything in search field and press enter.

Comment 3

[Honza Bambas (:mayhemer)]

OK, this is a cookie service bug.  Not a regression, this is there since e10s.  CookieServiceChild::SetCookieStringFromHttp [1] is not implemented on the child.

Amy, could you take a look?  If you are busy, bounce back.

[1] https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/netwerk/cookie/CookieServiceChild.cpp#208

Comment 4

[Amy Chung [:Amy]]

Hi Honza, 
I will test this bug and try to find the root-cause first.
If this bug is urgent, it is my view that this bug have to find someone else to fix it.
otherwise, I would take a look at this bug.

Comment 5

[Shian-Yow Wu [:swu]]

CookieServiceChild::SetCookieStringFromHttp was removed by bug 1339129 to avoid child process to access the HTTP-only cookies.  Shall we also avoid setting cookie in this case here in [1]?

[1] https://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/netwerk/streamconv/converters/nsMultiMixedConv.cpp#1027

Comment 6

[(no longer active)]

Attachment: Assertion stack

This way the next person to look at the bug doesn't need to spend the time to reproduce it.  :-)

Comment 7

[(no longer active)]

OK, as far as I can tell, in bug 1339129 we have broken the handling of multipart/mixed responses with an embedded Set-Cookie header inside them in e10s mode.  :-(

Josh, if we revert parts of those changes to allow the child process to only set HTTP-only cookies like it used to before, how big of a wrench would that throw into our plans for bug 1331680?  :-/

Comment 8

[Julien Cristau [:jcristau]]

sounds like a bad regression, tracking for 54/55

Comment 9

[(no longer active)]

Bug 1331680 only depends on not having access to HTTP-only cookies in the child process.  Bug 1339129 lumped that in together with removing support for CookieServiceChild::SetCookieStringFromHttp() from the content process but I don't think that part is necessary at all, we can just bring back that part and it wouldn't interfere with bug 1331680.

Comment 10

[(no longer active)]

Another discovery, we never seemed to have handled the HTTP-only case here correctly, even before bug 1339129!

Comment 11

[(no longer active)]

Attachment: Honor Set-Cookie headers in multipart/mixed response boundary headers in e10s mode

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/630cc9a74b64
Honor Set-Cookie headers in multipart/mixed response boundary headers in e10s mode; r=jdm

Comment 13

[(no longer active)]

Comment on attachment 8869669 [details] [diff] [review]
Honor Set-Cookie headers in multipart/mixed response boundary headers in e10s mode

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1339129
[User impact if declined]: Set-Cookie headers in multipart/mixed response boundary headers aren't honored in e10s mode.
[Is this code covered by automated tests?]: The patch has a test.
[Has the fix been verified in Nightly?]: No, the patch just landed.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not that much.  We're piping the boolean flag that we should have passed to the parent process to the right place.
[Why is the change risky/not risky?]: I just answered the question.
[String changes made/needed]: None.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/630cc9a74b64

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8869669 [details] [diff] [review]
Honor Set-Cookie headers in multipart/mixed response boundary headers in e10s mode

Fix a cookie issue in e10s mode. Beta54+. Should be in 54 beta 11.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a9c5f2fbb7fb

Comment 17

[Andrei Vaida [:avaida]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #13)
> [Is this code covered by automated tests?]: The patch has a test.
> [Has the fix been verified in Nightly?]: No, the patch just landed.
> [Needs manual test from QE? If yes, steps to reproduce]:  No.

Setting qe-verify- based on Ehsan's assessment on manual testing needs and the fact that this fix has automated coverage.