Firefox 110 Forget the last x time deletes all cookies
Categories
(Toolkit :: Data Sanitization, defect)
Tracking
()
People
(Reporter: Jatrondi, Assigned: h.sofie.p)
References
(Regression)
Details
(Keywords: dataloss, regression)
Attachments
(1 obsolete file)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/110.0
Steps to reproduce:
Use the "Forget the last 5 minutes" button for quick forget, or Forget the last two hours or Forget the last 24 hours.
Actual results:
All the cookies are deleted from the browser. Not just the cookies from the past 5 minutes, 2 hours, or 24 hours
Expected results:
Only deletes cookies from the selected time range; five minutes, two hours or 24 hours.
Additional notes:
The bug doesn't happen on my other machine running Firefox 109.1 stable/release build. If I switch the channel on the machine that's running 109.1 to the beta channel and update to Firefox 110 (post beta 9 - pre-release), the issue appears. If I revert back to Firefox 109.1, the issue is gone.
There's no "Cookie and site Data" exception set on the machine running Firefox 109.1 that doesn't have this issue.
Comment 1•2 years ago
|
||
The Bugbug bot thinks this bug should belong to the 'Core::Networking: Cookies' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.
Comment 2•2 years ago
|
||
I was able to reproduce this issue with my nightly 111.0a1.
Comment 3•2 years ago
|
||
I am not sure if Necko has anything to do with this functionality. Moving it to the toolbars team for taking an initial stab.
Comment 4•2 years ago
|
||
Quickly checked the UI part, it seems fine. I see browser/modules/Sanitizer.sys.mjs has been updated a lot recently, so moving there.
Comment 5•2 years ago
|
||
It seems we always pass principalsForShutdownClearing
to the cookies clearData helper, which makes the method ignore the provided range:
if (principalsForShutdownClearing) {
await maybeSanitizeSessionPrincipals(
progress,
principalsForShutdownClearing,
Ci.nsIClearDataService.CLEAR_COOKIES
);
} else {
// Not on shutdown
await clearData(range, Ci.nsIClearDataService.CLEAR_COOKIES);
}
hpeuckmann: Do you think it could be related to Bug 1578273 ?
Comment 6•2 years ago
|
||
This is pretty bad as dataloss bugs go. I wouldn't mark S1 given comparatively few people use this button, and there's a work around (the other data clearing mechanisms) but the fact that stuff just goes missing unexpectedly is... bad.
I ran mozregression and got this nightly window: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=91a9bbbe6bea41de5be2721f17cca52ba7986c8e&tochange=dd4482632694e2bc4e64e4651210e1c1815285a2
Off-hand from that, bug 1578273 seems like the regressor, although Hannah or Paul, if you think I'm wrong you may wish to mozregression it further than that window.
Comment 7•2 years ago
|
||
Looking into it, also looping in Hannah when she's around.
Automated test coverage seems sparse 😕
Updated•2 years ago
|
Comment 8•2 years ago
|
||
The test for time range clearing does not cover cookies, there is even a comment here: https://searchfox.org/mozilla-central/rev/5ccb73c0217d1710b10d6e6e297cf3396d10ec23/browser/base/content/test/sanitize/browser_sanitize-timespans.js#64-65
Comment 9•2 years ago
|
||
Ok I've explored a bunch of fixes, Hannah wants to take over the bug. So I'm leaving my drafts / findings here:
For a quick fix we can probably do something like: https://gist.github.com/Trikolon/bf998eabc5efe8ded7d69e3d9ef266f8
I have not tested this in depth so it might not fully work.
For a proper fix we should clean up the Sanitizer logic:
- We shouldn't use an options object like that which makes it really hard to see what's going on from just looking at the code.
- Also we should not use
principalsForShutdownClearing
as a signal for cleaners to ignore the range argument, that's very brittle. - Lastly we should improve test coverage. To extend the test mentioned in comment 8 we need a nsICookieManager method that allows tests to set custom creation time. I have a draft here for adding a separate test method that allows overriding creation time: https://gist.github.com/Trikolon/9f216496e1d6a3ac9a5cf3d6c80b0267 Note that this is also not tested yet, we should make sure it doesn't break anything in the cookie service.
Seems worth uplifting to at least Fx111.
Comment 10•2 years ago
|
||
This affects the "clear recent history" feature too, not just the "forget" toolbar button.
Assignee | ||
Comment 11•2 years ago
|
||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
Comment 12•2 years ago
|
||
Update: We're currently looking if we can cleanly backout https://hg.mozilla.org/mozilla-central/rev/733f6ba8c1c6 and https://hg.mozilla.org/mozilla-central/rev/a34115b334e2 . That seems like the lowest risk option and could be done for Fx111 and potentially as a ride-along for a dot release for 110.
Comment 13•2 years ago
|
||
[Tracking Requested - why for this release]:
Tracking has already been request for Fx110. If possible we would like to get the patches listed in comment 12 backed out for Fx110, Fx111 and Fx112. If Fx110 does not work please consider including it in the next dot release. Thank you!
Here is a try run with the two patches linked in comment 12 backed out, nothing stands out: https://treeherder.mozilla.org/jobs?repo=try&revision=0276d84e9a7144746dee08cd894591ca1dc42a07
Updated•2 years ago
|
Comment 14•2 years ago
|
||
I just experienced this myself. I had done "2 hours" though and it cleared the whole session storage.
Comment 15•2 years ago
|
||
Central backout: https://bugzilla.mozilla.org/show_bug.cgi?id=1578273#c19
Comment 17•2 years ago
|
||
Setting 111 to fixed.
Bug 1578273 has been backed out of beta
https://bugzilla.mozilla.org/show_bug.cgi?id=1578273#c22
Comment hidden (advocacy) |
Comment 19•2 years ago
|
||
I'll mark this as fixed by the backout of Bug 1578273. Note that Fx110 is still affected.
Comment 21•2 years ago
•
|
||
backout bugherder uplift |
Backed out 2 changesets (bug 1578273, bug 1806620) for causing Bug 1816279 a=pascalc
https://hg.mozilla.org/releases/mozilla-release/rev/7eab05d73968
Comment 22•2 years ago
|
||
Sorry, shouldn't this bug specifically be resolved/fixed because of the backout?
Comment 23•2 years ago
|
||
BugHerder reopened it automatically.
Updated•2 years ago
|
Description
•