834000 - Removing passwords in Firefox/Thunderbird 18.* always randomly reorders list of passwords in password manager

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

This issue is new in Thunderbird 19.0b1. Whenever you want to remove a password from within the password manager the list of passwords is getting reordered in a weird way. If you need a screencast I might be able to create one in the next days. 

Steps:
1. Save a couple of passwords by adding new Mail accounts, or LDAP servers
2. Open the password manager and delete an previously added password

With step 2 repeating the list gets reordered.

Comment 1

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Actually this also happens with the current Firefox 19.0b version. Marking as blocker.

Comment 2

[Matthew N. [:MattN]]

Are you sure this is new? This sounds like an old bug: bug 211352

Comment 3

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Thanks for the hint, Matthew! I have checked the other bug but that's a different use case. I also did some more checks and can now confirm that the problem has been started between 17.0 and 18.0. Same as what Christoph on bug 211352 comment 7 stated. I still think we have to handle this case separately because it's a regression.

Comment 4

[Loic]

Confirmed. Regression range:

m-c
good=2012-09-26
bad=2012-09-27
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ca4af4af5334&tochange=b038e9e2023f

Suspected bug:
Andres Hernandez — Bug 395681 - Password manager dialogs observe topics that are never fired r=MattN

Comment 5

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Thanks Loic! Looks like a cross-platform issue then (also I haven't tested yet)

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

While this may be a regression it seems to cause only a minor annoyance so we wouldn't hold a release for this but would consider a safe uplift to branches when one was available.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Andres, are you able to look into this?

Comment 8

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Any idea why bug 395681 caused this?

Comment 10

[Andres Hernandez [:andreshm]]

On bug 395681 we fixed the observers, so now that the observers are being fired the password list is being reloaded, but the following condition was reversing the list. 

> if (lastSignonSortColumn == "hostname") {
>   lastSignonSortAscending = !lastSignonSortAscending; // prevents sort from being reversed
> }

I think because |LoadSignons| is already handling that: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/content/passwordManager.js#71

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 708709 [details] [diff] [review]
Patch v1

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

A simple test would be good.

::: toolkit/components/passwordmgr/content/passwordManagerCommon.js
@@ -57,5 @@
>              return;
>            }
>            signons.length = 0;
> -          if (lastSignonSortColumn == "hostname") {
> -            lastSignonSortAscending = !lastSignonSortAscending; // prevents sort from being reversed

This seemed kind of weird when I reviewed bug 395681.

Comment 12

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Improved the observers test to check the logins order.

Comment 13

[Andres Hernandez [:andreshm]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1b1247070eaf

Comment 14

[Andres Hernandez [:andreshm]]

All green on try: https://tbpl.mozilla.org/?tree=Try&rev=1b1247070eaf

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbeec0d0afd7

Andres, for future reference, commit messages that summarize what the patch is actually doing (instead of what it's fixing) are appreciated. Thanks!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fbeec0d0afd7

Comment 17

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:21.0) Gecko/20130205 Firefox/21.0 ID:20130205031033

Andres, will you take care of the necessary backports? Thanks.

Comment 18

[Andres Hernandez [:andreshm]]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:21.0)
> Gecko/20130205 Firefox/21.0 ID:20130205031033
> 
> Andres, will you take care of the necessary backports? Thanks. 

I don't think I have access to push the patch to the older versions.

Comment 19

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

You will have to request for approval of the patch first and make sure it can be applied on the other branches. The flags can be found in the details view of the attachment.

Comment 20

[Andres Hernandez [:andreshm]]

Comment on attachment 709103 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 395681
User impact if declined: Reverse list of passwords when adding/removing a password
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.

I built a local version of Aurora and Beta and this patch is compatible with both.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 709103 [details] [diff] [review]
Patch v2

At this stage we're too late to take it in the final Beta 19 so approving for Aurora and we'll ship the fix in 20.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c89bc433bcb0