Closed Bug 1483440 Opened 2 years ago Closed 2 years ago

Removing site data and cookies should not affect session store


(Toolkit :: Data Sanitization, defect, P1)

60 Branch



Tracking Status
firefox66 --- fixed


(Reporter: kidhanis, Assigned: johannh)




(4 files)

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

Steps to reproduce (I'll use 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 on tab 1, which creates an entry for 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 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 got deleted from the 'Recently Closed Tabs' list.

Expected results:

about:blank is restored to the left of the remaining tab.
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:

As far as the order of the tabs after deletion is concerned, that's a P5 at best, in my opinion.
Flags: needinfo?(jhofmann) → needinfo?(mak77)
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 ( 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...
Flags: needinfo?(mak77)
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):

Looking at the code that receives this, it should have the same effect:
Assignee: nobody → jhofmann
Ever confirmed: true
Priority: -- → P1
Summary: Position of recently closed tabs is not updated after removing site data → Removing site data and cookies should not affect session store
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.
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).
Pushed by
Part 1 - Remove purge-domain-data, add purge-sessionStorage notification. r=baku
Part 2 - Don't use purge-domain-data to clear session history for a domain. r=mikedeboer,baku
Part 3 - Use purge-localStorage instead of purge-domain-data to clean up localStorage. r=baku
Part 4 - Don't send purge-domain-data when cleaning up site preferences. r=baku

Did you mean for the notification at 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.

Flags: needinfo?(jhofmann)

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

Did you mean for the notification at 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.


Flags: needinfo?(jhofmann)
You need to log in before you can comment on or make changes to this bug.