361433 - More descriptive error messages for network level error on sending mail

Status: RESOLVED FIXED

(MailNews Core :: Networking: SMTP, defect)

Description

[Christian Eyrich]

Currently only one type of message is displayed when the error is not a problem on SMTP protocol level. It basically just says "Unable to connect".
Analogical to retrieval (nsMsgProtocol::OnStopRequest()) we should use more detailed messages.

Comment 1

[Christian Eyrich]

Attachment: proposal for new texts

This patch introduces more error messages. Changed for host name unresolvable and default and new ones for connection refused, connection interrupted and connection timed out.
NOTE: The texts are only raw proposals. Please read them over if they're useful and could be even more specific.

Comment 2

[Christian Eyrich]

I'd also like to improve the behaviour if the user is prompted for a password but clicks Cancel in that dialogue. Currently the standard error "The message could not be sent because connecting to SMTP server ... failed." is displayed. That's nonsense.

Either we don't display a message or a really descriptive. I personally prefer no error, but I fear then some user will ask themselves if the message has been sent and if not, why not. More opinions?

Comment 3

[Magnus Melin [:mkmelin]]

> Either we don't display a message or a really descriptive. I personally prefer
> no error, but I fear then some user will ask themselves if the message has been
> sent and if not, why not. More opinions?

If the message compose window stays open after the cancel I imagine a user would assume it wasn't sent.

I don't know which status it gets for the very common case that a firewall and/or antivirus is causing problems. Maybe consider mentioning that antivirus and firewalls sometime are the cause in the relevant error message.

Comment 4

[Christian Eyrich]

(In reply to comment #3)
> If the message compose window stays open after the cancel I imagine a user
> would assume it wasn't sent.

If just composed, the compose window stays open. But when the mail is tried to be sent from Unsent Messages there's not compose window.
So assume you're against supressing an error, right?

> I don't know which status it gets for the very common case that a firewall
> and/or antivirus is causing problems. Maybe consider mentioning that
> antivirus and firewalls sometime are the cause in the relevant error message.

That status depends on the caused problem.
I've listed some quite common problems (some of them apply to SMTP) on http://www.eyrich-net.org/mozilla/mozvsav.html?en.
For error because not advertised STARTTLS keyword by the server I can imagine mentioning to check the antivirus. The current text is "An error occurred sending mail: Unable to connect to SMTP server %S via STARTTLS since it doesn't offer STARTTLS in its EHLO response. Please verify that your Mail/News account settings are correct and try again."

Or have you something special in mind?

Comment 5

[Magnus Melin [:mkmelin]]

(In reply to comment #4)
> If just composed, the compose window stays open. But when the mail is tried to
> be sent from Unsent Messages there's not compose window.
> So assume you're against suppressing an error, right?

I think that at least when compose window stays open no error message is needed. For Unsent Messages I think it would be fine without one too, I assume they still stay in the Unsent folder. 

> Or have you something special in mind?

Nothing special. Come to think of it, maybe it's more of a POP problem since that is usually used first.

Comment 6

[Christian Eyrich]

Attachment: extended patch

This one adds a more specific error in case the server refuses the recipient and an extra message if password couldn't gathered (e.g. cancelling the password prompt - the current error is very missleading).
Also relaxed some tests for response codes.

Comment 7

[Christian Eyrich]

David, what do you think on that?

Comment 8

[David :Bienvenu]

In general, this is all good - unfortunately, it won't make 2.0 since we're in a string freeze for 2.0.

Re cancelling the password prompt, I tend to agree that if we return to the compose window, we really don't need to tell the user that the message wasn't sent because they cancelled the password prompt. But a correct error message is better than an incorrect one.

But if the user cancels the password on the fcc (in the imap case), I think we should tell the user about that...

Comment 9

[Christian Eyrich]

> In general, this is all good - unfortunately, it won't make 2.0 since we're
> in a string freeze for 2.0.

Unfortunate but I didn't aim at 2.0 anyway.

I had the idea to remove the alert in the pw cancel case completely. But asking in the german mozilla newsgroup on that, I only got responses to keep but correct the message.

> But if the user cancels the password on the fcc (in the imap case), I think
> we should tell the user about that...

I was concentrating on the SMTP part itself, but you're right fcc belongs to the send process. Have to test what it currently does.

Comment 10

[Tuukka Tolvanen (sp3000)]

Now if the dialog asking for credentials described what action they were for with as much specificity as an alert after Cancel would, ...

Comment 11

[Christian Eyrich]

Attachment: up-to-date patch

This is mainly an update of the last patch to current trunk.
I don't exactly know what Tuukka meant, but I extended the password prompts title to make clear it's the SMTP server we need the password for.

Comment 12

[David :Bienvenu]

Thx, in general, this looks good. There are a couple places where the braces style needs to be fixed, but I could do that once you attach a -u patch.

Are these the places where you've relaxed the response code checking?

Does that mean in this case, we'll never get here with an OK (250) response?

-  if((m_responseCode != 354) && (m_responseCode != 250)) {
+  if (m_responseCode/10 != 35)
+  {
     nsresult rv = nsExplainErrorDetails(m_runningURL, NS_ERROR_SENDING_DATA_COMMAND, m_responseText.get());
     NS_ASSERTION(NS_SUCCEEDED(rv), "failed to explain SMTP error");
 

And we won't get here with a 354 - which I take it means send the msg...?

@@ -1534,8 +1540,7 @@ PRInt32 nsSmtpProtocol::SendPostData()
 
 PRInt32 nsSmtpProtocol::SendMessageResponse()
 {
-
-  if(((m_responseCode != 354) && (m_responseCode != 250)) || CHECK_SIMULATED_ERROR(SIMULATED_SEND_ERROR_12)) {
+  if((m_responseCode/10 != 25) || CHECK_SIMULATED_ERROR(SIMULATED_SEND_ERROR_12)) {

Some nits about the error messages:

+12591=The message could not be sent because connection to SMTP server %S went away in midst of the transaction. Try again or contact your network administrator.

I think probably instead of "went away in midst of the transaction", maybe "stopped responding in the middle of the operation".

"The message was not sent, reduce the message size and try again."

There are several messages like this. Technically, these are two sentences, and it might be better to separate them with a ';' instead of a ','. But that's me being incredibly picky!

Comment 13

[David :Bienvenu]

Comment on attachment 272468 [details] [diff] [review]
up-to-date patch

r=bienvenu, but I'd like a -u patch to try out...

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 272468 [details] [diff] [review]
up-to-date patch

Not that I'm a native English speaker, but shouldn't it be "your SMTP server settings" (with an s on the end)?

SMTP server %S in unknown. -> SMTP server %S is unknown.

The server responded:  %s.
Seems it was like this before too, but why the extra space?

could not be sent because connection -> could not be sent because the connection

Comment 15

[Christian Eyrich]

(In reply to comment #12)
Two braces indentations in AuthLoginStep1 and step2 seem to be wrong in the -uw but right in the -u patch for unknown reason. But if you want the braces which are
... {
instead of
...
{
to be fixed I can do that for the -u patch too. I like the latter style more but didn't want to change too much style only stuff.

Regarding the code 250 and 354 respectively you're right. In both cases only that single response is specified and we've only a single path to that code.

Replacing that , by ; (I see three cases) is ok for me.

> I think probably instead of "went away in midst of the
> transaction", maybe "stopped responding in the middle of
> the operation".

It's generated by NS_ERROR_NET_INTERRUPT from the network, so I understand that we've been disconnected. "Stopped responding" is technically different for me, but if you think that would be more apprehensible for users I've to belief that.

Comment 16

[Christian Eyrich]

(In reply to comment #14)
> Not that I'm a native English speaker

Me too, so I appreciate any comments.

> but shouldn't it be "your SMTP server settings" (with an
> s on the end)?

I think you're right. BTW, what about "your SMTP server's settings"?

> The server responded:  %s.
> Seems it was like this before too, but why the extra
> space?

Don't know, we've two spaces in many strings so I guess they've been added intentionally.

Comment 17

[Magnus Melin [:mkmelin]]

(In reply to comment #16)
> I think you're right. BTW, what about "your SMTP server's settings"?

At least to me that sounds like it were some setting on the server side.

Comment 18

[David :Bienvenu]

yes, I definitely prefer the braces style

if (a)
{
}
else
{
}

Re SMTP server setting(s), it might depend on if we're talking about the server name (which I would think of as the "server setting", or some attribute of the server, e.g., the port # or auth method, which I would think of as one of the "server settings". But I think "server settings" is more generally correct, if you think of the host name as one of the attributes.

re NS_ERROR_NET_INTERRUPT, you're right, stopped responding is incorrect, how about:

The message could not be sent because the connection to SMTP server %S was broken in the middle of the operation.

or "was lost" or "was interrupted".

Comment 19

[Christian Eyrich]

Attachment: -u patch addressing comments #12 and #14

I agree what you both wrote on "server setting" and "server's settings".

This one addresses the comments. I also changed one response code check back to
  m_responseCode != 354
because 354 is the only specified response code in the 35x (even 3xx) range.

Comment 20

[David :Bienvenu]

Comment on attachment 273126 [details] [diff] [review]
-u patch addressing comments #12 and #14

+12518=The message could not be posted because connecting the news server failed. The server may be unavailable or is refusing connections. Please verify that your news server settings are correct and try again, or else contact your network administrator.


this shouldn't have changed - it should still say "connecting to" since there's no news server name here. 

Other than that, it looks good.

Comment 21

[Christian Eyrich]

Attachment: another try

Fixed string 12518.

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 273129 [details] [diff] [review]
another try

12590 "because connecting the SMTP"  should also be "connecting to"

Comment 23

[Christian Eyrich]

I can't decide for one. At the moment I'd vote for "because connecting SMTP server %S failed."
Suggestion from a native speaker, David?

Comment 24

[David :Bienvenu]

Magnus is right - it should be "connecting to..."

Comment 25

[Christian Eyrich]

Attachment: addressing comment #22

Comment 26

[David :Bienvenu]

Comment on attachment 273397 [details] [diff] [review]
addressing comment #22

looks good, Thx, Christian.

Comment 27

[Scott MacGregor]

Comment on attachment 273397 [details] [diff] [review]
addressing comment #22

looks good. Thanks Christian.

Comment 28

[Karsten Düsterloh]

Landed on trunk.

Comment 29

[David :Bienvenu]

thx for landing that, Karsten, and thx for the patch, Christian.