100167 - Add server name to 'connection failed/refused' error messages.

Status: VERIFIED FIXED

(MailNews Core :: Networking, defect, P1)

Description

[risto.kankkunen]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.4) Gecko/20010913
BuildID:    2001091303

When starting mailnews without network connection, it pops up numerous identical
alerts: "Failed to connect to the server." This happens once for every expanded
news server you have configured, but to the user this is not obvious nor convenient.

Reproducible: Always
Steps to Reproduce:
1. Configure multiple news accounts
2. Expand the accounts in the account pane
3. Exit Mozilla
4. Unplug the network cable
5. Start Mozilla mailnews


Actual Results:  You get multiple identical alert boxes (see attachment).

Expected Results:  In increasing order of preference:

1. You should see alert boxes that show the source of the error. This would make
them more informative and not identical.

2. The error messages could be combined into one alert box listing all the error
that occured. (Maybe not feasible.)

3. You should get no alert boxes. Rather the server icons in the account pane
should change to an error icon showing there is a problem with that server. The
content pane should state the error status for the selected server.

Comment 1

[risto.kankkunen]

Attachment: Uninformative alert shown at startup

Comment 2

[risto.kankkunen]

Attachment: Patch to add server name to alert dialogs

Comment 3

[risto.kankkunen]

I added a patch to implement the first option to improve things: to include the
server name to the error message shown in the alert box. This affects both news
and imap server alerts. I'll attach an example of this more informative alert.

Comment 4

[risto.kankkunen]

Attachment: More informative alert shown at startup

Comment 5

[Keyser Sose]

Marking NEW so this can get looked at...Need a review

Comment 6

[R.K.Aa.]

*** Bug 104467 has been marked as a duplicate of this bug. ***

Comment 7

[Håkan Waara]

Nice patch Risto. It needs some work, below are some review comments:

1. Could you please use the nsIStringBundle API FormatStringFromName() rather
than nsTextFormatter? Just reuse the bundle object you have around, that you use
to fetch the .properties strings with.

For some sample code of how to use FormatStringFromName(), check out
nsNntpIncomingServer::GroupNotFound

2. Use nsXPIDLString when possible. nsXPIDLString is an "autoptr" (much like
nsCOMPtr), so you don't have to free it manually. It is freed automatically when
it goes out of scope. You can use nsXPIDLString as a replacement to PRUnichar*
(which is raw; you need to free it by hand), and do .get() on the nsXPIDLString
when you need to convert it to PRUnichar*.

3. Only change the en-US .properties file. The others are maintained by other
people, so we shouldn't touch them.

Thanks

Comment 8

[Karen Huang]

dup of bug 59682?

Comment 9

[risto.kankkunen]

Thanks for your comments. Now I need more!

> 1. Could you please use the nsIStringBundle API FormatStringFromName() rather
> than nsTextFormatter? Just reuse the bundle object you have around, that you use
> to fetch the .properties strings with.

Can you shed some light to how FormatStringFromName() is better? I don't see any
bundle object around, so it seems using that API would make the code more
complicated. GetStringByID() does all the .properties stuff in this file (though
it's used just here, I could convert it...).

> 2. Use nsXPIDLString when possible.

OK. I wanted to keep the changes to minimum in the first patch. Now I've changed
the PRUnichars to nsXPIDLStrings and at the same time simplified the structure
of the methods by adding some early returns. But now I have a problem: I made
two alternatives to convert the errorID into a string (see the #if 1 part). The
first one uses the horrid nsTextFormatter, the second is almost what was there
already. It seems stupid to use nsAutoString just for the AppendInt method, but
the real problem is that this doesn't work. When I changed the if() condition to
succeed, I got the real error message OVERWRITTEN with the [StringID... stuff
(i.e. "[StringID 102?]ct to server news.mozilla.org.")! It's like the Assign()
didn't set the string length right. Am I doing something silly or should I debug
this?

> 3. Only change the en-US .properties file.

OK, I have removed the other parts from the patch.

I'll attach a diff -wu patch, because the structural changes affected
indentation of many lines.

Comment 10

[risto.kankkunen]

Attachment: Patch to add server name to alert dialogs, v2, draft

Comment 11

[Håkan Waara]

1. Please don't change the return value stuff, it's much cleaner to wrap code 
around if-clauses and have one sole return at the end of the function rather 
than doing a bunch of early returns.  I think you should concentrate on your 
bugfix and not change to much unrelated, if possible. :)

2. Word on the street is that nsTextFormatter will soon be removed (AFAIK) and 
it makes use of C-strings which is the reason you should use the (slightly more 
complicated) API FormatStringByID(). That way I don't think you will need to 
much around with temporary strings too much.

3. Why are you using .Adopt(), is there a reason for this?  I think you may 
have confused that with a simple = assignment. :)

4. On a second though, I am thinking that the if (!errorMsgTemplate) clause 
should be either removed or wrapped with #if DEBUG since it looks like some 
debug test.

Comment 12

[scottputterman]

*** Bug 47866 has been marked as a duplicate of this bug. ***

Comment 13

[scottputterman]

reassigning to cavin.

Comment 14

[Karen Huang]

Adding this bug for bug 59682 dependance. The major bug is bug 59682... 

Comment 15

[Cavin Song]

Attachment: Patch to add server name to the alert msgs for imap, pop and news accounts.

Comment 16

[Jean-Francois Ducarroz]

Comment on attachment 63896 [details] [diff] [review]
Patch to add server name to the alert msgs for imap, pop and news accounts.

Looks good. R=ducarroz

Comment 17

[David :Bienvenu]

I don't like this because it makes all error messages with %S have to have the
server name as the first string.

Comment 18

[(not reading, please use [e-mail] instead)]

are the first two patches obsolete?

Comment 19

[Håkan Waara]

Cavin's patch uses some of risto's code (btw, I think risto should get credit
for that) - so yes they are obsolete AFAICS.

Comment 20

[David :Bienvenu]

OK, I've talked this over with Seth over IM and have the following comments:

 if (nsCRT::strcmp((const char *)scheme, "pop") == 0)
             scheme.Adopt(nsCRT::strdup("pop3"));
+        // we use "nntp" in the server list so translate it here.
+        if (nsCRT::strcmp((const char *)scheme, "news") == 0)
+            scheme.Adopt(nsCRT::strdup("nntp"));
         nsCOMPtr<nsIMsgAccountManager> accountManager = 
                  do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv);

we should fix these to be 
if (scheme.Equals("pop"))
  scheme.Assign("pop3");
else if (scheme.Equals("news"))
  scheme.Assign("nntp");

For the imap code, we suggest leaving the old GetImapStringByID() alone
and  make GetImapStringWithHostnameByID() do the formatting. Then you won't need
the extra routine. All you're saving is a call to GetStringBundle.

I think these routines that format the string with the hostname should make that
explicit by having the method name be something like:
FormatStringWithHostNameByID so that the callers will understand the string will
get formatted. 

Comment 21

[David :Bienvenu]

Here's my suggestion for rewriting GetImapStringByID:

NS_IMETHODIMP  nsImapIncomingServer::GetImapStringByID(PRInt32 aMsgId, PRUnichar
**aString)
{
  nsresult res = NS_OK;
  
  GetStringBundle();
  if (m_stringBundle)
  {
    res = m_stringBundle->GetStringFromID(aMsgId, aString);
    if (NS_SUCCEEDED(res))
      return res;
  }
  nsAutoString	resultString(NS_LITERAL_STRING("String ID "));
  resultString.AppendInt(aMsgId);
  *aString = ToNewUnicode(resultString);
  return NS_OK;
}

I don't think duplicating those lines of error handling code is bad. Or we could
just remove that code completely - we should never be asking for a string that
we don't have - we could just return an error and the empty string, like all the
other string bundle code does.

Comment 22

[Cavin Song]

Attachment: Made FormatStringWithHostNameByID a separate rotuine,fixed scheme.Adopt() & 2 obsolete error ids.

Comment 23

[David :Bienvenu]

you can use .Equals for more readable code, as I suggested earlier:

if (scheme.Equals("pop"))
  scheme.Assign("pop3");
else if (scheme.Equals("news"))
  scheme.Assign("nntp");

The rest looks fine. If you fix the code to use .Equals, r=bienvenu

Comment 24

[David :Bienvenu]

Comment on attachment 64472 [details] [diff] [review]
Made FormatStringWithHostNameByID a separate rotuine,fixed scheme.Adopt() & 2 obsolete error ids.

r=bienvenu, if you use .Equals

Comment 25

[(not reading, please use [e-mail] instead)]

1) switch to .Equals()
2) kStringBundleServiceCID, use the contract ID instead.
3) since you are right there, can you remove that #ifdef DEBUG_chrisf code

fix those before you check in, and sr=sspitzer

Comment 26

[Cavin Song]

Fix checked in.

Comment 27

[Cavin Song]

Really marking fixed.

Comment 28

[grylchan]

Using 2002-01-15-09-trunk on Nt 4.0 and I ran into this problem.

I noticed that %S is displayed instead of the server name:
  -create bogus mail acct or mistype a user id when creating a imap/web mail act
  -try to login to that acct
  -enter a password
  result: The mesg window says 'Login to server %S failed'
  expected: 'Login to server judge.mcom.com failed'

Works fine if you have a valid mail act and you mistype the password
as the mesg does not display %S and displays the server name instead.

Talked to Cavin already. Reopening

Comment 29

[Cavin Song]

OK, I found one problem in that if the login error is coming from "redirection" 
then it would cause the problem because in 
nsImapIncomingServer::OnLogonRedirectionError() the call to GetImapStringByID()  
should be changed to FormatStringWithHostNameByID(). I'll check all error 
numbers that I changed to make sure they are all fine.

That explains why putterman and I could not see the problem (ie, no redirection 
for our logins).

Comment 30

[Cavin Song]

*** Bug 120326 has been marked as a duplicate of this bug. ***

Comment 31

[David :Bienvenu]

You'll want to be careful there - I'm not sure we want to put the hostname in
the error messages for redirected servers since it may or may not be meaningful.
You'll have to check it out.

Comment 32

[Henrik Gemal]

the errors could be improved more.
"Connection to the mail server killer.gemal.dk using IMAP was refused"
"Connection to the mail server killer.gemal.dk using POP3 was refused"
"Connection to the news server news.gemal.dk using NNTP was refused"
etc
etc...

Comment 33

[David :Bienvenu]

we have common error messages for those errors, and I think putting the protocol
type in there is just going to confuse people who don't know what they mean, and
people who do know what they mean will know what they were doing in the first place.

Comment 34

[scottputterman]

From talking to Cavin it sounds like we'll be displaying the server that the
user has entered into Account Settings in the error message rather than the
server we get redirected to.  I think this may be ok since one of the reasons
behind this was to let the user know where the error occurred for actions that
can affect multiple accounts.  This would let them know what account is having
the problem (whereas as the server we redirect to might not).

Perhaps in this case we could just mention the account name that's having
problems if we don't want to give them false information.

Comment 35

[Cavin Song]

Attachment: Fix for the rediretion login error.

For aol and webmail accounts it'll display something like "Login to server
imap.mail.aol.com failed.", instead of "Login to server %S failed.".

Comment 36

[Cavin Song]

Seth and David, can you review the patch so that I can make it to 0.9.8?

Comment 37

[David :Bienvenu]

Is that what we want to see when we fail to connect to a web mail server?
imap.mail.aol.com? I thought we'd like to hide that from those users? Jennifer?

Comment 38

[scottputterman]

good point.  Maybe for the moment we just make it so that people can login and
not see the wrong error.  And then we can decide later what we'd want to put for
this case if anything.

Comment 39

[Cavin Song]

OK, the decision is to not show the server name at this point. I'll rework it 
and submit a patch soon.

Comment 40

[Cavin Song]

Attachment: Fix for the rediretion login error. v2

Added error number IMAP_REDIRECT_LOGIN_FAILED for the redirect login error. 
Let me know if the text string needs to be changed as I just used the original
one.

Comment 41

[David :Bienvenu]

Comment on attachment 65350 [details] [diff] [review]
Fix for the rediretion login error. v2

r=bienvenu

Comment 42

[(not reading, please use [e-mail] instead)]

Comment on attachment 65350 [details] [diff] [review]
Fix for the rediretion login error. v2

sr=sspitzer

Comment 43

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for 0.9.8

Comment 44

[Cavin Song]

Fix checked in.

Comment 45

[Karen Huang]

Verified fix on 05-22-08-1.0.0 builds that the error did specify the server name 
for IMAP, POP and News now.
I did see the following errors for IMAP, POP & News:
IMAP: Could not connect to mail server parp.mcom.com; the connection was 
refused.
POP: Could not connect to server parp.mcom.com; the connection was refused.
News: Could not connect to server poisonivy.mcom.com; the connection was 
refused.
Marking asverified.