443344 - attachment filename corruption imported from Eudora (Japanese version).

Status: RESOLVED FIXED

(Thunderbird :: Migration, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.14) Gecko/20080419 Ubuntu/8.04 (hardy) Firefox/2.0.0.14 Kazehakase/0.5.4
Build Identifier: 

Japanese filename (Shift-JIS encoded) gets corrupted.

ex.

2001テキスト.txt -> 2001

I think the filename should be converted.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proposed patch against cvs HEAD

Just copy and paste code from nsOutlookMail.cpp.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: A patch for 1.8 branch

IMO, the patch should be applied in 1.8 branch, since new comer from Eudora can not migrate to TB.

Comment 3

[Magnus Melin [:mkmelin]]

Your patch needs reviews. You can get that by setting the review? and superreview? flags on the patch. (And fill in a reviewer address, you can try [e-mail] for for both review and superreview.)

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Magnus, thank you for your advices. I've set both of it.

Comment 5

[David :Bienvenu]

Comment on attachment 328083 [details] [diff] [review]
A patch for 1.8 branch

I'd like a Eudora person to have a quick look as well...

Comment 6

[David :Bienvenu]

Comment on attachment 327928 [details] [diff] [review]
Proposed patch against cvs HEAD

This does look OK to me - at least, it's what we're doing in Outlook.

Comment 7

[David :Bienvenu]

Comment on attachment 328083 [details] [diff] [review]
A patch for 1.8 branch

It looks like there are tabs in this patch. Could you convert them to spaces? thx!

Comment 8

[Jeff Beckley]

Comment on attachment 327928 [details] [diff] [review]
Proposed patch against cvs HEAD

Looks good from a Eudora standpoint.

One thing that would make this patch less risky would be to fall back on the old "aAttachmentName = name;" code if there is an error doing the new code, i.e. rv is not success.  With that, r=me.

Comment 9

[Jeff Beckley]

Comment on attachment 328083 [details] [diff] [review]
A patch for 1.8 branch

My comment in the other patch applies here as well.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Patch for cvs HEAD

Fall back if the conversion is failed.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Patch for 1.8 branch

Expandtab and fall back if the conversion is failed.

David and Jeff, thank you for your kindly response!

Comment 12

[David :Bienvenu]

Comment on attachment 328276 [details] [diff] [review]
Patch for cvs HEAD

Actually, doesn't this leak memory? ToNewUTF8String will return an allocated buffer, and then we're assigning it to a a CString, which will copy the buffer. I think you want to do something like aAttachmentName.Adopt(ToNewUTF8String(description));

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Patch for cvs HEAD

Plug a memory leak.

David, thank you for your pointing out. I am not familiar with Gecko code.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Patch for 1.8 branch

Ditto.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #13)
> Created an attachment (id=328283) [details]
> Patch for 1.8 branch

I am sorry, the patch is not for 1.8 branch, it is for cvs HEAD.

Comment 16

[Jeff Beckley]

Comment on attachment 328283 [details] [diff] [review]
Patch for cvs HEAD

Looks good now.  Bienvenu should be superreview.

Comment 17

[Jeff Beckley]

Comment on attachment 328284 [details] [diff] [review]
Patch for 1.8 branch

Looks good now.  Bienvenu should be superreview.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Patch for cvs HEAD

Fix missing a header file inclusion.

I do not know why I noticed the missing. I would build with the patch...

Jeff, could you please review again? 

Thank you.

Comment 19

[David :Bienvenu]

Comment on attachment 328299 [details] [diff] [review]
Patch for cvs HEAD

the other option would be to do aAttachmentName = NS_ConvertUTF16toUTF8(description);

Comment 20

[David :Bienvenu]

Comment on attachment 328284 [details] [diff] [review]
Patch for 1.8 branch

can't you go directly to aAttachmentName, i.e., rv = impSvc->SystemStringToUnicode(name.get(), aAttachmentName); ?

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Attachment: [checked in] Patch for CVS head.

Use NS_ConvertUTF16toUTF8 instead of nsCString::Adopt.

Comment 22

[David :Bienvenu]

What I was suggesting is this:

+    rv = nsMsgI18NConvertToUnicode(nsMsgI18NFileSystemCharset(), name, aAttachmentName);
+ 
+    if (NS_FAILED(rv))
+      aAttachmentName = name

Does that work for you?

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Unfortunately, no. 

Compiler says: "cannot convert nsCString to nsAString_internal &"

Comment 24

[David :Bienvenu]

Comment on attachment 328415 [details] [diff] [review]
[checked in] Patch for CVS head.

ah, sorry, I got mixed up about what was an 8 bit string and what was a 16 bit string.

Comment 25

[David :Bienvenu]

Comment on attachment 328284 [details] [diff] [review]
Patch for 1.8 branch

sr=me, but I'd prefer NS_ConvertUTF16toUTF8 instead of aAttachmentName.Adopt(ToNewUTF8String(description))

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch for 1.8 branch

Use NS_ConvertUTF16toUTF8 instead of
aAttachmentName.Adopt(ToNewUTF8String(description)).

David, thank you for your specific instruction.

Comment 27

[Jeff Beckley]

Comment on attachment 328415 [details] [diff] [review]
[checked in] Patch for CVS head.

In the future, you can just continue on the r+ and/or sr+ if someone has given it to you with some small remaining corrections.

Comment 28

[David :Bienvenu]

Comment on attachment 330673 [details] [diff] [review]
Revised patch for 1.8 branch

thx for the patch!

Comment 29

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #27)
> (From update of attachment 328415 [details] [diff] [review])
> In the future, you can just continue on the r+ and/or sr+ if someone has given
> it to you with some small remaining corrections.

Thank you! I surely do so next time.

Comment 30

[Mark Banner (:standard8)]

(In reply to comment #26)
> Created an attachment (id=330673) [details]
> Revised patch for 1.8 branch

You will need to request approval1.8.1.17 for this patch to be checked in (the HEAD patch will be done later). When requesting, you should also put a couple of comments about the likely risk to Thunderbird 2/SeaMonkey 1.1 as they are in stable release. I expect the risk here is low however.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 330673 [details] [diff] [review]
Revised patch for 1.8 branch

I think there is no risk since falling back on the original name works if the conversion fails as Jeff advised in comment #8.

Comment 32

[Mark Banner (:standard8)]

Comment on attachment 328415 [details] [diff] [review]
[checked in] Patch for CVS head.

Checked in, changeset id 721bb048b120

Comment 33

[Hiroyuki Ikezoe (:hiro)]

Is there any action what should/can I do for checking the patch in stable branch?

Comment 34

[Magnus Melin [:mkmelin]]

After/if it gets approval1.8.1.17+, add the keyword "checkin-needed" and put "[checkin-needed: 1.8 branch]" in status whiteboard.

Comment 35

[Phil Ringnalda (:philor)]

The other thing that sometimes helps speed approval is to resolve it FIXED when the trunk patch lands, so it's clear at a glance that it has already landed there.

Comment 36

[Phil Ringnalda (:philor)]

mailnews/import/eudora/src/nsEudoraWin32.cpp 1.43.24.2