123068 - SMTP error does not tell me which server it can't connect to

Status: RESOLVED FIXED

(MailNews Core :: Networking: SMTP, defect)

Description

[Jesse Ruderman]

The SMTP error message added in bug 114060 does not tell me which server it 
failed to connect to.  It should tell me which server it was trying to connect 
to, and whether the server was my default SMTP server or an account-specific 
SMTP server.

Here's the current text of the message:
"Sending of message failed.
An error occurred sending mail: Unable to connect to the SMTP server.  The 
server may be down or may be incorrectly configured.  Please verify that your 
Mail/News account settings are correct and try again."


Reasons for including the server name in the dialog:

1. My profile became corrupted so that my mail.softhome.net account was set to 
send mail through "" rather than the default "odin.ac.hmc.edu".  I did not 
notice this corruption while looking through the account settings dialog 
because the account-specific SMTP settings are hidden under an "Advanced..." 
button in one of the panels.  As a result, I assumed Mozilla was having trouble 
connecting to odin.ac.hmc.edu rather than that it was connecting to the wrong 
server.

2. I might have forgotten to switch SMTP servers when moving between home, 
where I use mail1.pv.ca.home.com, and college, where I use odin.ac.hmc.edu.  
Seeing the server name in the error dialog would remind me to make that change.

3. I might have moved switched ISPs recently and changed the default SMTP 
server to the correct value, but forgotten that I had set an account-specific 
SMTP server for one of my accounts.  Seeing the address-specific mail server 
name appear in the dialog would at least help me narrow down the problem 
to "Mozilla is trying to connect to the wrong server".

4. It's inconsistent with other dialogs in mailnews and in the browser, which 
usually include the server hostname.

Comment 1

[Cavin Song]

Attachment: Proposed patch, v1

Just a note: nsMsgSend.cpp uses nsTextFormatter::smprintf() calls to format
strings in a few places so that's what I use in the patch as well. Should we be
using string bundle's FormatStringFromID() call like the following?

hostStr.AssignWithConversion(hostName.get());
const PRUnichar *params[] = { hostStr.get() };
rv = sBundle->FormatStringFromID(stringID, params, 1, &ptrv);

Comment 2

[Jean-Francois Ducarroz]

looks good but I wonder if there is a better way to get the smtp server info!
The smtp service should have already decided which server to use and therefore
instead of redoing the job in nsMsgSend, we should be some how able to retreive
that info from the smtp service!

Comment 3

[Scott MacGregor]

Comment on attachment 83616 [details] [diff] [review]
Proposed patch, v1

Cavin, I'd probably just use the formatting method that comes with the string
bundle (FormatStringFromID) like you mentioned.

Do that and sr=mscott

Comment 4

[Cavin Song]

Attachment: Proposed patch, v2

Changes made:

1. Store the chosen smtp server (for sending mail msg) in nsSmtpService and
provide a new method to get the hostname.

2. Use FormatStringFromID() (from nsIStringBundle) to format string.

Comment 5

[Scott MacGregor]

I'm sorry Cavin. I actually disagree with the suggestion of adding a method to
the nsISmtpService for getting the smtp server host name. The smtp service is a
service and as such processes multiple send requests for all accounts /
identities. It shouldn't have any special cache of the last used smtp host. What
if I quickly sent another compose window after I click send on the first one.
You could get the wrong one back from this method. 

I thought the code you originally had was the appropriate thing to do. If you
wanted to make that easier by putting that code snippet in a method off
nsISmtpService, that's fine but not required IMHO.

Comment 6

[Jean-Francois Ducarroz]

Comment on attachment 83616 [details] [diff] [review]
Proposed patch, v1

r=ducarroz

Comment 7

[Cavin Song]

Ok, Scott I see your point.

Comment 8

[Cavin Song]

Attachment: Proposed patch, v3

OK, I move the code to figure out the right smtp server for a given identity to
a new nsSmtpService method. This way we won't have duplicate code doing the
same thing.

Comment 9

[Scott MacGregor]

Comment on attachment 83829 [details] [diff] [review]
Proposed patch, v3

nice!
sr=mscott

Comment 10

[Jean-Francois Ducarroz]

Comment on attachment 83829 [details] [diff] [review]
Proposed patch, v3

I like it! R=ducarroz

Comment 11

[Cavin Song]

Fix checked in.

Comment 12

[Aleksey Nogin]

This commit have added a warning:

+mailnews/compose/src/nsSmtpService.cpp:901
+ `nsresult rv' might be used uninitialized in this function
+

Indeed, nsSmtpService::GetSmtpServerByIdentity(aSenderIdentity, aSmtpServer) can
return an uninitialized nsresult in the (very unlikely) case it's called with
the NULL aSenderIdentity and aSmtpServer that points to a non-NULL pointer.