WinCE XPCOM Explicit Unicode API Calls, Wide Char CheckVersion Function

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: wolfe, Assigned: blassey)

Tracking

({fixed1.9.1, mobile})

Trunk
ARM
Windows Mobile 6 Professional
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Reporter

Description

11 years ago
Converted implicit char API calls to explicit wide character API calls.

Added explicit PRUnichar CheckVersion() function and PRUnichar version of NS_CompareVersions

Added NextChar() function if MOZILLA_INTERNAL_API is defined
Attachment #338711 - Flags: review?(doug.turner)
Comment on attachment 338711 [details] [diff] [review]
WinCE XPCOM Explicit Unicode API changes

why this:

+#if defined(XP_WIN)
+#include <tchar.h>
+#include "nsString.h"
+#endif
+


Why the explicit cast here:

+    if(GetModuleFileNameW(GetModuleHandleW(L"xpcom.dll"), (LPWCH)executable, MAX_PATH) &&

avoid the casts as they can hide problems.  CreateProcessW has the same casts.

Does the stack walking code actually work on wince -- or are these just narrow->wide changes.

bsmedberg needs to review these changes after you post another patch.
Attachment #338711 - Flags: review?(doug.turner) → review-
Reporter

Comment 2

11 years ago
Same attachment as before, removed WCHAR casts as per dougt request.
Attachment #338711 - Attachment is obsolete: true
Attachment #339982 - Flags: review?
Reporter

Comment 3

11 years ago
Comment on attachment 339982 [details] [diff] [review]
Same attachment, with some WCHAR casts removed

doug said pass this on to stuart for review
Attachment #339982 - Flags: review? → review?(pavlov)
Can't we move completely to unicode versions? Why do we need to keep the narrow versions around too?

Comment 5

11 years ago
Comment on attachment 339982 [details] [diff] [review]
Same attachment, with some WCHAR casts removed

this looks ok to me...
you should be able to get rid of the
+#include <tchar.h>
and remove the ifdef around the nsString.h includes i believe
Attachment #339982 - Flags: review?(pavlov) → review?(benjamin)

Updated

11 years ago
Assignee: nobody → wolfe

Comment 6

11 years ago
Comment on attachment 339982 [details] [diff] [review]
Same attachment, with some WCHAR casts removed

>+static PRBool
>+CheckVersion(const PRUnichar* toCheck,
>+             const GREVersionRange *versions,
>+             PRUint32 versionsLength);
> static PRBool
> CheckVersion(const char* toCheck,
>              const GREVersionRange *versions,
>              PRUint32 versionsLength);

Duplicating this code seems bad... can you just template it instead?

>diff --git a/xpcom/glue/nsVersionComparator.cpp b/xpcom/glue/nsVersionComparator.cpp

>+#include <wchar.h>
>+#ifdef MOZILLA_INTERNAL_API
>+#include "nsString.h"
>+#else
>+#include "nsStringAPI.h"
>+#endif

replace this with #include "nsStringGlue.h"

> 
> struct VersionPart {
>   PRInt32     numA;
>@@ -49,7 +55,24 @@
>   PRInt32     numC;
> 
>   char       *extraD;  // null-terminated
>+
> };
>+
>+
>+struct VersionPartW {

more templates

>diff --git a/xpcom/glue/standalone/nsGlueLinkingWin.cpp b/xpcom/glue/standalone/nsGlueLinkingWin.cpp

>+    const PRUnichar* xpcomFile = 
>+        NS_ConvertUTF8toUTF16(aXpcomFile).get();

1) It's hard to believe that aXpcomFile is UTF8... I'm pretty sure in most cases it won't be. If it is, we need to re-document a bunch of stuff in nsXPCOMGlue.h, and fix the relevant callers.
2) We can't use mozilla string code here, because we haven't loaded XPCOM yet and so all string functions (and even basic functions like NS_Alloc) will fail.

>diff --git a/xpcom/obsolete/component/Makefile.in b/xpcom/obsolete/component/Makefile.in
>--- a/xpcom/obsolete/component/Makefile.in
>+++ b/xpcom/obsolete/component/Makefile.in
>@@ -49,7 +49,9 @@
> SHORT_LIBNAME	= xpcomctc
> endif
> 
>+ifndef MINIMO
> EXPORT_LIBRARY  = 1
>+endif
> IS_COMPONENT	= 1
> GRE_MODULE	= 1
> LIBXUL_LIBRARY = 1

I can't see why you'd touch this code... only Thunderbird if anyone is still building it... and wasn't MINIMO removed in any case?

>diff --git a/xpcom/string/public/nsUTF8Utils.h b/xpcom/string/public/nsUTF8Utils.h

I don't understand what the changes in this file are for.

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

>-static int assembleCmdLine(char *const *argv, char **cmdLine)
>+static int assembleCmdLine(char *const *argv, PRUnichar **cmdLine)

You're zero-extending the characters in argv. This is incorrect.
Attachment #339982 - Flags: review?(benjamin) → review-
Posted patch updated (obsolete) — Splinter Review
(In reply to comment #6)
> (From update of attachment 339982 [details] [diff] [review])
> >+static PRBool
> >+CheckVersion(const PRUnichar* toCheck,
> >+             const GREVersionRange *versions,
> >+             PRUint32 versionsLength);
> > static PRBool
> > CheckVersion(const char* toCheck,
> >              const GREVersionRange *versions,
> >              PRUint32 versionsLength);
> 
> Duplicating this code seems bad... can you just template it instead?

Agreed, but I don't see how to use templates without a pretty major rewrite.

> >-static int assembleCmdLine(char *const *argv, char **cmdLine)
> >+static int assembleCmdLine(char *const *argv, PRUnichar **cmdLine)
> 
> You're zero-extending the characters in argv. This is incorrect.

not sure I understand this comment, can you elaborate?
Assignee: wolfe → blassey
Attachment #339982 - Attachment is obsolete: true
Attachment #343183 - Flags: review?(benjamin)
Posted patch fixed a typo (obsolete) — Splinter Review
Attachment #343183 - Attachment is obsolete: true
Attachment #343362 - Flags: review?
Attachment #343183 - Flags: review?(benjamin)
Attachment #343362 - Flags: review? → review?(benjamin)

Comment 9

11 years ago
You're assigning a char* to a PRUnichar* without any conversion at all (in assembleCmdLine). The character set of the char* isn't documented but is by convention the "native" character set.

Please split the nsprocess change to a different bug, because it's going to be a more involved problem to fix.

Updated

11 years ago
Attachment #343362 - Flags: review?(benjamin) → review-
Attachment #343362 - Attachment is obsolete: true
Attachment #346946 - Flags: review?(benjamin)
Posted patch nsCommonProcess changes (obsolete) — Splinter Review
Attachment #347231 - Flags: review?(benjamin)

Comment 12

11 years ago
Comment on attachment 346946 [details] [diff] [review]
removed nsCommonProcess 

diff --git a/xpcom/glue/nsVersionComparator.cpp b/xpcom/glue/nsVersionComparator.cpp

 #include <stdlib.h>
 #include <string.h>
+#include <wchar.h>
+#include "nsStringGlue.h"

You'll need #ifdef XP_WIN here and elsewhere in this file. The wide version of NS_CompareVersion should only be available on windows.
Attachment #346946 - Flags: review?(benjamin) → review+

Comment 13

11 years ago
Comment on attachment 347231 [details] [diff] [review]
nsCommonProcess changes

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

Please document that the wideCmdLine parameter in assembleCmdLine must be freed with PR_Free.

>-static int assembleCmdLine(char *const *argv, char **cmdLine)
>+static int assembleCmdLine(char *const *argv, PRUnichar **wideCmdLine)

>-    retVal = CreateProcess(NULL,
>+    retVal = CreateProcessW(NULL,
>                            // const_cast<char*>(mTargetPath.get()),
>                            cmdLine,

nit, please re-indent
Attachment #347231 - Flags: review?(benjamin) → review+
pushed
http://hg.mozilla.org/mozilla-central/rev/ce8fbd7d222e
and
http://hg.mozilla.org/mozilla-central/rev/0f1b8ab7cd8e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
I backed out http://hg.mozilla.org/mozilla-central/rev/ce8fbd7d222e because it caused unittest and mochitest failures:

Error: uncaught exception: [Exception... "Component returned failure code:
0x80520003 (NS_ERROR_FILE_EXECUTION_FAILED) [nsILocalHandlerApp.launchWithURI]"
 nsresult: "0x80520003 (NS_ERROR_FILE_EXECUTION_FAILED)"  location: "JS frame
::
http://localhost:8888/tests/uriloader/exthandler/tests/mochitest/handlerApps.js
:: test :: line 110"  data: no]

while running
http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch fixed mochitest failure (obsolete) — Splinter Review
Attachment #351546 - Flags: review?(benjamin)
Attachment #347231 - Attachment is obsolete: true

Comment 17

11 years ago
Comment on attachment 351546 [details] [diff] [review]
fixed mochitest failure

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

> #if defined(XP_WIN)
>-static int assembleCmdLine(char *const *argv, char **cmdLine)
>+// out param must be freed by caller
>+static int assembleCmdLine(char *const *argv, PRUnichar **wideCmdLine)

Please be specific in the comment:

Out param `wideCmdLine` must be PR_Freed by the caller.

>+    char *p, *q, *tmpCmdLine, **cmdLine;

As discussed on IRC, the *tmpCmdLine, **cmdLine change is just confusing. Please just use *cmdLine and remove the indirection below.
Attachment #351546 - Flags: review?(benjamin) → review-
Attachment #351546 - Attachment is obsolete: true
Attachment #351591 - Flags: review?(benjamin)

Updated

11 years ago
Attachment #351591 - Flags: review?(benjamin) → review+
Assignee: bugmail → nobody
Component: General → XPCOM
Product: Fennec → Core
QA Contact: general → xpcom
Attachment #346946 - Flags: approval1.9.1?
Attachment #351591 - Flags: approval1.9.1?
Comment on attachment 346946 [details] [diff] [review]
removed nsCommonProcess 

a191=beltzner
Attachment #346946 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 351591 [details] [diff] [review]
gets rid of tmpCmdLine

a191=beltzner
Attachment #351591 - Flags: approval1.9.1? → approval1.9.1+
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 478086
Assignee: nobody → bugmail
Keywords: fixed1.9.1

Comment 22

10 years ago
(In reply to comment #21)
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/66c7e78d564e

This broke runtime GRE discovery on Windows. Details in bug 492948.
You need to log in before you can comment on or make changes to this bug.