When I booted up the newest nightly none of my accounts had passwords, I suspect bug 1018624 (which ports changes from bug 853549, which "makes initializing the password storage happen asynchronously"). In particular, attachment 8435440 [details] [diff] [review] might be necessary? I have no idea why this would be Windows only though. Does Mac use a different password manager (e.g. the OS keychain?)

Is this a bug you can reproduce consistently, or something that only happened once?

It happens every time. I've rolled back to 1.5 and my passwords reappeared.

Also Bug 1017696 is the SeaMonkey version of this. I can reproduce this, I've tried various different iterations with yielding the initialization promise as in attachment 8435440 [details] [diff] [review], but have been unable to get this to work. (I tried in both imCore.js and imAccounts.js.) Attachment 8436039 [details] [diff] seems to take a different approach which I also could not get to work. I'm unsure exactly what needs to be changed due to bug 853549.

aleth had a few good ideas that helped me out here (namely to look at storage-json.js and LoginStore.jsm). From here I was able to find the pref that says whether the storage provider has been changed to JSON yet. If I reset this pref and delete logins.json I get this issue (the account manager says "A password is required to connect this account"). If I then restart, voila I have passwords. http://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/storage-json.js http://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginStore.jsm

I can confirm this also happens on Linux - as we expected, it's an OS-independent race condition.

Paolo, could you possibly provide some insight into what could be happening here? Instantbird, unlike Firefox, pretty much access passwords immediately upon boot. I think there might be an issue that we're trying to access passwords while the migration is occurring. Thanks!

(In reply to Patrick Cloke [:clokep] from comment #6) > Paolo, could you possibly provide some insight into what could be happening > here? Instantbird, unlike Firefox, pretty much access passwords immediately > upon boot. I think there might be an issue that we're trying to access > passwords while the migration is occurring. Thanks! Definitely. When the nsILoginManager service is accessed for the first time after migration, it starts importing passwords from SQLite in the background, but no passwords are available until this operation has finished. Indeed, waiting for initializationPromise should prevent this race condition from occurring. However, we expect that all the passwords will be available later, unless some code is creating a new password entry in the meantime, which would prevent the migration from occurring at all. Do you create internal special-purpose password entries for any reason if no password is found on startup? In Firefox, for performance reasons, we initialize the password manager three seconds after startup. Code that uses passwords is synchronous and can't wait for the initialization promise, so if a web page requests a passwords within three seconds from the first startup of the updated version, we simply don't display the password in the page.

I'm not entirely in love with this patch, but it does fix the issue.

Comment on attachment 8483183 [details] [diff] [review] Patch v1 Review of attachment 8483183 [details] [diff] [review]: ----------------------------------------------------------------- You have to check everything calling these two functions that are now Tasks doesn't also have to be async and yield on it. For example this now is likely broken: http://mxr.mozilla.org/comm-central/source/mail/components/im/content/imAccounts.js#38 This will also need adjusting as init() now returns a promise: http://mxr.mozilla.org/comm-central/source/im/components/ibCommandLineHandler.js#29 ::: im/content/blist.js @@ +790,5 @@ > elt.getBoundingClientRect(); > elt.focus(); > }, > > + load: Task.async(function *() { No space in function*, so either function*() or function* (). ::: im/modules/ibCore.jsm @@ +39,5 @@ > false, true); > } > }, > > + init: Task.async(function *() { Same here.

Comment on attachment 8483183 [details] [diff] [review] Patch v1 Review of attachment 8483183 [details] [diff] [review]: ----------------------------------------------------------------- The changes here seem in the good direction. I agree with aleth's comment 9 and have nothing to add after looking at the patch. I'll probably want to give the next version of the patch a try.

This chains everything I could find that accesses passwords during startup on the initialization promise. WFM, but please test on Windows, as the original problem never showed up for me on OSX.

Comment on attachment 8511554 [details] [diff] [review] passwords.diff WIP Review of attachment 8511554 [details] [diff] [review]: ----------------------------------------------------------------- This seems to work! :) Is this ready for review or does it need to be cleaned up at all? Thanks for looking at this, by the way!

Comment on attachment 8511554 [details] [diff] [review] passwords.diff WIP Review of attachment 8511554 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/components/src/imAccounts.js @@ +176,4 @@ > > + // Check for errors that should prevent connection attempts. > + if (this._passwordRequired && !this.password) > + this._connectionErrorReason = Ci.imIAccount.ERROR_MISSING_PASSWORD; This is used to prevent the account from being connected (eg. by the user from the account manager), so setting it asynchronously seems hazardous. I wonder if we need a new error code to indicate to the account manager that the account isn't fully initialized. @@ +482,5 @@ > > _password: "", > get password() { > + // Warning: Don't attempt to access passwords before > + // Services.login.initializationPromise has resolved. Shouldn't this comment live in the .idl file? ::: chat/components/src/imCore.js @@ +270,5 @@ > > + // Wait with automatic connections until the password service > + // is available. > + Services.logins.initializationPromise.then(() => { > + if (accounts.autoLoginStatus == Ci.imIAccountsService.AUTOLOGIN_ENABLED) This closure that keeps the "accounts" variable alive from the outer function makes me vaguely uncomfortable. I can't find a real case where this would actually cause issues, but I would prefer moving the |let accounts = Services.accounts;| line to inside the closure.

Completed patch.

(In reply to Florian Quèze [:florian] [:flo] from comment #14) > Comment on attachment 8511554 [details] [diff] [review] > passwords.diff WIP > > Review of attachment 8511554 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: chat/components/src/imAccounts.js > @@ +176,4 @@ > > > > + // Check for errors that should prevent connection attempts. > > + if (this._passwordRequired && !this.password) > > + this._connectionErrorReason = Ci.imIAccount.ERROR_MISSING_PASSWORD; > > This is used to prevent the account from being connected (eg. by the user > from the account manager), so setting it asynchronously seems hazardous. > > I wonder if we need a new error code to indicate to the account manager that > the account isn't fully initialized. Did you make any change related to this comment?

(In reply to Florian Quèze [:florian] [:flo] from comment #16) > > This is used to prevent the account from being connected (eg. by the user > > from the account manager), so setting it asynchronously seems hazardous. > > > > I wonder if we need a new error code to indicate to the account manager that > > the account isn't fully initialized. > > Did you make any change related to this comment? Yes, the account manager waits on the initialization promise on load and closes itself again if the initialization fails.

(In reply to aleth [:aleth] from comment #17) > > > I wonder if we need a new error code to indicate to the account manager that > > > the account isn't fully initialized. > > > > Did you make any change related to this comment? > > Yes, the account manager waits on the initialization promise on load and > closes itself again if the initialization fails. Ah! How long can this promise take to resolve at worst? (wondering if we may end up with the account manager UI appearing broken/frozen). Do we need the same changes on the Thunderbird UI?

(In reply to Florian Quèze [:florian] [:flo] from comment #18) > Ah! How long can this promise take to resolve at worst? (wondering if we may > end up with the account manager UI appearing broken/frozen). Since it involves filesystem I/O and sqlite, I suppose it's the usual range of not noticeable to many seconds. So to avoid any "empty account manager" glitches, I've chained the call to the account manager as well, which seems cleaner. > Do we need the same changes on the Thunderbird UI? Probably, but I would prefer to look at TB startup in a separate patch.

Comment on attachment 8512591 [details] [diff] [review] passwordpromise.diff v2 Review of attachment 8512591 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable to me! Thanks again for taking a look at this. :)

Comment on attachment 8512591 [details] [diff] [review] passwordpromise.diff v2 Review of attachment 8512591 [details] [diff] [review]: ----------------------------------------------------------------- Let's get this into the nightlies. (Florian, if you really want to look at this...please add yourself back, but it isn't necessary.)

https://hg.mozilla.org/comm-central/rev/295ec981ed4c Removed an extra closing parenthesis that caused total bustage.

(In reply to Nihanth Subramanya [:nhnt11] from comment #23) > https://hg.mozilla.org/comm-central/rev/295ec981ed4c > > Removed an extra closing parenthesis that caused total bustage. Nice catch! Thanks.