1363012 - Implement browsingData.removePasswords WebExtension API method on android

Status: RESOLVED WONTFIX

(WebExtensions :: Android, enhancement, P5)

Description

[Tushar Saini (:shatur)]

This method clears saved passwords. The doc can be found at [1].

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

Comment 1

[Tushar Saini (:shatur)]

Attachment: Bug 1363012 - Implement browsingData.removePasswords WebExtension API method on android.

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

Comment 2

[:Grisha Kruglov]

Comment on attachment 8879759 [details]
Bug 1363012 - Implement browsingData.removePasswords WebExtension API method on android.

https://reviewboard.mozilla.org/r/151114/#review157348

r-, because I think this should be done in a safer fashion.

::: mobile/android/components/extensions/ext-browsingData.js:14
(Diff revision 1)
>  XPCOMUtils.defineLazyModuleGetter(this, "SharedPreferences",
>                                    "resource://gre/modules/SharedPreferences.jsm");
>  
> +const YIELD_PERIOD = 10;
> +
> +let clearPasswords = async function(options) {

Why are the implementions in your different PRs spread amongst this file and Sanitizer.jsm? That seems wrong to me. Is it possible to consolidate code that actually does the work in one location?

Here especially, since there's already "clear passwords" method in Sanitizer.jsm.

::: mobile/android/components/extensions/ext-browsingData.js:21
(Diff revision 1)
> +  let yieldCounter = 0;
> +
> +  if (options.since) {
> +    // Iterate through the logins and delete any updated after our cutoff.
> +    let logins = loginManager.getAllLogins();
> +    for (let login of logins) {

We're backed by SQL, and so you could be doing this in a safer way, as a single transaction, especially considering the fact that the underlying API will also need to store deletions in a separate table.

Unfortunately, our current APIs don't let you perfrom this safely:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/storage-mozStorage.js#283

While we want to move ownership of these databases to Android side, meaning that at some point this code will hopefully die, it shouldn't be too much effort to provide a transactional API for these compound operations in the meantime.

We have two storage implementations for login information, which share the same interface: storage-mozStorage (SQL-backed, used by Android), and storage-json (json-backed, used by desktop).

The two implementations have obviously different semantics and consistency guarantees. Change to nsILoginManager propsed in 1332611 will allow us to actually do this safely.

Comment 3

[Matthew Wein [:mattw]]

Comment on attachment 8879759 [details]
Bug 1363012 - Implement browsingData.removePasswords 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 4

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8879759 [details]
Bug 1363012 - Implement browsingData.removePasswords WebExtension API method on android.

https://reviewboard.mozilla.org/r/151114/#review161296

Nice work, Tushar. I'm fine with the WebExtensions code, except for the issue about the implementation of `clearPasswords`.

::: mobile/android/components/extensions/ext-browsingData.js:14
(Diff revision 1)
>  XPCOMUtils.defineLazyModuleGetter(this, "SharedPreferences",
>                                    "resource://gre/modules/SharedPreferences.jsm");
>  
> +const YIELD_PERIOD = 10;
> +
> +let clearPasswords = async function(options) {

I agree.

Comment 5

[Caitlin Neiman [:caitmuenster]]

Hey Sebastian, apologies for the late ping. Could you advise next steps for this?

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

> Hey Sebastian, apologies for the late ping. Could you advise next steps for this?

Oh, good question.

I just un-assigned me as mentor. Back then I was mentoring a Google Summer of Code student who was working on adding WebExtension APIs to Firefox for Android.

I think there are no clear steps forward. Given that Fennec is in "maintenance mode" I'd recommend WONTFIX-ing this bug.

Comment 7

[Caitlin Neiman [:caitmuenster]]

Woks for me! Thanks, Sebastian.