Change uses of CP_ACP to UTF8 in conversions where needed

RESOLVED FIXED

Status

RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: mfinkle, Assigned: hiro)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

nsWindowsWMain converts strings from UTF16 to UTF8 and then passes them on as const char* strings. Several places in nsXULStub.cpp and nsGlueLinkingWin.cpp convert those const char* strings back to UTF16 using:

MultiByteToWideChar(CP_ACP, ...)

This can cause problems when the native codepage is not UFT8 compatible. We should be explicitly converting these buffers from UTF8, not the native codepage. I'm not 100% sure MultiByteToWideChar(CP_UTF8, ...) is supported on WinCE. If not we could use ConvertUTF8toUTF16.
(Assignee)

Comment 1

10 years ago
I am working on this.
(Assignee)

Comment 2

10 years ago
Created attachment 372500 [details] [diff] [review]
A patch

Use CP_UTF8 instead of CP_ACP.

The patch works on Windows Mobile Emulator 6.1.4.
Attachment #372500 - Flags: review?(benjamin)
changes in this bug could be conflicting to bug 436998 's change for the stub part.

they as well as some further ones are addressed there...
(Assignee)

Comment 4

10 years ago
Created attachment 373214 [details] [diff] [review]
Update patch

Adapt to the latest trunk.
Assignee: nobody → ikezoe
Attachment #372500 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #373214 - Flags: review?(benjamin)
Attachment #372500 - Flags: review?(benjamin)

Comment 5

10 years ago
(In reply to comment #4)

This patch doesn't work if a native charset string is included in the path, because XPCOMGlueLoadDependentLibs() calls fopen(), and fopen() expects native chartset.

  http://hg.mozilla.org/mozilla-central/file/cb56ac937481/xpcom/glue/standalone/nsXPCOMGlue.cpp#l98

I've confirmed it by launching the fennec.exe from a dir which includes a Japanese string in the path.

In addition, nsINIParser::Init() is called with UTF-8 path at nsXULStub.cpp, but it uses fopen() too.

  http://hg.mozilla.org/mozilla-central/file/cb56ac937481/xulrunner/stub/nsXULStub.cpp#l374

Comment 6

10 years ago
Created attachment 373622 [details] [diff] [review]
Temporal patch to enable lauching a xulapp from non-ASCII path

When I unify charset conversion to CP_ACP, I can launch the fennec.exe from Japanese path. I've attached a patch.
And it may be harmless in most cases. Because, only characters which can be converted to native characters will be passed arguments since CLI is used with native charset in most cases.

But, of course this patch isn't ideal as some pepole mentioned before.

If we want to unify charset conversion to CP_UTF8, we should resolve above fopen() issue.
But I'm not sure how should I do it in Mozilla's rule.
(In reply to comment #6)
> Created an attachment (id=373622) [details]
> Temporal patch to enable lauching a xulapp from non-ASCII path

interesting ... those changes ( CP_ACP -> CP_UTF8 ) were suggested by the module owner in 
https://bugzilla.mozilla.org/show_bug.cgi?id=436998#c38
Trying to get the path in a format that can be used with all charsets is a pain and I suspect you will need to use _wfopen in a similar manner as was done here
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/src/updater/readstrings.cpp#46

Comment 9

10 years ago
Yes, the bug is using fopen.
(Assignee)

Comment 10

10 years ago
Created attachment 373777 [details] [diff] [review]
Revised patch

Use _wfopen for Windows.

I did not define NS_fopen since the conversion from UTF8 to WideChar is needed for Windows so ifdef macro is also needed anyway.
Attachment #373214 - Attachment is obsolete: true
Attachment #373777 - Flags: review?(benjamin)
Attachment #373214 - Flags: review?(benjamin)
Created attachment 373783 [details] [diff] [review]
Use NS_NewLocalFile for Windows platform

The latest patch still fails to launch a app from non-ASCII path at NS_NewNativeLocalFile.

I've replaced it with NS_NewLocalFile.
Now it works file with non-ASCII path.
Attachment #373783 - Flags: review?(benjamin)

Updated

10 years ago
Attachment #373622 - Attachment is obsolete: true
(Assignee)

Comment 12

10 years ago
After some thought, I think it's better to define NS_tfopen macro or inline function. Can I use xpcom/glue/nsCRTGlue.h for NS_tfopen?
(Assignee)

Updated

10 years ago
Attachment #373777 - Attachment is obsolete: true
Attachment #373777 - Flags: review?(benjamin)
(Assignee)

Comment 13

10 years ago
Comment on attachment 373783 [details] [diff] [review]
Use NS_NewLocalFile for Windows platform


>   { // Scope COMPtr and AutoAppData
>     nsCOMPtr<nsILocalFile> iniFile;
>+#ifdef XP_WIN
>+    wchar_t wideINIPath[MAX_PATH];
>+    MultiByteToWideChar(CP_UTF8, 0, iniPath, -1, wideINIPath, MAX_PATH);
>+    rv = NS_NewLocalFile(nsDependentString(wideINIPath), PR_FALSE,
>+                         getter_AddRefs(iniFile));
>+#else
>     rv = NS_NewNativeLocalFile(nsDependentCString(iniPath), PR_FALSE,
>                                getter_AddRefs(iniFile));
>+#endif

How about the following code? There is no ifdef macro. 

    // On Windows and Windows CE, thier native encoding is not UTF-8,
    // so we need to convert the path (UTF-8 encoded) to UTF16
    // and use NS_NewLocalFile instead of NS_NewNativeLocalFile.
    NS_ConvertUTF8toUTF16 path(iniPath);
    rv = NS_NewLocalFile(path, PR_FALSE, getter_AddRefs(iniFile));

Comment 14

10 years ago
(In reply to comment #13)

Probably you are misunderstanding this issue.

>     // On Windows and Windows CE, thier native encoding is not UTF-8,
>     // so we need to convert the path (UTF-8 encoded) to UTF16
>     // and use NS_NewLocalFile instead of NS_NewNativeLocalFile.

Native encoding is not always UTF-8 also on other platform, and iniPath is not always UTF-8 because it comes from argv.
Probably only windows platform guarantee iniPath as UTF-8 encoding because it comes from toolkit/xre/nsWindowsWMain.cpp.
(Assignee)

Comment 15

10 years ago
(In reply to comment #14)
> (In reply to comment #13)
> 
> Probably you are misunderstanding this issue.
> 
> >     // On Windows and Windows CE, thier native encoding is not UTF-8,
> >     // so we need to convert the path (UTF-8 encoded) to UTF16
> >     // and use NS_NewLocalFile instead of NS_NewNativeLocalFile.
> 
> Native encoding is not always UTF-8 also on other platform, and iniPath is not
> always UTF-8 because it comes from argv.
> Probably only windows platform guarantee iniPath as UTF-8 encoding because it
> comes from toolkit/xre/nsWindowsWMain.cpp.

Ah, I see.
What do you think about use of NS_ConvertUTF8toUTF16 instead of MultiByteToWideChar with CP_UTF8?
(Assignee)

Comment 16

10 years ago
Created attachment 374228 [details] [diff] [review]
Modified attachment 373783 [details] [diff] [review]

Changes from attachment 373783 [details] [diff] [review]

* define TS_tfopen macro in both nsINIParser.cpp and nsXPCOMGlue.cpp
* Use NS_ConvertUTF8toUTF16 except in nsGlueLinkingWin.cpp
Attachment #373783 - Attachment is obsolete: true
Attachment #373783 - Flags: review?(benjamin)
(Assignee)

Comment 17

10 years ago
Created attachment 374403 [details] [diff] [review]
Revised patch

I was wrong. NS_ConvertUTF8toUTF16 can not be used until XPCOM libraries loaded.
So this patch use inline function that convert path string and invoke _wfopen with it.
And this patch does leave ifdef macro for fopen and _wfopen in   nsINIParser::Init(nsILocalFile* aFile) since I have no idea to handle these macros clearly.
Attachment #374228 - Attachment is obsolete: true
(Assignee)

Comment 18

10 years ago
Comment on attachment 374403 [details] [diff] [review]
Revised patch

I can reproduce the issue mentioned in comment #6 on Windows XP with Japanese folder name. And I confirmed the patch fixes the issue.
(Assignee)

Updated

10 years ago
Attachment #374403 - Flags: review?(benjamin)

Updated

10 years ago
Attachment #374403 - Flags: review?(benjamin) → review+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/8836a87bcdb9
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.