Japanese attachment file name garbled, after fix for Bug 243504 and Bug 263462

RESOLVED FIXED in mozilla1.8rc1

Status

MailNews Core
Composition
P1
major
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: Hidehiro Kozawa, Assigned: masayuki)

Tracking

(5 keywords)

Trunk
mozilla1.8rc1
x86
Windows XP
fixed-aviary1.0, fixed1.7.5, fixed1.8, intl, regression
Dependency tree / graph
Bug Flags:
blocking1.7.5 +
blocking1.8a6 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: Mozilla Japan wants to fix on 1.8 branch)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

13 years ago
Bug 273109 for Suite.
Japanese Localization team think that this bug is blocker for Japanese 1.7.5.
(Reporter)

Updated

13 years ago
Flags: blocking1.8a6?
Flags: blocking1.7.5?
(Reporter)

Comment 1

13 years ago
If this bug cannot stop 1.7.5 thread 'Boycott Thunderbird 1.0' become waste of time.

Is to backout not easy?

All Thunderbird, Trunk and 1.7branch has same patch.
Why do the bug make blocker only Thunderbird?
Why nobody wonder whether this bug cause or not?
too late to jerry.



Add to list in Bug 274366 (regressions meta-bug)
Blocks: 274366
m-wada@japan.com:

Bug 274366 is for bugzilla.mozilla.org regressions due to the upgrade, it has
nothing to do with any of the products also on b.m.o.
No longer blocks: 274366
Keywords: regression

Comment 4

13 years ago
Hidehiro Kozawa, the patch was backed out last night so this should be fixed in
today's builds. Can you verify?
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Flags: blocking1.7.5? → blocking1.7.5+
Resolution: --- → FIXED

Comment 5

13 years ago
oops, reopening for trunk fixing too. this has been fixed on the 1.7 branch (we
believe). Please verify there. Thanks.
Status: RESOLVED → REOPENED
Keywords: fixed1.7.5
Resolution: FIXED → ---
(Reporter)

Comment 6

13 years ago
17branch becomes better in Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP;
rv:1.7.5) Gecko/20041217.
not trunk.

not all fixed in 17branch yet. see detail bug 273109.
17branch is same status with thunderbird now.

Comment 7

13 years ago
jshin, can you look at this?
Assignee: jshin → sspitzer
Status: REOPENED → NEW

Comment 8

13 years ago
Oops, fixing assignee. jshin, we're getting very close to the 1.8a6 release. Can
you look into quickly?
Assignee: sspitzer → jshin
Flags: blocking1.8a6? → blocking1.8a6+

Comment 9

13 years ago
Sorry I'm looking at this now. A patch is available in bug 273109
Status: NEW → ASSIGNED

Comment 10

13 years ago
Created attachment 170776 [details] [diff] [review]
patch 

r=me 
This is the patcy Ahn Dal-Soo in bug 273109 (with a little modification by me)

asking for a1.8a6 as well to expedite things.
Attachment #170776 - Flags: superreview?(darin)
Attachment #170776 - Flags: review+
Attachment #170776 - Flags: approval1.8a6?

Comment 11

13 years ago
Comment on attachment 170776 [details] [diff] [review]
patch 

a=asa pending reviews.
Attachment #170776 - Flags: approval1.8a6? → approval1.8a6+

Comment 12

13 years ago
Comment on attachment 170776 [details] [diff] [review]
patch 

>Index: intl/uconv/src/nsUTF8ConverterService.cpp

> 
>+
>+
> NS_IMETHODIMP  

did you mean to add these extra newlines?  it is usually not a good
idea to add random newlines to source files.  or was this intentional?


>   // NS_UnescapeURL does not fill up unescapedSpec unless there's at least 
>   // one character to unescape.
>+  NS_UnescapeURL(PromiseFlatCString(aSpec).get(), aSpec.Length(), 
>+                 esc_AlwaysCopy, unescapedSpec);

The comment is now incorrect since you are passing esc_AlwaysCopy.  You
can also more efficiently write this code like so:

    NS_UnescapeURL(PromiseFlatCString(aSpec), esc_AlwaysCopy, unescapedSpec);


>+  // convert to UTF-8 unless ASCII only or UTF-8
>+  if (!IsASCII(unescapedSpec) && !IsUTF8(unescapedSpec)) {

It seems to me that you only need to check |if (!IsUTF8(...))| since that
covers the not ascii case as well.


>+    nsCAutoString UTF8Spec; 
>+    nsresult rv = ToUTF8(unescapedSpec, aCharset, UTF8Spec);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    unescapedSpec = UTF8Spec;
>   }

From a quick inspection of the source code for ToUTF8, which is defined
in this very same file, it seems that you can pass |unescapedSpec| as
both the input and output arguments to this function.  The point is that
that allows you to perhaps reuse the same buffer, which could help.  So,
you could write those 4 lines as:

      nsresult rv = ToUTF8(unescapedSpec, aCharset, unescapedSpec);
      NS_ENSURE_SUCCESS(rv, rv);


>+  NS_EscapeURL(unescapedSpec.get(), unescapedSpec.Length(),
>+               esc_FilePath | esc_OnlyASCII | esc_AlwaysCopy, aUTF8Spec);

This can also be written like this:

    NS_EscapeURL(unescapedSpec, esc_FilePath | esc_OnlyASCII | esc_AlwaysCopy,
		 aUTF8Spec);


Please submit a revised patch, thanks!
Attachment #170776 - Flags: superreview?(darin) → superreview-

Updated

13 years ago
Attachment #170776 - Flags: approval1.8a6+

Comment 13

13 years ago
Created attachment 170900 [details] [diff] [review]
updated patch

thanks. I should  have been more 'vigilant'...
Attachment #170776 - Attachment is obsolete: true
Attachment #170900 - Flags: superreview?

Comment 14

13 years ago
Comment on attachment 170900 [details] [diff] [review]
updated patch

sr=darin
Attachment #170900 - Flags: superreview? → superreview+

Comment 15

13 years ago
Comment on attachment 170900 [details] [diff] [review]
updated patch

a=asa for checkin to 1.8a6. time is short so please land quickly. Thanks.
Attachment #170900 - Flags: approval1.8a6+

Comment 16

13 years ago
fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Blocks: 263462

Comment 17

13 years ago
Comment on attachment 170900 [details] [diff] [review]
updated patch

risk : pretty low
platforms : all
test : done on Windows and Linux

The patch for bug 263462 was backed out from TB 1.0 and suite 1.7.x branches
because of this bug. To land that patch again, we need to fix this on branches.
Attachment #170900 - Flags: approval1.7.6?
Attachment #170900 - Flags: approval-aviary1.0.1?

Comment 18

13 years ago
*** Bug 273109 has been marked as a duplicate of this bug. ***

Comment 19

13 years ago
for grins i put this on the thunderbuird aviary branch too.
Keywords: fixed-aviary1.0

Updated

13 years ago
Version: 1.7 Branch → Trunk

Comment 20

13 years ago
(In reply to comment #19)
> for grins i put this on the thunderbuird aviary branch too.


Because the patch had been reflected in branch, it tried. 

The attachment Japanese file name is correct.
But, when the semicolon(;) is included in the file name, the file name is cut. 
(bug243504)
However, the problem is not in numbersign(#). 

Mac OS X 10.3.7
version 1.0 (20050114)

Comment 21

13 years ago
In EscapeChars array:
EscapeChars[0x24] for "#" is 512 which is 1000000000 in binary and represents
esc_Ref.
EscapeChars[0x3B] for ";" is 912 which is 1110010000 in binary and represents
esc_Ref | esc_Query | esc_Param | esc_Directory.

When we pass esc_SkipControl | esc_AlwaysCopy | esc_FilePath to NS_EscapeURL(),
it does not escape ";".
This probably broke mailto links.  See bug 278727.
Blocks: 278727

Comment 23

13 years ago
Wrote a comment in bug 278727 and requesting backing out this patch. I think we
have to write, to solve this issue, some code pretty specific to ShiftJIS.

Comment 24

13 years ago
Well, it's not just SJIS(Windows-932) that has  the problem but also
Big5(Windows-950) , UHC(Windows-949) and GBK(Windows-936)/GB18030 have the same
problem. So, if we have to write an encoding-specific hack, we have to take care
of all of them. GB18030 is worst because it's up to 4bytes long. It's possible....
Ack.. I hate all these encodings that violate ISO 2022 and 'infringe upon' the
ASCII range. 


Perhaps, we have to add another flag to indicate whether the encoding is
multibyte and non-ASCII-preserving and special case it in NS_UnescapeUR when
esc_NonASCII is on. 
It's getting uglier...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

13 years ago
Attachment #170900 - Flags: approval1.7.6?
Attachment #170900 - Flags: approval-aviary1.0.1?

Comment 25

13 years ago
Yep, writing encoding-specific patches is not a quick hack...
I've backed this out from the trunk to fix bug 278727.  mscott, you may want to
do the same with aviary branch.

Comment 27

12 years ago
I noticed that Thunderbird 1.5 Beta 2 has this issue.
(1) "blocking1.7.5" was approved by Asa, but 1.7.5 to 1.7.12 were already released.
(2) "blocking1.7.13" is not set yet.
Does it mean Thunderbird 1.5 will be released without fixing of this bug,
ignoring "blocking1.8a6" approved by Asa?
Jshin, can you fix this bug easily before release of Thunderbird 1.5, without
breaking bug 278727("?" in mailto:)/bug 243504("#" in filename of attacment),
with satisfaying your comment #24?
If not, many many Japanese Tb 1.5.0 users will complain at many many forums,
once Japanese Tb 1.5.0 will be released...
Assignee: jshin1987 → masayuki
Status: REOPENED → NEW
Component: MailNews: Main Mail Window → MailNews: Composition
Product: Mozilla Application Suite → Core
Status: NEW → ASSIGNED
Flags: blocking1.8rc1?
Priority: -- → P1
Created attachment 199012 [details] [diff] [review]
Patch rv1.0

I think that we should not use nsIURI for getting the leaf name. We should use
nsIFile instaed of one. This way is very simple.
Attachment #170900 - Attachment is obsolete: true
Attachment #199012 - Flags: review?(bienvenu)
Whiteboard: Mozilla Japan wants to fix on 1.8 branch
*** Bug 273109 has been marked as a duplicate of this bug. ***
Keywords: intl

Updated

12 years ago
Attachment #199012 - Flags: superreview?(bienvenu)
Attachment #199012 - Flags: review?(jshin1987)
Attachment #199012 - Flags: review?(bienvenu)
On Linux, this patch works fine too. I have tested on Windows. Of course, it
worked fine on Windows.
A Japanese tester said, this patch works fine on Mac.

Comment 34

12 years ago
jshin, david, can you tell us what you think of this patch and any risk in
taking it? Thank.

Comment 35

12 years ago
cc'ing bz - I'll try the patch locally to see if it regresses anything obvious.
So.. where is this string coming from, and what encoding is it in?
I guess the answer to that last one is "ascii, somehow", given that it's
|string| in the IDL.  Which means percent encoding from some charset.  Where do
we actually get the string from, though?
I think that file:// URL should be encoded by local system charset that is used
by |net_GetURLSpecFromFile|.

Comment 39

12 years ago
time is about out for us on this. 

Comment 40

12 years ago
I tried this patch and it didn't regress anything obvious on windows, and
handled non-7-bit ascii characters fine, and characters that need to be escaped,
etc. But jshin is really the expert here.

Comment 41

12 years ago
Masayuki seems to have come up with a clever alternative. Thanks ! 
I'll add 'r' flag after making sure that what his code expects to get is what it
actually gets (bz's question). 

Comment 42

12 years ago
jshin, it'd be great if you can verify this today so we can get it into the
trunk. That will improve the chances that we may take this for tbird 1.5.

Comment 43

12 years ago
jshin, this bug just isn't going to happen for 1.5 if we can't get some traction
on this patch ASAP. It needs bake time on the trunk and we don't have much time
left.
Now, we cannot attach the file that has non-ASCII name(Trunk only). Because bug
309335 is fixed. We cannot use UTF-8 URL for file://. But my patch may fix this
problem too.
Blocks: 309335

Comment 45

12 years ago
Masayuki, can you verify for us that your patch does indeed allow you to attach
file names with non ascii characters with Bug 309335?
I'm building the Tb with my patch. Please wait.
O.K. This patch fixes the regression of bug 309335.
why not use NS_GetFileFromURLSpec?
http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsNetUtil.h#623

Comment 49

12 years ago
> why not use NS_GetFileFromURLSpec?

Yes, prefer NS_GetFileFromURLSpec as it helps to simplify the code.
Attachment #199012 - Flags: superreview?(bienvenu)
Attachment #199012 - Flags: review?(jshin1987)
Attachment #199012 - Flags: review-
Created attachment 199626 [details] [diff] [review]
Patch rv1.1

Yeah, you're right. Thanks!
Attachment #199626 - Flags: superreview?(bienvenu)
Attachment #199626 - Flags: review?(jshin1987)
Attachment #199012 - Attachment is obsolete: true
Attachment #199626 - Flags: superreview?(bienvenu)
Attachment #199626 - Flags: review?(jshin1987)
Attachment #199626 - Flags: review-
Created attachment 199627 [details] [diff] [review]
Patch rv1.1
Attachment #199626 - Attachment is obsolete: true
Attachment #199627 - Flags: superreview?(bienvenu)
Attachment #199627 - Flags: review?(jshin1987)

Updated

12 years ago
Attachment #199627 - Flags: superreview?(bienvenu) → superreview+

Comment 52

12 years ago
Comment on attachment 199627 [details] [diff] [review]
Patch rv1.1

sorry for the delay. here goes my r. I tracked down its callers and concluded
that it should be safe.
Attachment #199627 - Flags: review?(jshin1987) → review+

Comment 53

12 years ago
Comment on attachment 199627 [details] [diff] [review]
Patch rv1.1

asking for a to 1.8rc1. 
This is a simple enough and safe patch and non-English users would be for sure
happy with this patch.
Attachment #199627 - Flags: approval1.8rc1?
Thanks.

checked-in to trunk.
-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #199627 - Flags: approval1.8rc1? → approval1.8rc1+
checked-in to 1.8 Branch too.
Flags: blocking1.8rc1?
Keywords: fixed1.8
Target Milestone: --- → mozilla1.8rc1

Updated

11 years ago
Blocks: 285073
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.