Last Comment Bug 527667 - DOM Storage (localStorage, sessionStorage) data is not cleared when "Clear Recent History" is used with Time range not "Everything"
: DOM Storage (localStorage, sessionStorage) data is not cleared when "Clear Re...
Status: NEW
[sg:want]
: dev-doc-needed, privacy, sec-want
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 All
: P1 major with 21 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 598479 1125336 (view as bug list)
Depends on: 600366 536544
Blocks: 599724 857888 1102808
  Show dependency treegraph
 
Reported: 2009-11-10 06:16 PST by Nader Moussa
Modified: 2016-04-30 13:49 PDT (History)
39 users (show)
u557809: needinfo? (nwmoussa)
dao+bmo: firefox‑backlog+
mmucci: qe‑verify?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 [Backed out due to oranges] (40.69 KB, patch)
2010-09-28 16:21 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Review
v1 update idiff to fix the orange (1.24 KB, patch)
2010-10-27 07:11 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Review
v1.1 [Backed out comment 16] (32.05 KB, patch)
2010-10-27 11:17 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Review

Description Nader Moussa 2009-11-10 06:16:37 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)

DOM Storage (https://developer.mozilla.org/en/DOM/Storage) allows websites to store cookie-like information in a persistent way.  This data appears to be loaded into the user's profile directory into a file called webappsstore.sqlite

This file can contain private and personally-identifiable data, so it should be cleared along with cookies, browsing history, etc.  However, when I perform the "Clear Recent History" (which is configured to clear everything), the file persists and still contains its original data.  This data should be cleared along with cookies and other persistent-storage.

Reproducible: Always

Steps to Reproduce:
1. Visit a website which uses the DOM Storage feature to store persistent or session data.  (e.g. http://www.cnn.com or many others)
2. Clear private history via the Firefox menu
3. Observe that the DOM Storage file, webappsstore.sqlite in the user profile directory, still contains identifiable information (in both binary and plaintext) that identifies the user's browsing history.
Actual Results:  
The DOM Storage file, webappsstore.sqlite in the user profile directory, still contains identifiable information (in both binary and plaintext) that identifies the user's browsing history.

Expected Results:  
I would expect that DOM Storage is cleared, just like cookies and other data are cleared.

I know DOM Storage is a fairly recent addition, but it is important to address this standard and make sure that it is properly handling data.

Web sites can use DOM Storage for personally-identifiable data - this is the entire reason why we have a "Clear Private Data" option.  This persistent storage file (webappsstore.sqlite) should be cleaned or deleted too.
Comment 1 Nickolay_Ponomarev 2010-01-08 06:55:50 PST
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#133
133     cookies: {
134       clear: function ()
..
139         if (this.range) {
..
150         else {
151           // Remove everything
152           cookieMgr.removeAll();

If the time range is not specified ("Everything"), the cookieMgr.removeAll() call clears the DOM storage data (implemented in bug 352704).

If the time range *is* specified, we try to clear the data by enumerating cookies, and DOM storage items are not part of this enumeration (see also bug 506692).
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-09-21 16:24:41 PDT
*** Bug 598479 has been marked as a duplicate of this bug. ***
Comment 3 Michael Coates [:mcoates] (acct no longer active) 2010-09-21 16:30:18 PDT
Saw my bug was a dup of this one.  Any thoughts on when this might get addressed?

There has been an increased focus on tracking users via local storage and a few lawsuits filed against ad companies.  It seems we should give users the ability to clear local storage data.

http://arstechnica.com/apple/news/2010/09/rldguid-tracking-cookies-in-safari-database-form.ars
Comment 4 Honza Bambas (:mayhemer) 2010-09-21 16:49:06 PDT
When this gets blocking+, I can work on this.
Comment 5 Daniel Veditz [:dveditz] 2010-09-21 17:15:18 PDT
We probably wouldn't actually take this fix in a 3.5.x release, but maybe. We'd definitely want this in 3.6.x (assuming there is unlikely to be any l10n impact from something like this).
Comment 6 Honza Bambas (:mayhemer) 2010-09-26 06:11:07 PDT
Nickolay, thanks for you comment 1.

Summary a bit out of scope of this bug:
1. localStorage can be used by a common web sites to save data locally on the user's computer, and those are actually close to http cookies
2. localStorage can be used as a storage of data for offline applications ; a web site becomes considered an offline application after user permits that through an already existing UI ; at this moment localStorage can no longer be considered as http cookies, this is (AFAIK) fixed since Gecko 1.9.2

In the first case we are missing few things to really treat localStorage as cookies:
a. we do not present the values in the cookies UI (only http cookies are displayed, bug 506692 seems to be dedicated to that)
b. we do not always delete localStorage data that keeps "localStorage cookies" along with http cookies (this bug should be about it)

--------

To implement deletion of recent localStorage values that are "localStorage cookies" we need to add a new column to the database with date-time of value addition.  Not sure we can add such a change to Fx 3.6.x.
Comment 7 Honza Bambas (:mayhemer) 2010-09-28 16:21:15 PDT
Created attachment 479219 [details] [diff] [review]
v1 [Backed out due to oranges]

- based on patch from bug 536544 (using a lot of code from that patch)
- new method nsIDOMStorageManager.clearStorageDataSince taking the time range
- it deletes data newer then the range from both temp and disk table or PB or SO database
- each storage instance need to get its mItems cache updated ; achieved by observer call to all existing storages
- mItems entry type now contains insertion time ; used only for sessionStorage (i.e. when not using DB)
- updating the database schema: a new column+index inserttime is added
- added tests for localStorage, also in PB mode, sessionStorage, globalStorage (found bug 600366)
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2010-10-22 17:29:52 PDT
Comment on attachment 479219 [details] [diff] [review]
v1 [Backed out due to oranges]

r=jst!
Comment 9 Honza Bambas (:mayhemer) 2010-10-25 11:32:04 PDT
A new method added:
https://bugzilla.mozilla.org/attachment.cgi?id=479219&action=diff#a/dom/interfaces/storage/nsIDOMStorageManager.idl_sec2

BTW, documentation of nsIDOMStorageManager interface says it "provides methods for managing data stored in the offline apps cache".

It is not true at all, the storage manager is more or less an internal service.  It is managing DOM storage data (allow selective deletion and usage determination of that data) and instances (creation of shared localStorage objects).  The storage objects and data created with them can also be used by non-offline allowed web sites.
Comment 10 Honza Bambas (:mayhemer) 2010-10-25 11:43:34 PDT
Comment on attachment 479219 [details] [diff] [review]
v1 [Backed out due to oranges]

http://hg.mozilla.org/mozilla-central/rev/2ccadd55dc4c
Comment 11 Honza Bambas (:mayhemer) 2010-10-25 12:16:55 PDT
Backed out:
http://hg.mozilla.org/mozilla-central/rev/c6ba385c4435

Caused orange:
s: talos-r3-fed-033
5875 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/localstorage/test_cookieSession-phase1.html | undefined - got null, expected "persistent value 1"
5876 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/localstorage/test_cookieSession-phase1.html | undefined - got null, expected "persistent value 2"
5880 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/localstorage/test_cookieSession-phase2.html | Persistent value present - got null, expected "persistent value 1"
5881 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/localstorage/test_cookieSession-phase2.html | Persistent value present - got null, expected "persistent value 2"
Comment 12 Honza Bambas (:mayhemer) 2010-10-25 13:12:47 PDT
Also seen:
6263 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/localstorage/test_localStorageDeleteSinceAPIPrivateBrowsing.html | key2 left - got null, expected "value2"
6264 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/localstorage/test_localStorageDeleteSinceAPIPrivateBrowsing.html | key3 left - got null, expected "value3"
6266 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/localstorage/test_localStorageDeleteSinceAPIPrivateBrowsing.html | Expected 2 items after delete in localStorage - got 0, expected 2

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288034097.1288037332.7694.gz
Comment 13 Honza Bambas (:mayhemer) 2010-10-26 08:34:52 PDT
So, the failure from comment 11 is cooperation of 3 tests:

1. 
- general/test_windowProperties.html reads all properties of the window, that instantiates an empty localStorage 
- we never get or set an item on it, so, we never call nsDOMStorage::CacheStoragePermissions that properly sets mSessionOnly 
- mSessionOnly is set to PR_TRUE by default

2. 
- globalStorage/test_globalStorageDeleteSinceAPU.html (introduced in this bug) invokes the clearStorageDataSince method that also our previously created localStorage listens to (introduced in this bug)
- it calls CacheKeysFromDB() that leads to call to nsDOMStorageMemoryDB::GetItemsTable (called on nsDOMStorageDBWrapper.mSessionOnlyDB) that creates for the first time an in-mem storage for this localStorage and pre-loads it with data from DB for this localStorage, at this moment empty
- this happens because the storage has mSessionOnly set to PR_TRUE

3. 
- localStorage/test_coockieSession-phase1.html stores some values to the localStorage, those are expected to be in the persistent database
- at that moment it really happens as expected because SetItem call on that storage now properly sets mSessionStorage to PR_FALSE
- problem starts at the moment we switch to ACCESS_SESSION behavior
- expected is, that this is the first time we create an in-mem db for the storage and copy data from the persistent DB to it
- but we actually already have created that in-mem DB sooner, so the new values added to the persistent DB are not copied to the in-mem DB as expected
==> persistent key/value pairs are not present in the localStorage when in ACCESS_SESSION mode



At the moment I'm not sure how to fix this.  CacheStoragePermission, that properly sets mSessionOnly, is using the subject (caller's) principal to check the permission state against.  We don't have it at the moment we observer for cut-off (step 2 above) in nsDOMStorage::Observe for NS_DOMSTORAGE_CUTOFF_OBSERVER.

We could use principal that the storage keeps, as it can never be accessed by a codebase principal different from the one it was created for.  I'll check this approach and create a patch in a different bug.

To have a quick fix, that also might be considered correct, is to drop the in-mem db also when switching to ACCESS_SESSION and not only when switching to ACCESS_DEFAULT.  It makes perfectly sense, by the way.
Comment 14 Honza Bambas (:mayhemer) 2010-10-27 07:11:20 PDT
Created attachment 486334 [details] [diff] [review]
v1 update idiff to fix the orange

I decided to go the simple way.  We now drop the session only in-mem DB in both cases when we turn the session only access off and on (off because we want to wipe the data out, on because we want to make sure it will be loaded with actual data from the persistent DB).

Passed try server.

To explain how it works w/o this little fix:
1. it may happen that a session only in-mem DB is created (and at that moment preloaded with data from the persistent DB to keep a copy) while we are in ACCESS_DEFAULT (not session-only) mode for a host
2. we modify the persistent database for the host (by accessing its localStorage)
3. we switch to ACCESS_SESSION and we expect that all data that are in the persistent DB are copied to the session only in-mem DB AT THIS MOMENT ; but it has already been created sooner and is not therefor re-filled with the recent modifications to the persistent DB made in the step 2
Comment 15 Honza Bambas (:mayhemer) 2010-10-27 11:17:21 PDT
Created attachment 486377 [details] [diff] [review]
v1.1 [Backed out comment 16]

http://hg.mozilla.org/mozilla-central/rev/e2b9ce694430
Comment 16 Honza Bambas (:mayhemer) 2010-10-27 11:51:52 PDT
Backed out again, due to new oranges, not catch by try before:

http://hg.mozilla.org/mozilla-central/rev/1e4671765e79

s: talos-r3-fed-042
5851 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/globalstorage/test_globalStorageDeleteSinceAPI.html | Expected 2 items after delete - got 1, expected 2
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288204013.1288204332.16521.gz
Comment 17 Honza Bambas (:mayhemer) 2010-10-27 11:57:18 PDT
This ~could~ be related to bug 600366.  We should then fix it before landing this bug.
Comment 18 Honza Bambas (:mayhemer) 2010-10-27 12:23:53 PDT
Also seen (intermittent):
s: talos-r3-leopard-032
6280 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/localstorage/test_localStorageDeleteSinceAPI.html | Expected 4 items before delete in localStorage - got 0, expected 4
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288204717.1288207002.29555.gz
Comment 19 Benjamin Smedberg [:bsmedberg] 2010-12-15 09:16:01 PST
Not a regression, and we need to ship. Not going to block on this now, and probably wouldn't take it given the UI changes and size of the patch.
Comment 20 Honza Bambas (:mayhemer) 2010-12-15 09:27:26 PST
Seconded, this is a complex patch and we are probably building it on another regression from bug 536544 (bug 607418) which I was trying to figure out for weeks before working on this bug.
Comment 21 Ian Nartowicz 2011-12-14 08:17:59 PST
It appears the sessionStorage is not being removed at all by the cookie manager removeAll() method (via the "cookie-changed" "cleared" observer notification).
Comment 23 Florian Bender 2014-02-17 07:41:37 PST
This is bad for privacy (may also have grave adverse effects on Persona!). Honza, will you find time to work on this for Fx 30?
Comment 24 Honza Bambas (:mayhemer) 2014-02-17 07:59:16 PST
Unlikely.  As a workaround I suggest to delete just all data, as we do with the HTTP cache.  That should be easy to do.
Comment 25 Florian Bender 2014-02-17 08:01:20 PST
Workaround is clear (to us), though virtually all users have no idea about that bug and may depend on this working for all time ranges (and not just "everything"). 

Anybody else who can take this?
Comment 26 Honza Bambas (:mayhemer) 2014-02-17 08:06:29 PST
No, what I suggest is to delete all data even when only a time-range is selected.  I.e. ALWAYS delete all data.  See how the HTTP cache is deleted, we do the same, always clear all.
Comment 27 :Gavin Sharp [email: gavin@gavinsharp.com] 2014-02-24 16:43:25 PST
HTTP cache generally contains easily replaceable data, it's bit of a different story for DOM Storage though (sometimes used for "valuable"/hard to replace data).
Comment 28 Honza Bambas (:mayhemer) 2014-02-24 17:19:45 PST
Hmm.. we treat (or at least would like to) localStorage same as cookies.  Are cookies also that hard to replace data?
Comment 29 Victor Nagy 2014-08-18 01:57:31 PDT
I don't like the "treat localStorage same as cookies" since localStorage can be used to other things than cookies.

Yes, some sites may use localStorage to store cookies and other identifiable data. I agree there should be an easy way to remove these when needed, but other uses may be affected as well, which may not be what the user wants.

For example, take a game that saves its game state to localStorage. Or a document editor that uses localStorage to store documents. When the user clears the history to remove some tracking cookies for privacy reasons, I am pretty certain the user might want to keep their save state in games or their documents, as these can contain valuable data to the user.

What's needed is probably out of scope for this bug, but it would be more optimal to have a view where the user can select what should be deleted, or just select all if the user doesn't care. If I understand correctly from multiple bug reports I've read so far, currently it's all or nothing, which can make it a hard decision, "Do I want to get rid of these tracking cookies, or do I want to keep my save states and documents?".

Currently it seems like localStorage is cleared when "cookies" are selected in the "Clear recent history", if this hasn't changed and I understand the other bug reports correctly. I don't really like that, and believe localStorage should be separate, and with time, a view for more finegrained control of what should be removed would be implemented. I would gladly look into it myself if I had the knowledge, but my knowledge of C++ isn't good at all and I never really been involved so I'm not the right person to do it.

Now I don't have any examples of games or apps that uses localStorage like this, but it is something I can envision apps and websites wanting to do. Therefore I don't really like this "treat localStorage as cookies", since localStorage isn't just cookies.
Comment 30 Ricardo 2014-09-29 16:14:35 PDT
Why nothing can clean and delete these folders inside profile (unless manually)?
storage
|-persistent
  |-http+++example.com
    |-idb
    |-.metadata
  |-[...]
|-temporary
  |-http+++example2.com
  |-[...]

E.g. You may be downloading big files from Mega and something make the download fail and the big incomplete files will stay inside those folders forever.

Other sites create those folders for almost no reason, and more folders start accumulating there.

Imo, this bug is severe and too old already.
Comment 31 Steven Penny 2014-10-06 14:45:17 PDT
Yes, this is a problem. Sites like this

http://mothereff.in/regexpu

are using DOM Storage, so that means if I want to delete that data I have to
delete ALL my cookies. This is not desirable because some sites have useful
cookies, for example Google Search Settings.
Comment 32 Steven Penny 2014-10-06 15:15:01 PDT
This is a workaround

    localStorage.clear();

http://stackoverflow.com/a/7667973
Comment 33 Dão Gottwald [:dao] 2014-11-20 10:11:45 PST
*** Bug 602743 has been marked as a duplicate of this bug. ***
Comment 34 Dão Gottwald [:dao] 2014-11-21 01:59:05 PST
*** Bug 1047098 has been marked as a duplicate of this bug. ***
Comment 35 Honza Bambas (:mayhemer) 2014-11-21 07:57:48 PST
i don't understand why bug 1047098 and bug 602743 were marked as dups of this one.
Comment 36 Dão Gottwald [:dao] 2014-11-21 08:07:09 PST
I'm not even sure if bug 602743 was about localStorage or indexedDB; bug 602743 comment 5 implies the former, which makes it a duplicate of this bug even if we don't cover indexedDB here. If covering indexedDB here doesn't make sense, we should reopen bug 1047098. Finally, if bug 602743 was actually about indexedDB and bug 1047098 is reopened, bug 602743 should be made a duplicate of that.
Comment 37 Nickolay_Ponomarev 2015-04-03 02:58:28 PDT
*** Bug 1125336 has been marked as a duplicate of this bug. ***
Comment 38 Ian Nartowicz 2015-07-28 07:02:22 PDT
Bug 1125336, duplicated to this one, requests a separate entry in the clear history dialog for both DOM storage and indexedDB (two entries?).  Is that a plan on the table here?  It seems like a no-brainer to me, but doesn't really seem to have been discussed.  Some reason why it shouldn't happen that way?
Comment 39 github 2015-10-01 06:05:21 PDT
Just a comment from a user perspective, it would be nice if there was a way to clear the storage for a particular website / base url. I recently had an issue with some persistent cloudflare caching that i just couldnt get rid of, eventually tracked it down to them storing javascript in local storage to speed up the site load time. It would be really nice to be able to delete local storage for just the site i'm debugging.. fwiw.
Comment 40 Honza Bambas (:mayhemer) 2016-01-06 08:27:43 PST
Jan, I think this may rely on you QM work for DOM storage.
Comment 41 Honza Bambas (:mayhemer) 2016-02-17 04:38:16 PST
*** Bug 857888 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.