1079280 - Thunderbird does not handle a possible "NO / BAD" answer to a non-literal+ append command

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[thomas.cataldo]

Attachment: Patch for the nsImapProtocol.cpp issue

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.101 Safari/537.36

Steps to reproduce:

Tried to append a Draft with a big (bigger than a configured server ratio) attachment, the server refuses with <tag> NO message is too big.


Actual results:

Thunderbird pops a notification with the "message is too big" text, but proceeds with the literal upload. Of course the server parser is not expecting a literal at this point.


Expected results:

The upload should have been aborted.

Comment 1

[WADA:World Anti-bad-Duping Agency]

Tb's behavior when NO to append, when BAD to fllowing data send, is knwn in Bug 693353.
Setting dependncy to this bug.

Comment 2

[thomas.cataldo]

Thanks for the link to the other bug. I don't know how I could speed up the review process for my patch.

Comment 3

[Nikolay Shopik]

Put review flag on patch and ask one of peers/owners of thunderbird module
https://wiki.mozilla.org/Modules/Thunderbird

Comment 4

[Wayne Mery (:wsmwk)]

Comment on attachment 8501085 [details] [diff] [review]
Patch for the nsImapProtocol.cpp issue

David is no longer active in reviews

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 8501085 [details] [diff] [review]
Patch for the nsImapProtocol.cpp issue

Review of attachment 8501085 [details] [diff] [review]:
-----------------------------------------------------------------

Passing to Irving. With my very limited knowledge of this, the patch looks ok though

Comment 6

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8501085 [details] [diff] [review]
Patch for the nsImapProtocol.cpp issue

Review of attachment 8501085 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch. I'll r+ this for landing, but if you're interested it would be great to have an automated test case for this. You can see an example of a test for IMAP append in http://dxr.mozilla.org/comm-central/source/mailnews/imap/test/unit/test_saveImapDraft.js.

Comment 7

[thomas.cataldo]

Thanks for the review. Would be happy to add a test case but this one requires carefully crafted server responses, eg:
C: A0 login bla bla 
S: A0 OK Complete.
C: A1 APPEND Drafts {6666666666}
S: A1 NO message is too big.

Then ensure that the client does not send anything after that.

I don't really see how to craft this dialog from saveImapDraft.js.

Comment 8

[WADA:World Anti-bad-Duping Agency]

Bug 47311 was found. It looks pretty old problem.
Setting dependency for ease of tracking.

Comment 9

[thomas.cataldo]

I don't think my patch depends #47311 but it fixes it too.

Comment 10

[Wayne Mery (:wsmwk)]

> I don't really see how to craft this dialog [test] from saveImapDraft.js.

Gene, can you offer advice?

Comment 11

[gene smith]

(In reply to Wayne Mery (:wsmwk) from comment #10)
> > I don't really see how to craft this dialog [test] from saveImapDraft.js.
> 
> Gene, can you offer advice?

I haven't looked closely at this yet but I don't think this involves just saving a draft that is too big. I think what this is about is moving a too large message from account A to B or dragging a too large .eml file to a folder so that the imap append command occurs and returns NO or BAD response.

I will try to duplicate this with a large email, maybe 30M or larger depending on server, not sure.

It looks like the attached patch is from v45 so it will also requires some changes to work with the latest.

Making a test for this would probably require changes to the fake server code. Is it worth it?

Another thing not clear to me yet is what problem does the user actually see. Only a pop-up error is mentioned, I think. Don't think tb hangs or crashes when this occurs.

Comment 12

[gene smith]

Attachment: 1079280-review-append-fix-v0.patch

Here's an updated patch. I have tested this with a real imap server (charter) that rejected my appended message as too large. I still see the helpful pop-up error message but, after tb sees the bad append response, it now no longer transmits the full email (e.g., 60Mbytes of data) that was already rejected by the server.

Note: This fix has no effect on the append of a too large message if literal+ capability is supported. In that case, the RFCs suggest that the server send an untagged imap BYE or tcp disconnect to stop the client's email transmission.

Testing with another server supporting literal+ appends (outlook), the full email is transmitted and then the server reports the email is too big in the BYE response and disconnects. Then tb reconnects and tries the append again, which it shouldn't. I don't see an error pop-up in this case at all. The same full email transmission occurs (but twice!) as without this patch.

Actually, the literal+ case looks like a bigger bug! Maybe a more effective fix would be to keep this patch but don't do literal+ when emails are appended regardless of the server's capability. It looks like literal+ only actually saves 1 response message from the server.

Also, I don't see a reason to include a test for this as part of the patch since it would require a change to fake server and this fix is better tested with real servers, like I just did.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8999071 [details] [diff] [review]
1079280-review-append-fix-v0.patch

Thanks, but this should be attributed to the original author, Thomas Cataldo <[e-mail]>.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/069924792281
Don't transmit email when imap append response is bad. r=Irving,GeneSmith DONTBUILD

Comment 15

[Jorg K (CEST = GMT+2)]

Thanks, Gene, I added you as reviewer since you tested this.

Comment 16

[Magnus Melin [:mkmelin]]

Thanks Gene! Would you mind filing a new bug based on your comment 12 (for the cases not covered by the patch landed)?

Comment 17

[gene smith]

(In reply to Magnus Melin from comment #16)
> Thanks Gene! Would you mind filing a new bug based on your comment 12 (for
> the cases not covered by the patch landed)?

Well, after testing with my many tb accounts, it appear the the problem I saw in comment 12 really only applies to outlook. Only outlook receives the file up to the maximum append size, flags an error and then disconnects. The disconnect is what causes the lack of an error notification.

All other servers that I tried that support literal+ (yahoo, aol, mdaemon zimbra dovecot ovh, (beta)newCloud) receive the whole message regardless of length, report the error and do not disconnect so the notification occurs. However, I'm not certain that there are not other servers or versions of these servers that have the same problem as outlook.

I have put this in new Bug 1484400.