1845870 - Cookie per host quota enforcement is off by one

Status: RESOLVED FIXED

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

Description

[Daniel Veditz [:dveditz]]

In CookieCommons.h we define kCookieQuotaPerHost = 150;, but after a purge we have 151 cookies left. To trigger a purge we have to have 181 cookies, but that appears intended because we define kMaxCookiesPerHost = 180;

STR:

1. pick a domain you don't care about, like https://example.org, and clear all it's cookies
2. in the DevTools console on that site run

for (i = 1; i <= 181; i++)  document.cookie = `c${String(i).padStart(3,0)}=0`;
document.cookie.split(';').length

Expected result: 150
Actual result: 151

Being off by one isn't that big a deal because the quota is more of a suggestion in the spec. But we differ from Chrome and that might have unwanted effects on some site someday.

Comment 1

[Daniel Veditz [:dveditz]]

In CookieStorage.cpp when we go to add our 181st cookie we find that we already have kMaxCookiesPerHost (180) stored
https://searchfox.org/mozilla-central/rev/50588a0b728b365afdd298debd35e8302efe7850/netwerk/cookie/CookieStorage.cpp#535

So we calculate how many cookies we ought to delete and get 180-150 = 30:

     uint32_t limit = mMaxCookiesPerHost - mCookieQuotaPerHost;

So we delete that many and are left with the expected 150 cookies for that host. But remember we were in the middle of adding a cookie when we checked the quotas. So now we add that new cookie and end up with 151.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Should be trivial to fix. @Dylan?

Comment 3

[tannal2409]

Attachment: Bug 1845870 - Cookie per host quota shouldn't be greater than mMaxCookiesPerHost. r?edgul

Comment 4

[tannal2409]

Attachment: Bug 1845870 - Cookie per host quota shouldn't be greater than mMaxCookiesPerHost. r?edgul

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 6

[edgul]

Thanks for your work on this tannel,
Anything I can do to help this along?

Comment 7

[tannal2409]

Hi, I updated the revision, please take a look again when you have a moment.
I finally got that trick test pass.
Thank you for reminding me about this bug.

Comment 8

[tannal2409]

(In reply to edgul from comment #6)

Thanks for your work on this tannel,
Anything I can do to help this along?

I have a few questions:

1. Should I add new tests for these changes?
2. Where should I add them?
3. How should I implement them?

Comment 9

[edgul]

The coverage we have looks sufficient. Just need to make sure all the exiting tests pass in a way that maintains their integrity. I'll review your patch again.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 11

[edgul]

Attachment: Bug 1845870 - Cookie purge down to the purgeQuota number exactly

Comment 12

[edgul]

I should have taken a closer look at the behaviour in question earlier, because I don't think it's as simple as it seems.

For the purposes of discussion let's use this function, which doesn't overwrite any previous cookies by the same name:

let count = 0;
function addUniqueCookies(n) {
    for (let i = 1; i <= n; i++) {
        document.cookie = `c${String(count).padStart(5,0)}=0`;
        count+=1;
    }
}

From cleared cookie state:
addUniqueCookies(150);
document.cookie.split(";").length -> 150

Then let's go to the maxPerHost limit (we see that it's 180 that is the trigger point, not 181. We also see that we purge down to the correct value):
addUniqueCookies(30)
document.cookie.split(";").length -> 150

But when we go over we see the reported behaviour:
addUniqueCookies(31)
document.cookie.split(";").length -> 151

Similarly, from 151:
addUniqueCookies(31)
document.cookie.split(";").length -> 152

---

To understand the reported behaviour we have to understand the way the IPC works.
A single cookie is set on the content process.
That single cookie goes across IPC.
That single cookie triggers a purge, which happens on the same thread.

We are not batching the cookies across the IPC, and if many cookies go out at once, they are processed one at a time due to the threading.

Rob makes a good point (3) in this comment: https://phabricator.services.mozilla.com/D244814#inline-1351022
That if we change the pref on the fly then we have an issue. But we could also look at this as breaking an invariant of browser settings.
This is a different issue (maybe worth discussing though!)

As for the behaviour that rob mentions:

{
for (let i = 1; i <= 205; i++)  document.cookie = `c${String(i).padStart(3,0)}=0`;
console.log(document.cookie.split(';').length)
// ^ 205

setTimeout(() => console.log(document.cookie.split(';').length), 2000);
// ^ 175, instead of 150.
}

and as for the reported behaviour, these are expected. And as written I would say these are a WONTFIX/INVALID. If we want to talk about batching cookies across the IPC, we can but that is sufficiently more complex as it will involve threading consideration on both the content and parent side. I am also purposely omitting discussion on the purge signalling going back to the content process for brevity, but this will need to be considered as well when we start changing how we synchronize cookies across processes.

Dan, Rob, Baku, are we okay to close this or should we think about discussions for a large cookie architecture change?

Comment 13

[Rob Wu [:robwu]]

(In reply to edgul from comment #12)

From cleared cookie state:
addUniqueCookies(150);
document.cookie.split(";").length -> 150

Then let's go to the maxPerHost limit (we see that it's 180 that is the trigger point, not 181. We also see that we purge down to the correct value):
addUniqueCookies(30)
document.cookie.split(";").length -> 150

This is not what I am observing - I am seeing that the cookie count is still 180. Were you testing on your local build with the patch?
Based on the code, this is my understanding of what happens when 181 cookies are created:

1. create 180 cookies - no issues (even the 180th cookie: before the 180th cookie is created, the total number of cookies is 179, so the purge logic does not kick in)
2. create 1 cookie: cookie count before adding a new one is 180 (>= mMaxCookiesPerHost), so the cookie purge logic is triggered, which removes mMaxCookiesPerHost - mCookieQuotaPerHost = 180 - 150 = 30 cookies, leaving 150 cookies. After that, the cookie is added, ending at 151.

This is indeed an off-by-one, as originally reported.

The question is - what is the intention of purging?

• for sure, to ascertain that the mMaxCookiesPerHost is not exceeded. This does indeed happen (except when the pref is flipped, as I wrote in https://phabricator.services.mozilla.com/D244814?id=1012810#inline-1351022).
• is the intent to end at mCookieQuotaPerHost, after purging? If so, the patch is needed.
• is the intent to drop below the maximum, and is ending above the soft quota acceptable? If so, then this bug can indeed be wontfixed.

I have no super strong opinions here, but think that the current implementation has a logical off-by-one, so it is worth trying to fix that.

Comment 14

[edgul]

I am testing on nightly 139.0a1 (2025-04-09) (64-bit).
Though, I see now that on release 137.0.1 (64-bit), the behaviour is as you describe.

Comment 15

[edgul]

Ugh, fresh profile on nightly behaves the same as release. There is something off with my main nightly profile.

Comment 16

[edgul]

Properly clearing my site cookies seems to have resolved my issue (unfortunately, I didn't get a look at why this way happening before). Now my behaviour shows what everyone else is reporting. Sorry for the noise.

To summarize:
a. We definitely have an off-by-one error that I will continue to attempt to address in the WIP patch
b. Because we don't batch send the cookies across IPC the "rollover" of 175 Rob showed (174, once we fix the off-by-one) is expected. Unless we want to revisit the current architecture (?).
c. Rob proposed a change that would protect us from prefs being manipulated on the fly. I think we should handle this in a follow up, but is there any reason we shouldn't do this?

Comment 17

[Daniel Veditz [:dveditz]]

I don't think this warrants a whole rewrite. If we know we're in an addCookie operation and want to purge 30+1 instead of 30 that would be easy. Or not—151 cookies is not a horrible tragedy.

Changing the prefs in the middle of a purge isn't fair, and not very realistic. So we're at a weird number for a bit, but eventually we'll hit the new purge limit and get back on track.

Comment 18

[Rob Wu [:robwu]]

The pref thing is not necessarily in the middle of a purge; e.g. if a site has 150 cookies, and the 150/180 pref values were to be changed to 100/101, then it would take 50 cookie writing attempts to actually get to the preferred quota/maximum sizes, because the number of cookies purged at a time is currently solely the difference between the preference values, and independence of how many cookies are actually over quota.

The fix is straightforward, search for "uint32_t excess" at https://phabricator.services.mozilla.com/D244814?id=1012810#inline-1351022

Comment 19

[Andrea Marchesini [:baku]]

I agree with dveditz. We could also remove that preference and hard-code the value. That preference has never been touched in the last years.

Comment 20

[edgul]

If we ever get around to investigating the cookie jar sizes for Bug 1865714, then we might want to keep these as prefs.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27555fc53447
Cookie purge down to the purgeQuota number exactly r=cookie-reviewers,baku,robwu,emz

Comment 22

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/27555fc53447

Comment 23

[edgul]

Attachment: Bug 1845870 - Disable CHIPS partition limit purging. r?valentin

Comment 24

[Phabricator Automation]

Comment on attachment 9493901 [details]
Bug 1845870 - Disable CHIPS partition limit purging. r?valentin

Revision D253216 was moved to bug 1971020. Setting attachment 9493901 [details] to obsolete.