718817 - Create a deleted passwords table for deleted passwords

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Wesley Johnston (:wesj)]

Sync needs a way to track when a password is deleted by Gecko. One solution is to create a new table with the deleted entries in it. Sync can query that to find out what's been removed. In it, we'll just store the guid and deletionTime of the deleted logins.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

Patch to add this functionality to the login manager storage component, ifdef'd for Android. I increased the revision number of the db in order to do that. Hope that's ok?

I'll get someone from mobile to review the java parts once I've settled them a bit more (still dealing with the proper way to fail when Gecko isn't loaded, but that patch is near ready as well).

Comment 2

[Wesley Johnston (:wesj)]

I should/could/will probably write some xpcshell based tests for this too I think.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

This is pretty much the same. Java pieces just moved over to but 704682

Comment 4

[Richard Newman [:rnewman]]

Marking as a dependency for our repository layer.

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 590932 [details] [diff] [review]
Patch

Review of attachment 590932 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly just some minor changes to make...

::: toolkit/components/passwordmgr/Makefile.in
@@ +63,5 @@
>    passwordmgr.manifest \
>    storage-Legacy.js \
> +  $(NULL)
> +
> +EXTRA_PP_COMPONENTS = \

What's this for?

::: toolkit/components/passwordmgr/storage-mozStorage.js
@@ +50,2 @@
>  const DB_VERSION = 4; // The database schema version
> +#endif

Let's just go ahead and make (almost) all of this common to both desktop and mobile. The extra table won't be useful on desktop, but it won't hurt either.

You'll also need to update CURRENT_SCHEMA in toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_migrate.js.

...and add a test, but I'll just go ahead and whip that up for you because that's easier than explaining it. ;-)

@@ +156,5 @@
>  
>              moz_disabledHosts:  "id                 INTEGER PRIMARY KEY," +
>                                  "hostname           TEXT UNIQUE ON CONFLICT REPLACE",
> +
> +#ifdef ANDROID

Also no #ifdef here

@@ +159,5 @@
> +
> +#ifdef ANDROID
> +            moz_deleted_logins: "id                  INTEGER PRIMARY KEY," +
> +                                "guid                TEXT,"                +
> +                                "timeDeleted         INTEGER"

Nit: add trailing comma.

@@ +396,5 @@
>          }
>  
>          this._sendNotification("removeLogin", storedLogin);
> +
> +#ifdef ANDROID

Move this #ifdef'd block up a couple lines (ie, before the _sendNotification).

Then add a Transaction + .commit/.rollback [see _dbMigrate() for an example], to ensure  that we never get in a state where one of the operations fail after the other succeeded.

@@ +399,5 @@
> +
> +#ifdef ANDROID
> +        // on android we also add this entry to a database marked as deleted
> +        // so that sync can later query any deleted records
> +        storedLogin.QueryInterface(Ci.nsILoginMetaInfo);

Shouldn't need the QI here, storedLogin gets created in _searchLogins, which does that while setting it up.

@@ +719,5 @@
>       */
>      removeAllLogins : function () {
> +        let query;
> +        let stmt;
> +#ifdef ANDROID

You'll be wanting a Transaction here too.

@@ +733,5 @@
> +
> +            let now = Date.now();
> +            let params = stmt.newBindingParamsArray();  
> +            for each (let login in logins) {
> +              login.QueryInterface(Ci.nsILoginMetaInfo);

Shouldn't need this QI either.

@@ +740,5 @@
> +              bp.bindByName("timeDeleted", now);
> +              params.addParams(bp);  
> +            }  
> +            stmt.bindParameters(params);
> +            stmt.executeAsync(null);

Hmm, mixing async and sync stuff can potentially cause problems, aiui. Just make this .execute(), the transaction should make it fast enough for the rare case it's used.

@@ +1629,5 @@
> +     *
> +     * Version 5 adds the moz_deleted_logins table
> +     */
> +    _dbMigrateToVersion5 : function () {
> +        this._dbConnection.createTable("moz_deleted_logins", this._dbSchema.tables.moz_deleted_logins);

Use  ._dbConnection.tableExists() to see if it exists before creating it.

Why, you might ask?

Because if an older pwmgr is run against a DB, it will set the schema version to the older value. Next time the new pwmgr touches the DB, it will see the old schema and remigrate.

This isn't particularly useful for V5 (other than making it not explode), but it is for other versions where they need to check to see what the older pwmgr might have added to the DB to update those rows. This makes the DB both forwards and backwards compatible.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: zomg tests

Added xpcshell upgrade/downgrade tests.

Should work when you fix your patch with previous review comments (probably, I just did a quick'n'dirty hack to test that it wasn't stupidly broken).

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch

Addresses comments. I think I need the EXTRA_PP_COMPONENTS section in the Makefile.in in order to preprocess this file, right?

Wrote some simple tests too. Building Firefox to tset them, but will mark them Android only.

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 593910 [details] [diff] [review]
Patch

Review of attachment 593910 [details] [diff] [review]:
-----------------------------------------------------------------

Still need to tweak the transactions and commented.

::: toolkit/components/passwordmgr/storage-mozStorage.js
@@ +387,5 @@
>              if (stmt) {
>                  stmt.reset();
>              }
>          }
> +#ifdef ANDROID

I think you still need a transaction here? If the code above removes the login from moz_logins but the code to add the entry to moz_deleted_logins fails, you end up with a deleted login without sync being able to know about. What happens then?

With a transaction, you'd rollback when the moz_deleted_logins failure happens, and this the login would still exist in moz_logins. (And thus the caller/user can retry later.)
 
Since |transaction| then needs to be used outside your #ifdefs, I guess you might as well move the transaction creation outside as well (instead of doing |if (transaction) transaction.commit()|). The transaction's not doing anything useful on non-Android, but it shouldn't hurt either.

@@ +740,5 @@
> +        } finally {
> +            if (stmt) {
> +                stmt.reset();
> +            }
> +            transaction.commit();

Similarly for the moving the transaction's comment here... If the "DELETE FROM moz_logins" operation fails after the guids have been added to moz_deleted_logins, things are inconsistent. [I suppose it might all work out: sync might propagate the deletes, and then later re-add them all when it unexpectedly discovers them as new logins? Wouldn't want to rely on that, though.]

Oh, lastly... If you want I think you could just move both blocks of code to a common storeDeletedGUIDs() function. removeLogin would just call it as foo([oneLogin]). You could just just #ifdef the function's body, and have the transaction and stuff in removeLogin/removeAllLogins be entirely common to desktop and mobile. Seems like that might be safer too!

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Patches again. Pulled the new db stuff into its own method. Call it from both remove and removeAll. Wrap both in transactions.

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 594287 [details] [diff] [review]
Patch v3

Review of attachment 594287 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the tweaks to where .commit() is called.

Also, don't forget to land the tests. :)

::: toolkit/components/passwordmgr/storage-mozStorage.js
@@ +389,5 @@
>          } finally {
>              if (stmt) {
>                  stmt.reset();
>              }
> +            transaction.commit();

.commit() should go in the try block, otherwise you'll try to commit after the rollback. :) (Which, I hope, is just an error)

@@ +692,5 @@
> +     * Moves a login to the deleted logins table
> +     *
> +     */
> +     storeDeletedLogin : function(aLogin) {
> +#ifdef ANDROID

Oh, you got rid of the newBindingParamsArray() and just call this in a loop instead? I think that's ok, but I'd suggest asking mak if there's any performance benefit (+/-) to using newBindingParamsArray().

It could be a tradeoff anyway... The singular (removeLogin) case is most common, but newBindingParamsArray() with just [login1] might be extra overhead. The multiple case (removeAllLogins) is rare, but might have the most to gain from newBindingParamsArray with [login1, login2, logi3, ...].

Either way is fine, I think.

@@ +737,2 @@
>              stmt = this._dbCreateStatement(query);
>              stmt.execute();

Call .commit() for the transaction here.

Comment 11

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fa903fa8887
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a74c7738877

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7a74c7738877
https://hg.mozilla.org/mozilla-central/rev/8fa903fa8887