Closed Bug 718817 Opened 13 years ago Closed 13 years ago

Create a deleted passwords table for deleted passwords


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




Firefox 13
Tracking Status
fennec 11+ ---


(Reporter: wesj, Assigned: wesj)




(2 files, 3 obsolete files)

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.
Attached patch Patch (obsolete) — Splinter Review
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).
Attachment #589374 - Flags: review?(dolske)
I should/could/will probably write some xpcshell based tests for this too I think.
Attached patch Patch (obsolete) — Splinter Review
This is pretty much the same. Java pieces just moved over to but 704682
Attachment #589374 - Attachment is obsolete: true
Attachment #589374 - Flags: review?(dolske)
Attachment #590932 - Flags: review?(dolske)
tracking-fennec: --- → 11+
Priority: -- → P1
Marking as a dependency for our repository layer.
Blocks: 709385
Assignee: nobody → wjohnston
Comment on attachment 590932 [details] [diff] [review]

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

Mostly just some minor changes to make...

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

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 =;
> +            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.
Attachment #590932 - Flags: review?(dolske) → review-
Attached patch zomg testsSplinter Review
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).
Attached patch Patch (obsolete) — Splinter Review
Addresses comments. I think I need the EXTRA_PP_COMPONENTS section in the in order to preprocess this file, right?

Wrote some simple tests too. Building Firefox to tset them, but will mark them Android only.
Attachment #590932 - Attachment is obsolete: true
Attachment #593910 - Flags: review?(dolske)
Comment on attachment 593910 [details] [diff] [review]

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!
Attachment #593910 - Flags: review?(dolske) → review-
Attached patch Patch v3Splinter Review
Patches again. Pulled the new db stuff into its own method. Call it from both remove and removeAll. Wrap both in transactions.
Attachment #593910 - Attachment is obsolete: true
Attachment #594287 - Flags: review?(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.
Attachment #594287 - Flags: review?(dolske) → review+
Blocks: 704682
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.