231474 - attachments mix contents

Status: RESOLVED FIXED

(MailNews Core :: Attachments, defect)

Description

[Vladimir Muller]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; cs-CZ; rv:1.5) Gecko/20031007
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; cs-CZ; rv:1.5) Gecko/20031007

If I get a lot of attachments (30 for example) with only one short line then the content of this file is damaged. Example: File name OP700176.txt, content of this file: 700148022004       113012004. 
Mozilla and thunderbird all version. Another mail clients (webmail, outlook, outlook express) work OK.

Reproducible: Always

Steps to Reproduce:
1. Create 30 files *.txt with only one short line.
2. Send all fhis files as attachment in one mail.
3. Read this mail by mozilla or thunderbird mail client.

Actual Results:  
Demage content of this attached files.

Expected Results:  
The same content of files as origin.

Comment 1

[Pavel Janik]

This bug is independent on the host operating system. I have reproduced it on
SuSE Linux 9.0.

Comment 2

[Stanislav Brabec]

This bug ocurs only for saved attachments and attachments opened via extertal
viewer. Inline view is OK.

First and second files is damaged in all test cases.

It is repeatable also on mozilla (at least 1.4). Repeatable over both IMAP and POP3.

How to repeat:
for (( i=0 ; i < 30 ; i++ )) ; do echo "This is attachment $i." >file$i.txt ; done
str= ; for (( i=0 ; i < 30 ; i++ )) ; do str="$str -a file$i.txt" ; done
echo "This is a body." | mutt$str -s "Test mail" [e-mail]

Now open this mail in Mozilla or Thinderbird and click to attachment file0.txt.

This file contains:
This is attachment 0.
This is attachment 18 [details].
This is attachment 19 [details].
This is attachment 20 [details].
This is attachment 21 [details].
This is attachment 22 [details].
This is attachment 23 [details].
This is attachment 24 [details].
This is attachment 25 [details].
This is attachment 26 [details].
This is attachment 27 [details].

Comment 3

[Stanislav Brabec]

Attachment: mozilla-many-attachments.diff

This code is obsolete for two reasons:

It nearly duplicates code few lines above. Since result is assigned to
Boolean, there is no reason to re-compare the same again.

Comparison using strncmp() limited to strlen(part_to_load) is invalid.
For many MIME parts (>19), comparing part_to_load=1.2 and current
id=1.20, the original (correct) result "unmatches" is changed to invalid
"matches".

Comment 4

[Stanislav Brabec]

Attachment: mozilla-many-attachments-with-formatting.diff

I am attaching another versions of patch. First is only code fix. This one
fixes code formatting around this fix.

Comment 5

[Stefan Borggraefe]

Confirming with TB build 20031217 on Linux and Seamonkey 2004012509 on Windows
XP. The first attachment in the received mail contains data from the following
attachments.

Comment 6

[Stefan Borggraefe]

Comment on attachment 139725 [details] [diff] [review]
mozilla-many-attachments-with-formatting.diff

I'm not the patch author. I was just asked in #mozilla.de to help out using
bugzilla.

But I have tested the patch and it fixes the bug for me.

Comment 7

[David :Bienvenu]

Can I get a non-whitespace diff, -uw, so I can be sure of what's being changed? thx.

Comment 8

[Stanislav Brabec]

See first attached patch -
http://bugzilla.mozilla.org/attachment.cgi?id=139724&action=view - it contains
only code change.

Both patches are identical except formatting.

You can use first one and them reformat manually.

Comment 9

[David :Bienvenu]

Comment on attachment 139724 [details] [diff] [review]
mozilla-many-attachments.diff

asking JF for a review. But one comment - if this is the right thing to do,
can't you remove the whole else if clause since there's no code left in it?

Comment 10

[Stanislav Brabec]

Oops. My patch is bad! You are true. Either remove whole else clause, or fill
braces by right code.

Comment 11

[Jean-Francois Ducarroz]

Comment on attachment 139725 [details] [diff] [review]
mozilla-many-attachments-with-formatting.diff

I don't have a build environment to test this patch but it does not seems right
to me! The else clause is missing its code!
Now if you totally suppress the else clause, I am pretty sure we wont be able
to display an attached message in a stand alone window (tried to double click
on a forwarded (as attachment) message which itself contains some attachment.

I am worry also about apple double attachment.

Comment 12

[Stanislav Brabec]

Attachment: mozilla-mix-attachments.diff

There is another, really minimalistic patch, which fixes the problem for me (it
includes terminal NUL to comparison, so 2<NUL> and 20<NUL> are recognized as
different). But, please review the code around this line.

Comment 13

[Stanislav Brabec]

Can somebody review and commit this new patch (eventually original code around)?
This fixes this bug, too, and shouldn't break anything. Thanks.

Comment 14

[Jean-Francois Ducarroz]

Comment on attachment 140956 [details] [diff] [review]
mozilla-mix-attachments.diff

Sure this fix solves your problem but it introduces a regresion as well: You
want be able to process multipart attachment such as forwarded message (as
attachment) and appledouble.
The goal of the test is to set output_p to true for the part we are processing
as well all it's sub-part: part=1.3, subpart = 1.3.1, 1.3.2, 1.3.2.1, etc...

Comment 15

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v4

This path takes care of the problem. It correctly detect subpart of the part we
are processing.

BTW, The trunk build get sometime confuse when opening an attachment and
display the whole message instead of just the selected part. Saving the
attachement however works fine. This is not a regression cause by that fix

Comment 16

[Jean-Francois Ducarroz]

Reassigning the bug to myself...

Stanislav, can you test the patch and make sure it fix correctly your problem?

Comment 17

[Stanislav Brabec]

Yes, it fixes my problem correctly.

Comment 18

[David :Bienvenu]

Comment on attachment 143309 [details] [diff] [review]
Proposed fix, v4

sr=bienvenu, but do you really need the cast to const char * when calling
strncmp? the params are already char *

Comment 19

[Jean-Francois Ducarroz]

you are correct, don't need those (const char *) left from the original code.
I'll supress them before check in...

Comment 20

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

Comment on attachment 143309 [details] [diff] [review]
Proposed fix, v4

r=sspitzer

Comment 21

[Jean-Francois Ducarroz]

Requesting approval for checkin into 1.7b

Comment 22

[David :Bienvenu]

Comment on attachment 143309 [details] [diff] [review]
Proposed fix, v4

requesting 1.7b approval

Comment 23

[David :Bienvenu]

clearing blocking...

Comment 24

[chris hofmann]

Comment on attachment 143309 [details] [diff] [review]
Proposed fix, v4

a=chofmann for 1.7b

Comment 25

[Jean-Francois Ducarroz]

Fix checked in.