206252 - 'send page/image' assume non-ASCII filenames are always in ISO-8859-1

Status: RESOLVED FIXED

(MailNews Core :: Internationalization, defect)

Description

[Jungshik Shin]

When inline image is sent by email via 'send image' menu, 
non-ISO-8859-1 file names are treated as if they're ISO-8859-1.

The filename parameter of Content-Disposition header should
use either UTF-8 or |originCharset| of the URL.   

I thought there was a bug filed for this, but couldn't find it. 
If it's dupe, I'm sorry.

Comment 1

[Jungshik Shin]

I'm changing the summary line because 'send page' (arguably more important than
'send image') is also affected.


Related to this is bug 193439. In cleaning up mime attachment code(bug 86089),
one attribute (|originCharset| of |url|) was not added to |nsIMsgAttachment|.
And, in mailNavigatorOverlay.xul[1], 'url' is passed unescaped. Instead of
adding |originCharset| attrib. to nsIMsgAttachment and changing mailnews code to
'infer' the correct filename from url and originCharset, we could just set
|name| attrib. of nsIMsgAttachment in xpfe code ( similar to what's done in
contentAreaUtils.js [2]), but it may be easier to do that on the other side. 

[1]http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/mailNavigatorOverlay.xul#112
[2]http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/contentAreaUtils.js#765

Comment 2

[Jungshik Shin]

test case added to the URL field.
The page is in EUC-KR and has a link to a PDF file with the filename in EUC-KR.
 Selecting 'send page' in the context menu brings up a mail composition window
with the attachment name wrong (EUC-KR misinterpreted as ISO-8859-1)

Comment 3

[Jungshik Shin]

Attachment: preliminary patch (working)

this patch works, but needs some clean-up.

Comment 4

[Jungshik Shin]

Attachment: patch (cleaned-up)

basically the same, but changed the interface slightly

Comment 5

[Jungshik Shin]

Comment on attachment 136886 [details] [diff] [review]
patch (cleaned-up)

asking for r/sr.
This is kinda needed for bug 227547. (it's possible to separate two patches,
but...)

Comment 6

[David :Bienvenu]

Comment on attachment 136886 [details] [diff] [review]
patch (cleaned-up)

this looks OK - my one question is - there are several places where you use
NS_ENSURE_SUCCESS(rv, rv) after string conversions. Is that always the right
thing to do since presumably it just aborts the attach operation? Could those
conversions fail for a reason that should cause us just to pass through the
original name?

Comment 7

[Jungshik Shin]

Attachment: a patch with fallback (alternative)

Thank you for r.

I'm not sure how much sense a couple of fallbacks I added make sense. Depending
on how you look at it, the downright failure may be considered better than
using 'garbage-like' names...

I'm fine with either way. bienvenu and mscott, let me know which one you like.


BTW, I switched to ACString(from string) for 'urlCharset' to simplify
getter/setter.

Comment 8

[David :Bienvenu]

Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

r=bienvenu - I prefer the fallback code if it's at all meaningful, but I leave
it up to you to decide if it's helpful.

Comment 9

[Jungshik Shin]

Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

Mocing sr request to this patch.

Thanks for the prompt review.
Because I'm not inclined either way, you prefer this way, and I found at least
one case where the fallback code is useful (when one has file names in a
character encoding different from the default filesystem charset), I'll go with
the fallback code.

Comment 10

[Jungshik Shin]

let's try to get this in before 1.6final. 

mscott, can you take a look?

Comment 11

[Jungshik Shin]

Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

let's off-load this from mscott.
sspitzer, would you care to review?

Comment 12

[Scott MacGregor]

Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

I can review it.

Comment 13

[Jungshik Shin]

Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

mscott, thanks for sr.

now asking for a1.6final.

affected platforms : all

affected users : anyone wishing to attach html documents/images with non-Latin1
names via 'context-menu' to emails 

the patch was tested on Linux and Win32. 

Risk : low (with fallback code paths). In a sense, the patch is a
'prerequisite' of fixing bug 227547 (a rather high-profile bug on Mac OS X).

Comment 14

[Jungshik Shin]

Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

Sorry for spamming (I pressed the wrong button).

mscott, thanks for sr.

now asking for a1.6final.

affected platforms : all

affected users : anyone wishing to attach html documents/images with non-Latin1
names via 'context-menu' to emails 

the patch was tested on Linux and Win32. 

Risk : low (with fallback code paths). In a sense, the patch is a
'prerequisite' of fixing bug 227547 (a rather high-profile bug on Mac OS X).

Comment 15

[Brendan Eich [:brendan]]

Asa, can you help drive this?  Thanks,

/be

Comment 16

[Asa Dotzler [:asa]]

Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

a=asa (on behalf of drivers) for checkin to Mozilla 1.6

Comment 17

[Jungshik Shin]

fix checked into the trunk.
thanks you all.

Comment 18

[Scott MacGregor]

This patch made a core API change that to mail compose APIs. The change was
never propogated to thunderbird which is now broken. Please always check
mozilla/mail mozilla/toolkit, mozilla/browser when making API changes per the
new tinderbox rules. The partitions are all open now. Thanks! :)

Patch coming up on for mozilla/mail.

Comment 19

[Scott MacGregor]

I checked in a fix for thunderbird bustage. 

Comment 20

[Jungshik Shin]

Attachment: thunderbird patch (

I missed this (as found in bug 227547).

Comment 21

[Jungshik Shin]

Oops. I didn't go down to read new comments. Sorry for spamming abd TB bustage.
BTW, I think AttachmentPrettyName had beeter be called with 'null' as the second
parameter (as in my patch) in TB because attachment.urlCharset is never set in TB. 

Comment 22

[PikeUK]

This patch seems to have broken Firebird's "Send Image" menu item, you've
changed the line in browser.js from sendLink to sendImage but FB doesn't use
mailNavigatorOverlay.xul so it has no access to sendImage (it just sends the URL
of the image by using a mailto generated from sendLink in browser.js).

Could you look into this as soon as possible (since I think FB 0.8 is supposed
to be released shortly).

p.s. And if I'm wrong and this problem has nothing to do with this bug I
apologise in advance.

Comment 23

[PikeUK]

Correction: These changes were made just after Firebird branched so this is not
a problem for FB 0.8, it only needs fixing on the trunk.

Comment 24

[Jungshik Shin]

Thanks for the note. Can you try reverting my patch for browser.js and see if
that solves the problem? 

Comment 25

[PikeUK]

(In reply to comment #24)
> Thanks for the note. Can you try reverting my patch for browser.js and see if
> that solves the problem? 

Yes reverting back fixes the Send Image item.

Comment 26

[Jungshik Shin]

Attachment: one liner to back out firebird part

this is to back out what shouldn't have been included in attachment 136886 [details] [diff] [review].

Comment 27

[Scott MacGregor]

jshin, I think this fix may have caused Bug #229872 if I'm reading the comments
in that bug correctly. We are no longer showing attachments correctly in the
message compose attachment window if they have non ascii characters in them. Do
you have time to take a look?

Comment 28

[Jungshik Shin]

Comment on attachment 140127 [details] [diff] [review]
one liner to back out firebird part 

bryner, can you review this ? I don't mind if you just go ahead and land it.