1355576 - Implement clearing of LocalStorage in browsingData API

Status: VERIFIED FIXED

(WebExtensions :: Compatibility, enhancement, P5)

Description

[Thomas Wisniewski]

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

Comment 1

[Tim Nguyen :ntim]

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

Comment 2

[Thomas Wisniewski]

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?

Comment 3

[Bob Silverberg [:bsilverberg]]

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.

Comment 4

[Thomas Wisniewski]

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

Comment 5

[Tim Nguyen :ntim]

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

Comment 6

[Thomas Wisniewski]

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

Comment 7

[Bob Silverberg [:bsilverberg]]

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.

Comment 8

[Thomas Wisniewski]

Attachment: Bug 1355576 - Add ability to clear all localStorage with the browsingData API;

Review commit: https://reviewboard.mozilla.org/r/154330/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154330/

Comment 9

[Thomas Wisniewski]

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.

Comment 10

[Tim Nguyen :ntim]

\o/ Thanks!

Comment 11

[Shane Caraveo (:mixedpuppy)]

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

Bob should review browsingData changes.

Comment 12

[Shane Caraveo (:mixedpuppy)]

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 13

[Bob Silverberg [:bsilverberg]]

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.

Comment 14

[Thomas Wisniewski]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154330/diff/1-2/

Comment 15

[Thomas Wisniewski]

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

Comment 16

[Erin]

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.

Comment 17

[Bob Silverberg [:bsilverberg]]

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

Comment 18

[Thomas Wisniewski]

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 19

[Bob Silverberg [:bsilverberg]]

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.

Comment 20

[Thomas Wisniewski]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154330/diff/2-3/

Comment 21

[Thomas Wisniewski]

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?

Comment 22

[Jan Varga [:janv]]

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

[Jan Varga [:janv]]

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.

Comment 24

[Thomas Wisniewski]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154330/diff/3-4/

Comment 25

[Thomas Wisniewski]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154330/diff/4-5/

Comment 26

[Thomas Wisniewski]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154330/diff/5-6/

Comment 27

[Thomas Wisniewski]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154330/diff/6-7/

Comment 28

[Thomas Wisniewski]

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5020a2dcb8a7
Add ability to clear all localStorage with the browsingData API; r=bsilverberg,janv

Comment 30

[Phil Ringnalda (:philor)]

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

Comment 31

[Thomas Wisniewski]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154330/diff/7-8/

Comment 32

[Thomas Wisniewski]

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

Comment 33

[Thomas Wisniewski]

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.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1da9f146635c
Add ability to clear all localStorage with the browsingData API; r=bsilverberg,janv

Comment 35

[Carsten Book [:Tomcat]]

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

Comment 36

[Thomas Wisniewski]

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?

Comment 37

[Bob Silverberg [:bsilverberg]]

(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

Comment 38

[Thomas Wisniewski]

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.

Comment 39

[Andy McKay]

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.

Comment 40

[Will Bamberg [:wbamberg]]

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.

Comment 41

[Thomas Wisniewski]

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

Comment 42

[marius.santa]

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.

Comment 43

[Liz Henry (:lizzard) (relman/hg->git project)]

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 44

[Jan Varga [:janv]]

Attachment: patch for ESR52

Comment 45

[Jan Varga [:janv]]

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 46

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 47

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr52/rev/cf34a0574e58

Comment 48

[Thomas Wisniewski]

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

Comment 49

[marius.santa]

Attachment: clear storage.gif

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.