490141 - [autoconfig] doesn't start detection from my suggested server settings

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Mark Banner (:standard8)]

Autoconfig doesn't detect my server address correctly (because its too far fetched from my email address), therefore I stopped autoconfig and entered my server hostname. I also selected the connection type as "None".

I then clicked "Go", and autoconfig started from SSL/TLS on port 993 rather than none on 143 as was entered in the configuration by myself.

Comment 1

[David Ascher (:davida)]

Sounds like there are a few bugs here:

1) why is there a "None" connection type?

2) autoconfig ignores the port # if entered manually?

Comment 2

[Phil Ringnalda (:philor)]

There isn't a "None" connection type, there's a "None" connection _security_ type.

What's supposed to happen is that oninput fires when you change the port number, which sets _userChangedIncomingPort to true, onpopuphidden fires when you close the connection security menulist which sets _userChangedIncomingSocketType, and then we check those in _startProbingIncoming and don't set those bits to undefined. Not sure how to go about getting those things to not happen like they apparently didn't.

Comment 3

[Ben Bucksch (:BenB)]

There should IMO be an "Auto" value, which is the default, when the user manually edits. If the user changes that to a specific setting, we use that, otherwise we try to guess.

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

assigning this to blake as he's already been diving in the code and this piece is pretty important.

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Attachment: A re-working of the try order calculation function.

The problem, at least when I replicated it, was in mailnews/base/prefs/resources/content/accountcreation/guessConfig.js, line 648.  You can see that in the else block below it, we check "if (this._specifiedSSL == UNKNOWN)", but if this._specifiedPort is UNKNOWN, there is no such check.

When I made the if block more like the else block, I noticed that there was a lot of code duplication, so I made a function to encapsulate it.  Then I noticed the same duplication in the blocks near line 668, so I made the function more general.  Then I saw the OutgoingHostDetector, so I threw the SMTP values in there as well.  And that's how I got to where it is now.

(I'm working on a unit test, but it's going slowly, so I thought I would get this reviewed, and follow up with a separate patch, or include the tests in a later version of this patch if there are other things I need to change.)

Thanks,
Blake.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

(Rats.  I've fixed the indentation on lines 713-720.)

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, heavily refactored, and with unit tests.

A couple of notes:

1) I've noticed that has changed is that the selected protocol now overrides the hostname.  Previously, if you entered a hostname of "imap.latte.ca", and selected a protocol of "POP", it would check IMAP; now it will check POP.

2) I've ignored a license block on the top of test_autoconfigUtils.js, and have copied a license block from a different file for the top of autoconfigUtils.jsm (but have assigned copyright for the portions I wrote to "Mozilla Messaging").  If there is a standard license I should be using, I would be happy to submit a new patch with that fixed.

3) There are a shocking number of combinations of things to test.  :)

Thanks,
Blake.

Comment 8

[David :Bienvenu]

Comment on attachment 385530 [details] [diff] [review]
The previous patch, heavily refactored, and with unit tests.

thx, looks good in general. Some nits:

don't need braces here:

+  if (protocol == UNKNOWN &&
+      (!lowerCaseHost.indexOf("pop.") || !lowerCaseHost.indexOf("pop3.")))
+  {
+    protocol = POP;
+  }
+  else if (protocol == UNKNOWN &&
+           !lowerCaseHost.indexOf("imap."))
+  {
+    protocol = IMAP;
+  }
+

should be 2009 here:

+ * Portions created by the Initial Developer are Copyright (C) 2008

you can move the if up to the else, i.e.,

else if (ssl == UNKNOWN)

and outdent everything in the else clause.

+  else
+  {
+    if (ssl == UNKNOWN)
+      return [getHostEntry(IMAP, TLS, port),
+

r=me, with those nits addressed.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, updated with Bienvenu's suggestions.

(In reply to comment #8)
> (From update of attachment 385530 [details] [diff] [review])
> don't need braces here:

Fixed.

> should be 2009 here:

Fixed.

> you can move the if up to the else, i.e.,
> and outdent everything in the else clause.

Fixed.

> r=me, with those nits addressed.

Cool, thanks!

Later,
Blake.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 386596 [details] [diff] [review]
The previous patch, updated with Bienvenu's suggestions.

>+ * The Initial Developer of the Original Code is
>+ *   Blake Winton <[e-mail]>
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * Mozilla Messaging. All Rights Reserved.
>+ *
>+ * Contributor(s):

If you want to attribute to MoMo, then the correct way to do this is:

 * The Initial Developer of the Original Code is
 * Mozilla Messaging.
 * Portions created by the Initial Developer are Copyright (C) 2009
 * the Initial Developer. All Rights Reserved.
 *
 * Contributor(s):
 *   Blake Winton <[e-mail]>

as described at http://www.mozilla.org/MPL/boilerplate-1.1/

>+let IMAP4_CMDS = ["1 CAPABILITY\r\n", "2 LOGOUT\r\n"];
>+let POP3_CMDS  = ["CAPA\r\n", "QUIT\r\n"];
>+let SMTP_CMDS = ["EHLO\r\n", "QUIT\r\n"];
>+
>+function getHostEntry(protocol, ssl, port) {
>+  var IMAP_PORTS = {}
>+  IMAP_PORTS[TLS]=143;
>+  IMAP_PORTS[SSL]=993;
>+  IMAP_PORTS[NONE]=143;
>+
>+  var POP_PORTS = {}
>+  POP_PORTS[TLS]=110;
>+  POP_PORTS[SSL]=995;
>+  POP_PORTS[NONE]=110;
>+
>+  var SMTP_PORTS = {}
>+  SMTP_PORTS[TLS]=587;
>+  SMTP_PORTS[SSL]=465;
>+  SMTP_PORTS[NONE]=25;
>+
>+  var CMDS = {}
>+  CMDS[IMAP]=IMAP4_CMDS;
>+  CMDS[POP]=POP3_CMDS;
>+  CMDS[SMTP]=SMTP_CMDS;

Any reason these aren't constant objects to save them being recreated each time?

>+  var commands = CMDS[protocol];
>+
>+  return [protocol, ssl, port, commands];

commands seems to be an unnecessary intermediate variable.

>+    if (ssl == UNKNOWN)
>+      return [getHostEntry(protocol, TLS, port),
>+              getHostEntry(protocol, SSL, port),
>+              getHostEntry(protocol, NONE, port)];
>+     else
>+       return [getHostEntry(protocol, ssl, port)];

The alignment is wrong, but there is no need for an else after a return statement (ditto in a couple of other places).

Please also use K & R style of braces in the file.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with standard8's suggestions

(In reply to comment #10)
> If you want to attribute to MoMo, then the correct way to do this is:

Done.

> >+function getHostEntry(protocol, ssl, port) {
> >+  var IMAP_PORTS = {}
> >+  IMAP_PORTS[TLS]=143;
> >+  IMAP_PORTS[SSL]=993;
> >+  IMAP_PORTS[NONE]=143;
> Any reason these aren't constant objects to save them being recreated each
> time?

Fixed.

> >+  var commands = CMDS[protocol];
> >+  return [protocol, ssl, port, commands];
> commands seems to be an unnecessary intermediate variable.

Yeah, I did it so that all the values in the return line would look the same.
Fixed.

> >+    if (ssl == UNKNOWN)
> >+      return [getHostEntry(protocol, TLS, port),
> >+              getHostEntry(protocol, SSL, port),
> >+              getHostEntry(protocol, NONE, port)];
> >+     else
> >+       return [getHostEntry(protocol, ssl, port)];
> The alignment is wrong, but there is no need for an else after a return
> statement (ditto in a couple of other places).

Fixed.

> Please also use K & R style of braces in the file.

Fixed.  As a side note, I didn't realize that K&R put the { on a new line, but only for functions.

Thanks,
Blake.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 387202 [details] [diff] [review]
The previous patch, with standard8's suggestions

Thanks for the update, sr=me. I've checked this in on your behalf:

http://hg.mozilla.org/comm-central/rev/4f6ab99f4b72

Comment 13

[Ben Bucksch (:BenB)]

Why did you put part of the autoconfig guess stuff in the autoconfigUtils.jsm module, in a different directory from the other code? the only callers are accountcreation/guessConfig.js and the test.
This makes reading the code much harder, if a random part is in an entirely different directory.
If this was just for the testcase, then the testframework should be fixed. guessConfig.js is pure logic code, should have been easy to use.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

It was a while ago, so I'm not entirely sure, but looking at it I would guess that I figured that since it was a utility class it could reasonably go in util.

If you'ld like to move it to, say, /mailnews/base/prefs/resources/content/accountcreation and fix the tests, I would be more than happy to review that patch.  (I'm not a mailnews peer, so I don't know if my review would count for anything, but I'ld be happy to review it.  ;)

Thanks,
Blake.