1482055 - Site Data is not deleted after closing Firefox

Status: VERIFIED FIXED

(Toolkit :: Data Sanitization, defect, P1)

Description

[l33t_h4xx0r]

Attachment: site-data-not-purged.png

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

Comment 1

[l33t_h4xx0r]

Attachment: site-data-settings.png

Comment 2

[Hani Yacoub, Desktop QA]

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.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 4

[Johann Hofmann [:johannh]]

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>

Comment 5

[Andrea Marchesini [:baku]]

Attachment: cleanup.patch

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.

Comment 6

[Johann Hofmann [:johannh]]

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

Comment 7

[Johann Hofmann [:johannh]]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c633215c514
Cleanup old cookies at shutdown if network.cookie.lifetimePolicy is ACCEPT_SESSION, r=johannh

Comment 9

[Andrea Marchesini [:baku]]

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.

Comment 10

[Johann Hofmann [:johannh]]

[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!

Comment 11

[Pascal Chevrel: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

Comment 12

[Johann Hofmann [:johannh]]

Alright, fair enough, thanks!

Comment 13

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/2c633215c514

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Andrea Marchesini [:baku]]

Attachment: cleanup.patch

Comment 16

[Johann Hofmann [:johannh]]

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 :)

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 18

[Johann Hofmann [:johannh]]

(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?).

Comment 19

[Mike Conley (:mconley) (:⚙️)]

(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

Comment 20

[Andrea Marchesini [:baku]]

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.

Comment 21

[Mike Conley (: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!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c0adad8a50
Cleanup old cookies at shutdown if network.cookie.lifetimePolicy is ACCEPT_SESSION, r=johannh

Comment 23

[Andrea Marchesini [:baku]]

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

Comment 24

[Ed Lee :Mardak]

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

Comment 25

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/14c0adad8a50

Comment 26

[Ryan VanderMeulen [:RyanVM]]

I'd like to see this get some QA testing and Nightly bake time before taking it on Beta.

Comment 27

[[:philipp]]

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

Comment 28

[Andrea Marchesini [:baku]]

Attachment: cleanup2.patch

With tests.

Comment 29

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Attachment: 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?

Comment 30

[Andrea Marchesini [:baku]]

> 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?

Comment 31

[Andrew Sutherland [:asuth] (he/him)]

It's probably the shutdown concern I had in https://bugzilla.mozilla.org/show_bug.cgi?id=1400678#c20

Comment 32

[Ed Morley [:emorley]]

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..)

Comment 33

[Andrew Sutherland [:asuth] (he/him)]

(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?

Comment 34

[Ed Morley [:emorley]]

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 35

[Johann Hofmann [:johannh]]

Comment on attachment 9019976 [details] [diff] [review]
cleanup2.patch

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

Thanks!

Comment 36

[Ed Lee :Mardak]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf243201204
Cleanup cookie at shutdown must support domain cookies, r=johannh

Comment 38

[Adrian London]

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

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/1cf243201204

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Can anybody confirm whether 2018-10-28 and newer Nightlies have resolved the remaining issues now that the follow-up patch has landed?

Comment 41

[Catalin Sasca, Desktop Test Engineering [:csasca]]

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.

Comment 42

[Adrian London]

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

[Ed Lee :Mardak]

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?

Comment 44

[Catalin Sasca, Desktop Test Engineering [:csasca]]

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

[[:philipp]]

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.

Comment 46

[Catalin Sasca, Desktop Test Engineering [:csasca]]

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.

Comment 47

[Andrea Marchesini [:baku]]

> 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.

Comment 48

[[:philipp]]

Attachment: 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

[[:philipp]]

Attachment: www.camp-firefox.de

same data for www.camp-firefox.de

Comment 50

[Andrea Marchesini [:baku]]

can you tell me also if the permission is set to www.camp-firefox.de or camp-firefox.de? Thanks

Comment 51

[[:philipp]]

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".

Comment 52

[Ed Lee :Mardak]

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

[Ed Lee :Mardak]

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

[Ed Lee :Mardak]

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

[Ed Lee :Mardak]

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

Comment 56

[Andrea Marchesini [:baku]]

I file a follow up to fix this regression. Thanks for the patience.

Comment 57

[Julien Cristau [:jcristau]]

(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?

Comment 58

[Andrea Marchesini [:baku]]

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.

Comment 59

[Julien Cristau [:jcristau]]

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.

Comment 60

[Catalin Sasca, Desktop Test Engineering [:csasca]]

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.