549040 - Account Wizard: Parallelize the guessConfig() heuristic

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, enhancement)

Description

[Ben Bucksch (:BenB)]

When we failed all other methods and start checking common mail server hostnames and ports, we currently do that sequentially, one hostname/port at a time (albeit incoming and outgoing in parallel, I think). The whole process takes a lot of time, often between 30s and 3 minutes - if you do it the first time, second time for the same domain is faster. The latter suggests that a lot of the time is spent with DNS lookups. The rest of the time is probably TCP handshakes - they are low bandwidth, high latency.

I think we should save a *lot* of time, with only little waste of resources, if we did all the attempts in parallel. I.e. we start trying to connect to imap.example.com:995, imap.example.com:143, pop.example.com:993, pop.example.com:143, pop3.example.com:143 etc. all at the same time.

We record the results in an internal data structure, and at the end compare and return the best result. As optimization, we may decide to abort the search once we get a success on a host/port and all higher-preference host/ports already failed (are no longer outstanding), to avoid a full search when a high-preference like imap.example.com:995 succeeds.

Comment 1

[Ben Bucksch (:BenB)]

*snicker*. Comment in guessConfig.js:
  // TODO we could make all host/port combinations run in parallel, store their
  // results in an array, and as soon as one finishes successfully and all
  // higher-priority ones have failed, abort all lower-priority ones.

Comment 2

[Ben Bucksch (:BenB)]

<http://hg.mozilla.org/comm-central/annotate/89ea90131812/mailnews/base/prefs/content/accountcreation/guessConfig.js#l392>

Comment 3

[Ben Bucksch (:BenB)]

2010-02-27 18:38:44     mail.wizard     INFO    SUCCESS
2010-02-27 18:38:44     mail.wizard     INFO    progress callback host mail.web.de port 25 type smtp
2010-02-27 18:38:44     mail.wizard     INFO    wiredata: 421 smtp05.web.de: Too many concurrent SMTP connections from one IP address; please try again later.

2010-02-27 18:38:44     mail.wizard     INFO    STARTTLS wanted, but not offered
2010-02-27 18:38:44     mail.wizard     INFO    progress callback host mail.web.de port 587 type smtp
2010-02-27 18:38:44     mail.wizard     INFO    wiredata: 421 smtp05.web.de: Too many concurrent SMTP connections from one IP address; please try again later.                                      

Well, bad luck for them :-). (And we have a config for web.de in the DB anyways.)
Effect: we notice that the host exists, but we can't detect STARTTLS or auth schemes. We should still be able to detect SSL, but I'd have to test more.

Comment 4

[Ben Bucksch (:BenB)]

Attachment: Fix, v1

Implemented.

Not tested very carefully, but seems to work for the ISPs I tried (web.de, gmx.de, orange.fr, free.fr) and return the best possible config.

Re throttling: The other ISPs I tried didn't seem to do that, and even for web.de I eventually got a proper config.


It's really fast, it's finishing now within max. 14s (10s + 2s per timeout, plus maybe a bit), most are faster, GMX takes 1s (!), comcast, roadrunner and aol.com took ~7s.

Comment 5

[Ben Bucksch (:BenB)]

Attachment: Fix, v3

Properly prefer SSL and IMAP.


For comparison, I tried it without my patch. web.de took 13s (instead of 1s), comcast.net and aol.com took >40s (instead of 7s).

Comment 6

[Ben Bucksch (:BenB)]

Compare bug 533680, including comment 14 from bienvenu: "We've had enough complaints about this that I think some improvement should block 3.1."
I think this patch fixes that bug, because I fixed the cancel() implementation here.

Comment 7

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

Comment on attachment 429355 [details] [diff] [review]
Fix, v3

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js
>@@ -964,18 +964,18 @@ EmailConfigWizard.prototype =
>-    gEmailWizardLogger.warn(id + " setting icon and tooltip " +
>-                            state + ":" + details);
>+    //gEmailWizardLogger.warn(id + " setting icon and tooltip " +
>+    //                        state + ":" + details);

Did you mean to leave this commented out?

>+++ b/mailnews/base/prefs/content/accountcreation/guessConfig.js
>@@ -37,17 +37,17 @@
>-const TIMEOUT =  10; // in seconds
>+const TIMEOUT =  5; // in seconds TODO 10

I think perhaps we should put this back to 10.  Or, since we're no longer waiting for TIMEOUT*n, maybe we could make it even longer.  :)

>@@ -87,38 +87,47 @@ var outgoingDone = false;
>+  assert(typeof(progressCallback) == "function", "need progressCallback");
>+  assert(typeof(successCallback) == "function", "need successCallback");
>+  assert(typeof(errorCallback) == "function", "need errorCallback");
>+  assert(typeof(incomingErrorCallback) == "function",
>+    "need incomingErrorCallback");
>+  assert(typeof(outgoingErrorCallback) == "function",
>+    "need outgoingErrorCallback");

Did you really run into these errors, or are they just defensive coding?

>+  if (!resultConfig)
>+    resultConfig = new AccountConfig();

If we're going to create a resultConfig, then perhaps we should mention that in the docs up above?

Maybe just add "optional", like this:
 * @param resultConfig: an optional AccountConfig object which may be
 *   partially filled in. If so, it will be used as base for the guess.

>-  if (which == 'incoming')
>+  var outgoingDone = false;
>+  if (which == "incoming")
>     outgoingDone = true;
>-
>-  if (which == 'outgoing')
>+  else if (which == "outgoing")
>     incomingDone = true;
>+  // else "both" = none done

Isn't that set of lines the same as:
var outgoingDone = (which == "incoming");
var incomingDone = (which == "outgoing");
?

>@@ -170,18 +179,16 @@ function guessConfig(domain, progressCal
>-    ddump("incomingSuccess outgoingDone = " + outgoingDone + "\n");
>-    ddump("incoming success username = " + resultConfig.incoming.username + "\n");

I'm okay with not using ddump, but I think we need to log some stuff, to help us debug problems with the probes.

>@@ -201,117 +208,103 @@ function guessConfig(domain, progressCal
>   };
> 
>   let incomingHostDetector = null;
>   let outgoingHostDetector = null;
>   incomingHostDetector = new IncomingHostDetector(progress, incomingSuccess,
>                                                   incomingError);
>   outgoingHostDetector = new OutgoingHostDetector(progress, outgoingSuccess,
>                                                   outgoingError);
>-  if (which == 'incoming' || which == 'both')
>+  if (which == "incoming" || which == "both")
>   {
>-    incomingHostDetector.autoDetect(domain,
>-                                    resultConfig.incoming.hostname ? true : false,
>-                                    resultConfig.incoming.protocol ? resultConfig.incoming.protocol : undefined,
>-                                    resultConfig.incoming.port ? resultConfig.incoming.port : undefined,
>-                                    resultConfig.incoming.socketType ? resultConfig.incoming.socketType : undefined);
>+    incomingHostDetector.start(domain,
>+        resultConfig.incoming.hostname ? true : false,

Could we use:
  !!resultConfig.incoming.hostname
instead, to achieve the same effect?  Or, if you want to be more explicit:
  new Boolean(resultConfig.incoming.hostname)
?

>+        resultConfig.incoming.protocol
>+            ? resultConfig.incoming.protocol : undefined,

That looks weird.  How does it differ from:
  resultConfig.incoming.protocol || undefined
(which should fit on a single line)?

> function GuessAbortable(incomingHostDetector, outgoingHostDetector,
>                         updateConfig)
> {
>-  this._init(incomingHostDetector, outgoingHostDetector, updateConfig);
>+  Abortable.call(this);

I totally don't understand why you're calling this line.

>+  this._incomingHostDetector = incomingHostDetector;
>+  this._outgoingHostDetector = outgoingHostDetector;
>+  this._updateConfig = updateConfig;
> }
> 
> GuessAbortable.prototype =
> {
[snip…]
>-      case 'incoming':
>-        if (this._incomingHostDetector)
>-        {
>-          this._incomingHostDetector.cancel();
>-          this._incomingHostDetector.autoDetect(domain, incomingHostIsPrecise,
>-                                                protocol, port, socketType);
>-        }
>-        else
>-        {
>-          ddump("no incoming host detector!"); // TODO use assert()
>-        }
>+      case "incoming":
>+        assert(this._incomingHostDetector, "need this._incomingHostDetector");
>+        this._incomingHostDetector.cancel();
>+        this._incomingHostDetector.start(domain, incomingHostIsPrecise,
>+                                              protocol, port, socketType);

I really like this change, but this line could use 5 fewer spaces of indentation.

>+        this._outgoingHostDetector.start(domain, outgoingHostIsPrecise,
>+                                              SMTP, port, socketType);

Ditto.

>-        if (this._incomingHostDetector)
>-        {
>-          this._incomingHostDetector.cancel();
>-          this._incomingHostDetector.autoDetect(domain, incomingHostIsPrecise,
>-                                                protocol, port, socketType);
>-        }
>-        if (this._outgoingHostDetector)
>-        {
>-          this._outgoingHostDetector.cancel();
>-          this._outgoingHostDetector.autoDetect(domain, outgoingHostIsPrecise,
>-                                                port, socketType);
>-        }
>+        assert(this._incomingHostDetector, "need this._incomingHostDetector");
>+        assert(this._outgoingHostDetector, "need this._outgoingHostDetector");
>+        this._incomingHostDetector.cancel();
>+        this._incomingHostDetector.start(domain, incomingHostIsPrecise,
>+                                              protocol, port, socketType);
>+        this._outgoingHostDetector.cancel();
>+        this._outgoingHostDetector.start(domain, outgoingHostIsPrecise,
>+                                              SMTP, port, socketType);

You've changed the meaning of this code, but didn't mention anything about it in the bug.  Was it intentional, or should this "both" case really be "either, or none"?

>@@ -343,516 +336,573 @@ function protocolToString(type)
>+const kNotTried = 0;
>+const kOngoing = 1;
>+const kFailed = 2;
>+const kSuccess = 3;
>+
> function HostDetector(progressCallback, successCallback, errorCallback)
[snip…]
>+  this._hostsToTry = []; /* Array (ordered by decreasing preference) of
>+  {
>+    hostname {String},
>+    port {Integer},
>+    socketType,
>+    protocol,
>+    commands {String}, // what to send to server
>+    status {Integer}, // kNotTried, kOngoing, kFailed, kSuccess
>+    abortable {Abortable} : null, // allows to cancel the socket comm
>+    accountConfig, // ?

Can I get a better comment than "?"?  ;)

And I'm thinking we want to indent that a little more, so that I don't think that _hostsToTry is a function again.

> HostDetector.prototype =
> {
>+  cancel : function()

docstring?

>   {
>+    this._cancel = true;
>+    // We have to actively stop the network calls, as
>+    // they may result in callbacks e.g. to the cert handler.  If the dialog is
>+    // gone by the time this happens, the javascript stack is horked.
>+    for (let i = 0; i < this._hostsToTry.length; i++)
>+    {
>+      let thisTry = this._hostsToTry[i];

Argh, another for-loop.  My kingdom for $.map!  ;)

>+      if (thisTry.abortable)
>+        thisTry.abortable.cancel();
>+      thisTry.status = kFailed; // or don't set? Maybe we want to continue.

What about adding kCancelled = 4;?

>+    }
>+    if (!this._done) // success also calls cancel()
>+      this.mErrorCallback("Canceled"); // TODO l10n

If we added kCancelled, then we could pass it back here, and let the callback deal with the l10n…

>+  start : function(domain, /* required */

docstring?

>+                        hostIsPrecise /* false */,
>+                        protocol /* UNKNOWN */,
>+                        port /* UNKNOWN */,
>+                        socketType /* UNKNOWN */)

I think these should line up with "domain" on the top line.

>   {
>+    domain = domain.replace(/\s*/g, ""); // Remove whitespace
>+    if (hostIsPrecise === undefined)
>+      hostIsPrecise = false;
[snip…]
>+    // if hostIsPrecise is true, it's because that's what the user input
>+    // explicitly, and we'll just try it, nothing else.
>+    if (hostIsPrecise == true && hostIsPrecise !== undefined)

If hostIsPrecise is true, isn't it by definition not undefined?  And, if it was undefined, then wouldn't it be converted to false up above.  Did you mean something else there?

>+    this._hostsToTry = this._sortByPreference(this._hostsToTry);
>+    this._tryAll();
>+  },
>+    // sort _hostsToTry by preference of SSL, IMAP, hostname etc.

Some blank space above this line please, and it would be nice to make this into an actual docstring.

>+  _sortByPreference : function(tries)
>+  {
>+    return tries.sort(function __sortByPreference(a, b)
>+    {
>+      // -1 = a is better; 1 = b is better; 0 = equal
>+      // Prefer SSL/TLS above all else
>+      if (a.ssl != NONE && b.ssl == NONE)
>+        return -1;
>+      if (b.ssl != NONE && a.ssl == NONE)
>+        return 1;

Should we order SSL vs STARTTLS?
If we do, we could short-cut that to either:
  a.ssl - b.ssl
or:
  sslConvertToSocketType(a.ssl) - sslConvertToSocketType(b.ssl)

>+      // Prefer IMAP over POP

This has been a surprisingly controversial decision.

>+  _tryAll : function()
>   {
>     if (this._cancel)
>         return;

Might as well fix the indentation here, if you're mucking with this much of the file.  ;)

>+    var me = this;
>+    var first = true;
>+    for (let i = 0; i < this._hostsToTry.length; i++)
>+    {
>+      let thisTry = this._hostsToTry[i];
>+      if (thisTry.status != kNotTried)
>+        continue;
>+      this._log.info("poking at " + thisTry.hostname + " port " +
>+          thisTry.port + " ssl "+ thisTry.ssl + " protocol " +
>+          protocolToString(thisTry.protocol));
>+      if (first) // showing 50 times at once is pointless

Isn't this the same as:
  if (i == 0)
?

>+        this.mProgressCallback(thisTry.protocol, thisTry.hostname,
>+             thisTry.port, thisTry.ssl);
>+      first = false;
> 
>+      thisTry.abortable = SocketUtil(
>+          thisTry.hostname, thisTry.port, thisTry.ssl == SSL,
>+          thisTry.commands, TIMEOUT,

Do we want to give the SSL sockets a little more time to respond?

>+  _checkFinished : function()
[snip…]
>+      this.mSuccessCallback(successfulTry.protocol, successfulTry.hostname,
>+          successfulTry.port, successfulTry.ssl,
>+          successfulTry._secureAuth,
>+          successfulTry._selfSignedCert, successfulTry._targetSite);

Should we perhaps just pass the whole successfulTry, instead of splitting it out into bits and pieces?

>+  _advertisesSecureAuth : function(protocol, wiredata)
>+  {
>+    // for imap to support secure auth,
>+    // capabilities needs to return 1 or more of the following:
>+    // "AUTH=CRAM-MD5", "AUTH=NTLM", "AUTH=GSSAPI", "AUTH=MSN"
>+    // for pop3, the auth mechanisms are returned in capa as the following:
>+    // "CRAM-MD5", "NTLM", "MSN", "GSSAPI"
>+    // For smtp, EHLO will return AUTH and then a list of the
>+    // mechanism(s) supported, e.g.,
>+    // AUTH LOGIN NTLM MSN CRAM-MD5 GSSAPI
>+    let line = wiredata.join("");

Should we upper-case the line, to handle poorly-configured servers?

>+IncomingHostDetector.prototype =
>+{
>+  _hostnamesToTry : function(protocol, domain)
>+  {
>+    var hostnamesToTry = [];
>+    if (protocol != POP)
>+      hostnamesToTry.push("imap." +  domain);
>+    if (protocol != IMAP)
>+    {
>+      hostnamesToTry.push("pop3." +  domain);
>+      hostnamesToTry.push("pop." +  domain);
>+    }
>+    hostnamesToTry.push("mail." + domain);
>+    hostnamesToTry.push(domain);
>+    return hostnamesToTry;
>+  },

You know, it would be cool if we could move this beside getIncomingTryOrder, to keep all these easily-testable functions together.

Then we could have tests for all of them.  :)

>+  _portsToTry : function(hostname, protocol, ssl, port)
>+  {
>+    return getIncomingTryOrder(hostname, protocol, ssl, port);
>+  },

Could this just be:
  _portsToTry = getIncomingTryOrder,
?

>+  var outstream = transport.openOutputStream(0,0,0);
>+  var stream = transport.openInputStream(0,0,0);

Shouldn't there be spaces after the commas here?

>+  } catch (e) { _error(e); }

I prefer the _error to be on a separate line, but I wouldn't block the patch if you would rather leave it the way it is.

>-          if (this._hostsToTry.indexOf(cert.commonName) == -1)
>+          if (this._hostsToTry.indexOf(cert.commonName) == -1) // TODO

shouldn't that be "TODO something"?

>   processSSLError : function(socketInfo, status, targetSite)
>   {
>-    ddump ("got SSL error\n");
>+    this._log.error("got SSL error");
>+    dump("\n\ngot an SSL error, please implement the handler!\n\n\n");

That should at least be a ddump.

>+++ b/mailnews/base/test/unit/test_autoconfigUtils.js
>@@ -34,16 +34,30 @@
> /*
>  * Test suite for the autoconfigUtils class
>  *
>+ * <TODO> remove this test, it's totally pointless.
>+ * Tests MUST be written in a way that they do NOT fail
>+ * when a proper code change is made, e.g. a bug fixed or a policy changed.
>+ *
>+ * getHostEntry() is a function, because it contains a policy which
>+ * is subject to change.
>+ * Whenever we change getHostEntry, this test breaks. That makes the test
>+ * totally pointless.
>+ *
>+ * I improved the API of getHostEntry() ([] -> {}). There were only 2 callers
>+ * in the real code, easy to adapt, but I'd have to adapt almost 50 (!) calls
>+ * here to the new API, so this is a good time to remove the test.
>+ * </TODO>
>+ *

Remove this TODO block please.

I understand that you think that this test isn't useful, but it helped me greatly when I moved the getIncoming/OutgoingTryOrder code into its own functions, and exposed a logic bug in the code I was moving from.

Furthermore, since we don't document the exact order of hosts or configs we test, this is the only place that information is spelled out, and so is useful for that as well.

Finally, this test is to ensure that our code conforms to our policy, so is pretty much the opposite of pointless.  This test _is_ the policy.

If you want to add a helper function that takes arguments and returns HostEntrys (as arrays or objects or whatever), then your API change from [] -> {} should be able to be made in one place.

>@@ -290,26 +304,29 @@ function test_getIncomingTryOrder()
> function test_getOutgoingTryOrder()
> {
>+  let host = "latte.ca";

We should probably use "example.com", or "mozilla.test", or something fake here.

>@@ -348,19 +365,17 @@ function test_copying_readFromXML()
> function run_test()
> {
>-  do_test_pending();
>+  return true; // TODO See top

This change alone earns this patch an r- from me.

I doubt you've implemented the replacement code perfectly, and without any tests, you have no way of knowing what you broke.

>+++ b/mailnews/base/util/autoconfigUtils.jsm
>@@ -51,32 +51,32 @@ const SMTP = 2;
> const IMAP4_CMDS = ["1 CAPABILITY\r\n", "2 LOGOUT\r\n"];
> const POP3_CMDS  = ["CAPA\r\n", "QUIT\r\n"];
>-const SMTP_CMDS = ["EHLO\r\n", "QUIT\r\n"];
>+const SMTP_CMDS = ["EHLO we-guess.mozilla.org\r\n", "QUIT\r\n"];

Heh.  Wasn't this change part of a different bug?

> const SMTP_PORTS = {}
> SMTP_PORTS[TLS]=587;
> SMTP_PORTS[SSL]=465;
>-SMTP_PORTS[NONE]=25;
>+SMTP_PORTS[NONE]=587;

So, there are really two SMTP ports we should be trying.  Any thoughts on how to handle that?

>@@ -120,20 +125,38 @@ function getIncomingTryOrder(host, proto
>-function getOutgoingTryOrder(port)
>+function getOutgoingTryOrder(host, protocol, ssl, port)
> {
>-  if (port == UNKNOWN)
>+  if (protocol != SMTP)
>+    return [];

That seems like an error, not a silent returning of the empty array, to me.

>+  if (ssl == UNKNOWN)
>+  {
>+    if (port == UNKNOWN)
>+      // neither ssl nor port known
>+      return [getHostEntry(SMTP, TLS, UNKNOWN),
>+              getHostEntry(SMTP, TLS, 25),
>+              getHostEntry(SMTP, SSL, UNKNOWN),
>+              getHostEntry(SMTP, NONE, UNKNOWN),
>+              getHostEntry(SMTP, NONE, 25)];
>+    // port known, SSL not
>     return [getHostEntry(SMTP, TLS, port),
>             getHostEntry(SMTP, SSL, port),
>-            getHostEntry(SMTP, TLS, 25),
>-            getHostEntry(SMTP, NONE, 587),
>             getHostEntry(SMTP, NONE, port)];
>-  return [getHostEntry(SMTP, TLS, port),
>-          getHostEntry(SMTP, SSL, port),
>-          getHostEntry(SMTP, NONE, port)];
>+  }
>+  // ssl known, port not
>+  if (port == UNKNOWN)
>+  {
>+    if (ssl == SSL)
>+      return [getHostEntry(SMTP, SSL, UNKNOWN)];
>+    else // TLS or NONE
>+      return [getHostEntry(SMTP, ssl, UNKNOWN),
>+              getHostEntry(SMTP, ssl, 25)];
>+  }
>+  // ssl and port known
>+  return [getHostEntry(SMTP, ssl, port)];

Uh, there are a lot of changes to the logic here, and no tests of the new behaviour, so I'll wait for the next version of this patch to review that bit of code.

And finally, I don't see any tests of the new functionality, which are required for a patch to be accepted these days.

Later,
Blake.

Comment 8

[Ben Bucksch (:BenB)]

> Did you mean to leave this commented out?

No, I should just remove or leave it.

-const TIMEOUT =  10; // in seconds
+const TIMEOUT =  5; // in seconds TODO 10

> I think perhaps we should put this back to 10.

Yup, sorry, I intended to. (that's why I added the TODO)

> Did you really run into these errors, or are they just defensive coding?

I ran into such problems repeatedly, because of frequent function signature changes.

+  if (!resultConfig)
+    resultConfig = new AccountConfig();

> If we're going to create a resultConfig, then perhaps we should mention
> that in the docs up above?
> Maybe just add "optional", like this:
>  * @param resultConfig: an optional AccountConfig object which may be
>  *   partially filled in. If so, it will be used as base for the guess.

No, the (non-existing) parameter object is overwritten here.
function foo(a) { a = 5; return a; }
var a = 4;
dump(a);
var b = foo(a);
dump(a);
dump(b);
gives 445, not 455.

We do, however, return the resultConfig object to the successCallback(config), so it still matters.

+ var outgoingDone = (which == "incoming");
+ var incomingDone = (which == "outgoing");

Nice.

> new Boolean(resultConfig.incoming.hostname)

Won't help with the |undefined|.

Frankly, I don't know why this code is so obsessed with testing specifically for undefined. Just using if (port) would be fine. It's not like port 0 was valid, and I intentionally didn't define protocol 0.

function GuessAbortable(incomingHostDetector, outgoingHostDetector,
			  updateConfig)
{
+  Abortable.call(this);

> I totally don't understand why you're calling this line.

Because GuessAbortable inherits from Abortable. Therefore, it should call super.

See <https://developer.mozilla.org/en/Core_JavaScript_1.5_Guide/Inheritance>

+	  this._outgoingHostDetector.start(domain, outgoingHostIsPrecise,
+						SMTP, port, socketType);

> You've changed the meaning of this code
> Was it intentional, or should this "both" case really be "either,
> or none"?

Did I? Inhowfar?
(I don't think I intended a change in meaning here.)

+  cancel : function()
> docstring?

You don't want me to document the cancel() function, do you? It's defined in Abortable super class.

> Argh, another for-loop.  My kingdom for $.map! ;)

Oh, I get annoyed every time I have to loop over an array. Why couldn't they define |for each (var el in array)| properly in the spec?

(will go over rest later)

Comment 9

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

(In reply to comment #8)
> > Did you really run into these errors, or are they just defensive coding?
> I ran into such problems repeatedly, because of frequent function signature
> changes.

Ah, cool.

> +  if (!resultConfig)
> +    resultConfig = new AccountConfig();
> > If we're going to create a resultConfig, then perhaps we should mention
> > that in the docs up above?
> > Maybe just add "optional", like this:
> >  * @param resultConfig: an optional AccountConfig object which may be
> >  *   partially filled in. If so, it will be used as base for the guess.
> No, the (non-existing) parameter object is overwritten here.

Right, therefore they don't need to pass one in, so maybe we can mark it as optional in the docs?

> > new Boolean(resultConfig.incoming.hostname)
> Won't help with the |undefined|.

Argh.

> Frankly, I don't know why this code is so obsessed with testing specifically
> for undefined. Just using if (port) would be fine. It's not like port 0 was
> valid, and I intentionally didn't define protocol 0.

I think I'm okay with removing those checks, if it would make the code simpler.

> function GuessAbortable(incomingHostDetector, outgoingHostDetector,
>               updateConfig)
> {
> +  Abortable.call(this);
> 
> > I totally don't understand why you're calling this line.
> Because GuessAbortable inherits from Abortable. Therefore, it should call
> super.
> See <https://developer.mozilla.org/en/Core_JavaScript_1.5_Guide/Inheritance>

Okay, I get that, but Abortable does nothing, and none of the other things that extend it have that call.

But with that explanation, I'm fine with it if you want to leave it in.

> +      this._outgoingHostDetector.start(domain, outgoingHostIsPrecise,
> +                        SMTP, port, socketType);
> > You've changed the meaning of this code
> > Was it intentional, or should this "both" case really be "either,
> > or none"?
> Did I? Inhowfar?
> (I don't think I intended a change in meaning here.)

With the old code:
>-        if (this._incomingHostDetector)
>-        {
>-          this._incomingHostDetector.cancel();
>-          this._incomingHostDetector.autoDetect(domain, incomingHostIsPrecise,
>-                                                protocol, port, socketType);
>-        }
>-        if (this._outgoingHostDetector)
>-        {
>-          this._outgoingHostDetector.cancel();
>-          this._outgoingHostDetector.autoDetect(domain, outgoingHostIsPrecise,
>-                                                port, socketType);
>-        }
you didn't need to have either this._incomingHostDetector or this._outgoingHostDetector, and it would all be fine.

With the new code:
>+        assert(this._incomingHostDetector, "need this._incomingHostDetector");
>+        assert(this._outgoingHostDetector, "need this._outgoingHostDetector");
you need to have both this._incomingHostDetector and this._outgoingHostDetector.

Now, the new case makes more sense to me when the value is "both", unless we remove the detectors as they detect something, in which case we might not have one or both of them, so the original code is more correct.

> +  cancel : function()
> > docstring?
> You don't want me to document the cancel() function, do you? It's defined in
> Abortable super class.

Yeah, okay, nevermind.

> > Argh, another for-loop.  My kingdom for $.map! ;)
> Oh, I get annoyed every time I have to loop over an array. Why couldn't they
> define |for each (var el in array)| properly in the spec?

:)

Later,
Blake.

Comment 10

[Ben Bucksch (:BenB)]

> you didn't need to have either this._incomingHostDetector or
> this._outgoingHostDetector, and it would all be fine.

Not really. The caller said "restart both", but if one of the detectors doesn't exist, we wouldn't restart both.
This isn't a case that's really happening. We create both objects (search for "new IncomingHostDetector") even if which == "incoming".

Comment 11

[Ben Bucksch (:BenB)]

(and we don't remove them, from what I could see.)

Comment 12

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

(In reply to comment #10)
> > you didn't need to have either this._incomingHostDetector or
> > this._outgoingHostDetector, and it would all be fine.
> Not really. The caller said "restart both", but if one of the detectors doesn't
> exist, we wouldn't restart both.
> This isn't a case that's really happening. We create both objects (search for
> "new IncomingHostDetector") even if which == "incoming".

So you did mean to change the meaning then?  ;)

I guess I'm okay with the new version of the code, since it better matches the comments.

Later,
Blake.

Comment 13

[Ben Bucksch (:BenB)]

What about adding kCancelled = 4;?
Not sure, but prefer to treat it as kFailed for now.

>+      this.mErrorCallback("Canceled"); // TODO l10n

> If we added kCancelled, then we could pass it back here, and let the callback
> deal with the l10n…

No, we can't return the integer 4 here. errorCallback is defined (everywhere) to return either an exception object (which can be toString()ed to get an error message) or a string, both with error messages presentable to the user.

I should be using the CancelledException from bug 534588 (also in bug 549045), after moving it from fetchhttp.js to util.js.

> Should we order SSL vs STARTTLS?

Not sure.

> Do we want to give the SSL sockets a little more time to respond?

No, it's not much, and 10 sec are plenty, IMHO. And we'd need to consider it in our setTimeout hard abort above.

> Should we perhaps just pass the whole successfulTry

Done.
Was quite some work, but found a few old bugs on they way. This code is a pure hack, all this stuff - esp. restart() etc. - was just hacked in without any regard for APIs whatsoever.

> Should we upper-case the line, to handle poorly-configured servers?

Good point. Backend also compares case-insensitive, so yes.

> You know, it would be cool if we could move this beside getIncomingTryOrder, to
> keep all these easily-testable functions together.

Yes. autoconfigUtils.jsm content should be in this file. All this code belongs together, not in different files, much less dirs.
You already use readFromXML() in your test, so you don't need a module to use it in a test.
I fixed that by just moving autoconfigUtils.jsm back in here.

> >+  let host = "latte.ca";

> We should probably use "example.com", or "mozilla.test", or something fake
here.

That's your own code:
  checkImap( "imap.latte.ca", UNKNOWN );
  checkImap( "pop.latte.ca", IMAP );
  ...
  let host = "latte.ca";

I just had to do the same for outgoing now to make it adhere to the new API.

>-const SMTP_CMDS = ["EHLO\r\n", "QUIT\r\n"];
>+const SMTP_CMDS = ["EHLO we-guess.mozilla.org\r\n", "QUIT\r\n"];

Heh.  Wasn't this change part of a different bug?

No. EHLO needs the domain as parameter, the old one is wrong.
Given that I hit the server hard, I want to give it a clue who we are and why we do that (and that we're not a spammer).

>-SMTP_PORTS[NONE]=25;
>+SMTP_PORTS[NONE]=587;
> So, there are really two SMTP ports we should be trying.  Any thoughts on how
> to handle that?

The (old and new) code already tries both. This is just the standard port.

> since we don't document the exact order of hosts or configs we
> test, this is the only place that information is spelled out,
> This test _is_ the policy.

The *code* is the policy, and it's the place where the order of tries is spelled out. Policies should be defined in one place, not duplicated.

I can't emphasize this enough: I expect to be able to fix bugs, and not run the tests, and be sure that all tests pass, unless I have a real bug. If a test breaks and my code was correct, the test is broken. Given that this test will *always* break when the tested code is changed (like now), I think the test is inherently broken.

To keep this test alive, I'll have to change the *whole* test, all 50 calls, due to the (small) API change. That's why I just disabled it.

I don't know how to fix that either.

Comment 14

[Ben Bucksch (:BenB)]

> I'll have to change the *whole* test, all 50 calls

Actually, it may not be that bad, thanks to assert_equal_host_entries(). I'll see whether I can fix it.

If I made us prefer SSL over STARTTLS, I'd indeed have to change a lot, but I refrained from that so far.

Comment 15

[Ben Bucksch (:BenB)]

OK, fixed the test (yay)

Comment 16

[Ben Bucksch (:BenB)]

Attachment: Fix, v7

Comment 17

[Ben Bucksch (:BenB)]

FWIW, I spent again all day (8h straight) on this.

Comment 18

[Ben Bucksch (:BenB)]

Attachment: Fix, v8

Updated to trunk after commit of bug 531267.

Comment 19

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

Comment on attachment 436073 [details] [diff] [review]
Fix, v8

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js
>@@ -1388,17 +1388,17 @@ EmailConfigWizard.prototype =
>       this._probeAbortable.restart(config.outgoing.hostname, config, "outgoing",
>-                                   "smtp", config.outgoing.port);
>+                                   "smtp", config.outgoing.port, config.outgoing.socketType);

Let's split the outgoing socketType onto a new line to keep it under 80 characters.

>+++ b/mailnews/base/prefs/content/accountcreation/fetchhttp.js
>@@ -212,26 +212,25 @@ FetchHTTP.prototype =
>       // error in callback or our fetchhttp._response() code
>-      try
>-      {
>-        ddump("Error in errorCallback or _response(): " + e);
>-        this._errorCallback(e);
>-      } catch (e) {
>-        //ddump("Error in errorCallback: " + e);
>+      //try
>+      //{
>+        //this._errorCallback(e);
>+      //} catch (e) {
>+        logException(e);
>         // XXX TODO FIXME BOGUS
>         alert(e); // error in errorCallback, too!
>         throw(e); // to error console
>-      }
>+      //}

Uh, that seems like a debugging change gone awry.  Did you want to revert that?

>+++ b/mailnews/base/prefs/content/accountcreation/guessConfig.js
>@@ -35,95 +35,102 @@
>+kDebug = true; // TODO remove

This should really be removed, no?  ;)

>@@ -141,750 +148,1000 @@ function guessConfig(domain, progressCal
>+    // TODO
>+    // cert is also bad when targetSite is set. (Same below for incoming.)
>+    // Fix SSLErrorHandler and security warning dialog in emailWizard.js.
>+    resultConfig.outgoing.badCert = thisTry.selfSignedCert; //|| !!thisTry.targetSite;

The comment at the end of this line takes the line over 80 characters, and I'm not entirely sure what you're trying to say with it.

>+HostTry.prototype =
>+{
[snip…]
>+  // {AccountConfig} ?
>+  accountConfig : null,

I still want a better comment than "?".  ;)

>+      for (let j = 0; j < hostEntries.length; j++)
>+      {
>+        let hostTry = hostEntries[j]; // from getHostEntry()
>+        //let hostTry = new HostTry();
>+        hostTry.hostname = hostname;
>+        //hostTry.port = hostEntry.port;
>+        //hostTry.ssl = hostEntry.ssl;
>+        //hostTry.protocol = hostEntry.protocol;
>+        //hostTry.commands = hostEntry.commands;
>+        hostTry.status = kNotTried;
>+        this._hostsToTry.push(hostTry);

I think we can get rid of most of those comments.

>+    this._hostsToTry = this._sortByPreference(this._hostsToTry);
>+    this._tryAll();
>+  },
>+  /**
>+   * Sort by preference of SSL, IMAP etc.

We should still have a blank line between the "}," and the "/**".

>+    this._log.info("wiredata: " + wiredata.join(""));
>+    thisTry.authMethods = this._advertisesAuthMethods(thisTry.protocol, wiredata); // TODO

TODO what?  And can we put it on its own line?

>+IncomingHostDetector.prototype =
>+{
>+  _hostnamesToTry : function(protocol, domain)
>+  {
>+    var hostnamesToTry = [];
>+    if (protocol != POP)
>+      hostnamesToTry.push("imap." +  domain);
>+    if (protocol != IMAP)
>+    {
>+      hostnamesToTry.push("pop3." +  domain);
>+      hostnamesToTry.push("pop." +  domain);
>+    }
>+    hostnamesToTry.push("mail." + domain);
>+    hostnamesToTry.push(domain);
>+    return hostnamesToTry;
>+  },
>+  _portsToTry : function(hostname, protocol, ssl, port)
>+  {
>+    return getIncomingTryOrder(hostname, protocol, ssl, port);
>+  },

Please change this to:
  _portsToTry = getIncomingTryOrder,

>+OutgoingHostDetector.prototype =
>+{
>+  _hostnamesToTry : function(protocol, domain)
>+  {
>+    var hostnamesToTry = [];
>+    hostnamesToTry.push("smtp." + domain);
>+    hostnamesToTry.push("mail." + domain);
>+    hostnamesToTry.push(domain);
>+    return hostnamesToTry;
>+  },
>+  _portsToTry : function(hostname, protocol, ssl, port)
>+  {
>+    return getOutgoingTryOrder(hostname, protocol, ssl, port);
>+  },

And this to:
  _portsToTry = getOutgoingTryOrder,

>+// TODO prefer SSL over STARTTLS

I think this comment should go into the _sortByPreference method, if anywhere.

>           // then add this host to the hosts to try, and skip to the end.
>-          if (this._hostsToTry.indexOf(cert.commonName) == -1)
>+          if (this._hostsToTry.indexOf(cert.commonName) == -1) // TODO

What did you want to TODO here?

> function assert(aBeTrue, aWhy)
> {
>   if (!aBeTrue)
>-    do_throw(aWhy);
>+    dump(aWhy + "\n");
>+  do_check_true(aBeTrue);

So, if we throw aWhy, then the error message shows up in the test failure report.  But dumping will only show up in the middle of all the other dump statements.  Was there a reason you changed this (and assert_equal) to not throw?

>   assert_equal_try_orders(tryOrder,
>                           [[SMTP, TLS, 587],
>+                           [SMTP, TLS, 25],
>                            [SMTP, SSL, 465],
>-                           [SMTP, TLS, 25],
>                            [SMTP, NONE, 587],
>                            [SMTP, NONE, 25]]);

Hmm...  This makes it look like we prefer TLS over SSL.  Would you agree?

> function run_test()
> {
[snip…]
>   test_getIncomingTryOrder();
>   test_getOutgoingTryOrder();
> 
>+  test_copying_readFromXML();

I think we could get rid of the blank line above here.

Other than that, I think I'm happy, but with a 2200-line patch, there's likely something I missed.

Later,
Blake.

Comment 20

[Ben Bucksch (:BenB)]

Attachment: Fix, v9

- Fix review comments
- Remove XML test from test_autoconfigUtils.js, as it's now in
  test_autoconfigUtils.js
- Move static sortByPreference function out of object.
- add check for aborted in _error() in SocketUtils.

Commited as <http://hg.mozilla.org/comm-central/rev/34bf573fdf62>

FIXED

This is regression prone. Tester, please check that we're still getting good results (possibly compare with old code). Note that the behavior may be somewhat erratic in some cases, because it depends on ISP servers. Please file regressions as new bugs and mention them here.