343738 - deleting multiple stored passwords that are not in a row deletes only 1 password

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Christoph Voigt]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5.0.1

when selecting multiple stored passwords for deletion, they have to be in a row to get deleted properly. when they are not in a row and one/multiple passwords are in between the selected ones, only the first selected password gets deleted, all other marked passwords will stay

Reproducible: Always

Comment 1

[Christoph Voigt]

Attachment: Sketch of faulty deletion selection

When selecting these passwords, clicking delete will only delete the 1. password.

Comment 2

[Michael Wu [:mwu]]

Seems to work for me on FF2.0 beta 1. Mind checking?

Comment 3

[Michael Wu [:mwu]]

Please reopen if you can reproduce on branch or trunk. I can't.

Comment 4

[Phil Ringnalda (:philor)]

Really? I'm quite surprised you can't reproduce, since I can, on three platforms, and with slightly different visual effects, from today's trunk back as far as 0.8. Perhaps precise STR would help?

1. Go to attachment 170787 [details]
2. Enter "1" and "1" for the username and password, submit, tell Firefox to remember the password.
3. Back, enter "2" and "2", remember, back, then "3" and "3", then "4" and "4".
4. Open prefs, "Show passwords", click the "Username" column header to get them sorted 1-4
5. Click "1" to select it, then ctrl-(cmd-)click "3" to select it as well
6. Click "Remove"

In 2.0 and trunk, you'll be left with 2, 3, 4 showing, and 2, 3, 4 remembered. In 1.5 back at least as far as 0.8, you'll be left with 2 and 3 showing, as though you deleted 1 and 4 when you selected 1 and 3, but when you close and reopen the Show Passwords window, you'll see that only 1 was actually deleted.

I suspect, without having checked, that if you found a nightly from before 2003-08-08 02:42, when rev. 1.3 of toolkit/components/passwordmgr/resources/content/passwordManager.js landed, that you would find it worked then, since that change was from essentially what SeaMonkey uses for working multiple deletes to this day.

Comment 6

[Phil Ringnalda (:philor)]

Attachment: My approach

mconnor expressed some reservations about this, but I think there are several advantages (never mind the busy-looking diff, this is just a straight copy-paste of the function of the same name in suite/common/permissions/treeUtils.js):

- it works; since we've been broken since 2003, I think that's a big plus
- its more readable; I tried to puzzle out what was wrong with our version, but it's terse enough, and doing enough things at once, that I failed
- it avoids duplicating the code thirty lines down, in a slightly different way, to get the selection slightly differently
- having a single (copied) function is likely to be more resistant to tree API changes, since even a SeaMonkey-using API-changer will have to fix one copy of it

Comment 7

[Phil Ringnalda (:philor)]

Comment on attachment 251609 [details] [diff] [review]
My approach

Yeah, this still seems reasonable to me.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 251609 [details] [diff] [review]
My approach

Seems fine to me. what were mconnor's reservations though?

>Index: toolkit/components/passwordmgr/resources/content/passwordManagerCommon.js
>===================================================================
>   } else {
>-    removeBtn.setAttribute("disabled", "true");
>-    removeAllBtn.setAttribute("disabled", "true");
>+
>+    // disable buttons
>+    document.getElementById(removeButton).setAttribute("disabled", "true")
>+    document.getElementById(removeAllButton).setAttribute("disabled","true");
>+
>   }

empty lines are not _that_ cool.

r=mano.

Comment 9

[Phil Ringnalda (:philor)]

I think his reservations were from the way I phrased the question: "should we fix the way we're broken, even though we've been broken for years and nobody understands the code, or should we just steal SeaMonkey's function to do the same thing?"

toolkit/components/passwordmgr/resources/content/passwordManagerCommon.js: 1.2 (minus some of the more egregious blank lines)

I'm unsure what to do with in-testsuite - mano and dolske both seemed to think it's UI, suitable for Litmus if anything; I think maybe someone better than me with trees could maybe fake up one with some selection to pass through it, but anyone who plans on in-testsuite?ing better be prepared to hold my hand, character by character.

Comment 10

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a8pre) Gecko/2007083005 Minefield/3.0a8pre ID:2007083005 and created also a litmus testcase from this testcase from this bug.