1048791 - Mail account wizard (autoconfig) cannot be completed

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[schneija]

Attachment: Screenshots.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

- opened new mail account wizard to register a second mail account 
- incoming: IMAP with SSL/TLS & normal password (unencrypted) / outgoing: SMTP (default server settings from previous mail account)
See attachment Screenshot-1.



Actual results:

When I click on "Finish", the warning dialog pops up (see attachment Screenshot-2): although it tells me I can continue with these settings, there is no way to enable the "Finish" button! (because the checkbox "I understand the risks" is NOT being displayed)
- if I go back and enter another set of SMTP settings for this mail account, the checkbox "I understand the risks" however is displayed!


Expected results:

Since this security dialog is categorized as warning, the checkbox to enable the finish button should always be displayed, regardless whether default SMTP server settings are used or not.

Comment 1

[:aceman]

We have several reports of this but we couldn't reproduce it yet. But with your screenshots I could reproduce it now. It seems the catch is to use one of the existing SMTP servers (probably the settings of that one matters as I got different contents of the warning with different servers).

I'll look into this now.

As a temporary workaround, instead of "Finish", click "Advanced config", then you will not get this warning.

Comment 2

[:aceman]

Attachment: Settings in the Account wizard

Comment 3

[:aceman]

Attachment: Warning that cannot be dismissed

Comment 4

[:aceman]

Attachment: patch

OK, so the bug seems to be that if the user immediatelly selects an existing server, the fields for outgoing transport security and password security are hidden and are left at the values of "Autodetect". Then, the warning dialog checks if "outgoing.socketType > 1" and shows the the window if yes, but only shows there is a problem with security if ".socketType == 1" (None). But an unitialized (autodetect) field has a ".socketType == 0".

If you keep the SMTP server on the "new server" menuitem and fill in all the other fields to No security and no password encription, THEN switch to use an existing server, the warning comes up using the typed in values of the fields even though they are irrelevant now as the values are not used. The warning shows the name of the potential new server and its security settings even though you want to use a different, existing server.

So I propose to skip the warning if we use an existing SMTP server as we hopefully already warned about it when it was created and the user accepted that. Also other code in the wizard shortcuts if existing server is used as nothing needs to be created in that case.

The other fix is to solve an non-existent menulist.inputField error when switching from existing SMTP server to a new one.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 8565099 [details] [diff] [review]
patch

Review of attachment 8565099 [details] [diff] [review]:
-----------------------------------------------------------------

Nice detective work, aceman! LGTM, r=mkmelin

Comment 6

[:aceman]

Attachment: patch v2

Unfortunately, the fix was not complete. If the warning dialog was shown due to incoming server insecurity the uninitialized outgoing fields were still considered. It is not very good conceptually what the open() function does the same checks that needed() was doing (duplicates logic). So I rework it so that needed() sets up the logic (marks problems) and open() just blindly obeys and shows the needed info.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 8565195 [details] [diff] [review]
patch v2

Review of attachment 8565195 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/prefs/content/accountcreation/emailWizard.js
@@ +1658,5 @@
>  
> +  _inSecurityBad:  0x0001,
> +  _inCertBad:      0x0010,
> +  _outSecurityBad: 0x0100,
> +  _outCertBad:     0x1000,

There's little reason for these to be anything but booleans... too much c++ lately? :)

Comment 8

[:aceman]

So how do you propose to do it? How do I return the 4 values as a single one? Or should I make it an object or array?

Comment 9

[:aceman]

I intended those as constants and the real information transfer to be in the return value of needed() that could then be decoded using those constants. Do you propose to change those a "global variables" (members of the object) that are set (bool) from inside needed() and then read from open() ?

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 8565195 [details] [diff] [review]
patch v2

Review of attachment 8565195 [details] [diff] [review]:
-----------------------------------------------------------------

I see now.

Comment 11

[:aceman]

OK, thanks.

Comment 12

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/645484c48931

Comment 13

[Ben Bucksch (:BenB)]

Comment on attachment 8565195 [details] [diff] [review]
patch v2

Review of attachment 8565195 [details] [diff] [review]:
-----------------------------------------------------------------

I don't like the bitflags. They make the code substantially harder to read as it was before, and they don't gain anything.

Comment 14

[Ben Bucksch (:BenB)]

(I was on a work trip last week and couldn't respond earlier.)
> -    return !incomingOK || !outgoingOK;
> +    return incomingBad | outgoingBad;

I don't see a reason for this change, and the inversion makes it harder to read.

Comment 15

[:aceman]

So how do you propose to do it?

Comment 16

[:aceman]

Spun off the possible improvements to bug 1140748.

Comment 17

[Jonathan Starrett]

I am still experiencing this issue as of Thurderbird 31.5.0

If you would like additional debugging details, I would be happy to provide them.

Comment 18

[Jonathan Starrett]

[Blocking Requested - why for this release]:

Comment 19

[Magnus Melin [:mkmelin]]

Please don't set flags you don't understand.

Comment 20

[Jonathan Starrett]

Got your attention, didn't it? Just fyi, I have a Degree in Computer Science.

Comment 21

[:aceman]

(In reply to Jonathan Starrett from comment #17)
> I am still experiencing this issue as of Thurderbird 31.5.0
> 
> If you would like additional debugging details, I would be happy to provide
> them.

Please try TB38. The fix was not backported to TB31.x.