Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement clearing of LocalStorage in browsingData API

NEW
Unassigned
(NeedInfo from)

Status

()

Toolkit
WebExtensions: Compatibility
P5
normal
3 months ago
2 days ago

People

(Reporter: Thomas Wisniewski, Unassigned, NeedInfo)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [browsingData]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
+++ 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

a month ago
I think we can implement localStorage without support for "since", it would really be helpful.
(Reporter)

Comment 2

a month ago
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?
Flags: needinfo?(bob.silverberg)
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.
Flags: needinfo?(bob.silverberg)
(Reporter)

Comment 4

a month ago
Tim, I'm guessing that addons like Self-Destructing Cookies would benefit from this feature even without "since"? What do you think?
Flags: needinfo?(ntim.bugs)

Comment 5

a month ago
(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.
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 6

25 days ago
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"?
Flags: needinfo?(bob.silverberg)
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.
Flags: needinfo?(bob.silverberg)
Comment hidden (mozreview-request)
(Reporter)

Comment 9

18 days ago
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

17 days ago
\o/ Thanks!
Comment on attachment 8883427 [details]
Bug 1355576 - Add ability to clear all localStorage with the browsingData API;

Bob should review browsingData changes.
Attachment #8883427 - Flags: review?(bob.silverberg)

Comment 12

17 days ago
mozreview-review
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

16 days ago
mozreview-review
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.
Attachment #8883427 - Flags: review?(bob.silverberg) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 15

13 days ago
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

2 days ago
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?
Flags: needinfo?(wisniewskit)
(Reporter)

Comment 18

2 days ago
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.
Flags: needinfo?(wisniewskit)

Comment 19

2 days ago
mozreview-review
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.
Attachment #8883427 - Flags: review?(bob.silverberg) → review+
Comment hidden (mozreview-request)
(Reporter)

Comment 21

2 days ago
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?
Flags: needinfo?(jvarga)
You need to log in before you can comment on or make changes to this bug.