Closed Bug 455381 Opened 12 years ago Closed 11 years ago

WinCE XPCOM Explicit Unicode API Calls, Wide Char CheckVersion Function

Categories

(Core :: XPCOM, defect)

ARM
Windows Mobile 6 Professional
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: wolfe, Assigned: blassey)

References

Details

(Keywords: fixed1.9.1, mobile)

Attachments

(2 files, 6 obsolete files)

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-
Same attachment as before, removed WCHAR casts as per dougt request.
Attachment #338711 - Attachment is obsolete: true
Attachment #339982 - Flags: review?
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 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)
Assignee: nobody → wolfe
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-
Attached 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)
Attached 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)
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.
Attachment #343362 - Flags: review?(benjamin) → review-
Attachment #343362 - Attachment is obsolete: true
Attachment #346946 - Flags: review?(benjamin)
Attached patch nsCommonProcess changes (obsolete) — Splinter Review
Attachment #347231 - Flags: review?(benjamin)
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 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: 12 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 → ---
Attached patch fixed mochitest failure (obsolete) — Splinter Review
Attachment #351546 - Flags: review?(benjamin)
Attachment #347231 - Attachment is obsolete: true
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)
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: 12 years ago11 years ago
Resolution: --- → FIXED
Depends on: 478086
Assignee: nobody → bugmail
Keywords: fixed1.9.1
(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.