Implement browsingData.removeFormData WebExtension API method on android

RESOLVED FIXED in Firefox 57

Status

enhancement
P5
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: shatur, Assigned: shatur, Mentored, NeedInfo)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [triaged])

Attachments

(1 attachment)

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
Blocks: 1362118
Priority: -- → P5
Whiteboard: [triaged]
Assignee: nobody → tushar.saini1285
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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
Attachment #8879319 - Flags: review?(gkruglov) → review+
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 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.
Attachment #8879319 - Flags: review?(mwein) → review?(mixedpuppy)
Attachment #8879319 - Flags: review?(bob.silverberg)
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?
Attachment #8879319 - Flags: review?(bob.silverberg) → review+
(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 on attachment 8879319 [details]
Bug 1363004 - Implement browsingData.removeFormData webExtension API method on android.

I don't think a third r+ here is necessary
Attachment #8879319 - Flags: review?(mixedpuppy)
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9b98aee32911
Implement browsingData.removeFormData webExtension API method on android. r=bsilverberg,Grisha
https://hg.mozilla.org/mozilla-central/rev/9b98aee32911
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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!
Flags: needinfo?(tushar.saini1285)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.