+++ This bug was initially created as a clone of Bug #1333050 +++ Chrome's browsingData API allows for clearing of IndexedDB and LocalStorage for all visited sites. This has not yet been implemented in Firefox. However, the LocalStorage case will require further changes to support the "since" case. As such the localStorage case has been split out into this bug, so that we can land support for the indexedDB case separately.

I think we can implement localStorage without support for "since", it would really be helpful.

I'm for it, but :bsilverman said in bug 1333050 comment 4 that he'd rather that we don't implement it without support for "since". Bob, are you okay with implementing it halfway now?

I could go either way. If there's a feeling that it would be valuable without "since" support then I suppose it's okay to implement it that way now. It's doubtful the work to support "since" in localStorage will happen anytime soon.

Tim, I'm guessing that addons like Self-Destructing Cookies would benefit from this feature even without "since"? What do you think?

(In reply to Thomas Wisniewski from comment #4) > Tim, I'm guessing that addons like Self-Destructing Cookies would benefit > from this feature even without "since"? What do you think? Yes, totally agree. I would implement the feature without 'since' here, then file another bug to get 'since' support implemented.

Bob, would :ntim's agreement in comment 5 be enough, or should we discuss this further before I write a patch to implement localStorage-clearing without "since"?

I think it's fine to go ahead. Please also open a bug that depends on this bug which asks to add "since" support as Tim suggested in comment 5.

Here is my attempt at this; hopefully it's reasonably close to acceptable. Shane, I'm not 100% sure who would be best to review the localStorage-related code here... would you have any ideas? I will create related bugs for adding "since" and "hostnames" support for localStorage soon.

\o/ Thanks!

Comment on attachment 8883427 [details] Bug 1355576 - Add ability to clear all localStorage with the browsingData API; Bob should review browsingData changes.

Comment on attachment 8883427 [details] Bug 1355576 - Add ability to clear all localStorage with the browsingData API; https://reviewboard.mozilla.org/r/154330/#review159662 ::: browser/components/extensions/ext-browsingData.js:121 (Diff revision 1) > + } > + > + let stGetScopes; > + try { > + stGetScopes = > + db.createStatement("SELECT DISTINCT scope FROM webappsstore2 LIMIT -1"); I don't like db code like this in webext API land. At the very least this should be in a localstorage api somewhere.

Comment on attachment 8883427 [details] Bug 1355576 - Add ability to clear all localStorage with the browsingData API; https://reviewboard.mozilla.org/r/154330/#review160030 This is a good start, Thomas. Thanks! Just a few nits and suggestions for changes to the test, but the biggest issue is that we need someone to review the localStorage code, and maybe they can point you to something that already exists in m-c to do this, or somewhere that code could be added to support this. ::: browser/components/extensions/ext-browsingData.js:85 (Diff revision 1) > > const clearHistory = options => { > return sanitizer.items.history.clear(makeRange(options)); > }; > > +function* LocalStorageIterator() { I agree with Shane. Ideally this would use code that exists elsewhere in the tree that interacts with localStorage, and if no such thing exists, and there's no good place for this code, it should at least be extracted out into a module. It sounds like the top priority is to find a localStorage peer to review this part of the code, but I'm not sure who that is. ::: browser/components/extensions/ext-browsingData.js:143 (Diff revision 1) > + stGetScopes.finalize(); > + db.asyncClose(); > + } > +} > + > +let clearLocalStorage = Task.async(function* (options) { This should be `const clearLocalStorage = async function(options) {`, similar to other functions below it. ::: browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js:5 (Diff revision 1) > +function setupLocalStorage(browser) { > + return ContentTask.spawn(browser, null, function* () { > + content.localStorage.setItem("key", "value"); > + }); > +} > + > +function checkLocalStorageExists(browser) { > + return ContentTask.spawn(browser, null, function* () { > + return content.localStorage.getItem("key") === "value"; > + }); > +} Do you need to do this, or could the setting and checking of localStorage be done via a content script in the test extension? ::: browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js:38 (Diff revision 1) > + const host1 = "http://example.com"; > + let tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser, host1); > + let browser1 = gBrowser.getBrowserForTab(tab1); > + > + const host2 = "http://example.net"; > + let tab2 = await BrowserTestUtils.openNewForegroundTab(gBrowser, host2); > + let browser2 = gBrowser.getBrowserForTab(tab2); Maybe this stuff could be done in the test extension's background script, via the `tabs` API, rather than in the test? ::: browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js:46 (Diff revision 1) > + await setupLocalStorage(browser1); > + await setupLocalStorage(browser2); As per my comment above, if you had a content script you could send it a message to set the localStorage at this point. ::: browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js:56 (Diff revision 1) > + todo(await checkLocalStorageExists(browser1), true); > + todo(await checkLocalStorageExists(browser2), false); Why would one of these be true and one false at this point? ::: browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js:63 (Diff revision 1) > + tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser, host1); > + browser1 = gBrowser.getBrowserForTab(tab1); > + tab2 = await BrowserTestUtils.openNewForegroundTab(gBrowser, host2); > + browser2 = gBrowser.getBrowserForTab(tab2); I'm not clear on why you are creating and destroying tabs at different points in this test. Couldn't you just keep the tabs around for the entire length of the test? Also, as above, I think this might be cleaner if you used the `tabs` API for this.

I've taken another stab at this, and found a more solid way to clear the localStorage by hooking up a new Services.obs.notifyObservers signal handler, which does what the cookie-changed/cookie-cleared topics do, but only for localStorage (as the cookie ones are overloaded to also clear cookies and sessionStorage). This approach also clears the cached localStorages for open tabs, which lets me revise the test to not bother closing/reopening tabs. I've addressed your other review notes as well (removing the "since" code entirely, as it might as well be added in the ticket that adds support for "since").

I know everyone has their busy lives, but the FF57 merge date is in less than two weeks. Users really need an alternative to Self Destructing Cookies (discontinued), which is what CAD offers: https://github.com/mrdokenny/Cookie-AutoDelete/issues/44 For me participating in further Nightly channel is dependent on the "this" bug and https://bugzilla.mozilla.org/show_bug.cgi?id=1340511 The missing features of browsingData API are privacy essential. A key subject of Mozilla is privacy. Which is why I usually visit these two trackers regularly. Please I beg you to review Mr Wisniewski's attachment so there is a realistic chance FF57 will allow users to destroy Evercookies automatically with the webextension I've linked above, the code relies on these bugs.

(In reply to Thomas Wisniewski from comment #15) > I've taken another stab at this, and found a more solid way to clear the > localStorage by hooking up a new Services.obs.notifyObservers signal > handler, which does what the cookie-changed/cookie-cleared topics do, but > only for localStorage (as the cookie ones are overloaded to also clear > cookies and sessionStorage). > > This approach also clears the cached localStorages for open tabs, which lets > me revise the test to not bother closing/reopening tabs. I've addressed your > other review notes as well (removing the "since" code entirely, as it might > as well be added in the ticket that adds support for "since"). I see you updated the patch, but didn't respond to or mark any of my issues in MozReview. It makes re-reviewing easier if I can see which of my issues have been fixed and/or dropped. Would you mind doing that Thomas?

Sure, they were all basically "fixed", so I've marked them as such (the patch uses a different technique to clear LS, and I corrected the other issues with the test/etc). I just can't be 100% sure if this new approach is the best way to do things without review from an appropriate peer. It seems to me that it's much closer than the first patch, though.

Comment on attachment 8883427 [details] Bug 1355576 - Add ability to clear all localStorage with the browsingData API; https://reviewboard.mozilla.org/r/154330/#review164760 This looks good to me, Thomas, as far as the WebExtensions part goes. Someone else should review the code that actually clears the storage. Just one small comment about the test, but r+ otherwise. Nice work. ::: browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js:59 (Diff revision 2) > + > + function content() { > + browser.runtime.onMessage.addListener(msg => { > + if (msg === "resetLocalStorage") { > + localStorage.clear(); > + localStorage.setItem("test", "test"); Might it be worthwhile to check localStorage at some point to make sure the value you set is actually there before clearing? Otherwise the check in `checkLocalStorageCleared` could be a false positive.

Alright, I've added that check. I also added :janv to the list of reviewers, in the hopes that he can at least direct us to a DOM Storage peer who can take a look. Jan, sorry that I forgot to add a more detailed comment in the commit message, but could you please review the non-WebExtension localStorage changes here to see if they pass muster?

(In reply to Shane Caraveo (:mixedpuppy) from comment #12) > Comment on attachment 8883427 [details] > Bug 1355576 - Add ability to clear all localStorage with the browsingData > API; > > https://reviewboard.mozilla.org/r/154330/#review159662 > > ::: browser/components/extensions/ext-browsingData.js:121 > (Diff revision 1) > > + } > > + > > + let stGetScopes; > > + try { > > + stGetScopes = > > + db.createStatement("SELECT DISTINCT scope FROM webappsstore2 LIMIT -1"); > > I don't like db code like this in webext API land. At the very least this > should be in a localstorage api somewhere. I'm glad that you removed the SQL statement, it's really not a good idea to do it directly here. I'm looking at the patch now.

Comment on attachment 8883427 [details] Bug 1355576 - Add ability to clear all localStorage with the browsingData API; https://reviewboard.mozilla.org/r/154330/#review170568 Looks good.

Thanks a bunch, janv! The patch still applies cleanly to moz-central, and a try-run looks fine (aside from an eslint issue that I fixed): https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e3bd233a12eb4dc783b12b0d5df828278e27246 Requesting check-in. PS: Sorry for the review-spam, I didn't realize that "mach eslint" would alter a file that would become part of the commit, and I stumbled around reverting it :S).

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5020a2dcb8a7 Add ability to clear all localStorage with the browsingData API; r=bsilverberg,janv

Backed out in https://hg.mozilla.org/integration/autoland/rev/c712a6390a95 for https://treeherder.mozilla.org/logviewer.html#?job_id=121296906&repo=autoland

This was a trivial fix; I had just missed an unexpected sanity test (localStorage is now supported, so the test failed). I'm doing a new try-run here in case I missed something else: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49524974de5f2342c273d4badba426c027cd8b5e

The try-run isn't showing anything else that I can see would be affected by this patch, so I'm re-requesting check-in with the latest version.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1da9f146635c Add ability to clear all localStorage with the browsingData API; r=bsilverberg,janv

It just occurred to me that we might want to also have this API throw "NotImplementedError" when the caller tries to use it with the "since" option, as right now it will just clear *all* of the localStorages instead regardless of what they pass in for "since". Bob, do you agree? If so, should I just make a new ticket for that or reopen this one and add a quick patch?

(In reply to Thomas Wisniewski from comment #36) > It just occurred to me that we might want to also have this API throw > "NotImplementedError" when the caller tries to use it with the "since" > option, as right now it will just clear *all* of the localStorages instead > regardless of what they pass in for "since". Bob, do you agree? If so, > should I just make a new ticket for that or reopen this one and add a quick > patch? This is an interesting question. There are two other categories that do not implement `since`: cache and serviceWorkers. Cache also does not support `since` on Chrome, and we have documented that on MDN [1], but it's unclear whether `since` is supported on Chrome for serviceWorkers, and we do not seem to have documented the fact that it's unsupported on Firefox. [2] We could simply document that `since` is not supported for localStorage, but if it is supported on Chrome (the documentation [3] doesn't indicate either way, but I would assume it does based on that) then maybe we do need to throw an exception if someone tries to pass a value for `since` when clearing localStorage. Because a value of 0 for `since` is the same as no value, we should probably allow that, but if they try to pass a non-zero value it might make sense to throw. I'm not really sure which way we should go on that, so I'm needinfoing Andy to see what he thinks. [1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/removeCache [2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/DataTypeSet [3] https://developer.chrome.com/extensions/browsingData#method-removeLocalStorage

Note that in my patch in bug 1388428, I'm having it just reject the Promise with an "unsupported" message, as I see that being done elsewhere in WebExtension code. Maybe that's not the right solution, but it seemed reasonable.

In comment 36, it sounds like if a developer does pass since, the side effect can be unexpected clearing all data. That sounds bad, so raising unsupported as mentioned in comment 37 and comment 38 sounds like a good solution.

I've written a page on this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/removeLocalStorage Please let me know if this covers it.

Yes, that looks fine, thanks. (Sorry for the ni? delay!)

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Nothing that we're discussing uplift of this to ESR in bug 1047098, but that sounds a little risky to me. Thomas, Jan, what do you think?

Comment on attachment 8925780 [details] [diff] [review] patch for ESR52 [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is needed for bug 1047098 which already has ESR52 approval. User impact if declined: See bug 1047098. Fix Landed on Version: 57 Risk to taking this patch (and alternatives if risky): Low String or UUID changes made by this patch: None

Comment on attachment 8925780 [details] [diff] [review] patch for ESR52 Let's give this a try since we're aiming to fix the fingerprinting issue in bug 1047098.

FWIW, I agree on trying to get it in if it helps with bug 1047098.

Issue reproduced on Firefox 55.0a1 (20170411030208) under Wind 10 64-bit. This issue is verified as fixed on Firefox 57.0 (20171112125346), Firefox 58.0b3 (20171113130450) and 59.0a1 (20171113220112) under Wind 10 64-bit and Mac OS X 10.13.