210867 - mozilla should use esmtp size extension (rfc1870)

Status: RESOLVED FIXED

(MailNews Core :: Networking: SMTP, enhancement)

Description

[luca]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; es-AR; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Win98; es-AR; rv:1.4) Gecko/20030624

When sending mail to an esmtp server, mozilla should use the size extension to
avoid trying to send a message exceeding the size limit.
If the message is large enough it is useless to try to send it and fail after a
long time.

Reproducible: Always

Steps to Reproduce:
1. Try to send a message with a large attachment, exceeding the server size limit.
Actual Results:  
Mozilla will ignore the SIZE extension advertised by the server and try to send
the message.

Expected Results:  
Mozilla should have added a SIZE=xxxx to the "mail from" command, so the server
could reject the message right away, e.g.:

$ telnet localhost smtp
Trying 127.0.0.1...
Connected to localhost.localdomain (127.0.0.1).
Escape character is '^]'.
220 mail.wetron.es ESMTP Postfix
ehlo pippo
250-mail.wetron.es
250-PIPELINING
250-SIZE 20000000
250-ETRN
250-STARTTLS
250 8BITMIME
mail from:<[e-mail]> SIZE=30000000
250 Ok
rcpt to:<[e-mail]>
552 Message size exceeds fixed limit

Comment 1

[Christian Eyrich]

taking

Comment 2

[Christian Eyrich]

Attachment: proposed patch

This patch implements the extension for message size declaration.

I defined two new messages for the error codes 452 and 552 recognized. The
servers response alone is often not very helpful. But I also wanted to include
it, because it could contain info about the current temporary limit or the
permanent limit for a recipient a.s.o.

The problem with NS_ERROR_SMTP_PERM_SIZE_EXCEEDED is, that it is also used in
the case where Mozilla detects if a message is to big by itself. But in this
case there's no server response we could show the user.
Also the displayed size restriction in the string could be irritating if the
server announces the keyword SIZE without the optional parameter or 0.

So I could
1. remove text and placeholder for the server response.
2. define another message for this case

Any other option? Any suggestions welcome.

Comment 3

[Christian Eyrich]

Reporter, no comments? I can surely implement something, but please complain
afterwards if you don't like it.

Stephen - some words from an experienced person?

Comment 4

[Stephen Donner [:stephend] Not actively reading bugmail]

Christian - you'll want David or Scott for this, I'm no use on eSMTP, I'm afraid.

Comment 5

[Christian Eyrich]

It's no review thing. But AFAIK you're experienced in Mozilla, you CC'ed me and
you're interested in this bug I guess.
It's mostly a user side question. What informations does the user want, how much
effort in creating messages should be made to inform the user?

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

I'm 'experienced' as QA, but not development.  And I never tested our SMTP
interaction ;-)

I CC:ed you because I saw you patched a few good SMTP bugs in the past, and have
actively worked alongside David on POP and IMAP4 stuff a fair bit.

I'm not clear on when we even hit the NS_ERROR_SMTP_PERM_SIZE_EXCEEDED error
case, so I'm definitely not a good source to ask about this bug ;-)

Comment 7

[Christian Eyrich]

Ok, so I'll wait for some time and if noone makes any suggestion I'll solve it
somehow.

Comment 8

[luca]

> Reporter, no comments?

That's me ;-)
Sorry, I've been out of the office (as I'll be again when I finished writing
this note).
Is the patch included in one of the nightlies?
If so I can download it and test it at home, but I can only test if it works for
me or not, I have no comments on the implementation details (since I have no
idea of mozilla internals).

Comment 9

[Christian Eyrich]

Comment on attachment 131567 [details] [diff] [review]
proposed patch

Ok, so I ask for rv and see what happens.

Comment 10

[David :Bienvenu]

I think defining two messages is probably the way to go.

Comment 11

[Christian Eyrich]

So I now defined

NS_ERROR_SMTP_TEMP_SIZE_EXCEEDED
The mail size exceeds a temporary size restriction. The server responded:  %s.
NS_ERROR_SMTP_PERM_SIZE_EXCEEDED_1
The mail size exceeds the global size restriction (%d bytes).
NS_ERROR_SMTP_PERM_SIZE_EXCEEDED_2
The mail size exceeds the global size restriction. The server responded:  %s.

Where 1 will be used if the restriction is announced in the EHLO response. And 2
will be used without if server refuses the mail at MAIL FROM or RCPT TO.
Anything about the wording?

Comment 12

[David :Bienvenu]

I think instead of mail, I would say the "message you are trying to send" or
something like that. Also, you should indicate that the message was not sent,
and that the user should try to reduce the size of the message and try again.

Comment 13

[Christian Eyrich]

Attachment: patch v2

Patch like previous one but with three different error messages and all quite
long and detailed.

This is a -puw patch, -pu available for checkin.

Comment 14

[David :Bienvenu]

this code looks cloned - if so, could you put it in a helper function?

+    if(m_responseCode == 452)
+        rv = nsExplainErrorDetails(m_runningURL,
+                NS_ERROR_SMTP_TEMP_SIZE_EXCEEDED, m_responseText.get());
+    else
+    if(m_responseCode == 552)
+        rv = nsExplainErrorDetails(m_runningURL,
+                NS_ERROR_SMTP_PERM_SIZE_EXCEEDED_2, m_responseText.get());
+    else
+        rv = nsExplainErrorDetails(m_runningURL,
+                NS_ERROR_SENDING_FROM_COMMAND, m_responseText.get());
+

also, nit, but else if should be on one line.

other than that, looks fine.

Comment 15

[Christian Eyrich]

> this code looks cloned - if so, could you put it in a helper function?

The if cascade is the same in both functions, and it could reside in a helper
function. But we'd save 2 lines and have a 39 bytes bigger .so file for what?
While it would be consistent if a change to these line has to be made, one can't
see what happens in the error case at one glance.
If you say so, I'll create decodeAddressError() or so. But I don't think it's right.

Comment 16

[David :Bienvenu]

tell you what - I'll try it before I checkin and see if it really produces more
code to break those 9 lines into a separate helper routine.  Or, you could write
this:

rv = nsExplainErrorDetails(m_runningURL,
               (m_responseCode == 452) ? NS_ERROR_SMTP_TEMP_SIZE_EXCEEDED :
((m_responseCode == 552) ? NS_ERROR_SMTP_PERM_SIZE_EXCEEDED_2 :
NS_ERROR_SENDING_FROM_COMMAND) , m_responseText.get());

with appropriate formatting, it should be quite readable.

Comment 17

[Christian Eyrich]

Attachment: patch v3

Like v2 but with changes according to comment #16.

Comment 18

[David :Bienvenu]

Comment on attachment 132592 [details] [diff] [review]
patch v3

r=bienvenu

Comment 19

[David :Bienvenu]

fix checked in, thx, Christian.

Comment 20

[Jo Hermans]

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