Closed Bug 488157 Opened 12 years ago Closed 12 years ago

Change uses of CP_ACP to UTF8 in conversions where needed

Categories

(Toolkit Graveyard :: XULRunner, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mfinkle, Assigned: hiro)

Details

Attachments

(1 file, 6 obsolete files)

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.
I am working on this.
Attached patch A patch (obsolete) — Splinter Review
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...
Attached patch Update patch (obsolete) — Splinter Review
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)
(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
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
Yes, the bug is using fopen.
Attached patch Revised patch (obsolete) — Splinter Review
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)
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)
Attachment #373622 - Attachment is obsolete: true
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?
Attachment #373777 - Attachment is obsolete: true
Attachment #373777 - Flags: review?(benjamin)
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));
(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.
(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?
Attached patch Modified attachment 373783 (obsolete) — Splinter 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)
Attached patch Revised patchSplinter Review
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
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.
Attachment #374403 - Flags: review?(benjamin)
Attachment #374403 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/8836a87bcdb9
Status: ASSIGNED → RESOLVED
Closed: 12 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.