1363004 - Implement browsingData.removeFormData WebExtension API method on android

Status: RESOLVED FIXED

(WebExtensions :: Android, enhancement, P5)

Description

[Tushar Saini (:shatur)]

This method clears data that the browser has saved for auto-filling forms. The doc can be found at [1].

[1]. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/removeFormData

Comment 1

[Tushar Saini (:shatur)]

Attachment: Bug 1363004 - Implement browsingData.removeFormData webExtension API method on android.

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

Comment 2

[:Grisha Kruglov]

Comment on attachment 8879319 [details]
Bug 1363004 - Implement browsingData.removeFormData webExtension API method on android.

https://reviewboard.mozilla.org/r/150606/#review157342

I've only skimmed through the test code, but this seems OK.

::: mobile/android/modules/Sanitizer.jsm:238
(Diff revision 1)
>  
> -          FormHistory.update({ op: "remove" });
> +          // Convert time to microseconds
> +          let time = startTime * 1000;
> +          FormHistory.update([{
> +            op: "remove",
> +            firstUsedStart: time

Reading FormHistory.jsm, it seems like it will do the right thing here. We sync this data, and any deletions must be synced as well. In case of form history, that involves inserting "tombstones" into moz_deleted_formhistory for any deleted GUID. From reading the code, it seems that we do that correctly.

To double-check this as you're doing manual testing, please ensure that the deleted elements are being read by sync correctly, here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FormHistoryRepositorySession.java?q=FormHistoryRepositorySession.java&redirect_type=direct#155

Comment 3

[:Grisha Kruglov]

I realize now that we're creating a sync-powered footgun with these APIs.

Consider what happens in case of mass-deletions of data in presence of Sync - that deletion is propagated to other devices.

In case of "Clear private data" mass-delete functionality that Fennec currently exposes, we explicitly do not store tombstones of deleted records, to avoid unexpectedly deleting private data in user's entire sync constellation of devices when they really just meant to clear one device. See Bug 756701 for some related discussion. This behaviour has obvious flaws - deleted records may return if other clients upload them, if current client performs a full sync, etc.

WebExtension case is tricky, because neither the extension nor the user may realize the impact of their actions in case of trying to do something like "remove all form history since=0" when sync is present, and the browser will not have a chance to warn/educate the user either.

Proposed behaviour (sync deletions) _is_ in line with what we're already doing elsewhere (desktop), and generally this topic feels more like a product decision minefield than an engineering problem. In absence of other input, I'm in favor of at least making our footgun a consistent one, given that we've already built one part of it for desktop - that is, proceed with the current approach.

Comment 4

[Matthew Wein [:mattw]]

Comment on attachment 8879319 [details]
Bug 1363004 - Implement browsingData.removeFormData webExtension API method on android.

Moving review to Shane because I have a lot on my plate right now - sorry for not doing this earlier.

Comment 5

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8879319 [details]
Bug 1363004 - Implement browsingData.removeFormData webExtension API method on android.

https://reviewboard.mozilla.org/r/150606/#review161278

r+ from me on the WebExtensions pieces, but please don't forget to impement the generic `remove` method at some point.

::: mobile/android/components/extensions/ext-browsingData.js:60
(Diff revision 1)
>            }
>  
>            return Promise.resolve({options, dataToRemove, dataRemovalPermitted});
>          },
> +
> +        async removeFormData(options) {

I should have mentioned this on the review I did of the downloads one as well, but is your intention to implement the generic `remove` method in a separate bug?

Comment 6

[Tushar Saini (:shatur)]

(In reply to Bob Silverberg [:bsilverberg] from comment #5)
> I should have mentioned this on the review I did of the downloads one as
> well, but is your intention to implement the generic `remove` method in a
> separate bug?

Yes, I intend to implement remove() separately. Here is the bug 1363014.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8879319 [details]
Bug 1363004 - Implement browsingData.removeFormData webExtension API method on android.

I don't think a third r+ here is necessary

Comment 8

[Tushar Saini (:shatur)]

Comment on attachment 8879319 [details]
Bug 1363004 - Implement browsingData.removeFormData webExtension API method on android.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b98aee32911
Implement browsingData.removeFormData webExtension API method on android. r=bsilverberg,Grisha

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/9b98aee32911

Comment 11

[CosminB]

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

Thanks!