Closed Bug 834000 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Password Manager, defect)

18 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla21
Tracking Status
firefox18 --- wontfix
firefox19 --- wontfix
firefox20 --- fixed
firefox21 --- verified

People

(Reporter: whimboo, Assigned: andreshm)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Actually this also happens with the current Firefox 19.0b version. Marking as blocker.
Summary: Removing passwords in Thunderbird 19.0b1 reorders list of passwords in password manager → Removing passwords in Firefox/Thunderbird 19.0b1 reorders list of passwords in password manager
Version: 10 Branch → 19 Branch
Are you sure this is new? This sounds like an old bug: bug 211352
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.
Version: 19 Branch → 18 Branch
Summary: Removing passwords in Firefox/Thunderbird 19.0b1 reorders list of passwords in password manager → Removing passwords in Firefox/Thunderbird 18.* always randomly reorders list of passwords in password manager
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
Thanks Loic! Looks like a cross-platform issue then (also I haven't tested yet)
OS: Mac OS X → All
Hardware: x86 → All
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.
Andres, are you able to look into this?
Assignee: nobody → andres
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #708709 - Flags: review?(gavin.sharp)
Any idea why bug 395681 caused this?
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
Attachment #708709 - Flags: review?(gavin.sharp) → review?(mnoorenberghe+bmo)
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.
Attachment #708709 - Flags: review?(mnoorenberghe+bmo) → review+
Attached patch Patch v2Splinter Review
Improved the observers test to check the logins order.
Attachment #708709 - Attachment is obsolete: true
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!
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fbeec0d0afd7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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.
Status: RESOLVED → VERIFIED
(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.
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 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.
Attachment #709103 - Flags: approval-mozilla-beta?
Attachment #709103 - Flags: approval-mozilla-aurora?
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.
Attachment #709103 - Flags: approval-mozilla-beta?
Attachment #709103 - Flags: approval-mozilla-beta-
Attachment #709103 - Flags: approval-mozilla-aurora?
Attachment #709103 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 848336
You need to log in before you can comment on or make changes to this bug.