Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 568153 - Re-test Configuration doesn't work: account.incoming.hostname.toUpperCase is not a function
: Re-test Configuration doesn't work: account.incoming.hostname.toUpperCase is ...
: regression
Product: Thunderbird
Classification: Client Software
Component: Account Manager (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 3.3a1
Assigned To: Blake Winton (:bwinton) (:☕️)
: 575422 (view as bug list)
Depends on:
Blocks: 546278 575422
  Show dependency treegraph
Reported: 2010-05-25 21:07 PDT by Kohei Yoshino [:kohei]
Modified: 2011-03-04 01:18 PST (History)
11 users (show)
standard8: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

possible patch (1.95 KB, patch)
2010-06-05 16:40 PDT, Steffen Wilberg
ben.bucksch: feedback+
Details | Diff | Splinter Review
alternative fix (2.80 KB, patch)
2010-06-06 13:14 PDT, Steffen Wilberg
no flags Details | Diff | Splinter Review
Fix, v3 (2.89 KB, patch)
2010-06-06 13:35 PDT, Ben Bucksch (:BenB)
no flags Details | Diff | Splinter Review
Fix, v4 (2.89 KB, patch)
2010-06-06 13:37 PDT, Ben Bucksch (:BenB)
steffen.wilberg: feedback+
Details | Diff | Splinter Review
Fix, v5 (2.88 KB, patch)
2010-06-06 14:29 PDT, Ben Bucksch (:BenB)
bwinton: review+
steffen.wilberg: feedback+
Details | Diff | Splinter Review
test is based on comment 2 (4.45 KB, patch)
2010-07-06 19:45 PDT, Boying Lu
bwinton: review+
Details | Diff | Splinter Review
updated patch (4.46 KB, patch)
2010-07-22 01:31 PDT, Boying Lu
no flags Details | Diff | Splinter Review
A patch which includes tests, and fixes other tests. (7.64 KB, patch)
2010-09-14 18:18 PDT, Blake Winton (:bwinton) (:☕️)
ben.bucksch: review+
standard8: superreview+
Details | Diff | Splinter Review
What I changed to get the test working. (1.03 KB, patch)
2010-09-14 18:21 PDT, Blake Winton (:bwinton) (:☕️)
ben.bucksch: review+
ben.bucksch: feedback+
Details | Diff | Splinter Review
A patch to fix the error with re-test configuration. (7.56 KB, patch)
2010-09-29 10:35 PDT, Blake Winton (:bwinton) (:☕️)
bwinton: review+
bwinton: superreview+
standard8: approval‑thunderbird3.1.5+
Details | Diff | Splinter Review

Description Kohei Yoshino [:kohei] 2010-05-25 21:07:39 PDT
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv: Gecko/20100521 Thunderbird/3.1

How to test:

1. open Mail Account Setup wizard
2. fill in the form (
3. click Continue button; settings found
4. click Edit button
5. click Re-test Configuration button
6. wait a sec

The result:

The looking up configuration stops, and the Error Console says

Error: account.incoming.hostname.toUpperCase is not a function
Source File: chrome://messenger/content/accountcreation/accountConfig.js
Line: 255

Um, my patch in Bug 546278 have broken something...
Comment 1 Mark Banner (:standard8) 2010-05-26 01:36:14 PDT
AFAICT this doesn't break anything obvious, but we'd probably take a fix for 3.1.1. If it does break other things, please let the drivers know!
Comment 2 Steffen Wilberg 2010-06-02 00:50:49 PDT
Wait, this breaks creating an account for a domain which is not listed in the database and where the servers are on a completely different domain, e.g., but the server is on e.g., and trying to use an smtp account already set up (e.g.

Slightly different STR:
0. Have an SMTP account set up ( in my case).
1. open Mail Account Setup wizard
2. fill in the form (Username: test, Email:
   (which is much faster than trying
3. click Continue button:
  "Lanikai failed to find the settings for your email account."
4. Select the existing Outgoing account.
5. Click Re-test configuration.

Error: see comment 0.

6. Click the Stop button.
Result: Just another error like above.

7. Click the Start Over link:
   The original dialog is displayed, with Username and Email filled in.
8. Click continue.

Now the dialog looks broken:
The last line it displays is Username: info, with the Stop button on the right.
Clicking the Stop button just yields another error like above.

Please don't ship 3.1 with this!
Comment 3 Steffen Wilberg 2010-06-02 00:53:57 PDT
After step 4, clicking Manual Setup instead of Re-test configuration is broken as well and displays the error above!
Comment 4 Steffen Wilberg 2010-06-02 00:59:56 PDT
And of course, first clicking Re-test configuration, then Manual Setup doesn't work either.

The only way I did manage to set up an account was to *not* select the existing Outgoing account, but click on Manual Setup.
Comment 5 Ben Bucksch (:BenB) 2010-06-02 03:25:05 PDT
I have also removed this check, see bug 549045 comment 77 and attachment 442834 [details] [diff] [review], first hunk.
Comment 6 Steffen Wilberg 2010-06-04 13:12:38 PDT
So what's the minimal fix here, short of taking all of bug 549045?
Comment 7 Steffen Wilberg 2010-06-04 14:29:54 PDT
Applying attachment 442834 [details] [diff] [review], first hunk, from bug 549045, regresses bug 546278: results in:
Incoming: mail.%emaildomain% POP 110 None
Outgonig: smtl.%emaildomain% SMTP 25 None
Comment 8 Ben Bucksch (:BenB) 2010-06-04 15:09:14 PDT
Ah, OK. I was confused about the purpose of the toUpperCase(). We still need it (but we don't need the sanitize here). Nevermind, then.
Comment 9 Steffen Wilberg 2010-06-04 15:19:38 PDT
But how do we fix the "account.incoming.hostname.toUpperCase is not a function" error (comment 0)?
Comment 10 Ben Bucksch (:BenB) 2010-06-04 16:08:04 PDT
I don't know. "if (account.incoming.hostname)"?
Comment 11 Steffen Wilberg 2010-06-05 15:37:06 PDT
Nope, same error, with or without sanitize.hostname.
Comment 12 Steffen Wilberg 2010-06-05 16:40:58 PDT
Created attachment 449485 [details] [diff] [review]
possible patch

Venkman told me that account.incoming.hostname and account.outgoing.hostname are -1 in this case, so I'm adding a check for that, which fixes the dialog.

I don't understand the code, especially where -1 comes from, but maybe it helps others to fix this properly.
Comment 13 Ben Bucksch (:BenB) 2010-06-05 22:55:49 PDT
Comment on attachment 449485 [details] [diff] [review]
possible patch

Yes, this is what I meant.

A few small things:
- Remove the {}, because this is a one-liner |if|
- Put the comment above the |if|

And maybe:
- Please replace the sanitize.hostname() with a toLowerCase().
It's not relevant for this bug, but will be needed for bug 549045, see the reasons in bug 549045 comment 77.

Basically, what's going on here is:
- the hostname may be null, empty or -1, if we don't know it. That happens e.g. when we go into manual config.
- We nevertheless need to replaceVariables() to prefill e.g. the username, and because we simply always do that. That's fine, and would be hard to avoid.
- In that case (know no proper hostname), of course this code here freaks out, because null and |Number|s don't have a function toUpperCase().
- In bug 549045, I prefill "" when we do into manual config. That is not null, but still not a valid hostname, that's why I had to remove the sanitize.hostname(), too.
- We do sanitize.hostname() immediately when we read in values from XML (network), so it's already checked. I think we also do sanitize.hostname() again in the end, before we use the values. So, checking here in replace() as well is triple-checking and unneeded and safe to remove.
- sanitize.hostname(), which we call after reading the values from XML, does toLowerCase() on the hostname. That's reasonable, but does not work when the hostname is a placeholder, which caused the original bug 546278, which is why we added the toUpperCase(). Hostnames should be lower case, though, so we should also add toLowerCase() instead of sanitize.hostname() here after the replacement.
Comment 14 Ben Bucksch (:BenB) 2010-06-05 23:05:47 PDT
An alternative fix (and maybe better) would be to add a check in sanitze.hostname() or the XML reading specifically for whether it's a placeholder.
Something like:
if (value.match(/%[A-Z]+%/)
  return value;
<continue normal hostname checks, but without allowing "%" now>
That would remove the need for toUpperCase() and toLowerCase() here in replaceVariables(), because sanitize.hostname() would not toLowerCase() the placeholder.
Comment 15 Steffen Wilberg 2010-06-06 13:14:19 PDT
Created attachment 449546 [details] [diff] [review]
alternative fix

This the alternative fix Ben suggested in comment 14.
In sanitize.hostname, I'm first testing for allowed characters like before, and then for variables. If there are variables, return the string. Otherwise, return the lower-cased string.

While I'm touching this function:
Should this be .test(str) instead of .test(unchecked)?
> var str = this.nonemptystring(unchecked);
> if (!/^[a-zA-Z0-9\-\.%]*$/.test(unchecked))
>   throw new MalformedException("hostname_syntax.error", unchecked);
Comment 16 Ben Bucksch (:BenB) 2010-06-06 13:35:10 PDT
Created attachment 449547 [details] [diff] [review]
Fix, v3

Actually, I meant it like this.
I didn't have time to test it. Steffen, does this work?
Comment 17 Ben Bucksch (:BenB) 2010-06-06 13:37:26 PDT
Created attachment 449548 [details] [diff] [review]
Fix, v4

Arg, I'm stupid. New try.
Comment 18 Steffen Wilberg 2010-06-06 14:19:45 PDT
Comment on attachment 449548 [details] [diff] [review]
Fix, v4

It works if you remove the second closing bracket here:
>   account.incoming.hostname =
>+    _replaceVariable(account.incoming.hostname, otherVariables));

and here:
>   if (account.outgoing.hostname) // will be null if user picked existing server.
>     account.outgoing.hostname =
>+      _replaceVariable(account.outgoing.hostname, otherVariables));
Comment 19 Ben Bucksch (:BenB) 2010-06-06 14:29:49 PDT
Created attachment 449549 [details] [diff] [review]
Fix, v5

Comment 20 Blake Winton (:bwinton) (:☕️) 2010-06-07 08:35:35 PDT
Comment on attachment 449549 [details] [diff] [review]
Fix, v5

I think we've had enough bugs in this code that I'm not going to r+ a patch that doesn't include tests.

(FWIW, the logic looks okay, but then, the previous logic looked okay, and I'm a little concerned that we aren't sanitizing hostnames that contain %s in them if those %s aren't one of the tokens we substitute…)

Comment 21 Ben Bucksch (:BenB) 2010-06-07 15:19:29 PDT
> I'm not going to r+ a patch that doesn't include tests.

If you'd rather ship with the bug than take the patch...

> we aren't sanitizing hostnames that contain %s in them
> if those %s aren't one of the tokens we substitute…

That would happen with the old code, too.
Please note my TODO, which would fix this (but I'd have to change some callers, and I wanted to keep the patch minimal for now).
Comment 22 Blake Winton (:bwinton) (:☕️) 2010-06-07 16:40:07 PDT
I'ld rather ship with a known bug than a bunch of unknown bugs due to untested code.  And it's unlikely that this would have been slipped into 3.1rc2 anyways, so you definitely have the time to write tests for it.  :)

Comment 23 Ben Bucksch (:BenB) 2010-06-08 02:40:28 PDT
FWIW, I won't provide the tests, I just attached the patch. Steffen or somebody else would have to do it.
Comment 24 Kohei Yoshino [:kohei] 2010-06-08 03:55:17 PDT
So is this not a 3.1 blocker? I think many users will encounter the issue.
Comment 25 Ben Bucksch (:BenB) 2010-06-08 04:11:44 PDT
I also think we should fix it, setting block-tb3.1 to ?
Comment 26 Blake Winton (:bwinton) (:☕️) 2010-06-08 06:50:24 PDT
I don't believe it is a 3.1 blocker.  It doesn't affect upgrading users, which is the main idea behind 3.1, and it's _way_ too late in the cycle to take a patch that affects this much of autoconfig, in my opinion.

Sure, we should fix it, just for 3.1.1 or 3.2, I think.

Comment 27 Ben Bucksch (:BenB) 2010-06-08 06:59:45 PDT
> It doesn't affect upgrading users

Irrelevant. There are 100000 users every day, continuously, who try to set up accounts.
Comment 28 Mark Banner (:standard8) 2010-06-08 07:10:38 PDT
(In reply to comment #27)
> > It doesn't affect upgrading users
> Irrelevant. There are 100000 users every day, continuously, who try to set up
> accounts.

I think it is unlikely that anywhere near a significant proportion of those users are going to already have one account defined and fail the automatic ISP config and choose to select an existing SMTP server - which I believe are the prerequisites for this bug.
Comment 29 Ben Bucksch (:BenB) 2010-06-08 07:32:06 PDT
> and choose to select an existing SMTP server - which I believe are the
> prerequisites for this bug.

Not per comment 0.

Anyways, if 3.1 final (not candidate) is this week, I agree that 3.1.1 is more appropriate.
3.2 doesn't make sense, because bug 549045 would fix it anyways.
Comment 30 Mark Banner (:standard8) 2010-06-08 08:11:26 PDT
(In reply to comment #29)
> > and choose to select an existing SMTP server - which I believe are the
> > prerequisites for this bug.
> Not per comment 0.

Just to clarify, when I tested it and reproduced comment 0 the account wizard didn't end up broken, even though the warnings were produced. comment 2 has the steps to repeat the broken wizard.

Given where we are and the expected low level of impact on users, we're not currently going to block 3.1 final on it. We will relnote it and block 3.1.1 on it - so it would be good to get the unit tests written and a patch landed reasonably soon - but thanks for all the work so far.
Comment 31 Boying Lu 2010-07-06 19:45:58 PDT
Created attachment 456219 [details] [diff] [review]
test is based on comment 2
Comment 32 Blake Winton (:bwinton) (:☕️) 2010-07-21 07:25:42 PDT
Comment on attachment 456219 [details] [diff] [review]
test is based on comment 2

So, in general I like this test, and thank you very much for writing it.

>+++ b/mail/test/mozmill/account/test-retest-config.js	Wed Jul 07 10:47:06 2010 +0800
>@@ -0,0 +1,126 @@
>+  if ( right > wizard_window.boxObject.height || bottom > wizard_window.boxObject.width)
>+    throw new Error("The autoconfig wizard dialog is broken");

My only two complaints are:
1) Please break the if line after the "||" to keep it under 80 characters, and
2) Can I please have a better error message here.  ;)  I can't tell what's broken from that message.

I'm hoping it'll end up something like:
  if (right > wizard_window.boxObject.height ||
      bottom > wizard_window.boxObject.width)
    throw new Error("The start over button didn't collapse the window.");

r=me with those nits fixed.

Comment 33 Blake Winton (:bwinton) (:☕️) 2010-07-21 07:27:04 PDT
Comment on attachment 449549 [details] [diff] [review]
Fix, v5

(And now that there's a test that fails without this and succeeds with it, I'm happy with this patch.)

Comment 34 WADA 2010-07-21 23:45:01 PDT
xref bug 575422 for ease of tracking.
Comment 35 Boying Lu 2010-07-22 01:31:49 PDT
Created attachment 459346 [details] [diff] [review]
updated patch
Comment 36 Ben Bucksch (:BenB) 2010-07-22 03:51:31 PDT
*** Bug 575422 has been marked as a duplicate of this bug. ***
Comment 37 Mark Banner (:standard8) 2010-07-23 08:13:04 PDT
Now that the tree has re-opened, I've checked both patches in:

Thanks to all those that have worked on this.
Comment 38 Mark Banner (:standard8) 2010-07-23 14:55:50 PDT
Unfortunately I had to back this out due to test failures:

There was one xpcshell-failure, and one or two mozmill failures on both Mac and Windows:

(ignore the test-message-header.js failures in that file).

xpcshell-test failure:

TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | Hostname is empty or contains forbidden characters. Only letters, numbers, - and . are allowed. - See following stack:
JS frame :: chrome://messenger/content/accountcreation/util.js :: Exception :: line 179
JS frame :: chrome://messenger/content/accountcreation/sanitizeDatatypes.js :: MalformedException :: line 234
JS frame :: chrome://messenger/content/accountcreation/sanitizeDatatypes.js :: anonymous :: line 133
JS frame :: chrome://messenger/content/accountcreation/readFromXML.js :: readFromXML :: line 89
JS frame :: e:/buildbot/win32-comm-central-check/build/objdir/mozilla/_tests/xpcshell/test_mailnewsbase/unit/test_autoconfigXML.js :: test_replaceVariables :: line 265
JS frame :: e:/buildbot/win32-comm-central-check/build/objdir/mozilla/_tests/xpcshell/test_mailnewsbase/unit/test_autoconfigXML.js :: run_test :: line 302
JS frame :: e:\buildbot\win32-comm-central-check\build\mozilla\testing\xpcshell\head.js :: _execute_test :: line 166
-- Exception object --
+ _message (string) 'Supplied value not in allowed list'
+ stack (string) 799 chars
+ message (string) 'Supplied value not in allowed list'
+ toString (function) 3 lines
-- Stack Trace --
Exception("Supplied value not in allowed list")@chrome://messenger/content/accountcreation/util.js:179
MalformedException("allowed_value.error",[object XML])@chrome://messenger/content/accountcreation/sanitizeDatatypes.js:234
([object XML],[object Object])@chrome://messenger/content/accountcreation/sanitizeDatatypes.js:219
readFromXML([object XML])@chrome://messenger/content/accountcreation/readFromXML.js:190

MozMill failures:

NEXT ERROR TEST-UNEXPECTED-FAIL | e:\buildbot\win32-comm-central-check\build\mail\test\mozmill\account\test-mail-account-setup-wizard.js | test_mail_account_setup
  EXCEPTION: timeout exceeded for waitForEval('subject._currentConfigFilledIn != null')
    at: controller.js line 499
       Error("timeout exceeded for waitForEval('subject._currentConfigFilledIn != null')")  0
            controller.js 499
       test_mail_account_setup() test-mail-account-setup-wizard.js 136
            frame.js 474
            frame.js 530
            frame.js 573
            frame.js 417
            frame.js 579
            server.js 164
            server.js 168
TEST-PASS |  setupModule
NEXT ERROR TEST-UNEXPECTED-FAIL | e:\buildbot\win32-comm-central-check\build\mail\test\mozmill\account\test-retest-config.js | teardownTest
  EXCEPTION: aController is undefined
    at: test-window-helpers.js line 540
       plan_for_window_close((void 0)) test-window-helpers.js 540
       close_window((void 0)) test-window-helpers.js 557
       teardownTest(test_re_test_config) test-retest-config.js 71
            frame.js 469
            frame.js 537
            frame.js 573
            frame.js 417
            frame.js 579
            server.js 164
            server.js 168
NEXT ERROR TEST-UNEXPECTED-FAIL | e:\buildbot\win32-comm-central-check\build\mail\test\mozmill\account\test-retest-config.js | test_re_test_config
  EXCEPTION: Timed out waiting for window open!
    at: test-window-helpers.js line 193
       Error("Timed out waiting for window open!")  0
       WindowWatcher_waitForWindowOpen("mail:autoconfig") test-window-helpers.js 193
       wait_for_new_window("mail:autoconfig") test-window-helpers.js 504
       open_mail_account_setup_wizard() test-retest-config.js 67
       test_re_test_config() test-retest-config.js 82
            frame.js 474
            frame.js 530
            frame.js 573
            frame.js 417
            frame.js 579
            server.js 164
            server.js 168
Comment 39 Mark Banner (:standard8) 2010-07-28 12:48:09 PDT
Ben or Stefan, can one of you guys take a look at the test failures? See if there's something obvious going on there?
Comment 40 Mark Banner (:standard8) 2010-08-24 06:54:55 PDT
I've taken a brief look at this, and either the patch is broken or the unit tests need broken. I'm not into this enough to work it out.

Reassigning as this doesn't appear to be going anywhere and we could really do with fixing it for our users.
Comment 41 Blake Winton (:bwinton) (:☕️) 2010-09-14 18:18:17 PDT
Created attachment 475365 [details] [diff] [review]
A patch which includes tests, and fixes other tests.

Okay, here's both the previous patches, merged, and with the test failures fixed.

Coming up, the diff of what I changed, to make Ben's job of reviewing easier.
Comment 42 Blake Winton (:bwinton) (:☕️) 2010-09-14 18:21:16 PDT
Created attachment 475368 [details] [diff] [review]
What I changed to get the test working.

There you go.  Simple enough.  The case that was failing was:
which has a placeholder, but also has other stuff.

(And, of course, that one failing test meant that we didn't use the example config, so we started searching for hostnames, which timed out, and then nothing closed, so we caused later tests to fail.  All in all, quite the annoying cascade of failures.)

Comment 43 Ben Bucksch (:BenB) 2010-09-15 10:17:16 PDT
I don't feel terribly comfortable with this. Would it be too much to ask to implement the hostnameOrPlaceholder()?
That would make the regexp easier again, because you could use the regexp we used before. The difference between the two functions would just be the addition of % as valid char and the omission of toLowerCase().
We'd then use hostnameOrPlaceholder() where placeholders are allowed, and hostname() where we expect a concrete one.
Comment 44 Ben Bucksch (:BenB) 2010-09-15 10:42:43 PDT
Comment on attachment 475368 [details] [diff] [review]
What I changed to get the test working.

Please alloe [A-Z0-9] for placeholders. With that, r=BenB
Comment 45 Blake Winton (:bwinton) (:☕️) 2010-09-29 10:35:12 PDT
Created attachment 479443 [details] [diff] [review]
A patch to fix the error with re-test configuration.

Fixed Ben's final comment.

Carrying forward r=BenB, sr=Standard8.
Comment 46 Mark Banner (:standard8) 2010-09-29 10:59:14 PDT
Checked in:
Comment 47 Mark Banner (:standard8) 2010-09-29 23:36:51 PDT
Checked into 1.9.2:
Comment 48 Mark Banner (:standard8) 2010-10-01 07:03:41 PDT
Comment on attachment 479443 [details] [diff] [review]
A patch to fix the error with re-test configuration.

a=Standard8, I'll land this in a few mins along with some other bugs.
Comment 49 Ludovic Hirlimann [:Usul] 2010-10-05 07:16:51 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv: Gecko/20101004 Thunderbird/3.1.5
Comment 50 [:Aureliano Buendía] 2010-11-23 05:41:48 PST
*** Bug 604325 has been marked as a duplicate of this bug. ***
Comment 51 Andrew Sutherland [:asuth] 2011-03-03 19:07:03 PST
hey, so I just got an intermittent orange on test_re_test_config in test-retest-config.js, the test added for this bug...

I could not help but notice that the e-mail address used for the test is "", that we then actually do a bunch of DNS lookups against, and even try and talk to the POP server.  (It took my local run 30 seconds to get a RST response to my SYN packets...*).  This probably explains why the test has a 100second timeout for the first step and 20 second timeouts for the following tests.

Can we back this test out entirely in preparation for the next phase of autoconfig stuff?  If not, can we make it locally standalone (it looks like test-mail-account-setup-wizard.js might operate along those lines), or failing that, point at MoMo-hosted test servers?

* Should I have gotten an actual response?  Is it possible that MoMo YVR's IP is blacklisted by Yahoo's pop server because our mac minis are poking it all day long with bogus logins?
Comment 52 Blake Winton (:bwinton) (:☕️) 2011-03-03 19:40:41 PST
Yeah, using seems like a bug here.  There was another bug where we switched from hitting external servers to local ones, but I forget which bug that was.

Making it locally standalone seems like the best option to me.  I would also accept commenting out the test if we file a bug to figure out another way to test this behaviour…
Comment 53 Mark Banner (:standard8) 2011-03-04 01:18:34 PST
(In reply to comment #52)
> Yeah, using seems like a bug here.  There was another bug where we
> switched from hitting external servers to local ones, but I forget which bug
> that was.

You're probably thinking of unresolved bug 604356.

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