Last Comment Bug 755988 - Mail Account Setup doesn't detect service on smtps (465)
: Mail Account Setup doesn't detect service on smtps (465)
Status: RESOLVED FIXED
Probably caused by bug 721369
: regression
Product: Thunderbird
Classification: Client Software
Component: Account Manager (show other bugs)
: 12 Branch
: All All
: P1 major (vote)
: Thunderbird 17.0
Assigned To: :Irving Reid (No longer working on Firefox)
:
Mentors:
: 781253 (view as bug list)
Depends on:
Blocks: 721369
  Show dependency treegraph
 
Reported: 2012-05-16 19:42 PDT by Yoshiaki Kasahara
Modified: 2012-09-25 07:51 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-
+


Attachments
Handle both number and string values for encryption type code (8.91 KB, patch)
2012-08-21 13:36 PDT, :Irving Reid (No longer working on Firefox)
ben.bucksch: review-
Details | Diff | Splinter Review
Additional logging to help diagnose the issue (5.58 KB, patch)
2012-08-22 10:21 PDT, :Irving Reid (No longer working on Firefox)
ben.bucksch: review-
Details | Diff | Splinter Review
Fix JavaScript strict errors in affected files (4.08 KB, patch)
2012-08-22 10:23 PDT, :Irving Reid (No longer working on Firefox)
ben.bucksch: review+
Details | Diff | Splinter Review
Handle string/integer differences in the manual configuration UI (2.29 KB, patch)
2012-08-22 11:02 PDT, :Irving Reid (No longer working on Firefox)
ben.bucksch: review-
Details | Diff | Splinter Review
Fix JS Strict warnings, updated to not depend on debug patch (3.29 KB, patch)
2012-08-23 08:56 PDT, :Irving Reid (No longer working on Firefox)
irving: review+
ben.bucksch: review+
Details | Diff | Splinter Review
Handle string/integer differences in the manual configuration UI, using sanitize.integer() (3.32 KB, patch)
2012-08-23 08:58 PDT, :Irving Reid (No longer working on Firefox)
irving: review+
ben.bucksch: review+
Details | Diff | Splinter Review

Description Yoshiaki Kasahara 2012-05-16 19:42:45 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120515 Firefox/14.0a2
Build ID: 20120515042006

Steps to reproduce:

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120516 Thunderbird/14.0a2

I noticed that "Mail Account Setup" wizard cannot detect service running on port 465 with SSL/TLS.  I found the problem with version 12.0.1 (for Windows and Mac).  I tried Earlybird (14.0a2) for Windows but it was still broken.

I did the following steps:
(Assume an (internal) SMTP server running services on standard port 25/465/587)

* Select Account Settings -> Account Actions -> Add Mail Account
* Enter a name and Email address, and click "Continue"
* Thunderbird found configuration by trying common server names
* Click "Manual config" and change outgoing SSL setting from "STARTTLS" to "SSL/TLS"
  (The port number was automatically changed from 587 to 465)
* Click "Re-test"


Actual results:

Thunderbird reported "Thunderbird failed to find the settings for your email account".

If I reverted outgoing SSL setting to "STARTTLS" and clicked "Re-test", Thunderbird reported "The following settings were found by probing the given server".

I'm sure that the server is configured correctly, because I could manually edit "Outgoing Server (SMTP)" in Account Settings to use "SSL/TLS" for "Connection security" and send messages without problems.


Expected results:

Thunderbird should be able to find service on port 465 with SSL/TLS by probing the given server. Actually, version 9.0.1 worked as expected, so it is a regression.

I captured packets between the client and server using wireshark, and noticed that Thunderbird didn't send SSL client hello to port 465 after 3-way handshake, and just closed the connection.  imaps (993) and pop3s (995) are detected properly. I guess something is wrong with the service detection logic.
Comment 1 Ben Bucksch (:BenB) 2012-06-07 16:30:22 PDT
We definitely do try to find servers on port 465. (This doesn't exclude the possibility of a bug.)

Does the server send the same certificate on both ports? We may ignore any servers with wrong certs, and servers can have different configurations on the 2 ports, so that is a possibility.
Comment 2 Ben Bucksch (:BenB) 2012-06-07 16:46:05 PDT
CONFIRMED. I tried 3-4 ISPs that, according to our database, have SMTP servers on port 465, but when I changed pref mailnews.auto_config_url and ran guess config, it never found port 465, only STARTLS on port 587 or 25. Even manually setting the server to port 465 and SSL and clicking "Re-Test" failed to find the server.

bsmith, could this be a regression in SSL?
Comment 3 Ben Bucksch (:BenB) 2012-06-07 17:18:31 PDT
This is definitely a regression. I tried a TB trunk from 2012-03-31, and it worked. The TB trunk from 2012-04-25 failed. So, it's a regression in that timeframe.

Bug 745950 falls in that time-frame, and changes code relevant to this, so could well be the culprit.

Reproduction:
1. Edit | Preferences... | Advanced | General | Config Editor ...
   Set pref "mailnews.auto_config_url" to "http://domain.invalid/" to force guess config
1. File | New | Existing Mail Account...
2. Realname: nonsense, Email address: nonsense@laposte.net , password: nonsense
   (Be sure to use @laposte.net as domain, because they have an SSL SMTP server at port 465,
   but no STARTTLS at SMTP.)
3. Continue and wait
4. Manual config

Expected result:
SMTP: smtp.laposte.net, port 465, SSL/TLS

Actual result:
SMTP: smtp.laposte.net, port 465, None (No encryption
Comment 4 Ben Bucksch (:BenB) 2012-06-07 17:20:17 PDT
Sorry, I meant bug 721369
Comment 5 Ben Bucksch (:BenB) 2012-06-07 17:22:24 PDT
> SMTP: smtp.laposte.net, port 465, None (No encryption)

And here I meant "..., port 587, None", of course. Sorry.
Comment 6 Ben Bucksch (:BenB) 2012-08-15 10:21:41 PDT
*** Bug 781253 has been marked as a duplicate of this bug. ***
Comment 7 Ben Bucksch (:BenB) 2012-08-15 10:23:48 PDT
As said, this is likely caused by bug 721369.
This bug is causing quite some grief. If somebody could investigate and fix this, this would be appreciated.
Comment 8 :Irving Reid (No longer working on Firefox) 2012-08-21 13:36:11 PDT
Created attachment 653920 [details] [diff] [review]
Handle both number and string values for encryption type code

It won't be easy to stop swearing at JavaScript for long enough to write this up...

The problem is a combination of two things. First, https://hg.mozilla.org/comm-central/rev/69177e1f55f4 commented out the probe for SSL support when we were dealing with a socket where the encryption type is UNKNOWN (-1).

Second, the switch statement at https://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/guessConfig.js#867 uses (under the hood) JavaScript's === comparator, which does NOT coerce strings to numbers. That means if you pass the number 2 in, you get back SSL, but if you pass in the string "2" you get back UNKNOWN

Third, when we read from our ISP database we get the number for the encryption type, and we initialize the manual settings UI with those numbers, and as long as the user doesn't change anything they stay numbers. BUT, if the user make a selection on the encryption type UI element, the value gets changed to a string.

So, if the user explicitly selects an encryption type of SSL in the manual settings UI, it gets converted to UNKNOWN by the JS switch() statement, and the UNKNOWN encryption type no longer probes for SSL support, so our config test fails.


Patch changes the switch() to a series of if statements so that string/integer coercion works and we correctly try SSL when the user explicitly chooses that in the UI.
Comment 9 Ben Bucksch (:BenB) 2012-08-21 14:53:17 PDT
Comment on attachment 653920 [details] [diff] [review]
Handle both number and string values for encryption type code

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

Thanks, irving, for looking into this!

> https://hg.mozilla.org/comm-central/rev/69177e1f55f4 commented out the probe
> for SSL support when we were dealing with a socket where the encryption type is
> UNKNOWN (-1).

That looks wrong to me, even though I wrote the patch.
Yet, I am not in the habit of throwing in useless changes. I wonder why I did that. Could you check in the comments of bug 721369 why we did that?

Speaking of unneeded changes: Please make the patch minimal, removing all changes that are not strictly necessary to fix this bug. If you feel that other changes are useful, please attach them separately, one per purose (e.g. one only logging statements, but I don't think we need all these).

> BUT, if the user make a selection on the encryption type UI element,
> the value gets changed to a string.

Yeah, that's a bug. Please fix this part as well, though: Both 1) the ConvertSocketTypeToSSL() (switch vs. if|, I agree in preferring |if|), as well as 2) what the UI returns: We should never have "2" in that enum variable in the first place, that's already the bug, not just ConvertSocketTypeToSSL().

Thanks a lot again for fixing this! I can see how horrible that was to debug. It is a hideous bug.
Comment 10 David Favor 2012-08-22 07:58:12 PDT
The real problem is having any of this Wizard code.

Every mail setup can be customized in many ways, making Wizard design complex
and as we've seen... many times broken so badly Thunderbird has to be rolled back
to 2.0.0.24 just to configure new accounts.

Please add a Manual Config button to the first panel of the Wizard which allows bypassing
the Wizard and going directly to the "Settings" for a given account ala the 2.0.0.24 way of
doing setups.
Comment 11 :Irving Reid (No longer working on Firefox) 2012-08-22 10:21:47 PDT
Created attachment 654253 [details] [diff] [review]
Additional logging to help diagnose the issue
Comment 12 :Irving Reid (No longer working on Firefox) 2012-08-22 10:23:15 PDT
Created attachment 654254 [details] [diff] [review]
Fix JavaScript strict errors in affected files

Always like to leave things cleaner than I found them...
Comment 13 :Irving Reid (No longer working on Firefox) 2012-08-22 11:02:28 PDT
Created attachment 654271 [details] [diff] [review]
Handle string/integer differences in the manual configuration UI

The code that copied config out of UI elements converted to integer for the incoming parameters but not for the outgoing. This patch adds parseInt to the outgoing parameters
Comment 14 :Irving Reid (No longer working on Firefox) 2012-08-22 11:08:18 PDT
(In reply to Ben Bucksch (:BenB) from comment #9)
> Comment on attachment 653920 [details] [diff] [review]
> Handle both number and string values for encryption type code
> 
> Review of attachment 653920 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, irving, for looking into this!
> 
> > https://hg.mozilla.org/comm-central/rev/69177e1f55f4 commented out the probe
> > for SSL support when we were dealing with a socket where the encryption type is
> > UNKNOWN (-1).
> 
> That looks wrong to me, even though I wrote the patch.
> Yet, I am not in the habit of throwing in useless changes. I wonder why I
> did that. Could you check in the comments of bug 721369 why we did that?

The code checked at https://bugzilla.mozilla.org/show_bug.cgi?id=721369#c53 is not the patch attached to the bug; you'll need to look at it yourself to figure out what's up. There are some remarks in https://bugzilla.mozilla.org/show_bug.cgi?id=721369#c51 about trying different combinations of probes, perhaps that is where the commented out lines came from?
Comment 15 Ben Bucksch (:BenB) 2012-08-22 18:05:44 PDT
Comment on attachment 654271 [details] [diff] [review]
Handle string/integer differences in the manual configuration UI

+      config.outgoing.socketType = parseInt(e("outgoing_ssl").value);
+      config.outgoing.auth = parseInt(e("outgoing_authMethod").value);

Please use
... = santize.integer(e("outgoing_ssl").value);
like the other code.
Comment 16 Ben Bucksch (:BenB) 2012-08-22 18:13:13 PDT
Comment on attachment 654253 [details] [diff] [review]
Additional logging to help diagnose the issue

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

I can see that this was useful for you during debugging, but I am not sure we need this in the code that we ship.
Particularly the guessConfig changes: I think the cancel/abort is being called for all other in-flight probes when we found a good host. Ditto when the user cancels the probe, you'll get 50 cancel messages, because we probe 50 hosts at the same time.
Comment 17 Ben Bucksch (:BenB) 2012-08-22 18:14:37 PDT
(In reply to Ben Bucksch (:BenB) from comment #15)
> Comment on attachment 654271 [details] [diff] [review]
...
> Please use
> ... = santize.integer(e("outgoing_ssl").value);
> like the other code.

With this change: r+

Thanks, irving!
Comment 18 :Irving Reid (No longer working on Firefox) 2012-08-23 08:56:28 PDT
Created attachment 654646 [details] [diff] [review]
Fix JS Strict warnings, updated to not depend on debug patch

Carrying forward r=benb
Comment 19 :Irving Reid (No longer working on Firefox) 2012-08-23 08:58:16 PDT
Created attachment 654648 [details] [diff] [review]
Handle string/integer differences in the manual configuration UI, using sanitize.integer()

r=benb from https://bugzilla.mozilla.org/show_bug.cgi?id=755988#c17
Comment 20 :Irving Reid (No longer working on Firefox) 2012-08-23 09:03:00 PDT
Pushed to comm-central:

https://hg.mozilla.org/comm-central/rev/82882c866072
https://hg.mozilla.org/comm-central/rev/115d3d4fa725
Comment 21 Ben Bucksch (:BenB) 2012-08-23 18:07:29 PDT
Thanks a lot, irving, for fixing this! Much appreciated.

Note You need to log in before you can comment on or make changes to this bug.