411511 - Make nsIProcess accept Unicode paths as well as native charset

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Simon Montagu :smontagu]

Split out from bug 364285. Requesting blocking1.9 since the original bug is a blocker.

Comment #18 [reply] Benjamin Smedberg [:bs] (bsmedberg)  2008-01-03 06:56:31 PST

(From update of attachment 294479 [details] [diff] [review])
This isn't correct... LaunchWithIProcess isn't expecting UTF8, it's expecting
the native charset.

To do this correctly, you really would need nsIProcess.runw or to make
nsIProcess accept UTF8 instead of native.

Comment 1

[Mike Schroepfer]

Simon any updates here?

Comment 2

[Simon Montagu :smontagu]

I have a patch which I'm currently in the middle of testing.

Comment 3

[Simon Montagu :smontagu]

Can we get this into litmus? I have two testing scenarii:

A) (from bug 364285)

 1) Try to open a non-HTML file with Unicode characters in the name, e.g. http://ivadm.ivanovo.ru/portal/request.nsf/38c42d7b098cf39fc3257105003998ed/70cade6ae52a6d89c32572170027b38d/$FILE/%D0%98%D0%B7%D0%B2%D0%B5%D1%89%20%E2%84%96%2081-%D1%82%D1%80%D0%B0%D0%BD%D1%81%D0%BF%D0%BE%D1%80%D1%82.doc or a locally saved copy
 2) In the "what should Minefield do with this file" dialog, choose to open it with any other, but not your default program (Notepad, Wordpad, Word or any other that supports Unicode).
 3) Expected results: the file should be opened without error messages, and the name should appear as "Извещ № 81-транспорт.doc"

B) (from bug 408923)
 1) in about:config set the pref to use an external editor for view-source:
    - view_source.editor.external=true
    - view_source.editor.path=<path to editor>
 2) Go to a page in UTF-8 with non-ASCII characters in the title, e.g. http://www.gialloporpora.netsons.org/
 3) View source
 4) Expected results: the source should open in the editor without error messages, and the name should appear as "Il blog che non c'è.htm"

C) Same as B, but use a local file with non-ASCII characters in the title, e.g. Русский.html

Comment 4

[Simon Montagu :smontagu]

Attachment: Patch

Comment 5

[Benjamin Smedberg]

Comment on attachment 313941 [details] [diff] [review]
Patch

I'm not comfortable with the approach of declaring that "in string" arguments should be UTF-8. XPConnect treats "string" as a truncated type. So this will need to be "wstring": and for the sake of stability and conversion, we shouldn't go modifying the existing API so close to release-time: instead, add a new API "runw" with wide characters.

Comment 6

[Mike Schroepfer]

So I'm going to remove this from the blocking list - given the phase of the release I'm not sure we want/need to wait for this.  Re-nom with reasons if you disagree...

Comment 7

[Simon Montagu :smontagu]

Attachment: Patch v.2

Works on Windows; not yet tested on other platforms

Comment 8

[Simon Montagu :smontagu]

Attachment: Tested on Windows, Mac and Linux

Comment 9

[Benjamin Smedberg]

I will get to this next week, probably 15-Dec

Comment 10

[Benjamin Smedberg]

Comment on attachment 416521 [details] [diff] [review]
Tested on Windows, Mac and Linux

>diff --git a/uriloader/exthandler/nsMIMEInfoImpl.cpp b/uriloader/exthandler/nsMIMEInfoImpl.cpp

> /* static */
>+nsIProcess*
>+nsMIMEInfoBase::InitProcess(nsIFile* aApp, nsresult* aResult)
>+{
>+  NS_ASSERTION(aApp, "Unexpected null pointer, fix caller");
>+
>+  nsCOMPtr<nsIProcess> process = do_CreateInstance(NS_PROCESS_CONTRACTID,
>+                                                   aResult);
>+  if (NS_FAILED(*aResult))
>+    return nsnull;
>+
>+  if (NS_FAILED(process->Init(aApp)))
>+    return nsnull;
>+
>+  return process;
>+}

Something is wrong with this API: it returns a raw nsIProcess*, but it returns that from the only reference (the automatic nsCOMPtr), so it seems that it is returning an invalid pointer. I think you probably want to be returning an already_AddRefed<nsIProcess> and `return process.forget();`

>diff --git a/xpcom/tests/TestUnicodeArguments.cpp b/xpcom/tests/TestUnicodeArguments.cpp

>+#include <string.h>
>+
>+static const char* expected_utf8[args_length] = {

Although on most Linux and Mac systems the native charset is UTF8, that is not universally true. Do you care?

>diff --git a/xpcom/threads/nsProcessCommon.cpp b/xpcom/threads/nsProcessCommon.cpp

> #if defined(XP_WIN)
> // Out param `wideCmdLine` must be PR_Freed by the caller.
>-static int assembleCmdLine(char *const *argv, PRUnichar **wideCmdLine)
>+static int assembleCmdLine(char *const *argv, PRUnichar **wideCmdLine,
>+                           UINT codePage)

Ugh. It seems silly here for us to convert the wide arguments to UTF8 just to convert them back to unicode in this function. But the alternative is having two copies of this quoting logic, which might be worse. Thoughts?

>+NS_IMETHODIMP
>+nsProcess::CopyArgsAndRunProcessw(PRBool blocking, const PRUnichar** args,
>+                                  PRUint32 count, nsIObserver* observer,
>+                                  PRBool holdWeak)
>+{
>+    // make sure that when we allocate we have 1 greater than the
>+    // count since we need to null terminate the list for the argv to
>+    // pass into PR_CreateProcess
>+    char **my_argv = NULL;
>+    my_argv = (char **)nsMemory::Alloc(sizeof(char *) * (count + 2) );
>+    if (!my_argv) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    }

who frees my_argv and the elements within it?

>-    PRInt32 numChars = MultiByteToWideChar(CP_ACP, 0, my_argv[0], -1, NULL, 0); 
>+    PRInt32 numChars = MultiByteToWideChar(codePage, 0, my_argv[0], -1, NULL, 0); 
>     PRUnichar* wideFile = (PRUnichar *) PR_MALLOC(numChars * sizeof(PRUnichar));
>     MultiByteToWideChar(CP_ACP, 0, my_argv[0], -1, wideFile, numChars);

my_argv[0] here is which charset? It appears to be the native charset even with this patch, which is unfortunate... we should be using GetPath and using the unicode name. We should also have a test that a unicode executable works (e.g. rename TestUnicodeArguments.exe to SomeThaiName.exe and run it).

Comment 11

[Simon Montagu :smontagu]

(In reply to comment #10)
> Something is wrong with this API: it returns a raw nsIProcess*, but it returns
> that from the only reference (the automatic nsCOMPtr), so it seems that it is
> returning an invalid pointer. I think you probably want to be returning an
> already_AddRefed<nsIProcess> and `return process.forget();

Fixed

> Although on most Linux and Mac systems the native charset is UTF8, that is not
> universally true. Do you care?

I used to care, but you convinced me not to a while back in bug 442627 ;-)

> Ugh. It seems silly here for us to convert the wide arguments to UTF8 just to
> convert them back to unicode in this function. But the alternative is having
> two copies of this quoting logic, which might be worse. Thoughts?

You reproduced my thought processes exactly in this comment. I don't like it, but I think it's the lesser of two evils.

> who frees my_argv and the elements within it?

Fixed

> my_argv[0] here is which charset? It appears to be the native charset even with
> this patch, which is unfortunate... we should be using GetPath and using the
> unicode name. We should also have a test that a unicode executable works (e.g.
> rename TestUnicodeArguments.exe to SomeThaiName.exe and run it).

Fixed, though I don't know how likely a scenario it is that the default application for something might have a name not expressible in the native charset. Still, it's also nice to have a test for nsIFile::MoveTo with a Unicode target.

Comment 12

[Simon Montagu :smontagu]

Attachment: Addressed review comments

Comment 13

[Benjamin Smedberg]

Comment on attachment 420287 [details] [diff] [review]
Addressed review comments

In CopyArgsAndRunProcess(W), nsMemory::Alloc/Free are older synonyms for NS_Alloc/NS_Free, so just the NS_ functions consistently.

Comment 14

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/rev/2cc5ad2cf917

Comment 15

[Jacek Caban]

Attachment: mingw fix

This patch broke mingw build that doesn't support wmain. The fix is easy - make sure that we're compiling on MSC before using wmain. While I was at this, I've also fixed printf arguments in main.

Comment 16

[Jacek Caban]

Attachment: rebased mingw fix

Thanks for review. I've attached a rebased fix against current m-c as part of the patch was committed in bug 557380.

Comment 17

[Jacek Caban]

Reopening for landing attachment 437537 [details] [diff] [review].

Comment 18

[Benjamin Smedberg]

Please don't reopen for that. You can use a new bug if you need to.

Comment 19

[Justin Wood (:Callek)]

(In reply to comment #16)
> Created an attachment (id=437537) [details]
> rebased mingw fix
> 

http://hg.mozilla.org/mozilla-central/rev/5153bb3533e6

Comment 20

[Simon Montagu :smontagu]

http://dafizilla.wordpress.com/2010/07/24/nsiprocess-now-works-with-unicode/ points out that this change isn't documented at https://developer.mozilla.org/en/nsIProcess

Comment 21

[Benjamin Smedberg]

smontagu, can you update them?

Comment 22

[Simon Montagu :smontagu]

Done.