1670678 - "removeAllLogins" listener in "passwords.js" does not behave as expected

Status: RESOLVED WORKSFORME

(Firefox :: Sync, defect, P3)

Description

[Tim Giles [:tgiles]]

I'm currently working on Bug 1613620, which will allow a Firefox user to delete all their logins, and I'm running into issues with password syncing. Adding and removing passwords works as expected, but removing all passwords does not behave as expected.

When removing all logins at once, via the removeAllLogins listener, all of the logins are deleted on the local profile but no changes are created for the Sync engine. Because of this, when I sync on my non-deleted logins account, no logins are updated. The removeAllLogins listener in passwords.js is correctly listening and reacting to the removeAllLogins event, but there are no outgoing records generated for syncing.

Currently there is no way to delete all logins in about:logins, but my patch for Bug 1613620 has a way to remove all logins to test the removeAllLogins listener in passwords.js

STR:

1. Apply patch D91198
2. Ensure you have a Firefox account with passwords sync enabled
3. Ensure you have some logins available for the previous Firefox account
4. Log into your Firefox account on two different Firefox instances, one of them being the local build of patch D91198
5. On the local build instance, navigate to about:logins and look for the meatball menu on the top right of the screen and select the "Remove All Logins" menu item
6. Confirm that you want to delete all logins in the dialog and select "Remove All" button
7. After logins are deleted, go to your other Firefox instance and trigger "Sync Now"
8. Logins on other Firefox instance are not deleted

Comment 1

[Mark Hammond [:markh] [:mhammond]]

I don't know the history of this notification, but it appears as though the sync code expects a removeLogin notification for each login before the removeAllLogins - this is because the removeAllLogins observer doesn't "track" anything. So I think there are 2 options:

• One is to do the above (ie, send a removeLogin notification for each item being removed, then at the end the removeAllLogins
• Ensure that this notification can discover details about all the deleted items and "track" then - this could be either by sending the notification before the delete (so that code could then enumerate all the logins and ensure they are marked as deleted), or by sending the entire list of logins as subject, much like is done for the other notification (but in this case there'd be an array of logins rather than a single login).

Comment 2

[Tim Giles [:tgiles]]

My gut says option two would be the way to go. I'd hate to send x "deleted login" messages back and forth for each login that is in the user's profile, seems like a performance concern to me. I can add the entire deleted logins list as subject as part of my removeAllLogins function so that the removeAllLogins observer over in passwords.js can handle the new subject data accordingly.

I believe I could handle the implementation, as part of Bug 1613620, for updating the removeAllLogins observer, as long as you, or someone you can point me to, is willing to do a review. It seems like the solution to the second option would be the same as the "removeLogin" case except that I need to iterate over the subject and track each of the logins in the subject array.

Let me know if I'm overlooking and/or overstepping my boundaries, just trying to keep my work moving!

Comment 3

[Mark Hammond [:markh] [:mhammond]]

That sounds great - happy to review. We'll also want a test, but that should be relatively easy - probably in test_password_tracker.js.

Comment 4

[Tim Giles [:tgiles]]

I've fixed this issue as part of Bug 1613620, so I believe we can go ahead and resolve this issue!

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Tim Giles [:tgiles] from comment #4)

I've fixed this issue as part of Bug 1613620,

👍