Site Data is not deleted after closing Firefox

VERIFIED FIXED in Firefox 65

Status

()

defect
P1
normal
VERIFIED FIXED
10 months ago
6 months ago

People

(Reporter: l33t_h4xx0r, Assigned: baku)

Tracking

(Blocks 1 bug)

61 Branch
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 wontfix, firefox62 wontfix, firefox63- wontfix, firefox64 wontfix, firefox65 verified)

Details

Attachments

(7 attachments, 1 obsolete attachment)

Reporter

Description

10 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180704003137

Steps to reproduce:

I set the option to accept cookies and site data from website and to keep until Firefox is closed.


Actual results:

I closed Firefox several times, but the site data was not deleted.
When i click "Manage Data..." there are websites listed, which i browsed 2 months ago.


Expected results:

Clear / Deleting all site data after I close Firefox
Reporter

Comment 1

10 months ago

Comment 2

10 months ago
I tried to reproduce this issue with a fresh profile on the latest Firefox Nightly, Firefox 62.0b16 and on Firefox 61.0.2 on Windows 10 x64.
I changed the History, cookies and site data settings as in the screenshot attached, then I accessed up to 10 websites, I closed Firefox and then opened it again, in "Manage cookies and Site Data" is still displaying one website from the 10 websites accessed.
Status: UNCONFIRMED → NEW
Component: Untriaged → Preferences
Ever confirmed: true
Yikes. I can kinda reproduce this.

Interestingly, configured this way, after several restarts, eventually the site data is cleared. The cookies stick around though.

I'm setting this as a P1, since we seem to be pretty broken here.

Looking at the Preferences code, we're correctly setting network.cookie.lifetimePolicy to 2, which should be clearing the cookies.

Hey johannh, do you happen to know what's going on here, or who I should ask?
Flags: needinfo?(jhofmann)
Priority: -- → P1
This sounds like bug 1400678 and should have been fixed in Firefox 62. I can reproduce it with cookies, though.

Baku, do you know off-hand whether we're retroactively clearing cookies the way that we did with quota storage in bug 1400678 when the global default changes to ACCEPT_SESSION? I had always just assumed we did, but now I'm not sure and I can't find the code that would do something like this.

Do we need to extend your solution to cookies, or am I just missing something?

<insert needinfo for baku when he comes back>
Component: Preferences → Data Sanitization
Flags: needinfo?(jhofmann)
Product: Firefox → Toolkit

Updated

8 months ago
Flags: needinfo?(amarchesini)
Assignee

Comment 5

8 months ago
Posted patch cleanup.patch (obsolete) — Splinter Review
We don't. We should check if the current network.cookie.lifetimePolicy is set to 2 in sanitizer.jsm and at shutdown, delete all the cookies. Wondering if we should extend this to something more than cookies.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #9017878 - Flags: review?(jhofmann)
Comment on attachment 9017878 [details] [diff] [review]
cleanup.patch

Review of attachment 9017878 [details] [diff] [review]:
-----------------------------------------------------------------

I would have put this in sanitizeSessionPrincipals and avoid the duplicate if condition but I'd leave that up to you :)

Thanks
Attachment #9017878 - Flags: review?(jhofmann) → review+
(And thanks philipp for the needinfo, I had totally forgotten about this bug again, I should needinfo myself in these cases in the future)

Comment 8

8 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c633215c514
Cleanup old cookies at shutdown if network.cookie.lifetimePolicy is ACCEPT_SESSION, r=johannh
Assignee

Comment 9

8 months ago
Do we want to uplift this? Would be nice to have this code in beta. In the past, we had negative reviews and articles because firefox was not cleaning all the cookies as expected.
Flags: needinfo?(jhofmann)
[Tracking Requested - why for this release]:

Hm, that would be a release uplift at this point. I guess it works as a ride-along, and this code is really harmless.

Pascal, what are our options if we're interested in getting this to release?

Thanks!
Flags: needinfo?(jhofmann) → needinfo?(pascalc)
I don't think it qualifies neither as an uplift for 63 or a ride-along for a 63 dot release since this is not a regression but a bug we have in Firefox since at least 61 and the fix isn't even in Nightly yet. I'd just let it ride the trains to 64. Thanks
Flags: needinfo?(pascalc)
Alright, fair enough, thanks!

Comment 13

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2c633215c514
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64

Updated

8 months ago
Depends on: 1500505
Given the timing of 64 going to the Beta channel next week and the incoming SUMO reports we're already seeing for bug 1500505, I'm backing this out next week so we don't ship that issue to the Beta population. We can consider uplifting this back during the 64 Beta cycle after it's had some time on Nightly for any remaining issues to shake out.

https://hg.mozilla.org/mozilla-central/rev/4888629028695c78b394cef273c00e70ae4b87df
Status: RESOLVED → REOPENED
Flags: needinfo?(amarchesini)
Resolution: FIXED → ---
Target Milestone: mozilla64 → ---
Assignee

Comment 15

8 months ago
Posted patch cleanup.patchSplinter Review
Attachment #9017878 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #9018796 - Flags: review?(jhofmann)
Comment on attachment 9018796 [details] [diff] [review]
cleanup.patch

Review of attachment 9018796 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

It would be great to have a test for this. While we can't test shutting down the browser, we could put expose this function to our test code and call it directly.

That's something we missed to do in the original implementation, so I won't block review on it :)

::: browser/modules/Sanitizer.jsm
@@ +742,5 @@
> +  let enumerator = Services.cookies.enumerator;
> +  let hosts = new Set();
> +  for (let cookie of enumerator) {
> +    let host = cookie.host + ChromeUtils.originAttributesToSuffix(cookie.originAttributes);
> +    if (hosts.has(host)) {

You don't need to do this. It's a Set :)
Attachment #9018796 - Flags: review?(jhofmann) → review+
(In reply to Johann Hofmann [:johannh] from comment #16)
> It would be great to have a test for this. While we can't test shutting down
> the browser

Sorry for jumping in here - we can actually test shutting down the browser, but it involves writing a firefox-ui test.

We've got a (very) limited set of SessionStore firefox-ui tests, for example:

https://searchfox.org/mozilla-central/rev/a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py

It might very well be worth writing a test for this sort of thing using the firefox-ui framework.
(In reply to Mike Conley (:mconley) (:⚙️) from comment #17)
> (In reply to Johann Hofmann [:johannh] from comment #16)
> > It would be great to have a test for this. While we can't test shutting down
> > the browser
> 
> Sorry for jumping in here - we can actually test shutting down the browser,
> but it involves writing a firefox-ui test.
> 
> We've got a (very) limited set of SessionStore firefox-ui tests, for example:
> 
> https://searchfox.org/mozilla-central/rev/
> a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/testing/firefox-ui/tests/functional/
> sessionstore/test_restore_windows_after_restart_and_quit.py
> 
> It might very well be worth writing a test for this sort of thing using the
> firefox-ui framework.

Thanks for the suggestion! My concern here is that this is really not a UI feature. So we would only use firefox-ui to be able to restart, and the rest of the test (setting cookies, asserting that cookies are still around) would need to be done with chrome JS (I suppose firefox-ui tests support injecting chrome JS?).
(In reply to Johann Hofmann [:johannh] from comment #18)
> Thanks for the suggestion! My concern here is that this is really not a UI
> feature. So we would only use firefox-ui to be able to restart, and the rest
> of the test (setting cookies, asserting that cookies are still around) would
> need to be done with chrome JS (I suppose firefox-ui tests support injecting
> chrome JS?).

Yep. I think "firefox-ui" is really a misnomer here - I don't think it means we should be only testing UI bits and pieces. We should, however, be taking 100% advantage of the fact that it allows us to test shutdowns / restarts.

Injecting chrome JS? Yep: https://searchfox.org/mozilla-central/rev/a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/testing/firefox-ui/tests/puppeteer/test_places.py#27-44
Assignee

Comment 20

8 months ago
Do you mind if I work on a test as follow up? I'm not familiar with firefox-ui and it could take a while to write a test correctly.
Flags: needinfo?(mconley)
Yeah, totally - I'm definitely not suggesting we block this fix on a firefox-ui test. But let's file a follow-up bug for it. Thanks!
Flags: needinfo?(mconley)

Comment 22

8 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c0adad8a50
Cleanup old cookies at shutdown if network.cookie.lifetimePolicy is ACCEPT_SESSION, r=johannh
Assignee

Comment 23

8 months ago
Comment on attachment 9018796 [details] [diff] [review]
cleanup.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1422365

User impact if declined: Old cookies are not deleted after the changes of network.cookie.lifetimePolicy

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See the description of the bug

List of other uplifts needed: none

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The patch follows the current implementation for QuotaManager

String changes made/needed: none
Attachment #9018796 - Flags: approval-mozilla-beta?
Assignee

Updated

8 months ago
Blocks: 1501716

Comment 24

8 months ago
A quick local test applying the new patch on inbound seems to verify the change does not cause dataloss regression bug 1500505.

Comment 25

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/14c0adad8a50
Status: REOPENED → RESOLVED
Closed: 8 months ago8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
I'd like to see this get some QA testing and Nightly bake time before taking it on Beta.
Flags: qe-verify+

Comment 27

8 months ago
users are reporting the same problem [1] as in bug 1500505 in today's nightly again unfortunately. i can reproduce it by setting "Accept cookies and site data" and Keep until "Nightly is closed", while configuring some exceptions for domains i want to stay logged-in. after a restart those cookies are deleted.

[1] https://www.camp-firefox.de/forum/viewtopic.php?p=1095870#p1095870
Flags: needinfo?(amarchesini)
Assignee

Comment 28

8 months ago
With tests.
Flags: needinfo?(amarchesini)
Attachment #9019976 - Flags: review?(jhofmann)
Assignee

Updated

8 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted image Data entries.png
I reproduced the issue on Firefox Beta 62.0b16 under Windows 10 (x64), Ubuntu 16.04 (x64) and macOS 10.12 using the STR from Comment 0.

On the latest Nightly 65.0a1 (2018-10-24), while having some browsing done, and by setting Keep until "Nightly is closed", after a restart, the cookies are deleted but the entries for Facebook, Twitter and Youtube are still there, and only after one more restart, the list will be empty (I'll attach an image with the entries). Is this an expected behavior?
Target Milestone: mozilla65 → ---
Flags: needinfo?(amarchesini)
Assignee

Comment 30

8 months ago
> an image with the entries). Is this an expected behavior?

Probably that is because QuotaManager haven't deleted the site directories yet. asuth, what do you think about comment 29?
Flags: needinfo?(amarchesini) → needinfo?(bugmail)
It's probably the shutdown concern I had in https://bugzilla.mozilla.org/show_bug.cgi?id=1400678#c20
Flags: needinfo?(bugmail)
I just lost all my cookies again.
(With 2FA and work+personal accounts for multiple services, signing back in to everything ends up taking quite a while..)
(In reply to Ed Morley [:emorley] from comment #32)
> I just lost all my cookies again.
> (With 2FA and work+personal accounts for multiple services, signing back in
> to everything ends up taking quite a while..)

Is it possible you're experiencing bug 1237527 or something like it?
Thank you for the reply. That bug seems different, since in my case Firefox did not crash (I clicking the "restart to update") and the disk is not full.

Browsing more since comment 32, it seems that only a proportion of cookies have been deleted (perhaps 50%?).
Comment on attachment 9019976 [details] [diff] [review]
cleanup2.patch

Review of attachment 9019976 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #9019976 - Flags: review?(jhofmann) → review+

Comment 36

8 months ago
Ah, on my main profile, I upgraded from 20181024133237 (excludes comment 25) to 20181025100246 (includes) to 20181025220044 and ended up logged out of gmail but still logged in to bugzilla.

I then upgraded to 20181026220839 and was still logged in to bugzilla. But then after a plain quit then restart, I lost my bugzilla cookies.

Comment 37

8 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf243201204
Cleanup cookie at shutdown must support domain cookies, r=johannh

Comment 38

8 months ago
Bug 1500505 has come back for me too (see Comment 27).  My "Exceptions" list is ignored and all cookies are removed on exit. Settings are "Accept cookies and site data" with "Keep until Nightly is closed".

(First ever post here - please let me know if I've missed any important info in my comment.)

Comment 39

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1cf243201204
Status: REOPENED → RESOLVED
Closed: 8 months ago8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Can anybody confirm whether 2018-10-28 and newer Nightlies have resolved the remaining issues now that the follow-up patch has landed?
Other than the shutdown concern mentioned by Andrew in Comment 31 (see attachment from Comment 29), the issue is verified fixed on latest Nightly 65.0a1 (2018-10-29). Tests were performed under Windows 10 (x64), Ubuntu 16.04 (x64) and macOS 10.12.
Status: RESOLVED → VERIFIED

Comment 42

8 months ago
It's still ignoring my Exceptions list and removing every cookie on shutdown when set to "Keep until Nightly is closed".

Version 65.0a1 (2018-10-29)

Comment 43

8 months ago
20181029100347 is still erasing my gmail (accounts.google.com) and bugzilla cookies. I just tested 20181017101626 without the original change, and those cookies do remain after restart.

I did test deleting the exception and re-adding for bugzilla, and the cookie data does seem to stay with 20181029100347. Maybe the data format is slightly different from old cookie exceptions vs new ones?
I did try with the Google account (gmail inbox website added on exceptions list, more exactly) and the cookies are still in the "Manage Data" section, even after some restarts. I tried on 65.0a1 (2018-10-29).

Comment 45

8 months ago
I am still able to reproduce the issue on *some* sites (www.camp-firefox.de/forum, mozilla.slack.com) - also in a new profile.

those cookies get deleted when firefox is set to keep cookies till the end of the session, despite these domains being in the exceptions list to allow permanently storing cookies (i've set those exceptions through a right-click on a page > view page infos > permissions).
i might still stay logged-into the sites through sessionrestore data but when i inspect the cookies.sqlite db upon exit i see that the cookie data got wiped.
After adding some more websites on the exceptions, and restarting, all the cookies are cleared out, even if they were not cleaned on the first 3 or 4 attempts.
Assignee

Comment 47

8 months ago
> i might still stay logged-into the sites through sessionrestore data but
> when i inspect the cookies.sqlite db upon exit i see that the cookie data
> got wiped.

Can you tell me the origins of the cookies you have in the cookie.sqlite before the restarting?
What I suspect is that, some websites set cookies with different origins (mozilla.slack.com maybe set 'slack.com' cookies). But the permission is not propagated to those origins and, we delete their cookies, during the shutdown.
Assignee

Updated

8 months ago
Flags: needinfo?(madperson)

Comment 48

8 months ago
Posted image mozilla.slack.com
this is the content in cookies.sqlite that's set by https://mozilla.slack.com before it's deleted at the end of the session

Comment 49

8 months ago
Posted image www.camp-firefox.de
same data for www.camp-firefox.de
Flags: needinfo?(madperson)
Assignee

Comment 50

8 months ago
can you tell me also if the permission is set to www.camp-firefox.de or camp-firefox.de? Thanks
Flags: needinfo?(madperson)

Comment 51

8 months ago
i've set the permission through the permission UI in the page info panel. in the about:preferences#privacy cookie exception list that entry is showing up as "https://www.camp-firefox.de".
Flags: needinfo?(madperson)

Comment 52

8 months ago
I believe the issue is that I have an Allow Exception for mail.google.com and accounts.google.com but *not* for plain google.com, so cookies are cleared for "https://google.com" principal resulting in everything under google.com getting cleared.

Comment 53

8 months ago
Just tested on a new profile logging in to "bugzilla.mozilla.org" with an Allow Exception for "https://bugzilla.mozilla.org" results in the cookies being cleared on quit as there's some other "mozilla.org" cookies being set and cleared on quit. However, if I manually delete the "mozilla.org" cookies leaving "bugzilla.mozilla.org" cookies, quitting correctly keeps bugzilla cookies.

Adding an Allow Exception for "mozilla.org" does seem to avoid the disappearing bugzilla cookies, but I don't necessarily want to allow saving cookies for the base domain.

Comment 54

8 months ago
Looks like a similar thing is happening for camp-firefox.de where the forum cookies are set with origin "https://www.camp-firefox.de" and some other session cookies are set on origin "https://camp-firefox.de" but the Allow Exception is for "https://www.camp-firefox.de" resulting in the non-www cookies causing everything under camp-firefox.de to get deleted.

Comment 55

8 months ago
I suppose now that these changes have landed early in Nightly 65, should fixing up other regressions be tracked in separate followup bugs?
Assignee

Updated

8 months ago
Blocks: 1503217
Assignee

Comment 56

8 months ago
I file a follow up to fix this regression. Thanks for the patience.
(In reply to Andrea Marchesini [:baku] from comment #23)
> Risk to taking this patch: Low

That assessment seems overly optimistic.  What's the reason we shouldn't let this ride the trains to 65, considering it's been around for a few releases already?
Flags: needinfo?(amarchesini)
Assignee

Comment 58

7 months ago
I think we need to fix this clear site-data as soon as possible because it seems people care a lot. It's not a common bug or a broken feature: people are very sensible on privacy topics and, in the past, we had strong articles about Firefox not cleaning correctly site-data.
Flags: needinfo?(amarchesini)
Comment on attachment 9018796 [details] [diff] [review]
cleanup.patch

It still seems to me like this is an area where we should be careful not to introduce regressions, and rushing a fix for a longstanding issue is not necessary.
Attachment #9018796 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
I tried again reproducing the issue on latest Firefox Beta 65.0b4, and with the "Delete cookies and site data when Firefox is closed" option selected, the cookies are erased after a restart.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.