568153 - Re-test Configuration doesn't work: account.incoming.hostname.toUpperCase is not a function

Status: VERIFIED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Kohei Yoshino [:kohei]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.4) Gecko/20100521 Thunderbird/3.1

How to test:

1. open Mail Account Setup wizard
2. fill in the form ([e-mail])
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)]

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]

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. [e-mail], but the server is on e.g. imap.strato.de, and trying to use an smtp account already set up (e.g. smtp.strato.de).

Slightly different STR:
0. Have an SMTP account set up (smtp.strato.de in my case).
1. open Mail Account Setup wizard
2. fill in the form (Username: test, Email: [e-mail])
   (which is much faster than trying example.com)
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]

After step 4, clicking Manual Setup instead of Re-test configuration is broken as well and displays the error above!

Comment 4

[Steffen Wilberg]

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)]

I have also removed this check, see bug 549045 comment 77 and attachment 442834 [details] [diff] [review], first hunk.

Comment 6

[Steffen Wilberg]

So what's the minimal fix here, short of taking all of bug 549045?

Comment 7

[Steffen Wilberg]

Applying attachment 442834 [details] [diff] [review], first hunk, from bug 549045, regresses bug 546278:

[e-mail] results in:
Incoming: mail.%emaildomain% POP 110 None
Outgonig: smtl.%emaildomain% SMTP 25 None

Comment 8

[Ben Bucksch (:BenB)]

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]

But how do we fix the "account.incoming.hostname.toUpperCase is not a function" error (comment 0)?

Comment 10

[Ben Bucksch (:BenB)]

I don't know. "if (account.incoming.hostname)"?

Comment 11

[Steffen Wilberg]

Nope, same error, with or without sanitize.hostname.

Comment 12

[Steffen Wilberg]

Attachment: 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)]

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 ".example.com" 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)]

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]

Attachment: 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)]

Attachment: 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)]

Attachment: Fix, v4

Arg, I'm stupid. New try.

Comment 18

[Steffen Wilberg]

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)]

Attachment: Fix, v5

Thanks.

Comment 20

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

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…)

Thanks,
Blake.

Comment 21

[Ben Bucksch (:BenB)]

> 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) (:☕️)]

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.  :)

Thanks,
Blake.

Comment 23

[Ben Bucksch (:BenB)]

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]]

So is this not a 3.1 blocker? I think many users will encounter the issue.

Comment 25

[Ben Bucksch (:BenB)]

I also think we should fix it, setting block-tb3.1 to ?

Comment 26

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

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.

Later,
Blake.

Comment 27

[Ben Bucksch (:BenB)]

> 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)]

(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)]

> 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)]

(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]

Attachment: test is based on comment 2

Comment 32

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

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.

Thanks,
Blake.

Comment 33

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

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.)

Later,
Blake.

Comment 34

[WADA:World Anti-bad-Duping Agency]

xref bug 575422 for ease of tracking.

Comment 35

[Boying Lu]

Attachment: updated patch

Comment 37

[Mark Banner (:standard8)]

Now that the tree has re-opened, I've checked both patches in:

http://hg.mozilla.org/comm-central/rev/f057c9009f60
http://hg.mozilla.org/comm-central/rev/534989dddc68

Thanks to all those that have worked on this.

Comment 38

[Mark Banner (:standard8)]

Unfortunately I had to back this out due to test failures:

http://hg.mozilla.org/comm-central/rev/d1e7997e8d2b
http://hg.mozilla.org/comm-central/rev/d1a8582347a4

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

http://tinderbox.mozilla.org/showlog.cgi?tree=Thunderbird&errorparser=unittest&logfile=1279902118.1279910328.31838.gz&buildtime=1279902118&buildname=WINNT%205.2%20comm-central%20check&fulltext=

(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
test_readFromXML_config1()@e:/buildbot/win32-comm-central-check/build/objdir/mozilla/_tests/xpcshell/test_mailnewsbase/unit/test_autoconfigXML.js:197
run_test()@e:/buildbot/win32-comm-central-check/build/objdir/mozilla/_tests/xpcshell/test_mailnewsbase/unit/test_autoconfigXML.js:301
_execute_test()@e:\buildbot\win32-comm-central-check\build\mozilla\testing\xpcshell\head.js:166


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)]

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)]

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) (:☕️)]

Attachment: 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) (:☕️)]

Attachment: What I changed to get the test working.

There you go.  Simple enough.  The case that was failing was:
<hostname>testin.%EMAILDOMAIN%</hostname>
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.)

Later,
Blake.

Comment 43

[Ben Bucksch (:BenB)]

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)]

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) (:☕️)]

Attachment: 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)]

Checked in: http://hg.mozilla.org/comm-central/rev/3b4c41cd5a42

Comment 47

[Mark Banner (:standard8)]

Checked into 1.9.2: http://hg.mozilla.org/releases/comm-1.9.2/rev/f78d6c4e2498

Comment 48

[Mark Banner (:standard8)]

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]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.2.11) Gecko/20101004 Thunderbird/3.1.5

Comment 51

[Andrew Sutherland [:asuth] (he/him)]

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 "[e-mail]", that we then actually do a bunch of DNS lookups against yahoo.com, 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) (:☕️)]

Yeah, using yahoo.com 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)]

(In reply to comment #52)
> Yeah, using yahoo.com 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.