105825 - "Edit As New" corrupts binary attachments

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P2)

Description

[parish]

This occurs in 0.9.4, 0.9.5, and in a build from the latest source (CVS'd
2001/10/20: 17:00 GMT+1).

Compose a new message with an Address, Subject, and a *binary* attachment, but
leave the body competely empty.

File->Send Later

Go to Unsent Messages and view the message; it is correct.

Messages->Edit As New

In the Composition Window note that the Attachments pane is empty and the body
contains a few garbage characters. These are the first few bytes of the
attachment (if you attach a ZIP file the first 2 chars will be ``PK'')

This occurs whether Content-Disposition is Attachment or Inline.

If you add just a single character to the body when composing the original
message then this problem does not occur.

I have seen this on W2K-SP2 (on 2 different machines) and FreeBSD 4.4-STABLE

Mozilla is composing it as a multi-part message but not including the first part
if the body is empty which appears to confuse Composer.


Here is the header from an orignal message:

MIME-Version: 1.0
To: Parish <[e-mail]>
Subject: jkljkljk
Content-Type: multipart/mixed;
 boundary="------------040306020606060705030709"

This is a multi-part message in MIME format.
--------------040306020606060705030709
Content-Type: application/x-zip-compressed;
 name="BLUESCRN.ZIP"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="BLUESCRN.ZIP"

UEsDBBQAAIAIAKZRmyR9HhgupgMAANIGAAAKAAAAUkVBRE1FLlRYVHVVwW7cNhC9C9A/THxp
C6yV2kVRJCh6cJwCBlIb8No+9UKRoxWxFKlyqFXVr8m1f9lHatfZAKkPWpkcvnnvzQxVV/dP


and this is after "Edit As New" and File->Send Later:


MIME-Version: 1.0
To: Parish <[e-mail]>
Subject: jkljkljk
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

PK

Comment 1

[parish]

Attachment: Patch for nsMsgSendPart.cpp which fixes this bug

Comment 2

[parish]

The attached patch removes the code which deletes empty parts of a multi-part
e-mail as it creates e-mails with a Content-Type of multipart/mixed but which
only contain a single part. When such an e-mail is opened to Edit As New or
Forward the editor assumes that the first (i.e. the only) part is the message
body. This is incorrect in this case.

With this patch the empty body and its header are left in, which matches the
behaviour of both NS 4.x and O.E. and the message is interpretted correctly by
the Composer.

Comment 3

[Daniel Bratell]

ducarroz, what do you think of parish' patch? I think this is an short fix (7
lines) for a bad problem so it might be worth getting into an earlier milestone.

Comment 4

[Jean-Francois Ducarroz]

I need to figure out the potential side effect of this patch. Moving to 0.9.8

Comment 5

[Jean-Francois Ducarroz]

Attachment: Message to reproduce the problem

I can reproduce the problem using this message on Win2K

Comment 6

[Jean-Francois Ducarroz]

Comment on attachment 55721 [details] [diff] [review]
Patch for nsMsgSendPart.cpp which fixes this bug

This is not the correct way to solve it. It's ok to send message with no boby
part! in fact the attachment become the body!
The real problem is in mime when we edit the message as new (in fact it's an
edit draft action).

Comment 7

[Jean-Francois Ducarroz]

Attachment: proposed fix, v2

The mime part of the fix treat any body which is not text/plain or text/html as
attachment. The fix also fix couple memory leak during the edit draft process.
The send part will alway output a body (main part) part even if this one is
empty.

Comment 8

[Cavin Song]

Comment on attachment 63633 [details] [diff] [review]
proposed fix, v2

Looks good. r=cavin.

Comment 9

[(not reading, please use [e-mail] instead)]

looks good.  three minor points.

1)

-  attachData = (nsMsgAttachmentData *) PR_MALLOC( ( (mdd->attachments_count+1)
* sizeof (nsMsgAttachmentData) ) );
+  PRInt32 totalCount = mdd->attachments_count;
+  if (bodyAsAttachment)
+    totalCount ++;
+  attachData = (nsMsgAttachmentData *) PR_MALLOC( ( (totalCount + 1) * sizeof
(nsMsgAttachmentData) ) );
   if ( !attachData ) 
     return nsnull;
 
-  nsCRT::memset ( attachData, 0, (mdd->attachments_count+1) * sizeof
(nsMsgAttachmentData) );
+  nsCRT::memset ( attachData, 0, (totalCount + 1) * sizeof (nsMsgAttachmentData) );


why not just use PR_CALLOC()?

2)

 else
+      if (*body)
       NS_MsgSACat(&newBody, *body);
 
this gets treated as else if.  is that right?  I'd prefer:

else if (*body)
  NS_MsgSACat(&newBody, *body);

or

else {
+      if (*body)
       NS_MsgSACat(&newBody, *body);
}
  
3)

+        if( PL_strcasestr(mdd->messageBody->type, "text/html") != NULL )
+          composeFormat = nsIMsgCompFormat::HTML;
+        else if ( ( PL_strcasestr(mdd->messageBody->type, "text/plain") != NULL
) ||


instead of NULL, do nsnull.  or just do

+        if( PL_strcasestr(mdd->messageBody->type, "text/html") )
+          composeFormat = nsIMsgCompFormat::HTML;
+        else if ( ( PL_strcasestr(mdd->messageBody->type, "text/plain")  ) ||
+          ( !PL_strcasecmp(mdd->messageBody->type, "text") ) )
+          composeFormat = nsIMsgCompFormat::PlainText;

fix that, and sr=sspitzer

Comment 10

[Jean-Francois Ducarroz]

Fix checked in. Thanks parish for your help.

Comment 11

[esther]

Using build 20020322 on winxp and 20020318 on linux and mac this is fixed. verified.