Thomas Wisniewski Assignee
	Description • 7 years ago

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

	Bob Silverberg [:bsilverberg]
	Comment 2 • 7 years ago
mozreview-review

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.

	Thomas Wisniewski Assignee
	Comment 3 • 7 years ago

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.

	Bob Silverberg [:bsilverberg]
	Comment 4 • 7 years ago

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

	Thomas Wisniewski Assignee
	Comment 5 • 7 years ago

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?

	Thomas Wisniewski Assignee
	Comment 6 • 7 years ago

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

	Jan Varga [:janv]
	Comment 7 • 7 years ago
mozreview-review

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.

	Jan Varga [:janv]
	Comment 8 • 7 years ago

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

	Andrew Sutherland [:asuth] (he/him)
	Comment 10 • 7 years ago

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.

	Thomas Wisniewski Assignee
	Comment 11 • 7 years ago

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

	Jan Varga [:janv]
	Comment 12 • 7 years ago

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

	Andrew Sutherland [:asuth] (he/him)
	Comment 13 • 7 years ago

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

	Thomas Wisniewski Assignee
	Comment 14 • 7 years ago

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

	Thomas Wisniewski Assignee
	Comment 16 • 7 years ago

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

	Jan Varga [:janv]
	Comment 17 • 7 years ago
mozreview-review

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.

	Thomas Wisniewski Assignee
	Comment 20 • 7 years ago

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?

	Jan Varga [:janv]
	Comment 21 • 7 years ago

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.

	Andrew Sutherland [:asuth] (he/him)
	Comment 22 • 7 years ago
mozreview-review

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.

	Thomas Wisniewski Assignee
	Comment 23 • 7 years ago

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

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

	Thomas Wisniewski Assignee
	Comment 25 • 7 years ago

The patch is now rebased (just in case).

Requesting checkin.

	Andrew Sutherland [:asuth] (he/him)
	Comment 26 • 7 years ago

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.

	Pulsebot
	Comment 27 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7ae47699361c
Extend browsingData to restrict removing localStorage to a given list of hostnames; r=asuth,bsilverberg

	Phil Ringnalda (:philor)
	Comment 28 • 7 years ago
bugherder

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

	Thomas Wisniewski Assignee
	Comment 29 • 7 years ago

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

	Bob Silverberg [:bsilverberg]
	Comment 30 • 7 years ago

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

	Will Bamberg [:wbamberg]
	Comment 31 • 7 years ago

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

	Thomas Wisniewski Assignee
	Comment 32 • 7 years ago

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

	marius.santa
	Comment 33 • 7 years ago

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.