193142 - chars (above U+00FF:e.g. Japanese) in file name are lost when saving/open an attachment

Status: VERIFIED FIXED

(MailNews Core :: Attachments, defect)

Description

[u69748]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030210
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030210

When I open or save a attachment which name contains Japanese,
file name will be destroyed: empty or random string.

I will upload sample mbox later.


Reproducible: Always

Steps to Reproduce:
1. Select message A3
2. Save the attachment by "Save As"
3. Open the attachment by double clicking.

Actual Results:  
Result of "Save As":
Filename is empty in dialog.

Result of Open:
Opened with filename ".zip" at first time,
 or "-1.zip" at second time.

If file name is ASCII (message A1), there is no problem.

Comment 1

[u69748]

Attachment: testcase mbox

Message A1

Content-Type: application/x-zip-compressed;
 name="test.zip"
Content-Transfer-Encoding: base64
Content-Disposition: inline;
 filename="test.zip"

Message A3

Content-Type: application/x-zip-compressed;
 name="=?ISO-2022-JP?B?GyRCJUYlOSVIGyhCLnppcA==?="
Content-Transfer-Encoding: base64
Content-Disposition: inline;
 filename="=?ISO-2022-JP?B?GyRCJUYlOSVIGyhCLnppcA==?="

Comment 2

[Jungshik Shin]

> Content-Type: application/x-zip-compressed;
>  name="=?ISO-2022-JP?B?GyRCJUYlOSVIGyhCLnppcA==?="
> Content-Disposition: inline;
>  filename="=?ISO-2022-JP?B?GyRCJUYlOSVIGyhCLnppcA==?="

Using RFC 2047 style encoding in filename parameter of C-D header
is illegal. RFC 2231 style encoding should be used there.
However, Mozilla(-mail) is generous enough to accept RFC 2047 style
encoding, too. I'll try to see if I can reproduce the problem.

BTW, Emura-san, what's your system locale(of Win2k/XP)? Is it set
to  Japanese? 

Comment 3

[Koike Kazuhiko]

Reproduced with 2003021308-trunk/Linux.

Mozilla Mail generates RFC2047 style, and can't handle it.
*********************************
Content-Type: application/zip;
 name="=?ISO-2022-JP?B?GyRCJUYlOSVIGyhCLnppcA==?="
Content-Transfer-Encoding: base64
Content-Disposition: inline;
 filename="=?ISO-2022-JP?B?GyRCJUYlOSVIGyhCLnppcA==?="
*********************************

Comment 4

[Jungshik Shin]

Actually, my comment #2 is not so relevant to this bug because
Mozilla accepts both styles, RFC 2047 and RFC 2231. The issue here is why
it  fails to convert Unicode string(filename) that it properly
obtains from RFC2231/2047 encoded string to the local
encoding(filesystem encoding). This issue sounds resolved
a couple of years ago(I was involved in fixing it), but
may haev been resurrected recently. I'll check. 

BTW, it's likely that the cause of this bug lies not in
mailnwes but in file-handling code. 

Comment 5

[Jungshik Shin]

Adding Katakai-san and nhotta to CC because either of them provided the patch (for
nsFilePicker) in the past
for a similar problem IIRC(or did I?..) 

Comment 6

[Jungshik Shin]

It seems like I was wrong in pointing fingers at nsFilePicker. 
Test-cases at http://jshin.net/moztest/download.html works fine
with 2003021008. The cause should be somewhere else. One possibility is
'imap://' and 'mailbox://' uri handler, but I'm not sure, yet.


 

Comment 7

[u69748]

> BTW, Emura-san, what's your system locale(of Win2k/XP)? Is it set
> to  Japanese? 

Yes, I'm using WinXP/WinNT Japanese Edition.

Comment 8

[Jungshik Shin]

Thank you for the answer.
I guess the problem is somewhere (|SaveAttachment|) in
http://lxr.mozilla.org/seamonkey/source/mailnews/base/src/nsMessenger.cpp,
but I can't confirm nor refute it until I'm done with making a build/debug env.
on a local machine where I'm now(it's too painful to debug over a remote link) 

Comment 9

[Jungshik Shin]

It seems like chars >= U+1000 are lost in the conversion from JS string to C string in xpcom. My debug build of Mozilla cried out loudly that data is lost in xpcconvert.cpp when I tried to open or save attachment.   Now, the interface responsible for  opening of attachment is |OpenAttachment| defined in nsIMessenger.idl (which in turn invokes |OpenAttachment| defined in nsIMsgMessageService.idl).  This interface needs to be modified to accept either UTF8 string or wstring as the second argument to avoid data loss in xpcom. The only invocation of this interface is  in  http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/msgHdrViewOverlay.js#859  so that I guess it'd not pose any problem to modify this interface.  Likewise, I guess |saveAttachment| has to be modified.  I'll try to make a patch soon.    

Comment 10

[Jungshik Shin]

Attachment: a patch

With this patch, Mozilla can open and save attachments with
filenames (with characters beyond U+00FF). I only
tested under Linux, but this is a platform-independent
patch so that it should also work under Win/Mac. 
Somehow my build has trouble accessing my IMAP server so that
I wasn't able to try it via IMAP link, but it should work
just as well. The same should be true of NNTP. 

I also tested 'save all' feature and it worked fine.

Comment 11

[nhottanscp]

The patch looks fine. But is this a regression?

cc to ji, marina, can this be reproduced by NS7?

Comment 12

[Jungshik Shin]

ji and marina will give us a definitive answer, but in the  
mean time I can add a data point. This bug is reproducible  
in Mozilla 1.0.1 (which is 'close' to NS 7). 
Like nhotta may do, I'm wondering how on earth 
we haven't noticed and taken care of this obvious flaw  
if it is not a regression. I have a vague memory that  
it worked in the past, but am not sure. (my memory is not so reliable). 
 
BTW, probably it's too late for 1.3, but it would be great 
if this is fixed before 1.3 (with my patch) because  
saving/opening attachment is a pretty common operation. 
 

Comment 13

[Jungshik Shin]

> Mozilla Mail generates RFC2047 style

This also makes me wonder how we couldn't 
notice this before. I filed bug 193439.
(because Mozilla can decipher RFC 2231
in incoming emails, I assumed that it produced
that for outgoing emails, too). 

Comment 14

[u69748]

> This bug is reproducible  
> in Mozilla 1.0.1 (which is 'close' to NS 7). 

Really?

I can't reproduce this bug in Netscape 7.01,
Mozilla 1.2.1 and Mozilla 1.3a on WinXP/WinNT.
I think this is a regression of 1.3b.

I receive attachments which have Japanese file name
everyday. After updating to 1.3b from 1.3a, this bug appeared.

Nominating blocking1.3

Comment 15

[Boris Zbarsky [:bzbarsky]]

jshin, is the messageURI parameter a real URI?  If so, make it also AUTF8String
instead of string....

Comment 16

[Jungshik Shin]

nhotta and Emura-san,
It's indeed a regression due to the change made on Feb. 6
(see
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=msgHdrViewOverlay.js&root=/cvsroot&subdir=mozilla/mailnews/base/resources/content&command=DIFF_FRAMESET&rev1=1.100&rev2=1.101
)

An alternative to attachment 114484 [details] [diff] [review] is to url-escape displayName
before calling |saveAttachment| and |openAttachment|
in |dofunc| in msgHdrViewOverlay.js. In case of |saveAllAttachments|,
escaping is done.
(see
http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/msgHdrViewOverlay.js#1102
)

The downside of this is that JS code invokes url-escaping only to be undone
in C++ code. (btw, attachment 114484 [details] [diff] [review]  needs to be modified
to  remove url-unescaping of aDisplayName because it's passed as
AUTF8String). 

As for Boris's question about messageURI, I'm not sure. I have to check.

Comment 17

[Jungshik Shin]

Attachment: an alternative to attachment 114484

Comment 18

[nhottanscp]

Adding nsbeta1, reassign to sspitzer.

Comment 19

[Jungshik Shin]

Attachment: a new patch (removing unnecessary unescaping/escaping of fileName)

similar to attachment 114484 [details] [diff] [review], but I removed the unnecessary
url-unescaping/escaping
in JS and C++ code.

Comment 20

[Jungshik Shin]

Attachment: replacement for 114841 (with missing part added)

sorry for spamming. I forgot to include diff on mailnews/(news,imap,local) in
attachment 114841 [details] [diff] [review].

Comment 21

[Samir Gehani]

Mail triage team: nsbeta1+/adt3

Comment 22

[Jungshik Shin]

Which one would you go, attachment 114584 [details] [diff] [review] or attachment 114842 [details] [diff] [review]?
I prefer the latter.

Comment 23

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

I apologize for the regression.  

the change I made was landed on the both the trunk and soon after, the 1.0 branch.

looking at what I checked in, I left off this:

the old code:

return "('" + attachment.contentType + "', '" + attachmentUrl + "', '" +
escape(attachment.displayName) + "', '" + attachment.uri + "')";

the new code:

obj.displayName = aAttachment.displayName;

as [e-mail] pointed out with his initial patches, I just left off the
escape().

I think this could be a one line fix.

Comment 24

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

Attachment: c:\bug193142.txt

I think another way to reproduce this would be on en-US builds, by sending
attachments with an "escaped" name, which would be unescaped by the back end.

Comment 25

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

Attachment: patch

Comment 26

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

here's how I tested this:

send myself a message with an attachment named"

"this%20is%20a%20test.txt"

if I don't escape that, when I try to save it, it will try to save as:

"this is a test.txt"

which is not right.

testing this now...again, my apologies

Comment 27

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

I just tried the fix, and it solves my "this%20is%20a%20test.txt" case, and it 
is similar in spirit to 
http://bugzilla.mozilla.org/attachment.cgi?id=114584&action=view

[e-mail], can you review / test?

if it works for you, we can land this in the trunk and on the 1.0 branch.

Comment 28

[Cavin Song]

Comment on attachment 115076 [details] [diff] [review]
patch

r=cavin.

Comment 29

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

this should also land on the mozilla 1.0x branch

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 115076 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to 1.3. Also setting request flag for
1.0.x branch to get on branch drivers' radar.

Comment 31

[Daniel Veditz [:dveditz]]

Note this regression doesn't occur on the 1.0 branch, bug 191817 is still
waiting for 1.0.x approval. The regression *is* on the NETSCAPE_7_02_BRANCH and
Netscape might want it fixed there in case they ever release a 7.03, but that's
a different story.

I'd like to see 191817 fixed on the 1.0 branch, and if so then we'd need this
fix with it.

Comment 32

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

thinking about it,
http://bugzilla.mozilla.org/attachment.cgi?id=114584&action=view might be
better, since it does the unescaping at the end.  (like we do for
SaveAllAttachments())

but more than that, the arguments to methods on nsIMessenger and
nsIMsgMessageService are misleading (and in C++, ConvertAndSanitizeFileName())

if the code expects the displayName to be escaped, (which it does) the arguments
and code should be cleaned up.

remember, we have to escape the displayName since in ::OpenAttachment() we build
a url from it, and the displayName could contain characters that are not url
friendly (' ', @, :, /, etc)

I'll come up with a fix that also cleans up the arguments and variable names,
but still follows
http://bugzilla.mozilla.org/attachment.cgi?id=114584&action=view (and
http://bugzilla.mozilla.org/attachment.cgi?id=115076&action=view)

Comment 33

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

Comment on attachment 114584 [details] [diff] [review]
an  alternative to attachment 114484 [details] [diff] [review]

r/sr=sspitzer, carrying over asa's a=.

let's spin the code cleanup to another bug.

Comment 34

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

Comment on attachment 115076 [details] [diff] [review]
patch

let's go the other way.

Comment 35

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

Comment on attachment 114842 [details] [diff] [review]
replacement for 114841 (with missing part added)

rejecting.  what happens if the file name contains characters that aren't url
safe?

Comment 36

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

re-opening, I haven't checked in the fix yet!

Comment 37

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

since it was his fix, re-assign to [e-mail]

the reason I rejected the alternative approach is the urls that we'll generate
(Since we append the filename) require the filename to be escaped.

"this is a test.txt" ->
imap://sspitzer@nsmail-1:143/fetch%3EUID%3E/INBOX%3E177685?part=1.2&type=text/plain&filename=this%20is%20a%20test.txt


"this%20is%20a%20test.txt" -> 
imap://sspitzer@nsmail-1:143/fetch%3EUID%3E/INBOX%3E177685?part=1.3&type=text/plain&filename=this%2520is%2520a%2520test.txt

if we didn't escape the file name, the filename could contain characters that
would mess up the url.

I landed [e-mail]'s fix.  thank you for the fix.

Comment 38

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

Comment on attachment 114584 [details] [diff] [review]
an  alternative to attachment 114484 [details] [diff] [review]

this is what we'd need for 1.0, if #191817 lands

Comment 39

[Jungshik Shin]

sspitzer,
thank you for catching the problem in attachment 114842 [details] [diff] [review]
and commiting attachment 114584 [details] [diff] [review]. My build made of the CVS
today works fine and it should be true of 1.3b-branch as well.
So, I'm marking this as verified.