1828131 - Investigate if we still need GenerateUniqueCreationTime and gLastCreationTime for cookie creation

Status: NEW

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

Description

[Paul Zühlcke [:pbz]]

gLastCreationTime is used to ensure that cookie time for newly created cookie objects is monotonically increasing. It was originally added in Bug 591447 to fix issues with the system clock changing. Since the cookie store has changed over the years we should investigate whether we still need it.

The mechanism can lead to issues where if one cookie has an invalid large creation time all newly set cookies will inherit this invalid creation time.

See description for gLastCreationTime here: https://searchfox.org/mozilla-central/rev/7e1f58e993f362d5d16bd1230a4417ebb2aa07b3/netwerk/cookie/Cookie.cpp#23-29

Comment 1

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

Ok, so if I understand this correctly, this is to work around the fact that PR_Now() returns the number of usec from epoch as given by the system clock. That means if the user suddenly changes their clock to yesterday, PR_Now() will return a lower value than it did just before they changed the time.

gLastCreationTime holds the largest value passed to GenerateUniqueCreationTime. If a smaller value is passed, then it simply increments gLastCreationTime and returns that.
From what I can tell the reason is that we want to keep the correct order of cookies. If we change the date back and add a new cookie, we want the new cookie to actually be newer than current cookies, otherwise the cookie eviction algorithm might evict the newer cookie instead of the old ones.

I've thought a bit about fixing up the dates, but the fact remains that if the system clock changes, then we use that as a creation time for new cookies, the other cookies we have in memory would have to get updated. That seems tricky to do, especially considering we also have cookies in other processes.

I've also looked at using PR_IntervalNow. Might be slightly better than just increasing gLastCreationTime each time there's a mismatch, but I'm not sure it's worth making the code more complex.

However, I believe the fact that we still need this doesn't necessarily block bug 1828126.
We might still be able to fix timestamps that are too far in the future when loading the cookie DB, by setting them to PR_Now().
The only scenario that we'd break is if the user sets their time to 1970, restarts, all their cookies get set to 1970, they set the correct date, all of their cookies are now stale; I'm not sure if that's worse than cookies with a future creation date are never stale AND also contaminate newer cookies with their corrupted creation date.

Thoughts?

Comment 2

[Ed Guloien [:edgul]]

We might still be able to fix timestamps that are too far in the future when loading the cookie DB, by setting them to PR_Now()

It sounds like the original concern included duplicate timestamps across cookies due to multiple cookies being created at the same time. Is this only under clock rollback circumstances or is it possible that our current implementation assigns the same creation time for multiple cookies under "normal" circumstances? https://hg.mozilla.org/mozilla-central/rev/03f4d774f862#l1.24

I need to take another dive into this, but I'm wondering about the precision of PR_Now() and as we clone and pass the cookies across IPC barrier and create new ones on the other side could a creation-time dupe make it's way back in.

Comment 3

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

I'm not too worried about duplicate timestamps. They used to be more of a problem back when the creationTime was being used as the row id in the DB (see bug 591447).
Now I'm not sure if gLastCreationTime really needs to be incremented when a collision happens.

Comment 4

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

Looking at the probes from

(In reply to Valentin Gosu [:valentin] (he/him) from bug 1828126 comment #14)

https://glam.telemetry.mozilla.org/fog/probe/networking_cookie_timestamp_fixed_count/explore
https://glam.telemetry.mozilla.org/fog/probe/networking_cookie_creation_fixup_diff/explore
https://glam.telemetry.mozilla.org/fog/probe/networking_cookie_access_fixup_diff/explore

It seems we still have a problem with users having invalid timestamps in the future.
In release, around 1.5 million clients still record the fixup probe, indicating they have a wrong timestamp or a wrong computer time.
It is unclear if the wrong timestamp is fallout from bug 1827669, or something else is causing it.
In any case, I think removing the gLastCreationTime mechanism might improve things.