1903412 - WPT recent nameless cookie preceeded by equals

Status: ASSIGNED

(Core :: Networking: Cookies, defect, P2)

Description

[edgul]

Fix the WPT test cookies/name/name.html Return the most recent nameless cookie, even if preceded by =, in addition to other valid cookie

Comment 1

[Andrea Marchesini [:baku]]

This is what I have found so far:

1. The test sets the following cookies: test11, test11a, test11b, test=11c
2. the first 3 cookies are nameless and by spec, the last one (test11b) "wins" against the previous two: "If the user agent receives a new cookie with the same cookie-name, domain-value, and path-value as a cookie that it has already stored, the existing cookie is evicted and replaced with the new cookie."
3. After the cookies are parsed, we store the following data: Cookie 1 (name="", value="test11b", creationTime: t0) and Cookie 2 (name="test", value="11c", creationTime: t1).
4. The cookies are sent to the child process.
5. The child process ends up parsing the cookies again.
6. For the same logic as point 2, test11 replaces test11b; test11a replaces test11; test11b replaces test11a. Now test11b has a new creation time t2.
7. For the same logic as point 2, the cookie test is not replaced.
8. The child has the following cookie: Cookie 1 (name="test", value="11c", creationTime: t1), Cookie 2(name="", value="test11b", creationTime: t2).
9. The browser sorts the cookie as test=11c, test11b instead of test11b, test=11c.

I want to check why we parse the cookies again in the child process. If there is a valid reason, the test is wrong.

Comment 2

[edgul]

• cookies/name/name.html seems to just be using http cookies, so I don't think we need to worry about SetCookieStringFromDocument in any process.
• I'm also not readily seeing any way for a set-cookie and broadcast from the parent to trigger parsing on the child.

So I'm guessing the parsing is occurring again because the we are taking the alternative pathway to the child's SetCookieStringFromHttp.

IIUC, this pathway should only be used on a proxy-connect success, however Leander recently discovered that it's possible this function is called in child and parent (from at least a test environment). I looked at this a bit and found 1 way to get make the function run in both processes: use an ipc_unit test that uses run_test_in_child(). Maybe this test also elicits the race?

IMO this looks like a failure to restrict ourselves to one code-path for parsing in a test scenario, but maybe it happens in the wild and maybe there is code that depends on it, like say if it was needed for other edge cases like non-fission builds (?). @Leander do you have any more info on how you were able to run the SetCookieStringFromHttp in both content and parent with a single http cookie?

Comment 3

[Leander Schwarz]

The function CookieServiceChild::SetCookieStringFromHttp actually is always called when setting an Http cookie from some HttpResponse promise sent to the child, but it's a NOOP since the parent process already set the cookie and sent it in TackCookieLoad. So I think the cookie should just be dropped, maybe that is where something like last access time or anything gets updated for some reason? - And it might be this way to cover some edge case which I do not know.

Comment 4

[edgul]

Attachment: Bug 1903412 - Remove race to set an http cookie on the child process. r?valentin,baku

Local tests seem okay.

Comment 5

[edgul]

Attachment: WIP: Bug 1903412 - Fix failure to NOOP second http-only cookie broadcast on set-cookie race to child process

Comment 6

[edgul]

Attachment: Bug 1903412 - Fixed name/name.html WPT: recent nameless cookie preceeded by equals. r?valentin,baku

Comment 7

[Valentin Gosu [:valentin] (he/him)]

After having a chat with Ed and Baku today, we came up with a different plan for this. Let me know if you see any issues with this

1. We need an operationID like counter to make sure we don't set the cookie twice.
   • Make nsICookieService::setCookieStringFromHttp have an [optional] out uint64_t cookieNotificationIndex
   • Generate that index here and return it in the parameter.
   • Make sure the notificationIndex is returned from here to nsHttpChannel then passed into mozilla::net::HttpChannelParent::SetCookie and through the IPC HttpChannelParams.ipdlh
   • mozilla::net::CookieServiceParent::AddCookie should also take that index and send it to the content process through IPC
   • On the child side, in mozilla::net::CookieServiceChild::RecordDocumentCookie we should use that index to do the following:
     • if the notification's index is larger than the largest notification index, that means we can process the cookie notification and add/replace the cookie. Make the largestNotificationIndex be the current notification Index.
     • Otherwise, if the index is lower or equal, that means we've already processed that cookie.
   • Figure out if we need to worry about an overflow of the index
2. pass creation time with HTTP header similar to the notificationIndex, so the child doesn't pick a different ordering of the cookies set from HTTP headers.
3. null out httpOnly values. here

if (parser.CookieData().isHttpOnly()) {
  parser.CookieData().value().Truncate();
}

4. check if the cookieStore's operationID can be replaced with the uint64_t operationID type, and the code could be simplified.