1388428 - Extend browsingData to support removing localStorage by host

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Thomas Wisniewski]

Now that browsingData supports clearing localStorage and clearing cookies by hostname, we should also support clearing localStorage by hostname.

I've got a patch I'll put up on reviewboard shortly, for which a try run seems fine.. just unrelated intermittents: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84c70631d1bab10c727d325ecc04053b7a2d505a

Comment 1

[Thomas Wisniewski]

Attachment: Bug 1388428 - Extend browsingData to restrict removing localStorage to a given list of hostnames;

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

Comment 2

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8894976 [details]
Bug 1388428 - Extend browsingData to restrict removing localStorage to a given list of hostnames;

https://reviewboard.mozilla.org/r/166090/#review171704

The WebExtensions pieces look good to me. Thanks for taking this on, Thomas.

Comment 3

[Thomas Wisniewski]

Just to be 100% sure, you and Andy are alright with me rejecting the promise if a "since" parameter is given? (as per our discussion in bug 1355576)? If so, I'll make a note on the other bug for completeness' sake.

Comment 4

[Bob Silverberg [:bsilverberg]]

(In reply to Thomas Wisniewski from comment #3)
> Just to be 100% sure, you and Andy are alright with me rejecting the promise
> if a "since" parameter is given? (as per our discussion in bug 1355576)? If
> so, I'll make a note on the other bug for completeness' sake.

Good point. I think it's a good idea, but I guess we should pass it by Andy for a final say. It's possible it could create Chrome compatibility issues, where extensions generate errors on Firefox but not on Chrome, but one could argue that if an extension expects to be able to delete only some localStorage, and instead deletes all of it, that should be an error condition.

Comment 5

[Thomas Wisniewski]

Bob, given that Andy agreed that raising unsupported is a good idea, do you think what I'm doing in this patch (rejecting the promise with an "unsupported" message) will do?

Comment 6

[Thomas Wisniewski]

For those following along, Andy just said on IRC that the patch should be fine as is, so I'm clearing the needinfo (and have nudged janv to see if he can get to the review anytime soon).

Comment 7

[Jan Varga [:janv]]

Comment on attachment 8894976 [details]
Bug 1388428 - Extend browsingData to restrict removing localStorage to a given list of hostnames;

https://reviewboard.mozilla.org/r/166090/#review184682

Sorry, this took longer than I originally expected.

So I rebased the patch (will attach it later) and went through the changes.
I believe that this patch works for you, but I think the observer infrastructure is abused since it's not currently designed for multiple origin scopes.
The code is already messy, needs an overhaul and this makes it even more hackish.
We already have too many notification types and some of them use observer service for no reason and this approach is incorrectly followed here.
I'm also not very happy about joining hosts and using the ^ separator. We could use aSubject for passing an xpcom array, but again, the StorageObserver infrastructure is not designed for that currently and I'm not sure it's worth to extend it for this particular case.

I'll let :asuth to express his opinion on this too since he's doing a cleanup in this module.

I think instead of using observer service, we can add a new method to nsIDOMStorageManager and then also add a new IPC message for synchronizing caches in child processes.

This will get even more complicated once local storage is under quota manager.

Comment 8

[Jan Varga [:janv]]

Andrew, can you take a look if you have time ?
See comment 7. Thanks.

Comment 9

[Jan Varga [:janv]]

Attachment: Rebased patch with some changes

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Thomas, thank you for being brave enough to delve into this very ugly LocalStorage code.  This stuff is gross and it's not your fault it's gross!

Along those lines, I agree with :janv that we really need to clean this stuff up before making it any more gross. ext-browsingData.js should be consuming an XPCOM service (or ChromeOnly WebIDL interface) which should have a documented, defined contract.  Even if that XPCOM interface ends up using the observer service under the hood temporarily, the observer service should not be the API!

And along the lines of those lines, I don't quite get why the existing "browser:purge-domain-data" or "clear-origin-attributes-data" affordances just can't be used multiple times in a loop.  Well, other than there might be other collateral erasure that's not explicitly called for by the webext API, but that again seems like an argument for addressing the fundamental root problem that this all is very ad-hoc and insufficiently documented, etc.

I see 2 major to-do's here:
1) Cleanup the data erasure process by introducing that documented XPCOM helper.  For now it would want to be a shim on the existing trainwreck.
2) Ensure LocalStorage is hooked up to that mechanism.  I think we can get that largely for free via :janv's overhaul of localstorage to use QuotaManager in bug 1286798.  QuotaManager may need to grow some ability to only clear specific clients' data (IndexedDB, LocalStorage, Cache, etc.) since it seems the browsingData semantics may require it somewhat.

I think these are the existing consumers of these observers:
1) The Firefox UI, especially I think whatever storage UI the Taipei Center was working on?
2) WebExts API
3) Unit tests.
4) Formerly: Legacy Extensions, now moot.

This seems sufficiently cross-cutting that the simplest next step might be to send a message to firefox-dev after searching bugzilla for existing (active) bugs on cleaning things up.  (And if anyone already knows what's going on in the greater context, please speak up here!)

PS: I'm no longer doing LocalStorage cleanup because we punted on LocalStorage labeling and so :janv's overhaul is the next meaningful thing happening to LS.

Comment 11

[Thomas Wisniewski]

Thanks, guys.

Andrew, if you think it won't be *too* gross for me to just fire off multiple messages to an observer listening for a single host to erase (your loop suggestion), then I don't mind making a new version of the patch doing that for now (to minimize the work we need to do when we fix this properly).

Otherwise, I think I'm going to be too swamped to really help with the more proper fix for now (though if others can champion this and break the problem down into a list of mentorable bugs, I'll be more than happy to help).

Comment 12

[Jan Varga [:janv]]

I'm ok with sending multiple messages in a loop as a temporary solution.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

And if Jan's okay with it, I'm okay with it!

Comment 14

[Thomas Wisniewski]

Sounds good; I'll try to get a patch together ASAP.

Comment 15

[Thomas Wisniewski]

Comment on attachment 8894976 [details]
Bug 1388428 - Extend browsingData to restrict removing localStorage to a given list of hostnames;

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

Comment 16

[Thomas Wisniewski]

Alright, I had a chance to revise the patch tonight. It now sends multiple messages in a loop. A try run seems clean: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7ef19cce546ec4600f609194faed33c6141c98e

Comment 17

[Jan Varga [:janv]]

Comment on attachment 8894976 [details]
Bug 1388428 - Extend browsingData to restrict removing localStorage to a given list of hostnames;

https://reviewboard.mozilla.org/r/166090/#review188448

I'll submit a patch with suggested changes...

::: dom/storage/LocalStorageManager.cpp:380
(Diff revision 2)
>    }
>  
>    // Clear everything, caches + database
>    if (!strcmp(aTopic, "cookie-cleared") ||
>        !strcmp(aTopic, "extension:purge-localStorage-caches")) {
> -    ClearCaches(LocalStorageCache::kUnloadComplete, pattern, EmptyCString());
> +    ClearCaches(LocalStorageCache::kUnloadComplete, pattern, aOriginScope);

I think we should split this into separate |if (...)|, it seems that's the style here and also it will make more obvious that cookie-changed doesn't use aOriginScope.

::: dom/storage/StorageObserver.cpp:165
(Diff revision 2)
> +  nsresult rv;
> +  nsAutoCString aceDomain;
> +  nsCOMPtr<nsIIDNService> converter = do_GetService(NS_IDNSERVICE_CONTRACTID);
> +  if (converter) {
> +    rv = converter->ConvertUTF8toACE(aDomain, aceDomain);
> +    NS_ENSURE_SUCCESS(rv, rv);

These can be converted to:
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

::: dom/storage/StorageObserver.cpp:176
(Diff revision 2)
> +                      aceDomain,
> +                      fallible);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  return CreateReversedDomain(aceDomain, aOriginScope);

I think we can share more code here ...

::: dom/storage/StorageObserver.cpp:302
(Diff revision 2)
>  
>      return NS_OK;
>    }
>  
>    if (!strcmp(aTopic, "extension:purge-localStorage")) {
> +    if (!aData) {

The blame for this section is changed anyway, so I think we can do |if (aData)| here.

::: dom/storage/StorageObserver.cpp:326
(Diff revision 2)
> +        StorageDBChild* storageChild = StorageDBChild::GetOrCreate();
> +        if (NS_WARN_IF(!storageChild)) {
> +          return NS_ERROR_FAILURE;
> +        }
> +
> +        storageChild->SendClearMatchingOrigin(originScope);

Code here is identical with "browser:purge-domain-data" handling.

Comment 18

[Jan Varga [:janv]]

Attachment: patch

Comment 19

[Thomas Wisniewski]

Comment on attachment 8894976 [details]
Bug 1388428 - Extend browsingData to restrict removing localStorage to a given list of hostnames;

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

Comment 20

[Thomas Wisniewski]

Thanks, Jan. The changes make sense to me. I've folded them into my reviewboard request. Should we get asuth to give it a review as well, or will you rubber-stamp it?

Comment 21

[Jan Varga [:janv]]

Comment on attachment 8894976 [details]
Bug 1388428 - Extend browsingData to restrict removing localStorage to a given list of hostnames;

Yeah, Andrew should take a look.

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8894976 [details]
Bug 1388428 - Extend browsingData to restrict removing localStorage to a given list of hostnames;

https://reviewboard.mozilla.org/r/166090/#review188978

To try and better understand what's going on with all the observer stuff and general data clearing interactions, especially in terms of my raising of "browser:purge-domain-data" or "clear-origin-attributes-data" in comment 10, I took notes at https://public.etherpad-mozilla.org/p/data-clearing-apis

I think the relevant notes are:
- "browser:purge-domain-data" is generated by ForgetAboutSite.jsm supporting Places' "Forget About This Site" context menu item.  If generated directly, it will currently impact ServiceWorkers and the session store, so it's appropriate not to use.
- "clear-origin-attributes-data" doesn't support domains, just originattributes, so it's also not appropriate to use.

So these changes make sense and look good.

The one issue I see is that the test looks race-prone and so we would expect intermittents, but it was already intermittent as we see in bug 1389349.  I've taken that bug because I'm doing some other localStorage test cleanup and it's clear that's an orthogonal issue to this bug.

Comment 23

[Thomas Wisniewski]

Comment on attachment 8911948 [details] [diff] [review]
patch

Thanks Andrew. Then let's give this a checkin and see where that takes us.

Comment 24

[Thomas Wisniewski]

Comment on attachment 8894976 [details]
Bug 1388428 - Extend browsingData to restrict removing localStorage to a given list of hostnames;

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

Comment 25

[Thomas Wisniewski]

The patch is now rebased (just in case).

Requesting checkin.

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8894976 [details]
Bug 1388428 - Extend browsingData to restrict removing localStorage to a given list of hostnames;

I think something glitched with reviewboard from when :janv redirected his r? to me.  Setting r=asuth again.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ae47699361c
Extend browsingData to restrict removing localStorage to a given list of hostnames; r=asuth,bsilverberg

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7ae47699361c

Comment 29

[Thomas Wisniewski]

bsilverberg, should we request uplift to 57 for this, given the relative interest in this API?

Comment 30

[Bob Silverberg [:bsilverberg]]

(In reply to Thomas Wisniewski from comment #29)
> bsilverberg, should we request uplift to 57 for this, given the relative
> interest in this API?

Although it may be popular I don't think it qualifies for uplift for 57. They're being very strict about that especially for new features, so I think 58 will have to be fine.

Comment 31

[Will Bamberg [:wbamberg]]

I've updated:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/removeLocalStorage#Browser_compatibility to mention support for hostnames

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/RemovalOptions to mention support for local storage.

I'd like confirmation that the detailed rules here apply to local storage items as well as cookies:

"You must pass in just a hostname here, without protocol (for example: "google.com" not https://google.com"). You can use the URL interface to parse a raw URL and retrieve just the hostname. Items associated with subdomains of a given hostname will not be removed: you must explicitly list subdomains."

Comment 32

[Thomas Wisniewski]

Yes, those rules are the same for cookies and localStorage, thanks for checking.

Comment 33

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