Closed
Bug 834000
Opened 12 years ago
Closed 12 years ago
Removing passwords in Firefox/Thunderbird 18.* always randomly reorders list of passwords in password manager
Categories
(Toolkit :: Password Manager, defect)
Tracking
()
VERIFIED
FIXED
mozilla21
People
(Reporter: whimboo, Assigned: andreshm)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
6.55 KB,
patch
|
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
Actually this also happens with the current Firefox 19.0b version. Marking as blocker.
tracking-firefox19:
--- → ?
Reporter | ||
Updated•12 years ago
|
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
Reporter | ||
Updated•12 years ago
|
Version: 10 Branch → 19 Branch
Comment 2•12 years ago
|
||
Are you sure this is new? This sounds like an old bug: bug 211352
Reporter | ||
Comment 3•12 years ago
|
||
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
Reporter | ||
Updated•12 years ago
|
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
Blocks: 395681
status-firefox18:
--- → affected
tracking-firefox20:
--- → ?
tracking-firefox21:
--- → ?
Keywords: regressionwindow-wanted
Reporter | ||
Comment 5•12 years ago
|
||
Thanks Loic! Looks like a cross-platform issue then (also I haven't tested yet)
OS: Mac OS X → All
Hardware: x86 → All
Comment 6•12 years ago
|
||
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.
status-firefox19:
--- → affected
status-firefox20:
--- → affected
status-firefox21:
--- → affected
tracking-firefox19:
? → ---
tracking-firefox20:
? → ---
tracking-firefox21:
? → ---
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #708709 -
Flags: review?(gavin.sharp)
Comment 9•12 years ago
|
||
Any idea why bug 395681 caused this?
Assignee | ||
Comment 10•12 years ago
|
||
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
Updated•12 years ago
|
Attachment #708709 -
Flags: review?(gavin.sharp) → review?(mnoorenberghe+bmo)
Comment 11•12 years ago
|
||
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+
Assignee | ||
Comment 12•12 years ago
|
||
Improved the observers test to check the logins order.
Attachment #708709 -
Attachment is obsolete: true
Assignee | ||
Comment 13•12 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1b1247070eaf
Assignee | ||
Comment 14•12 years ago
|
||
All green on try: https://tbpl.mozilla.org/?tree=Try&rev=1b1247070eaf
Keywords: checkin-needed
Comment 15•12 years ago
|
||
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
Comment 16•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Updated•12 years ago
|
Reporter | ||
Comment 17•12 years ago
|
||
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.
Reporter | ||
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 18•12 years ago
|
||
(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.
Reporter | ||
Comment 19•12 years ago
|
||
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.
Assignee | ||
Comment 20•12 years ago
|
||
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 21•12 years ago
|
||
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+
Comment 22•12 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•