1645901 - Send necessary cookie through pBackground before OnStartRequest

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P2)

Description

[Junior [inactive]]

In bug 1633935, Set-Cookie is passed to child process on main thread, which is racy with PBackground. We should have a way to set cookie in child for Set-Cookie response header through pBackground.

Comment 1

[Junior [inactive]]

Attachment: Bug 1645901 - only waiting for main thread OnStartRequestSent for LOAD_DOCUMENT_NEEDS_COOKIE, r=mayhemer

Depends on D79771

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cb44457d718
only waiting for main thread OnStartRequestSent for LOAD_DOCUMENT_NEEDS_COOKIE, r=mayhemer,necko-reviewers

Comment 3

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on /cookies/path/match.html

backout: https://hg.mozilla.org/integration/autoland/rev/45273689ec099085c9dacde4458dd81814d6020c

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=os%2Cx%2C10.14%2Cdebug%2Cweb%2Cplatform%2Ctests%2Ctest-macosx1014-64%2Fdebug-web-platform-tests-e10s-18%2Cw%28wpt18%29&revision=8cb44457d71805d8300eaf3d54a2a08317112880&selectedTaskRun=HtVIG91VR6SmlqZe1QWHAg.0

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=307428436&repo=autoland&lineNumber=2551

[task 2020-06-24T21:12:04.476Z] 21:12:04 INFO - TEST-PASS | /cookies/path/match.html | document.cookie on /cookies/resources/echo-cookie.html DOES NOT set cookie for path: /w/
[task 2020-06-24T21:12:04.476Z] 21:12:04 INFO - TEST-PASS | /cookies/path/match.html | Set-Cookie on /cookies/resources/echo-cookie.html sets cookie with path: /
[task 2020-06-24T21:12:04.476Z] 21:12:04 INFO - TEST-UNEXPECTED-FAIL | /cookies/path/match.html | Set-Cookie on /cookies/resources/echo-cookie.html sets cookie with path: match.html - assert_not_equals: Cookie path from header should not be null got disallowed value null
[task 2020-06-24T21:12:04.476Z] 21:12:04 INFO - testCookiePathFromHeader/iframe</<@http://web-platform.test:8000/cookies/path/match.html:48:26
[task 2020-06-24T21:12:04.476Z] 21:12:04 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1977:25
[task 2020-06-24T21:12:04.476Z] 21:12:04 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:2002:35
[task 2020-06-24T21:12:04.477Z] 21:12:04 INFO - ......
[task 2020-06-24T21:12:04.477Z] 21:12:04 INFO - TEST-OK | /cookies/path/match.html | took 3608ms

Comment 4

[Junior [inactive]]

Thanks.

Comment 5

[Junior [inactive]]

I did the analysis in bug 1633935 Comment 27 but I totally forget.

Comment 6

[Junior [inactive]]

Hello :baku,
Bug 1633935 sends OnStartReuqest over PHttpBackgroundChannel, but bug 1633935 Comment 27 indicates that we rely on PContent IPC (PCookieServiceParent::SendAddCookie) before OnStartRequest. Hence, we're suffering the benefits of PBackground given we're still need to wait the PContent IPC.

I'm thinking that to bring the Set_Cookie header value via PHttpBackgroundChannel::OnStartRequest and do HttpBaseChannel::SetCookie again in HttpChannelChild::OnStartRequest.

I know there will be a redundant check in HttpChannelChild if I do HttpBaseChannel::SetCookie in both parent and child process, but seems no harm.
I did a quick experiment and it works.

Is it a plan good to go? Thanks.

Comment 7

[Andrea Marchesini [:baku]]

I'm thinking that to bring the Set_Cookie header value via PHttpBackgroundChannel::OnStartRequest and do HttpBaseChannel::SetCookie again in HttpChannelChild::OnStartRequest.

CookieService is not thread-safe and it's main-thread only. Are you going to call PBackground methods on the main-thread? If yes, I don't see issues. Do you mind if I review your code? Thanks!

Comment 8

[Junior [inactive]]

HttpBaseChannel::SetCookie is dispatched to main thread. It would be great to have your review.

Comment 9

[Phabricator Automation]

Comment on attachment 9158602 [details]
Bug 1645901 - only waiting for main thread OnStartRequestSent for LOAD_DOCUMENT_NEEDS_COOKIE, r=mayhemer

Revision D80682 was moved to bug 1649965. Setting attachment 9158602 [details] to obsolete.

Comment 10

[Junior [inactive]]

Attachment: Bug 1645901 - handle cookie setting in child process for Set-Cookie response, r=baku,mayhemer

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14420a624d0a
handle cookie setting in child process for Set-Cookie response, r=baku,mayhemer,necko-reviewers

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/14420a624d0a