397799 - password manager infobar doesn't disappear after clicking remember in some cases

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P3)

Description

[Asa Dotzler [:asa]]

I visited a page that had an already stored login. the login was prefilled for me but the password manager infobar came down anyway. I logged in and then clicked "remember" on the password manager infobar. the bar didn't go away. 

Error: [Exception... "'This login already exists.' when calling method: [nsILoginManager::addLogin]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///C:/Program%20Files/Minefield/components/nsLoginManagerPrompter.js :: anonymous :: line 344"  data: no]
Source File: file:///C:/Program%20Files/Minefield/components/nsLoginManagerPrompter.js
Line: 344

Tested on today's nightly build on Mac Vista.

Comment 1

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

I see this bug on flyporter.com. Dolske mentioned a discrepancy between the code that prompts to save the password and the code that decides whether to fill in a form when I asked him about it on IRC.

Comment 2

[Justin Dolske [:Dolske]]

There are two issues here:

1) The notification bar could probably have a try/catch wrapped around the button actions so that errors can be gracefully noticed and the bar dismissed. Then again, I don't think we want to hide errors (by silently failing), or it will taken longer to identify bugs.

2) There's code in both addLogin() and _onFormSubmit() [and promptAuth()] dealing with avoiding duplicate logins, and of course they're out of sync now.

The .equals() and .equalsIgnorePassword() methods on nsILoginInfo didn't turn out to be as useful as they seemed, because "equals" can mean different things in different contexts. I'll take a look at all the login comparison code to see how best to handle this.

Comment 6

[Justin Dolske [:Dolske]]

407605 was blocking-firefox3+, so rerequesting here.

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Patch, v.1

The most likely cause of what was happening here is that the onFormSubmit() code was looking for an existing login, using .equals(). That function looks at form field names (which we don't otherwise use), and so if the current form has different field names, .equals() would return false and we'd display the notification bar. The checks in addLogin() ignored field names, and so adding the login would correctly fail as a duplicate.

This patch fixes that by changing the two methods on nsILoginInfo to .equals() and .matches(), and using the correct one as needed. The first is strict, and the latter loose.

Comment 8

[Justin Dolske [:Dolske]]

Checking in toolkit/components/passwordmgr/public/nsILoginInfo.idl;
/cvsroot/mozilla/toolkit/components/passwordmgr/public/nsILoginInfo.idl,v  <--  nsILoginInfo.idl
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/components/passwordmgr/src/nsLoginInfo.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginInfo.js,v  <--  nsLoginInfo.js
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/components/passwordmgr/src/nsLoginManager.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js,v  <--  nsLoginManager.js
new revision: 1.25; previous revision: 1.24
done
Checking in toolkit/components/passwordmgr/test/unit/head_storage_legacy_1.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/unit/head_storage_legacy_1.js,v  <--  head_storage_legacy_1.js
new revision: 1.7; previous revision: 1.6
done

Comment 9

[Marcia Knous [:marcia]]

Verified fixed using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008011504 Minefield/3.0b3pre. I verified by visting a number of sites including nytimes, facebook, chicagotribune, and washingtonpost. Also tested on Win Vista using the latest nightly.