Closed Bug 1816279 Opened 2 years ago Closed 1 year ago

Firefox 110 Forget the last x time deletes all cookies

Categories

(Toolkit :: Data Sanitization, defect)

Firefox 110
defect

Tracking

()

RESOLVED FIXED
112 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox110 + fixed
firefox111 + fixed
firefox112 + fixed

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.

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.

Component: Untriaged → Networking: Cookies
Product: Firefox → Core

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

I am not sure if Necko has anything to do with this functionality. Moving it to the toolbars team for taking an initial stab.

Status: UNCONFIRMED → NEW
Component: Networking: Cookies → Toolbars and Customization
Ever confirmed: true
Product: Core → Firefox

Quickly checked the UI part, it seems fine. I see browser/modules/Sanitizer.sys.mjs has been updated a lot recently, so moving there.

Component: Toolbars and Customization → Data Sanitization
Product: Firefox → Toolkit

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 ?

Flags: needinfo?(hpeuckmann)
See Also: → 1578273

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.

Severity: -- → S2
Keywords: dataloss, regression
Regressed by: 1578273
See Also: 1578273

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

Assignee: nobody → pbz
Status: NEW → ASSIGNED

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.

Assignee: pbz → hpeuckmann

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

Flags: needinfo?(hpeuckmann)
Attachment #9317622 - Attachment description: WIP: Bug 1816279 - Only gather prinicpals if we clear on shutdown. r=pbz! → WIP: Bug 1816279 - Only gather principals if we clear on shutdown. r=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.

[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

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

Duplicate of this bug: 1815554

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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 112 Branch
Duplicate of this bug: 1817663
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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

Flags: needinfo?(pascalc)

BugHerder reopened it automatically.

Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Flags: needinfo?(pascalc)
Resolution: --- → FIXED
Attachment #9317622 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: