1483440 - Removing site data and cookies should not affect session store

Status: RESOLVED FIXED

(Toolkit :: Data Sanitization, defect, P1)

Description

[Kidhanis]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180814074659

Steps to reproduce:

Tested on Ubuntu Nightly and Beta. The output of mozregression is https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dc9ba6649af7290170dc76be598ea669a169c6a2&tochange=3200dd2ad6b0dffccdc9274b02d8632920edc9ef

Steps to reproduce (I'll use mozilla.org for the website that will create site data to later delete.  I'll then use about:blank in the tab that will be restored, but any other website that does not conflict with the site data of the first one should work):

1. Open a total of three tabs. Open mozilla.org on tab 1, which creates an entry for mozilla.org under Cookies and Site Data.  Then open about:blank on tab 2 and about:preferences on tab 3.
2. Close tab 2, then close tab 1.  The only remaining tab is about:preferences.
3. Go to Cookies and Site Data in about:preferences#privacy on the remaining tab, click on 'Manage Data', select the mozilla.org entry, and click on 'Remove Selected' and 'Save Changes'.
4. Restore recently closed tab (Ctrl + Shift + T). 


Actual results:

about:blank is restored to its original position as tab 2, which now is to the right of the remaining tab. It seems like the position of the closed about:blank tab was not updated after mozilla.org got deleted from the 'Recently Closed Tabs' list.


Expected results:

about:blank is restored to the left of the remaining tab.

Comment 1

[Johann Hofmann [:johannh]]

What I don't understand about this is, why/how are we clearing this from Recently Closed Tabs anyway?

Mak, do you know anything about Recently Closed Tabs and what part of this code removes the tab of the deleted domain: https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/browser/modules/SiteDataManager.jsm#429?

As far as the order of the tabs after deletion is concerned, that's a P5 at best, in my opinion.

Comment 2

[Marco Bonardo [:mak]]

Are you sure the steps provided call into removeSiteData? the only call to removeSiteData I can find is from gClearSiteDataDialog::onClear, that is in the dialog opened by the Clear Site Data button.
Here they used the Manage Data instead, that I think is managed by siteDataSettings.js? If so when he clicks on Save Changes, it invokes the saveChanges() method in siteDataSettings.js, that calls into SiteDataManager.remove().
Why does that matter? Well, SiteDataManager.remove() notifies "browser:purge-domain-data" that is observed by SessionStore.jsm (https://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#790) that invokes onPurgeDomainData that cleans up closed and open tabs.

Note I'm not an expert of all of this code and indirection, I'm not sure why this apparent duplication, nor if the other method properly notifies and clears this data as well...

Comment 3

[Johann Hofmann [:johannh]]

Oh, of course, you are right, I misread my own code, duh.

I seem to have misunderstood the meaning/effect of purge-domain-data, interpreting it as only clearing localStorage and sessionStorage. It's also not very practicable to have this as an observer notification which everyone can just hook into.

It's probably sensible to use purge-localStorage, then, also in ClearDataService (we will switch to using ClearDataService in bug 1460768):

https://searchfox.org/mozilla-central/search?q=Services.obs.notifyObservers%28null%2C+%22browser%3Apurge-domain-data%22&path=

Looking at the code that receives this, it should have the same effect:

https://searchfox.org/mozilla-central/rev/99cbc0aec3e1c0b65ff9052523fb5c181b248f57/dom/storage/StorageObserver.cpp#313
https://searchfox.org/mozilla-central/rev/99cbc0aec3e1c0b65ff9052523fb5c181b248f57/dom/storage/LocalStorageManager.cpp#415

Comment 4

[Johann Hofmann [:johannh]]

Attachment: Bug 1483440 - Part 1 - Remove purge-domain-data, add purge-sessionStorage notification. r=baku

The browser:purge-domain-data notification had two different intentions,
- to clear session store information and
- to clear all localStorage and sessionStorage
Firing the notification accomplished both at the same time, which from a
user perspective is of course totally not understandable. This commit
removes purge-domain-data in favor of the two distinct purge-localStorage and
purge-sessionStorage events. This gives callers more granular choice on
what they want to clear.

Comment 5

[Johann Hofmann [:johannh]]

Attachment: Bug 1483440 - Part 2 - Don't use purge-domain-data to clear session history for a domain. r=baku,mikedeboer

Comment 6

[Johann Hofmann [:johannh]]

Attachment: Bug 1483440 - Part 3 - Use purge-localStorage instead of purge-domain-data to clean up localStorage. r=baku

Comment 7

[Johann Hofmann [:johannh]]

Attachment: Bug 1483440 - Part 4 - Don't send purge-domain-data when cleaning up site preferences. r=baku

As far as I can tell this was only ever done to give legacy add-ons the chance to
clean up user data and isn't needed anymore (and had the strange side effect that e.g.
localStorage was cleared when removing the site zoom settings).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f9d7c3c98fa
Part 1 - Remove purge-domain-data, add purge-sessionStorage notification. r=baku
https://hg.mozilla.org/integration/autoland/rev/170f979a5f7d
Part 2 - Don't use purge-domain-data to clear session history for a domain. r=mikedeboer,baku
https://hg.mozilla.org/integration/autoland/rev/4b57fb875b42
Part 3 - Use purge-localStorage instead of purge-domain-data to clean up localStorage. r=baku
https://hg.mozilla.org/integration/autoland/rev/cb1a55c678c1
Part 4 - Don't send purge-domain-data when cleaning up site preferences. r=baku

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6f9d7c3c98fa
https://hg.mozilla.org/mozilla-central/rev/170f979a5f7d
https://hg.mozilla.org/mozilla-central/rev/4b57fb875b42
https://hg.mozilla.org/mozilla-central/rev/cb1a55c678c1

Comment 10

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

Did you mean for the notification at https://searchfox.org/mozilla-central/rev/e00ea598e52bbb35f8c45abf9c2eade17962bb5e/browser/modules/SiteDataManager.jsm#347 to notify "browser:purge-sessionStorage" instead of "extension:purge-sessionStorage"? There's no other instances of the latter in the codebase, and it seems likely to be a typo due to the emergent naming inconsistency.

Comment 11

[Johann Hofmann [:johannh]]

(In reply to Andrew Sutherland [:asuth] from comment #10)

Did you mean for the notification at https://searchfox.org/mozilla-central/rev/e00ea598e52bbb35f8c45abf9c2eade17962bb5e/browser/modules/SiteDataManager.jsm#347 to notify "browser:purge-sessionStorage" instead of "extension:purge-sessionStorage"? There's no other instances of the latter in the codebase, and it seems likely to be a typo due to the emergent naming inconsistency.

Ah, yes, I did, thanks for noticing, this will get fixed with bug 1523272, but it's probably a good idea to uplift a fix for this anyway. I'll file a bug.

Yeah these names are a little inconsistent, IMO we might want to rename extension:purge-localStorage, though.

Thanks!