Closed Bug 1645901 Opened 3 months ago Closed 3 months ago

Send necessary cookie through pBackground before OnStartRequest

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox80 --- fixed

People

(Reporter: CuveeHsu, Assigned: CuveeHsu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file, 1 obsolete file)

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.

Assignee: nobody → juhsu
Status: NEW → ASSIGNED
Pushed by juhsu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8cb44457d718
only waiting for main thread OnStartRequestSent for LOAD_DOCUMENT_NEEDS_COOKIE, r=mayhemer,necko-reviewers

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

Flags: needinfo?(juhsu)

Thanks.

Flags: needinfo?(juhsu)

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

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.

Flags: needinfo?(amarchesini)

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!

Flags: needinfo?(amarchesini)

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

Blocks: 1649965

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.

Attachment #9158602 - Attachment is obsolete: true
Pushed by juhsu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14420a624d0a
handle cookie setting in child process for Set-Cookie response, r=baku,mayhemer,necko-reviewers
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
You need to log in before you can comment on or make changes to this bug.