1816279 - Firefox 110 Forget the last x time deletes all cookies

Status: RESOLVED FIXED

(Toolkit :: Data Sanitization, defect)

Description

[Tascorla]

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

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

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

[Sunil Mayya]

I was able to reproduce this issue with my nightly 111.0a1.

Comment 3

[Sunil Mayya]

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

[Julian Descottes [:jdescottes]]

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

[Julian Descottes [:jdescottes]]

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

[:Gijs (he/him)]

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

[Paul Zühlcke [:pbz]]

Looking into it, also looping in Hannah when she's around.
Automated test coverage seems sparse 😕

Comment 8

[Paul Zühlcke [:pbz]]

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

[Paul Zühlcke [:pbz]]

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

[Paul Zühlcke [:pbz]]

This affects the "clear recent history" feature too, not just the "forget" toolbar button.

Comment 11

[Hannah Peuckmann]

Attachment: WIP: Bug 1816279 - Only gather principals if we clear on shutdown. r=pbz!

Comment 12

[Paul Zühlcke [:pbz]]

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

[Paul Zühlcke [:pbz]]

[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

Comment 14

[bsadowski1]

I just experienced this myself. I had done "2 hours" though and it cleared the whole session storage.

Comment 15

[Paul Zühlcke [:pbz]]

Central backout: https://bugzilla.mozilla.org/show_bug.cgi?id=1578273#c19

Comment 17

[Donal Meehan [:dmeehan]]

Setting 111 to fixed.
Bug 1578273 has been backed out of beta
https://bugzilla.mozilla.org/show_bug.cgi?id=1578273#c22

Comment 18

[zdzichs]

IMHO this is huge issue, should NEVER happen. This could lead to severe problems for non-experienced users and cause enormous amount of extra work for administrators. I'm shocked how thoughtlessly you guys deal with the problem, which is not the error itself, but complete lack of testing - this is impossible to miss such an issue. Maybe Mozilla should consider less updates, less version juggle, but more thoughtful and organized testing? I've been using Mozilla from the very beginning and now seriosly considering change - not surprised the loss of market share with this ridiculous lack of quality!

Comment 19

[Paul Zühlcke [:pbz]]

I'll mark this as fixed by the backout of Bug 1578273. Note that Fx110 is still affected.

Comment 21

[Pascal Chevrel:pascalc (PTO until April 26)]

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

[Paul Zühlcke [:pbz]]

Sorry, shouldn't this bug specifically be resolved/fixed because of the backout?

Comment 23

[Pascal Chevrel:pascalc (PTO until April 26)]

BugHerder reopened it automatically.