386005 - passwords deleted from drop down menu in gmail.com apear to be deleted but are still saved

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Hussam Al-Tayeb]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a6pre) Gecko/20070627 Firefox/3.0a6pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a6pre) Gecko/20070627 Firefox/3.0a6pre

when I try to delete a password using Shift+delete from the drop down menu ( for example gmail.com ), it appears to be deleted but the password is still saved. To correctly delete the password, I have to go to preferences > password manager and delete it from there.

Reproducible: Always

Comment 1

[Hussam Al-Tayeb]

the following error appears in error console when I try to delete a password from drop down menu in gmail.com

Error: [Exception... "'No logins found for hostname (null)' when calling method: [nsILoginManagerStorage::removeLogin]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///usr/lib/firefox/components/nsLoginManager.js :: anonymous :: line 425"  data: no]
Source File: file:///usr/lib/firefox/components/nsLoginManager.js
Line: 425

Comment 2

[Hussam Al-Tayeb]

Can someone else reproduce/confirm this problem or am I the only one who sees this bug?

Comment 3

[Marco Bonardo [:mak]]

i can confirm this behaviour on Trunk, probably due to the new Password Manager
Mozilla/5.0 (Windows; U; Windows NT 6.0; it; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4

See also Bug 381727 on password removing from preferences

confirming and moving to password manager

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1

This had me confused for a while, until I finally checked the docs and found that .splice() returns an array (which makes complete sense in hindsight).

I also noticed that the login sorting wasn't sorting on the correct values, so I fixed that here too.

Comment 5

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

Comment on attachment 276890 [details] [diff] [review]
Patch for review, v.1

>Index: toolkit/components/passwordmgr/src/nsLoginManager.js

>     function loginSort(a,b) {
>         var userA = a.username.toLowerCase();
>         var userB = b.username.toLowerCase();

>-        if (a < b)
>+        if (userA < userB)

>-        if (b > a)
>+        if (userB > userA)

Gah, who reviewed this stuff? :)

Comment 6

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

(Can we write a test for this?)

Comment 7

[Justin Dolske [:Dolske]]

(Yes, I was just filling your review queue with requests now, and writing tests for them next :)

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Patch for testcases, v.1

This adds some tests for the autocomplete dropdown. I wasn't able to get filtering working (ie, type "t", down arrow, and only see logins starting with "t"). But it works for me doing it manually, so there's probably some subtle key event thing I don't understand... But it tests the problem in this bug, so it's good enough.

Comment 9

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

Comment on attachment 277648 [details] [diff] [review]
Patch for testcases, v.1

I can't run these tests to verify, because the countLogins patch doesn't apply. I think it'd be a good idea to wrap the addLogin calls in a try/catch (and check the exception) to avoid needing to restart the browser to re-run the test. r=me assuming they all pass.

Comment 10

[Justin Dolske [:Dolske]]

Checked in.

Checking in toolkit/components/passwordmgr/src/nsLoginManager.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js,v  <--  nsLoginManager.js
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/components/passwordmgr/test/Makefile.in;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/Makefile.in,v  <--  Makefile.in
new revision: 1.8; previous revision: 1.7
done
Checking in toolkit/components/passwordmgr/test/test_0init.html;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/test_0init.html,v  <--  test_0init.html
new revision: 1.6; previous revision: 1.5
done
RCS file: /cvsroot/mozilla/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html,v
done
Checking in toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html,v  <--  test_basic_form_autocomplete.html
initial revision: 1.1
done

Comment 11

[Hussam Al-Tayeb]

Verified fixed.

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Patch to fix testcase.

The testcase checked in fails on Linux/Windows. This changes the test to delete logins with the "delete" key instead of the "back_space" key. Tested on OS X and Linux, and I manually confirmed the keyboard behavior on Windows.

Comment 13

[Justin Dolske [:Dolske]]

Checked in.

Checking in toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html,v  <--  test_basic_form_autocomplete.html
new revision: 1.3; previous revision: 1.2
done