756701 - Clearing Private Data on mobile has unintended consequences when Sync is enabled

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Allison Naaktgeboren :ally]

Exposition:
- discussion was originally on an email thread, irc & mobile-drivers triage meeting. General consensus is that this should block, possibly betaN

- Damon found the following review on the beta(1) in Google Play store "Madsen on May 16, 2012 (Version 14.0)
Much better than previous versions but...

All my saved passwords (saved with Firefox Sync) disappeared after I installed this, and thanks to Sync they're now gone on ALL computers. Fortunately I had the foresight to back up my profile dir on Linux before starting up Firefox when I saw all passwords were gone on my Windows and Mac boxes. This is really, really annoying. But apart from that, it's great. Fast start-up, fast rendering, good tab-handling/navigation and generally nice to navigate."

- We believe that the user hit "Clear Private Data" triggering the removal of all logins/passwords, which we believe caused the passwords to be marked deleted. Sync trusts the db to be the source of truth and deleted all passwords marked 'deleted' on all other devices, which is what Sync is expected to do.

- On desktop, clearing _all_ passwords is a local action only, and does not propagate. Same with all other data stores.


Options:
- On clear private data, locally wipe passwords, do not move them to deleted table. (This is what I believe to be the best option & mirrors desktop behavior)
- On clear private data, mark passwords as 'absent' not 'deleted'
- Wire sync to never sync deleted passwords. 

Path forward:
- options need investigation & sync & mobile should come to an agreement about which option should be implemented.

Comment 1

[Richard Newman [:rnewman]]

I'm not sure what it would mean to mark a password as "absent" in Gecko's database. And that doesn't seem to be any different from just truncation of the table, given that Sync then shouldn't regard the rows as either existing or deleted, so I would suggest option 1.

Comment 2

[Nick Alexander :nalexander [he/him]]

Option 1 is the right approach in my opinion.

If you had asked me an hour ago, I would have said, "Of course Fennec purges the PW table rather than moving the PW table entries to the deleted PW table."  Now I'm worried that Fennec is also moving the Form History entries to the deleted Form History table rather than purging!

Option 2 (ignore this record for sync) is a non-starter.

Option 3 (don't sync deleted passwords) seems asymmetric: either the system works a certain way, or it doesn't.  Why treat clearing passwords any differently than clearing form history or bookmarks?

Comment 3

[Wesley Johnston (:wesj)]

Form History does do that. I'm not sure what you guys want. The changes to undo that are extremely simple, but Sync will likely just add the entries back next time it runs. I'm not sure how this is honoring what the user wants.

Maybe we're better to remove the entry but leave its last updated time and guid in the table so that its effectively "deleted", but sync doesn't reinsert it?

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Wesley Johnston (:wesj) from comment #3)
> Form History does do that. I'm not sure what you guys want. The changes to
> undo that are extremely simple, but Sync will likely just add the entries
> back next time it runs. I'm not sure how this is honoring what the user
> wants.

Ah, I think I see why the current implementation moves things to the deleted table.

Sync maintains a timestamp of when it last fetched things; on each sync, we ask the server for things modified after that timestamp and then bump the timestamp forward.  If Fennec clears the passwords DB, old records will not be re-downloaded because the timestamp will be after the old record modification times.  (Unless the user takes some other action to reset the timestamp.)

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

So to summarize, 
* Fennec should really delete Form History and Passwords from the local DB
* Sync will not just re-add the Form History and Passwords because that data has not really changed

The only place Fennec user can delete Form History and Passwords ins in Clear Private Data, which we consider a "purge" and therefore should be local DB only.

If Fennec someday allows deleting individual passwords via a UI, we will need to consider if this action is a "purge" (and therefore local only) or a syncable action.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #3)
> Form History does do that. I'm not sure what you guys want. The changes to
> undo that are extremely simple, but Sync will likely just add the entries
> back next time it runs.

The only time Sync will re-add purged records is if something happens to cause it to perform a full sync again. At that point anything that wasn't marked as deleted on the server will be re-downloaded and re-inserted.

This is something of an issue, because it's not obvious to the user when this might occur. On the other hand, it's what happens right now. This might just be the time to start thinking about all this.

See also Bug 578694.

> I'm not sure how this is honoring what the user wants.

There is no clear answer here.

From one perspective, if I clear data on my phone, I don't want it hanging around on some server, maybe on my desktop, and reappearing if I uninstall Fennec and set it up again. Gone is gone, profile in the cloud, etc.

From another perspective -- and this is the previous behavior -- clearing a device is not the same thing as deleting your data in the abstract.

I suspect that these two perspectives correspond to two or more different kinds of users and user behavior, and I doubt there's a single size that fits all.

The point here, I think, is that we shouldn't be changing the user-facing behavior without taking pains to *design* the experience. Whether that means asking a user what they mean, or distinguishing between "your data" and "your device" somehow... I don't know.

> Maybe we're better to remove the entry but leave its last updated time and
> guid in the table so that its effectively "deleted", but sync doesn't
> reinsert it?

That's just a decision about what deletion merge logic should be. If you mark a record as deleted, but without bumping the modified time, then a re-sync will cause that record to be reconciled and either come back to life or be marked as deleted elsewhere.

It's more consistent to just drop the row locally.

(In reply to Mark Finkle (:mfinkle) from comment #5)

> If Fennec someday allows deleting individual passwords via a UI, we will
> need to consider if this action is a "purge" (and therefore local only) or a
> syncable action.

... and consider what the equivalent behavior is for desktop, too. The main goal here is consistency of experience.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Keeping this moving forward to a resolution, this are the code changes in question:
http://hg.mozilla.org/mozilla-central/rev/7a74c7738877

This patch creates the "deleted" logins table and transfers logins to the table when they are being removed individually (removeLogin) or being purged completely (removeAllLogins).

It seems that at the minimum, we should remove the code in removeAllLogins which stores deleted logins in the deleted table.

I have no idea what desktop Firefox and Sync do for removing individual logins. If desktop Firefox and Sync never propagate login deletions, then we should consider removing all of the support code that was added in the above patch.

Comment 8

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #7)

> It seems that at the minimum, we should remove the code in removeAllLogins
> which stores deleted logins in the deleted table.

If this is the only caller, then sure; killing

   2.129 +            let logins = this.getAllLogins();
   2.130 +            for each (let login in logins) {
   2.131 +                let [id, storedLogin] = this._getIdForLogin(login);
   2.132 +                this.storeDeletedLogin(storedLogin);
   2.133 +            }

would suffice.

I'd perhaps be inclined to have an argument that controls whether to store deleted logins, to avoid excessive future wrangling of toolkit code if a "propagating purge" comes into being. Seems wrong to not have the option, but that's up to dolske.

> I have no idea what desktop Firefox and Sync do for removing individual
> logins.

Desktop tracks and propagates individual login removals.

(It also 'tracks' *bulk* login removals, in that it bumps the Sync score, but doesn't otherwise do anything about it, which is stupid: it thinks the removal is important enough to cause a sync to occur, but not important enough to do anything with the data. This is some indicator that the current desktop behavior is not necessarily *designed* per se...)

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

> I'd perhaps be inclined to have an argument that controls whether to store
> deleted logins, to avoid excessive future wrangling of toolkit code if a
> "propagating purge" comes into being. Seems wrong to not have the option,
> but that's up to dolske.

Pulling in dolske...

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch

This just removes the "move to deleted table" step for removeAllEntries and removeAllLogins. Since that's they only way to clear in Fennec right now, its the only way things will be deleted. Cleary (my addon) will support clearing by timeframe, and those changes would propagate. I think that's probably what the user wants there.

This is simple, localized, and I think we could push forward to beta with very little risk.

In the longer term, maybe we could be better to add a "reason" column to the deleted tables and sync could query on it?

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Let's not sync _any_ password deletions. That would match desktop.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch

This just removes all of our deleted passwords table code. I'll write a separate patch to clean up the database. That can land on nightly, but this is lower risk for moving forward to beta.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Looks good to me. This should make Fennec have parity with desktop Firefox.

Comment 14

[Justin Dolske [:Dolske]]

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

Code churn. :(

I think we need to:

1) Figure out how CPD + Sync are intended to work together. Not sure if this was ever considered. EG, maybe CPD needs to have a checkbox for "do this for all my devices", or automatically unsync, etc.

2) Makes sense to nerf removeAllLogins() right now to eliminate the footgun.

3) We should arguably keep removeLogin() for an addon that might call it, but it doesn't matter much either way I guess. The thing that tilts me off the fence is that it would end up being a code path we might not have implemented/tested well, so... :(

Comment 15

[Wesley Johnston (:wesj)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 718817
User impact if declined: Clear Private data can result in data loss on desktop
Testing completed (on m-c, etc.): Landed on inbound 5/21
Risk to taking this patch (and alternatives if risky): Faily low risk. Affects desktop code that was ifdef to do nothing on desktop.
String or UUID changes made by this patch: none.

Comment 16

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4110f744518b

Comment 17

[Richard Newman [:rnewman]]

I think this patch falls between two stools. We should either:

* Completely remove everything to do with deletion that isn't already used, including the deleted items table, and perhaps removeAllLogins and removeLogin, and just abandon any idea that we'll allow users or addons to delete logins, or

* Fix the original bug, removing just the four lines from removeAllLogins, but keeping tracking for individual deletions.

With this patch we've got a table that code never modifies, and functions that remove logins in a way that Sync won't detect. That seems wrong to me.

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4110f744518b

Leaving open for comment 17.

Comment 19

[Mike Connor [:mconnor]]

I don't believe the patch is correct, as now we do not match desktop (or XUL's) behaviour for deleting individual passwords.

Mark in comment 11 states "Let's not sync _any_ password deletions. That would match desktop."

But Richard in comment 8 stated: "Desktop tracks and propagates individual login removals."

I believe comment 8 is correct, and the first patch in the bug should be landed instead of the newer one.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

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

This patch seems to be the "parity with desktop" patch. It stops the primary issue, which is propagating the removal of ALL passwords and form history.

Wes - Can you do the required backout/landing ?

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

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

And we need dolske to review this too.

Comment 22

[Justin Dolske [:Dolske]]

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

r+ with one small request: can you file a followup bug for making these work (assuming we end up deciding that they should), and for each of the 2 chunks of code you're removing in this patch add a "// XXX bug 1234, should call storeDeletedLogin / moveToDeletedTable here".


Also, two tiny things just to note for the future:

1) Desktop's search box uses removeEntriesForName() when you "Clear search History" via context menu. Mobile doesn't seen to have that call anywhere; if it's ever added that might result in clearing on all devices. Not really a big deal, IMO.

2) Desktop's password manager UI (ie, Prefs -> Security -> Show Passwords) has a "Remove All" button. It doesn't actually call removeAllLogins(), though, it just loops through removeLogin() repeatedly (because search filter). If mobile ever copies this UI, we'll have to ponder what's supposed to happen across devices.

Comment 23

[Wesley Johnston (:wesj)]

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/94f43049073b
new patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/1023c5d0cd78

Comment 24

[Wesley Johnston (:wesj)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 718817
User impact if declined: Clear Private data can result in data loss
Testing completed (on m-c, etc.): Relanded on inbound 5/22
Risk to taking this patch (and alternatives if risky): Faily low risk. Does affects desktop code but is #ifdef to do nothing on desktop, so mobile only for the most part.
String or UUID changes made by this patch: none.

Comment 25

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/420d9a20075d

Comment 26

[JP Rosevear [:jpr]]

Fixed on 14.

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/94f43049073b

Comment 28

[Adrian Tamas (:AdrianT)]

I am unable to reproduce the issue using Ubuntu Linux and Firefox 14.0b3 build 2, Aurora 14.0a2 2012-05-24 and Nightly 15.0a1 2012-05-24 on HTC Desire Z running Android 2.3.3. 

Usernames and passwords are available on Desktop after clearing private data on the device.