Closed Bug 1030059 Opened 10 years ago Closed 10 years ago

Passwords gone in newest nightly

Categories

(Instantbird Graveyard :: Other, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: aleth)

References

Details

(Whiteboard: [1.6-blocking])

Attachments

(2 files, 3 obsolete files)

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.
Severity: normal → major
Whiteboard: [1.6-blocking]
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.
OS: Windows 8.1 → All
Hardware: x86 → All
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!
Flags: needinfo?(paolo.mozmail)
(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.
Flags: needinfo?(paolo.mozmail)
Attached patch Patch v1 (obsolete) — Splinter Review
I'm not entirely in love with this patch, but it does fix the issue.
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attachment #8483183 - Flags: review?(florian)
Attachment #8483183 - Flags: review?(aleth)
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.
Attachment #8483183 - Flags: review?(florian)
Attachment #8483183 - Flags: review?(aleth)
Attachment #8483183 - Flags: review-
Attachment #8483183 - Flags: review?(florian)
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.
Attachment #8483183 - Flags: review?(florian) → feedback+
Attached patch passwords.diff WIP (obsolete) — Splinter Review
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.
Assignee: clokep → aleth
Attachment #8511554 - Flags: feedback?(clokep)
Attachment #8511554 - Attachment description: passwords.diff → passwords.diff WIP
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!
Attachment #8511554 - Flags: feedback?(clokep) → feedback+
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.
Attachment #8511554 - Flags: feedback+
Attached patch passwordpromise.diff (obsolete) — Splinter Review
Completed patch.
Attachment #8483183 - Attachment is obsolete: true
Attachment #8511554 - Attachment is obsolete: true
Attachment #8512575 - Flags: review?(florian)
Attachment #8512575 - Flags: review?(clokep)
(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.
Attachment #8512575 - Attachment is obsolete: true
Attachment #8512575 - Flags: review?(florian)
Attachment #8512575 - Flags: review?(clokep)
Attachment #8512591 - Flags: review?(florian)
Attachment #8512591 - Flags: review?(clokep)
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. :)
Attachment #8512591 - Flags: review?(clokep) → review+
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.)
Attachment #8512591 - Flags: review?(florian)
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Depends on: 1102892
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: