55993 - Forwarded attachments corrupt

Status: VERIFIED FIXED

(MailNews Core :: MIME, defect, P3)

Description

[michael.t.loeffler]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.2.13 i686; en-US; m18) Gecko/20001010
BuildID:    2000101010

         If forwarding a message with attachments, the atachments on the new
message are corrupt, they are all the same 27 byte long file.

Reproducible: Always
Steps to Reproduce:
1. Choose message with attachments
2. Forward the message
3. Open the forwarded message
4. Download an attachment from the forwarded message

Actual Results:  Forwarded attachment is corrupt, a 27 byte long file with trash
in it.

Expected Results:  Attachment on forwarded message should be same exact content
as original attachment.

I checked the staging area (/tmp) for the new forwarded attachment files, and
they are indeed the 27 byte garbage files. It appears they are never properly
downloaded from the original message during preparation of the forwarded
message.

Comment 1

[michael.t.loeffler]

Clarification of this behavior, it still exists in Linux Build ID: 2000101221.
The attachments that are corrupted are files of non-ascii formats, like MSWord
docs, ppt files, etc. The ascii text files that are attached seem to come though
just fine. I also notice that the attachments marked (Not Downloaded) in the
original message are the only ones that get botched.

Comment 2

[rhp (gone)]

Mime parts on demand problem?

Comment 3

[David :Bienvenu]

argh, this would be bad. When retrieving a message for forwarding, we're
supposed to fall back to downloading the whole message, and mpod is supposed to
be turned off. I wonder if we're getting bit by the memory cache. Seems worth
nominating for rtm since there's data loss.

Comment 4

[David :Bienvenu]

OK, I have a two part fix for this, which I'll attach. It involves adding a
clause to the uri for fetching a message when we're forwarding a message that
tells imap to not use mpod (or rather, tells it to fetch the whole message).
This only affects the snarf attachment code; all other code paths that fetch
messages are unchanged.

Comment 5

[David :Bienvenu]

Attachment: proposed fix (part 1)

Comment 6

[David :Bienvenu]

Attachment: proposed fix (part 2)

Comment 7

[David :Bienvenu]

This mainly affects forward as attachment, not forward inline, as near as I can
tell, because of the byzantine way compose and attachments mail sending is done.

Comment 8

[Scott MacGregor]

David, your patches look safe and good to me. Not sure if PDT will be interested 
in taking this fix at this point in time but we can try. 

sr=mscott

Comment 9

[jefft]

Looks good to me. r=jefft

Comment 10

[Jim Roskind]

This looks safe... and I'm betting on you, and the reviewers that it does what
you wanted it to... precisely.
We don't have a lot of room for adjusting, but this level of data corruption is
very bad.
I'm marking rtm double-plus.  Please land asap on the branch.

Comment 11

[David :Bienvenu]

It's definitely the smallest change I could make - the alternative was to change
the message service method to fetch a message to take an extra boolean parameter
to control that behaviour. That would have involved changing 20 or 30 functions
and function calls, I think (I stopped counting after 10 or so). The only worry
is that adding to this URI could expose some already broken URI parsing, but it
doesn't seem to break anything. I'll check this in today.

Comment 12

[David :Bienvenu]

Fix checked into branch and trunk.

To test, we need to test forwarding an imap message with an attachment that is
not downloaded with the message (i.e., one with a moderately large word
document), with the user pref for forwarding set to forward as attachment.

To test for regressions, you should try forwarding and sending smtp, news, and
imap messages with attachments, and editing and saving drafts and templates for
imap (these are all parts of the code that use the same fetch message code). It
would be good to test these thoroughly anyway because we've found several bugs
recently in these areas that apparently have slipped through for a long time.

Comment 13

[michael.t.loeffler]

This works for me now, with Linux Build ID: 2000101521. Thanks for the quick
attention!

Comment 14

[David :Bienvenu]

cool, thanks for reporting it, and verifying the fix!

Comment 15

[esther]

changing qa to pmock

Comment 16

[pmock]

Re-open bug on for all platform using branch build:
 win32 commercial seamonkey build 2000-101609-mn6 installed on P500 Win98
 linux commercial seamonkey build 2000-101609-mn6 installed on P200 RedHat 6.2
 macos commercial seamonkey build 2000-101608-mn6 installed on G3/400 OS 9.04

Forward as attachment - Works ok.
Forward inline -> Failed.

Steps to reproduce problem:
1) Exit Seamonkey to ensure you haven't download the attachment before.
2) Start seamonkey
3) Open Mail
4) Start a new mail message
5) Address message to yourself, add a subject title, and short message.
6) Attach a word document at least greater than 30K
7) Send message
8) Retrieve mail message
9) Open mail message
10) Click on the attachment paperclip button.
    The tooltip should read the attachment has not been downloaded.
11) Select the message menu and choose Forward as inline.
12) Send mail message
13) Retrieve the forwarded mail message
14) Click on the attachment paperclip.
    The tooltip says the word document does not need to downloaded.
15) Look at the Message Source
      Edit -> Message Source
    The message source says the attachment has not been downloaded.

Comment 17

[Jim Roskind]

Pushing back to need info for the re-open.  If the re-open is invalid, and this
is really "fixed," please mark the bug fixed again but also change the status to
rtm++ so that QA will regression test the bug.
Thanks,
Jim

Comment 18

[David :Bienvenu]

No, I think we should re-open it and accept a fix for it. I only fixed the
forward as attachment case (which is the default for mozilla). I believe forward
inline is the default for Netscape 6 (but this isn't my area at all, so I'm not
sure about this) and that needs a similar fix. I don't know why I thought it
worked for Forward Inline - maybe I tried a test case where the attachment was
already downloaded.  I'll try to come up with a similar fix. If forward inline
is broken, I *think* that means that editing and sending drafts and templates
with attachments is also broken, since it's the same code. But, again, I don't
really know this code.

Comment 19

[David :Bienvenu]

argh, my test attachment was just shy of 30K, which meant that in some cases
MPOD wasn't getting invoked. So drafts (and presumably templates) are broken
too.  I'm not going to be able to drive this to completion before I leave on
vacation. I'll attach a proposed fix and try to get reviews, but I think jeff or
mscott will need to check the fix in.

Comment 20

[David :Bienvenu]

Attachment: fix for forward inline, drafts, and templates case

Comment 21

[David :Bienvenu]

OK, I've attached a patch that fixes the rest of the cases (forward inline,
templates, and drafts) that I can think of, anyway. Can you guys review it? Thanks!

Comment 22

[jefft]

Looks good to me. r=jefft

Comment 23

[Scott MacGregor]

This new patch looks good to me as well. sr=mscott

Comment 24

[Scott MacGregor]

I take the sr back, can you use NS_LITERAL_STRING around
"?fetchCompleteMessage=true" and then call Append instead of append with
conversion. That will reduce the bloat.

o.t. sr=mscott

Comment 25

[selmer (gone)]

MScott, JeffT, can one of you take David's patch and apply Scott's comments and
then own the checkin?  David is on vacation all the rest of this week.

Comment 26

[jefft]

Okay, I'll work on it.

Comment 27

[jefft]

I think we should stay with David's original patch. If we want to convert to use
NS_LITERAL_STRING() we might as well to convert all the instance of using
AppendWithConversion() within the same routine or the same file which will end
up changing a lot of code.

Comment 28

[Scott MacGregor]

I disagree adding more unnecessary bloat just to follow a bad pattern that's
already in the file isn't a good pattern to follow.

Part of the review process is to make sure more of these poor patterns don't get
checked into the tree =). 

Comment 29

[jefft]

Attachment: Okay, here is the patch with NS_LITERAL_STRING

Comment 30

[jefft]

I have a different philosophy though - less code change less error prone 
especially at this late stage. I would think clean up work or bad code patterns 
correction can be done in 6.5 or 7.0. 

Comment 31

[jefft]

The patch seems work with the "inline" image attachments but not quiet working
with "attachment" attachments. Forward inline a message with "attachment"
attachment the attachment seems not following the rule of "mime part on demand".
View message source shows me the content of the downloaded attachment. However,
the display name shows "Attachmen (Not Downloaded)". In theory, if an attachment
isn't an inline attachment and the size is greater than the limit of an
attachment then the attachment should not be downloaded.

Comment 32

[Scott MacGregor]

Jeff, I didn't say we should clean up exisiting code or change bad patterns in
exisiting code. But even at this date, we should always check in correctly
written  code when checking in new code. I'm not sure why "especially at this
late stage" influences the correctness of the code being checked in....

Comment 33

[jefft]

Got you. I guess I prefer to do it all if I want to do it. I hate to do it 
partially.

Comment 34

[jefft]

The patch does work. I was confused with the test result with the release build. 
Everything works as expected. Scott, can you give a super review? The patch try 
to do a little bit more to correct the bad patterns. JF, can you do a review 
too?

Comment 35

[Scott MacGregor]

I don't think now is the time to change it every where =).

For the rtm branch, please only change the one line I told david to change
wrapping "?fetchCompleteMessage=true"  with NS_LITERAL_STRING and not calling
AppendWithConversion.

I'd recommend doing more cleanup on the tip if you want but not on the branch.

Other than that request to minmize the diff, sr=mscott. Thanks for testing this
and working with the patch for David jeff!

Comment 36

[jefft]

Oh, well ....

Comment 37

[jefft]

Attachment: patch with one NS_LITERAL_STRING, can someone check this in for me, I might sneak in the previous patch. r=jefft

Comment 38

[Jean-Francois Ducarroz]

looks good for me. R=ducarroz

Comment 39

[Scott MacGregor]

Once (if) PDT turns this into an rtm++, I'll check it in jeff. You wanna go
ahead and check your original patch that had some cleanup in it into the tip? No
sense in checking the reduced patch into the tip. 

Comment 40

[Peter Trudelle]

PDT says rtm++, please land on branch ASAP.

Comment 41

[jefft]

Fix checked in. Both trunk and branch.

F:\branch\mozilla\mailnews\compose\src>cvs commit -m "fixed bug 55993 -- forward
 attachment corrupt; r=ducarroz, sr=mscott" nsMsgComposeService.cpp
Checking in nsMsgComposeService.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgComposeService.cpp,v  <--  nsMsgCompo
seService.cpp
new revision: 1.38.4.1; previous revision: 1.38
done

F:\mozilla\mailnews\compose\src>cvs commit -m "fixed bug 55993 -- forward attach
ment corrupt; r=ducarroz, sr=mscott" nsMsgComposeService.cpp
Checking in nsMsgComposeService.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgComposeService.cpp,v  <--  nsMsgCompo
seService.cpp
new revision: 1.39; previous revision: 1.38
done

Comment 42

[varada]

Marking Verified on Branch
Linux 2000102009

Comment 43

[fenella]

Assign it to myself.

Comment 44

[fenella]

Cannot verify this bug until bug 67435 is fixed.

Comment 45

[fenella]

This bug is fixed. Using the following builds.
Linux (2001-02-12-08 mtrunk)
Win32 (2001-02-12-08 mtrunk)
Mac (2001-02-12-08 mtrunk)