530697 - Choosing "Connect" now spawns an ineffective second Master Password dialog, if I cancel the first one

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 0. Add a master password to your Firefox profile, and install Weave.
 1. Start up Firefox.  Dismiss any initial Master-Password dialogs.
 2. Choose "Connect" in weave status bar menu (or in Tools | Weave)
 3. Cancel the resulting Master password dialog.

ACTUAL RESULTS:
A *second* master password dialog appears....
 - If I dismiss it, nothing else weird happens.
 - If I enter my master password in it, Weave still doesn't actually connect -- I get "Error While Signing In / Weave encountered an error while signing you in: Unknown error. Please try again."

EXPECTED RESULTS: No second master password dialog should appear, if I cancel the first one.

VERSION INFO:
*  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091123 Minefield/3.7a1pre
*  Weave 1.0b2

Weave 1.0b2pre is also affected -- I just tested a not-yet-upgraded machine with that version. I'm pretty sure this is a regression since 1.0b1, though.

NOTE: This bug only affects manually-initiated "Connect" operations.  It doesn't affect Weave's autoconnect attempt at Firefox startup.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> I'm pretty sure this is a regression since 1.0b1, though.

Confirmed regression since 1.0b1.  Weave 1.0b1 doesn't re-prompt for master password, when I choose "Connect" and then cancel the fist dialog.

(Additionally: In Weave 1.0b1, when I cancel a "Connect" master password dialog, I get an "Unknown Error" Weave notification.  In Weave 1.0b2 and 1.0b2pre, when I cancel both "Connect" master password dialogs, I get _no_  Weave error-notifications.

I don't consider the lack-of-error-notification a regression in this case -- I'm just reporting it in case it's useful in tracking down what changed here.)

Comment 3

[Daniel Holbert [:dholbert]]

I think I know the problem here...

In WeaveGlue.connect, we have:
>   connect: function connect() {
>     Weave.Service.login(this._settings.user.value, this._settings.pass.value,
>       this._settings.secret.value);
http://hg.mozilla.org/labs/weave/file/a3a955e59f54/source/chrome/content/fennec-weave-overlay.js#l52

I'm pretty sure the use of "this._settings.pass.value" and "this._settings.secret.value" are both accesses to the password manager, and so both can trigger a master password dialog.

Fix is simple: Just separate out those queries into different lines, and if the first one fails, then bail out.

Now, why are we getting the "Unknown Error" notification *specifically* if I cancel the first dialog but complete the second?

Well, if I cancel both dialogs, then Weave has neither my password nor my passphrase, so when we get "onLoginError", we hit this clause and skip the warning:
>     // Don't notify on missing passphrase errors
>     if (!Weave.Service.passphrase)
>       return;
http://hg.mozilla.org/labs/weave/file/a3a955e59f54/source/chrome/content/sync.js#l136

But if I just cancel the first dialog & I complete the second, then my **passphrase** will be unlocked (since that's what the second dialog was asking for).  So we'll bypass the early return above, and we'll go ahead with the warning.

Anyway, I think we can avoid both issues by bailing out early if the password-value-lookup fails in "connect()".

Comment 4

[Daniel Holbert [:dholbert]]

> In WeaveGlue.connect, we have:
> >   connect: function connect() {
> >     Weave.Service.login(this._settings.user.value, this._settings.pass.value,
> >       this._settings.secret.value);
> http://hg.mozilla.org/labs/weave/file/a3a955e59f54/source/chrome/content/fennec-weave-overlay.js#l52

So, per the naming of this file, this code is actually fennec-specific (Mardak confirmed in IRC).  So this isn't the right place to patch, but there's presumably some corresponding spot in the desktop-firefox-login code...

Comment 5

[Daniel Holbert [:dholbert]]

Just kidding -- so, the second dialog is actually spawned **by the "// Don't notify on missing passphrase errors" check itself.  We shouldn't be touching |Weave.Service.passphrase| there (in onLoginError), because that can trigger a master password dialog.  (Or, we should only touch it if we've verified that password DB is unlocked.)

If I comment out that check entirely, we revert to the (better IMHO) Weave 1.0b1 behavior that I described in comment 1: 
 - No second master password dialog
 - "Unknown Error" notification if I cancel the first dialog.

(The "Unknown Error" is still minorly annoying, but not as bad as the re-prompting for master password :) )

Comment 6

[Mike Connor [:mconnor]]

Not critical for 1.3

Comment 7

[Mike Connor [:mconnor]]

bug 590763 makes this a non-issue.