Steps to reproduce: 1) Create a new profile and migrate data including passwords from an existing SeaMonkey profile. 2) Let FF start 3) Go into preferences, security and Show Passwords 4) Sites and usernames are displayed. 5) Select "Show Passwords" and "Yes" to the confirm dialog. Expected Results: Sites, usernames and Passwords displayed after confirmation. Actual Results: Whole list is emptied. Cannot get it back without restarting ff. Problems also occur if I have a password for bugzilla.mozilla.org (for example) and I go to that page twice. The problem is fixed if I make a change to the saved passwords (add/remove etc). Problem is here: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js&rev=1.15&mark=814-820#782 When password manager gets an old style password to load, it sets encryptedPassword to null. Next time round the GetAllLogins() loop, its null and that causes things to fail. I'm not sure of the best thing that should be done here. Maybe re-encrypt the password straight away, or not blank it out at all? Currently unless the user make changes to the password list, they won't be altered on shut down.

This fixes the bug by re-encrypting the username/password instead of nulling them out. This means that next time we try to decrypt, we'll be able to get them fine. I've also extended the unit tests to check for this case as well. I think this is probably the best fix given the options, but I'm open to suggestions.

Hmm. Looks like this was probably regressed by 381164. The original bug to support base64 encoded entries (bug 380961) also added unit tests (#2,3,4 in toolkit/components/passwordmgr/test/unit/test_storage_legacy_3.js), but it looks like they're not tickled by this bug. I think what happened was that in the window between the fixes for 380961 and 381164, anyone with base64 entries got they reencrypted, and so this hasn't popped up until now. Yuck.

Comment on attachment 285908 [details] [diff] [review] Possible fix and unit test >- this._readFile() >+ this._readFile(); *sigh* >- // Force any old mime64-obscured entries to be reencrypted. >+ // Reencrypt and old mime64-obscured entries to be reencrypted. > if (login.wrappedJSObject.encryptedUsername && >- login.wrappedJSObject.encryptedUsername.charAt(0) == '~') >- login.wrappedJSObject.encryptedUsername = null; >+ login.wrappedJSObject.encryptedUsername.charAt(0) == '~') { >+ [login.wrappedJSObject.encryptedUsername, userCanceled] = >+ this._encrypt(login.username); >+ } You'll need to store the _encrypt() result in a local variable, and only assign it to .encryptedUsername if userCanceled is false. If the user has enabled a master password, the encryption could require entry of the MP, and if the user canceled it that would clobber the base64 value. >+/* ========== 8 ========== */ >+testnum++; >+ >+testdesc = "checking double reading of mime64-obscured entries" >+LoginTest.initStorage(storage, INDIR, "signons-380961-1.txt"); >+LoginTest.checkStorageData(storage, [], [dummyuser1]); >+ >+testdesc = "checking double reading of mime64-obscured entries part 2" >+LoginTest.checkStorageData(storage, [], [dummyuser1]); We should probably be paranoid, and add a test #9 that does the same thing but also writes results out to a file. Something like: LoginTest.initStorage(storage, INDIR, "signons-380961-1.txt", OUTDIR, "output-400751-1.txt"); LoginTest.checkStorageData(storage, [], [dummyuser1]); LoginTest.checkStorageData(storage, [], [dummyuser1]); storage.addLogin(dummyuser2); // trigger a write LoginTest.checkStorageData(storage, [], [dummyuser1, dummyuser2]); testdesc = "[flush and reload for verification]" LoginTest.initStorage(storage, OUTDIR, "output-400751-1.txt"); LoginTest.checkStorageData(storage, [], [dummyuser1, dummyuser2]); r+ with that.

v2 - Addressed Justin's comments and added checks and extended unit tests.

Comment on attachment 286025 [details] [diff] [review] Fix and unit test v2 >Index: toolkit/components/passwordmgr/src/storage-Legacy.js >- // Force any old mime64-obscured entries to be reencrypted. >+ // Reencrypt and old mime64-obscured entries to be reencrypted. Fix this comment so that it makes sense? :)

Fixed the comment, carrying for r+ (times 2). Requesting approvalM9. This bug fixes an issue with migrated passwords (e.g. from SeaMonkey) sometimes not being displayed or presented for use. Unit Tests are included in the patch which help test the fix, and the existing unit tests show there are no regressions.

Comment on attachment 286029 [details] [diff] [review] Fix and unit test v3 (ready for checkin) a=endgame drivers for M9

Patch checked in -> fixed

Mark: in the future you should flip the in-testsuite flag to + when you check in a patch which includes automated testcases. :-)