161806 - Mail client changes format of attachments. LF -> CRLF

Status: NEW

(MailNews Core :: Attachments, defect)

Description

[Jeremy Mawson]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.0) Gecko/20020530
BuildID:    2002053012

When I attach a compiled Java class to an email the file is modified and becomes
corrupted. When the recipient attempts to execute the class the Java classloader
cannot recognise the file format.

I have confirmed this by sending two copies of a file, one via file transfer and
another by Mozilla. The file transfer version remained the same size and would
still run on the target machine. The emailed version was slightly larger and
would throw a java.lang.ClassFormatError from the Class Loader.

Mozilla is replacing LFs with CRLFs when it writes the mail. The corruption of
the attachment happens upon send from Mozilla, not upon receipt (I tried
receiving the attachment using different clients and they all displayed the
error when the mail was sent from Mozilla).

Reproducible: Always
Steps to Reproduce:
0. On windows only perhaps ...
1. Compose a new mail with an attachment that uses LF format
2. Send this email to anyone ... even yourself will do
3. Compare the original file with the received attachment. The LFs will have
grown to become CRLFs

Actual Results:  File corruption

Expected Results:  The attachment should have remained exactly as per the
original file. byte-for-byte, regardless of what platform it was sent from.

This is my first Bugzilla. Please be gentle. :)

Comment 1

[[e-mail] (gone - use [e-mail] instead)]

Attachment: Sendmail script

Yup, I see it also with this attachment.  Very annoying, as I send this script
from one machine to another (win2k --> linux) and by the time I try to read it
on my RedHat 8.0 box (using Vi), it's screwed up.

Comment 2

[[e-mail] (gone - use [e-mail] instead)]

Confirming with build 2002-12-13-08, Win2K.

Comment 3

[Jo Hermans]

RFC 2046 4.1.1

  "The canonical form of any MIME 'text' subtype MUST always represent a line  
   break as a CRLF sequence.  Similarly, any occurrence of CRLF in MIME 'text' 
   MUST represent a line break.  Use of CR and LF outside of line break 
   sequences is also forbidden."

All 'text' attachments should have their LF's converted to CRLF ???? I guess
that might be really annoying, unless we convert them back when saving the
text-files.

Comment 4

[Chris Rosewarne]

Confirmed with build 2003120808 on WinXP.

Comment 5

[Christian Franke]

Confirmed with build 20041012 on WinXP.

(In reply to comment #3)
> All 'text' attachments should have their LF's converted to CRLF ???? I guess
> that might be really annoying, unless we convert them back when saving the
> text-files.

Suggest the following heuristic: Only send as text/plain if *all* line endings
are identical to the Operating system's default (CRLF on Win, LF on Unix).


The current detection in nsMsgAttachmentHandler::AnalyzeDataChunk() is too weak
(See also bug 237118, comment 4):
http://lxr.mozilla.org/mozilla/source/mailnews/compose/src/nsMsgAttachmentHandler.cpp#199

  if (*s == nsCRT::CR || *s == nsCRT::LF) {
    if (s+1 < end && s[0] == nsCRT::CR && s[1] == nsCRT::LF)
      s++;

This code detects any mixed combination of CR, LF and CRLF as text file line
endings. If the remaining chars are considered printable, such a (binary!) file
will be sent as text/plain with all CR, LF converted to CRLF.

In this case it isn't even possible to repair the received file by some e.g.
DOS2UNIX converter.

Comment 6

[Christian Franke]

As a workaround, set "mail.file_attach_binary = true"

Comment 7

[Christian Franke]

This bug is about whether the newline encoding of "real" text files should
always be converted to destination OS's default (like e.g. CVS does) or not.

For the more serious mixed CR LF issue from comment 5, I filed a new bug 269390.

Comment 8

[Jo Hermans]

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

Comment 9

[Jo Hermans]

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

Comment 10

[Mike Cowperthwaite]

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

Comment 12

[Matěj Cepl]

This would be nice if fixed. And of course it is platform independent ... it screws up regularly patches I send among Linux computers.

Comment 13

[Zane U. Ji]

Attachment: Patch & Test cases

Comment 14

[David :Bienvenu]

this test also does not complete for me - here's the end of the log:

doTest 13
TEST-PASS | C:/mozilla-build/msys/tbirdhq/objdir-tb/mozilla/_tests/xpcshell/test
_compose/unit/test_attachment.js | [createMessage : 133] true == true
WARNING: NS_ENSURE_TRUE(parent) failed: file C:/mozilla-build/msys/tbirdhq/mailn
ews/base/src/nsMsgProgress.cpp, line 92
deliver mode: 4
spec=/161806_cr.txt
WARNING: malformed url: no scheme: file C:/mozilla-build/msys/tbirdhq/mozilla/ne
twerk/base/src/nsStandardURL.cpp, line 753
CopyListener::OnStartCopy()
nsMsgComposeSendListener::OnStartCopy()
CopyListener: SUCCESSFUL ON THE COPY OPERATION!
nsMsgComposeSendListener: Success on the message copy operation!
doTest 14
TEST-PASS | C:/mozilla-build/msys/tbirdhq/objdir-tb/mozilla/_tests/xpcshell/test
_compose/unit/test_attachment.js | [checkAttachmentBody : 287] 0 != 4294967295
WARNING: NS_ENSURE_TRUE(mHiddenWindow) failed: file C:/mozilla-build/msys/tbirdh
q/mozilla/xpfe/appshell/src/nsAppShellService.cpp, line 420
pldhash: for the table at address 03EEE460, the given entrySize of 48 probably f
avors chaining over double hashing.
++DOCSHELL 03EEE3F8 == 1
pldhash: for the table at address 03F3BF90, the given entrySize of 52 probably f
avors chaining over double hashing.
++DOMWINDOW == 1 (03FD9F10) [serial = 1] [outer = 00000000]

THE DESTRUCTOR FOR nsMsgComposeAndSend() WAS CALLED

THE DESTRUCTOR FOR nsMsgComposeAndSend() WAS CALLED

THE DESTRUCTOR FOR nsMsgComposeAndSend() WAS CALLED

THE DESTRUCTOR FOR nsMsgComposeAndSend() WAS CALLED

THE DESTRUCTOR FOR nsMsgComposeAndSend() WAS CALLED

THE DESTRUCTOR FOR nsMsgComposeAndSend() WAS CALLED

THE DESTRUCTOR FOR nsMsgComposeAndSend() WAS CALLED
++DOMWINDOW == 2 (004FFEE0) [serial = 2] [outer = 03FD9EE0]
Directory request for: UChrm that we (mailDirService.js) are not handling, leavi
ng it to another handler.
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004002: file C:/mozill
a-build/msys/tbirdhq/mailnews/base/src/nsMsgContentPolicy.cpp, line 704

Maybe you could get someone else to try the test w/ your patch on their setup to make sure it's not an issue with my test setup (though all the other tests work for me...)

Comment 15

[David :Bienvenu]

Zane, I'm also curious if you're running with all your patches applied, or testing them one at a time. I ask because I see exceptions referring to UMimTyp, which you've defined in a separate patch...

Comment 16

[Zane U. Ji]

I found that mime problem when I was working on the patch. It was included in the patch because I tried to disable the warning about UMimTyp. However, that warning can be just ignored.

Comment 17

[Zane U. Ji]

Attachment: Patch

Updated test case.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 452567 [details] [diff] [review]
Patch

sr from the same person is no longer a valid review scenario. See https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements for more details.

In any case, I don't think this patch needs sr, unless bienvenu wants someone else to review it as well.

Comment 19

[Wayne Mery (:wsmwk)]

(In reply to Mark Banner (:standard8) from comment #18)
> Comment on attachment 452567 [details] [diff] [review]
> Patch
> 
> sr from the same person is no longer a valid review scenario. See
> https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements
> for more details.
> 
> In any case, I don't think this patch needs sr, unless bienvenu wants
> someone else to review it as well.

Comment 20

[Zane U. Ji]

Attachment: Recreated patch and test

The test has been re-engineered to use MIME emitter.

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 700994 [details] [diff] [review]
Recreated patch and test

I'm sorry for not getting to this earlier. This isn't one of my strong areas, so I suggest that we see if Joshua can look at it for you.

Comment 22

[Joshua Cranmer [:jcranmer]]

Comment on attachment 700994 [details] [diff] [review]
Recreated patch and test

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

Unfortunately, this patch doesn't apply, so I can't give a full review right now.

About the tests: I am always wary of text files that rely on a particular EOL style, since that is easy to munch on by text editors. That said, given the nature of this bug, I am not sure it's avoidable. I would still like to see the text files at least clearly explain their peculiarities (like how the 161806_noeol.txt does it).

I'm also concerned about your tests; the Gloda mime emitter has logic in libmime that causes it to bypass some magic events. In a situation like this, I think it would be a better idea to not use it and use the "standard" attachment and libmime APIs, as that would better correlate with how most people would interact with this code.

::: mailnews/compose/test/unit/test_bug161806.js
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

That's a lie; this isn't a C++ file.

Comment 23

[Zane U. Ji]

(In reply to Joshua Cranmer [:jcranmer] from comment #22)
> Comment on attachment 700994 [details] [diff] [review]
> Recreated patch and test
> 
> Review of attachment 700994 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Unfortunately, this patch doesn't apply, so I can't give a full review right
> now.
> 
Good. That means a lot of work has been done. I'll create a new patch. However, I'm going to have a vacation this week.

> About the tests: I am always wary of text files that rely on a particular
> EOL style, since that is easy to munch on by text editors. That said, given
> the nature of this bug, I am not sure it's avoidable.

I believe that Quoted-printable encoding is the answer. 

> I would still like to
> see the text files at least clearly explain their peculiarities (like how
> the 161806_noeol.txt does it).
> 
Those text files are added to test QP encoding. "noeol" means no EOL. I saw somebody mentioned that ThunderBird/Seamonkey had problems with a file, which doesn't have an EOL at the end of the file. "crlfmxd" means that CLs and LFs are mixed.

> I'm also concerned about your tests; the Gloda mime emitter has logic in
> libmime that causes it to bypass some magic events. In a situation like
> this, I think it would be a better idea to not use it and use the "standard"
> attachment and libmime APIs, as that would better correlate with how most
> people would interact with this code.
> 
The standard emitter requires a DOM window and causes unexpected problems, such as the above one pointed out by David. I can switch back to standard one though.
 
> ::: mailnews/compose/test/unit/test_bug161806.js
> @@ +1,1 @@
> > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> That's a lie; this isn't a C++ file.

It was copied from a test file in that folder. I seldom use vi.

Comment 24

[Joshua Cranmer [:jcranmer]]

(In reply to Zane U. Ji from comment #23)
> > I would still like to
> > see the text files at least clearly explain their peculiarities (like how
> > the 161806_noeol.txt does it).
> > 
> Those text files are added to test QP encoding. "noeol" means no EOL. I saw
> somebody mentioned that ThunderBird/Seamonkey had problems with a file,
> which doesn't have an EOL at the end of the file. "crlfmxd" means that CLs
> and LFs are mixed.

What I meant is that 161806_noeol.txt contains the text:
No EOL and with spaces

I would like to see such text in the other files.

And, FWIW, please don't abbreviate "mixed" to "mxd"--it's completely opaque if you don't know the definition and there's no path pressure to shorten it.

> > ::: mailnews/compose/test/unit/test_bug161806.js
> > @@ +1,1 @@
> > > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > 
> > That's a lie; this isn't a C++ file.
> 
> It was copied from a test file in that folder. I seldom use vi.

Actually, it's an Emacs modeline, not a vim modeline. :-) (Not that I use emacs)

Comment 25

[Zane U. Ji]

(In reply to Joshua Cranmer [:jcranmer] from comment #24)
> What I meant is that 161806_noeol.txt contains the text:
> No EOL and with spaces
> 
> I would like to see such text in the other files.
> 
> And, FWIW, please don't abbreviate "mixed" to "mxd"--it's completely opaque
> if you don't know the definition and there's no path pressure to shorten it.
> 
Sure.

And should I use standard emitter?

Comment 26

[Joshua Cranmer [:jcranmer]]

(In reply to Zane U. Ji from comment #25)
> And should I use standard emitter?

If it's possible. In the worst case, it should be possible to write it as a mozmill test (a framework that tests our code by effectively using the UI). I'll admit that I don't fully understand how they work, but mconley should be able to answer any questions you might have.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Yes, I can help create a Mozmill test in the event that an XPCShell test proves to be insufficient. Just contact me at mconley at mozilla dot com if such a situation arises.