Add a UTF-16 API to load a library

RESOLVED FIXED in 4.6.2

Status

NSPR
NSPR
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Jungshik Shin, Assigned: Jungshik Shin)

Tracking

({fixed1.8.1, intl})

other
4.6.2
x86
Windows 2000
fixed1.8.1, intl
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 9 obsolete attachments)

19.40 KB, patch
Jungshik Shin
: review+
Darin Fisher
: superreview+
Details | Diff | Splinter Review
613 bytes, patch
Details | Diff | Splinter Review
7.24 KB, patch
Jungshik Shin
: review+
Darin Fisher
: superreview+
Details | Diff | Splinter Review
18.24 KB, patch
Wan-Teh Chang
: approval-branch-1.8.1+
Details | Diff | Splinter Review
827 bytes, patch
Wan-Teh Chang
: approval-branch-1.8.1+
Details | Diff | Splinter Review
1.13 KB, patch
Wan-Teh Chang
: review+
Darin Fisher
: superreview+
Mike Schroepfer
: approval1.8.1+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
spun off from bug 162361.
(Assignee)

Comment 1

11 years ago
Created attachment 210989 [details] [diff] [review]
patch 

I didn't add a UTF-16 API, but modified the NSPR part of my patch to bug 162361 to avoid making callers  cast PRUnichar* to char*. Instead, the casting is now done *internally* when calling pr_unlockedFindLibrary. Besides, the list of loaded library names are always kept in UTF-8. As I mentioned in bug 162361, it's not necessary in virtually all situations, but to be bullet-proof, we need to do this. 

PR_LoadLibraryWithFlags  still doesn't work on Windows 9x/ME if it's called with a UTF-16 pathname because  LoadLibraryW is not wrapped up with LoadLibrary, yet. Moreover, our own UTF-16 to UTF-8 converter has to be added because CP_UTF8 is not supported on Windows 95.  Despite that, we may just get away with this at least for now if we clearly document that  |PR_LibSpec_UTF16_Pathname| is only supported on Win NT4/2k/XP/Vista (just as PL_LibSpec_Mac* are only supported on Mac OS classic).

If PL_LibSpec_UTF16_Pathname has to be supported on Win 9x/ME as well, I can rather easily do two things mentioned above.  

Darin and wtc, what would you say?
(Assignee)

Comment 2

11 years ago
Created attachment 211583 [details] [diff] [review]
patch update

To avoid calling malloc too often, I added stack buffers for the conversion. Unicode path is still only supported on Win 2k/XP/NT4. Our own UTF16toUTF8 conversion routine is not yet added so that it doesn't work on Windows95 (even for 8bit paths).
Attachment #210989 - Attachment is obsolete: true
(Assignee)

Comment 3

11 years ago
Comment on attachment 211583 [details] [diff] [review]
patch update


>+#ifdef WIN32
>+    if (utf8name && utf8name != utf8name_stack) 
>+        PR_Free(utf8name);
>+    if ((flags & PR_LD_PATHW) && wname && wname != wname_stack)
       Here, I meant 

      if (!(flags & PR_LD_PATHW) && ..... )

>+        PR_Free(wname);
(Assignee)

Comment 4

11 years ago
Created attachment 212262 [details] [diff] [review]
patch update

the previous patch has a couple of mistakes.
Attachment #211583 - Attachment is obsolete: true
(Assignee)

Comment 5

11 years ago
Sorry for bug spam. The latest patch should work on Windows 95(where CP_UTF8 is not supported by WideCharToMultiByte). It also emulates LoadLibraryW on Win 9x/ME so that PR_LibSpec_UTF16PATH works on all Win32 platforms. 
(Assignee)

Comment 6

11 years ago
Created attachment 212469 [details] [diff] [review]
patch

Asking for r/sr. After landing this patch, I'll ask for r/sr for my patch for bug 162361.
Attachment #212262 - Attachment is obsolete: true
Attachment #212469 - Flags: superreview?(darin)
Attachment #212469 - Flags: review?(jackywtc)
(Assignee)

Updated

11 years ago
Attachment #212469 - Flags: review?(jackywtc) → review?(wtchang)

Comment 7

11 years ago
Comment on attachment 212469 [details] [diff] [review]
patch

>Index: nsprpub/pr/include/prlink.h

> typedef enum PRLibSpecType {
>     PR_LibSpec_Pathname,
>     PR_LibSpec_MacNamedFragment,   /* obsolete (for Mac OS Classic) */
>-    PR_LibSpec_MacIndexedFragment  /* obsolete (for Mac OS Classic) */
>+#ifdef WIN32
>+    PR_LibSpec_MacIndexedFragment, /* obsolete (for Mac OS Classic) */
>+    PR_LibSpec_UTF16_Pathname      /* supported only on Win32 */ 
>+#else 
>+    PR_LibSpec_MacIndexedFragment /* obsolete (for Mac OS Classic) */
>+#endif
> } PRLibSpecType;

As with MacIndexedFragment, I recommend not #ifdef'ing this value.
Define it for all platforms with the comment that it only works on
Windows in this version of NSPR.


>+#ifdef WIN32
>+        /* if type is PR_LibSpec_UTF16_Pathname */
>+        /* supported only on Win32 */
>+        const PRUnichar *utf16_pathname;
>+#endif

No need to #ifdef IMO.


>+#ifdef WIN32
>+#define PR_LD_PATHW  0x400  /* INTERNAL USE ONLY on Win32 */
>+#endif

I'd move this into the implementation file.


>Index: nsprpub/pr/src/linking/prlink.c

>+#ifdef WIN32
>+#ifndef WINNT
>+static PRBool _pr_UseUnicode = PR_FALSE;
>+static int pr_ConvertUTF16toUTF8(LPCWSTR wname, LPSTR name, int len);
>+#else
>+static int pr_ConvertUTF16toUTF8(LPCWSTR wname, LPSTR name, int len);
>+#endif
>+#endif

The function prototype seems to be identical in both cases.  Can it be
moved out of the #ifdef?

>+#ifdef DEBUG 
>+  // In debug builds, allow explicit use of ANSI methods for testing purposes.
>+   if (getenv("WINAPI_USE_ANSI"))
>+       _pr_UseUnicode = PR_FALSE;
>+#endif

It might be nice to change WINREG_USE_ANSI to WINAPI_USE_ANSI
as well.  That way one env var can be used to switch the entire
system over to simulating a Win9x environment.


>+#if 0
>+            if (usedDefault) {
>+                /* error code 583: this doesn't fit our need very well,
>+                 * but I can't find anything better
>+                 */
>+                SetLastError(ERROR_UNDEFINED_CHARACTER);
>+                oserr = ERROR_UNDEFINED_CHARACTER;
>+                goto unlock;
>+            }
>+#endif

Should this hunk of code be removed instead of commented out like this?
Or, should we try to figure out whatever to enable it again?


>+    /* if we're concerned with a lone high surrogate,
>+     * we have to take care of it here, but we just drop it 
>+     */
>+    if (len > 0 /* || name */)
>+        *p = '\0';
>+    return utf8Len + 1;

Can the commented out code be removed?


I did not check your UTF-16 to UTF-8 conversion code very carefully.
I assume it is based on the similar code in XPCOM.
(Assignee)

Comment 8

11 years ago
Created attachment 212923 [details] [diff] [review]
patch addressing wtc's concerns

Thanks for your comments. Here is a patch with your concerns addressed.

As for the converter, yes, it's taken from the corresponding xpcom code with the lone surrogate handling part removed.
Attachment #212469 - Attachment is obsolete: true
Attachment #212923 - Flags: superreview?(darin)
Attachment #212923 - Flags: review?(wtchang)
Attachment #212469 - Flags: superreview?(darin)
Attachment #212469 - Flags: review?(wtchang)

Comment 9

11 years ago
Comment on attachment 212923 [details] [diff] [review]
patch addressing wtc's concerns

>Index: nsprpub/pr/src/linking/prlink.c

>+    if (!(flags & PR_LD_PATHW) && wname && wname != wname_stack)
>+        PR_Free(wname);

I think that checking |flags| here is unnecessary.


>+static PRStatus 
>+pr_ConvertSingleCharToUTF8(PRUint32 usv, PRUint16 offset, int bufLen,
>+                           int *utf8Len, char * *buf)
>+{
>+    /* XXX No error checking. Add it for debug build */
>+    char* p = *buf;
>+    /*if (!bufLen || !*buf) { */

Sorry for not noticing this before, but I think that we should clean
up this XXX comment, and remove the commented out code.


sr=darin
Attachment #212923 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 10

11 years ago
Thanks for sr.

(In reply to comment #9)
> (From update of attachment 212923 [details] [diff] [review] [edit])
> >Index: nsprpub/pr/src/linking/prlink.c
> 
> >+    if (!(flags & PR_LD_PATHW) && wname && wname != wname_stack)
> >+        PR_Free(wname);
> 
> I think that checking |flags| here is unnecessary.

if PR_LD_PATHW is set, wname points to |(LPWSTR) name| (a buffer passed to the function) which is different from |wname_stack| and not null, but we shouldn't free it. That's why we need to check |flags|. Actually, I forgot to check it at first and firefox crashed immediately :-)

 

> >+    /* XXX No error checking. Add it for debug build */
> >+    char* p = *buf;
> >+    /*if (!bufLen || !*buf) { */
> 
> Sorry for not noticing this before, but I think that we should clean
> up this XXX comment, and remove the commented out code.

I took care of it in my tree.

Comment 11

11 years ago
OK

Comment 12

11 years ago
Created attachment 213404 [details] [diff] [review]
patch v6

jshin, Darin,

I made the following changes to jshin's patch.

1. I renamed PR_LibSpec_UTF16_Pathname as PR_LibSpec_PathnameU
   and utf16_pathname as pathname_u.

I also plan to rename the PR_XXXUTF16 functions as PR_XXXU in
the future.

This renaming is to make the API more pleasing to the eyes and
closer to the Win32 naming convention (XXXW).

2. I edited the comments in prlink.h.  Please review, especially
"a pathname in the native charset".

3. I made NSPR's definition of the PRUnichar type as close to
the definition of PRUnichar in nscore.h as possible.

I can't use HAVE_CPP_2BYTE_WCHAR_T in NSPR, so I replaced it
with the equivalent test.

4. I moved some code from prlink.c to w95io.c to prepare for
the addition of more Unicode I/O functions to NSPR.

jshin, I'd like you to test this patch.  Because of change 1,
you'll need to update your Mozilla patch.

Darin, I'd like you to at least review the changes to the
public headers (prlink.h and prtypes.h).

Thanks.
Attachment #212923 - Attachment is obsolete: true
Attachment #213404 - Flags: superreview?(darin)
Attachment #213404 - Flags: review?(jshin1987)
Attachment #212923 - Flags: review?(wtchang)

Comment 13

11 years ago
Comment on attachment 213404 [details] [diff] [review]
patch v6

jshin, I forgot to mention that in NSPR, WIN32 = WINNT || WIN95.
So I replaced your use of
    defined(WIN32) && !defined(WINNT)
with
    defined(WIN95)

The Mozilla clients use the WIN95 code.  The WINNT code is
only used by some server products.

Comment 14

11 years ago
Comment on attachment 213404 [details] [diff] [review]
patch v6

>Index: nsprpub/pr/include/prlink.h

> #define PR_LD_LAZY   0x1  /* equivalent to RTLD_LAZY on Unix */
> #define PR_LD_NOW    0x2  /* equivalent to RTLD_NOW on Unix */
> #define PR_LD_GLOBAL 0x4  /* equivalent to RTLD_GLOBAL on Unix */
> #define PR_LD_LOCAL  0x8  /* equivalent to RTLD_LOCAL on Unix */
>+/*                 0x400     reserved for NSPR internal use */

Perhaps at this time, we should reserve a block of flags for use
within NSPR.  Maybe we could set the high bit or something.


>Index: nsprpub/pr/src/linking/prlink.c

>+pr_ConvertSingleCharToUTF8(PRUint32 usv, PRUint16 offset, int bufLen,
...
>+static int pr_ConvertUTF16toUTF8(LPCWSTR wname, LPSTR name, int len)

It seems like we'll want to move these out into a more central
location at some point too.
Attachment #213404 - Flags: superreview?(darin) → superreview+

Comment 15

11 years ago
> Perhaps at this time, we should reserve a block of flags for use
> within NSPR.  Maybe we could set the high bit or something.

Nevermind, I was thinking about something else.  I'm happy with the current solution.
(Assignee)

Comment 16

11 years ago
Comment on attachment 213404 [details] [diff] [review]
patch v6

r=jshin looks good. thank you for enhancing my patch. The comment in prlinks.h looks fine. Perhaps, 'native character encoding' might be better because 'character encoding' is  what we use in our UI (although this has little to do with the UI) and Unicode Technical committee members agreed to use.

I haven't tried it yet, though because I moved yesterday and had a little problem with an old extension cord which triggered the surge protector at my place apparently due to a 'leak'.   That's taken care of now and I will begin to build shortly with your patch.
Attachment #213404 - Flags: review?(jshin1987) → review+
(Assignee)

Comment 17

11 years ago
(In reply to comment #12)
> Created an attachment (id=213404) [edit]
> patch v6

> 1. I renamed PR_LibSpec_UTF16_Pathname as PR_LibSpec_PathnameU
>    and utf16_pathname as pathname_u.

It's not very important, but I feel a bit uncomfortable using '_u' and 'U' to denote 'UTF-16' because UTF-16 is only one of several ways to represent Unicode (UTF-8, UTF-16, UTF-32, etc). It's up to you, but I prefer a more verbose 'UTF16' to '_u'/U'. 
(Assignee)

Comment 18

11 years ago
(In reply to comment #16)

> I haven't tried it yet, .. I will begin to build shortly with your patch.

I've just tested a debug build on Windows 2k (with and without WINAPI_USE_ANSI defined) with the default codepage set to CP949(Korean) and it worked fine. Without WINAPI_USE_ANSI, importing an IE bookmark with Hindi title went well. I'll try it on WinME soon.

Comment 19

11 years ago
jshin, in comment 10 you said, regarding a XXX comment:
>
> I took care of it in my tree.

Could you post your change here?

Thank you for reviewing and testing my changes to your
patch.
(Assignee)

Comment 20

11 years ago
(In reply to comment #19)
> jshin, in comment 10 you said, regarding a XXX comment:
> >
> > I took care of it in my tree.
> 
> Could you post your change here?

Actually, I just got rid of the commented-out code, but perhaps to be safe, we may add the following while remvloing |XXX comment|.

#ifdef DEBUG
if (bufLen && !*buf)
  PR_ASSERT("Either bufLen should be 0 or *buf should  be non-null", __FILE__,
            __LINE__);
#endif 



 
> Thank you for reviewing and testing my changes to your
> patch.

I tested it on WinME only to realize that thebe is enabled on trunk  so that I can't test it on  Win 9x/ME unless I build a thebe-disabled build. However, it went far enough to ensure me that  the library loading works fine. Therefore, I guess it's safe to check in the patch with the above change if you wish to. 
(Assignee)

Comment 21

11 years ago
(In reply to comment #20)

> I tested it on WinME only to realize that thebe is enabled on trunk  so that I
> can't test it on  Win 9x/ME unless I build a thebe-disabled build. However, it
> went far enough to ensure me that  the library loading works fine. 

I built a new debug build with toolkit=windows (instead of cairo) and tested it on Windows ME. As expected, it worked fine.


(Assignee)

Comment 22

11 years ago
Wan-Teh, would you please land the latest patch (possibly with the code snippet in comment #20)? 

Comment 23

11 years ago
jshin: I plan to check in the patch on Friday or this
weekend.  Darin, feel free to check it in.

Comment 24

11 years ago
fixed on: NSPR trunk and NSPRPUB_PRE_4_2_CLIENT_BRANCH

I think we should land this on the MOZILLA_1_8_BRANCH as well, but the patch does not apply cleanly there.  The InitUnicodeSupport method has conflicts.  I suppose we can wait to land it on the 1.8 branch until we have jshin's other patch ready to land there.

Marking FIXED.  Nice work guys!
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Blocks: 330150

Comment 25

11 years ago
This patch created bug #330150

Comment 26

11 years ago
unable to build under windows with gcc due to this patch!

Comment 27

11 years ago
Created attachment 214868 [details] [diff] [review]
Back out the incorrect change to PRUnichar typedef

I forgot that the HAVE_CPP_2BYTE_WCHAR_T macro could also
be defined on the compiler command line by configure.in.
I believe any compiler we support on Windows has to match
Microsoft's definition of wchar_t, so this patch reverts
the PRUnichar typedef in prtypes.h to the original code.

I checked in this patch on the NSPR trunk and
NSPRPUB_PRE_4_2_CLIENT_BRANCH.

Comment 28

11 years ago
Created attachment 214951 [details] [diff] [review]
Minor tweaks

jshin: I was planning to use "U8" for UTF-8 if we add UTF-8
support, but it's fine to change "U" back to "UTF16".  I can
take care of this after you have landed your XPCOM patch.  I
don't want to cause you more work.

darin: I can port these patches back to the MOZILLA_1_8_BRANCH
without the patch for bug 327448.  Whatever you decide is fine.

This patch contains the changes you suggested in your code
reviews, as well as the changes I made after reviewing the code
again.  Three changes are worth mentioning.

1. I now reserve the most significant bit 0x8000 in "PRIntn flags"
for PR_LD_PATHW, assuming PRIntn is >= 16 bits.

2. In pr_LoadLibraryByPathname, 'result' needs to be initialized
to NULL, otherwise we may "goto unlock" with 'result' uninitialized.

3. In pr_LoadLibraryByPathname, I use a better approach to handle
the freeing of 'utf8name' and 'wname'.  This technique requires one
more variable, but the freeing code becomes simpler and more robust
to code changes.  We use this technique in NSS.
Attachment #214951 - Flags: superreview?(darin)
Attachment #214951 - Flags: review?(jshin1987)

Comment 29

11 years ago
jshin, Microsoft documentation on WideCharToMultiByte
(http://msdn.microsoft.com/library/default.asp?url=/library/en-us/intl/unicode_2bj9.asp 
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/intl/unicode_2bj9.asp)
recommends that we use the WC_NO_BEST_FIT_CHARS flag
with WideCharToMultiByte to convert file names.

Do you think we should do that?  Note that this flag
is not supported on Win95.

I'm also wondering if we should detect whether a default
character was used.  Earlier versions of your patch did
that by passing &usedDefault to WideCharToMultiByte.

Comment 30

11 years ago
The second (duplicate) URL in my previous comment is wrong.
It should be
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/intl/sec_intl.asp.
(Assignee)

Comment 31

11 years ago
Comment on attachment 214951 [details] [diff] [review]
Minor tweaks

Thanks for another enhancement.
I meant to use 0x4000 (the highest bit in unsigned short. 0x400 was sorta typo ;-)) because I thought 0x8000 would be problematic for unsigned short, but I was wrong.
Attachment #214951 - Flags: review?(jshin1987) → review+
(Assignee)

Comment 32

11 years ago
(In reply to comment #29)
> jshin, Microsoft documentation on WideCharToMultiByte
> (http://msdn.microsoft.com/library/default.asp?url=/library/en-us/intl/unicode_2bj9.asp 

> recommends that we use the WC_NO_BEST_FIT_CHARS flag
> with WideCharToMultiByte to convert file names.

> Do you think we should do that?  Note that this flag
> is not supported on Win95.

I considered using it but the lack of support on Win95 made me decide against it. Revisiting the issue, I think it's probably just ignored on Win95 so that it might be all right. Moreover, we deal with loading a library so that I guess it's best to avoid any security problem arising from 'file name mixup'. In xpcom/io, we use '_' for the default character, but we may just use '?' (the default default char.) and let |LoadLibraryA| fail given an invalid path containing '?'. 

One minor problem is that on Win95, the behavior would be different from on Win 9x/ME. 
 
> I'm also wondering if we should detect whether a default
> character was used.  Earlier versions of your patch did
> that by passing &usedDefault to WideCharToMultiByte.

IMHO, that's a good conservative thing to do, but I was worried about the performance. Just using the default default char '?' and letting LoadLibraryA would work, wouldn't it? 

Updated

11 years ago
Attachment #214951 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 33

11 years ago
Created attachment 215823 [details] [diff] [review]
patch for 1.8.x branch with wtc's minor tweaks

Minor tweaks (attachment 214951 [details] [diff] [review]) and the change in 'PRUnichar' definition (attachment 214868 [details] [diff] [review]) were combined with patch v6(attachment 213404 [details] [diff] [review]) and the difference in context between branch and trunk was taken care of. This was made to expedite fixing bug 162361 in branch.
(Assignee)

Comment 34

11 years ago
Comment on attachment 215823 [details] [diff] [review]
patch for 1.8.x branch with wtc's minor tweaks

wtc, I guess the patch has been baking long enough so that we can go ahead and check this in for 1.8 branch. This patch needs to land befor e landing my patch for bug 162361 on branch.
Attachment #215823 - Flags: approval-branch-1.8.1?(wtchang)

Comment 35

11 years ago
Created attachment 217340 [details] [diff] [review]
patch for 1.8.x branch with wtc's minor tweaks (as checked in)

jshin, I also included the patch for bug 327448 to make
it easier to port this patch back to the MOZILLA_1_8_BRANCH.
Attachment #215823 - Attachment is obsolete: true
Attachment #217340 - Flags: approval-branch-1.8.1+
Attachment #215823 - Flags: approval-branch-1.8.1?(wtchang)

Comment 36

11 years ago
I checked in the "patch for 1.8.x branch" on the MOZILLA_1_8_BRANCH
(Mozilla 1.8.1) and NSPR_4_6_BRANCH (NSPR 4.6.2).
Keywords: fixed1.8.1
Target Milestone: --- → 4.6.2

Comment 37

11 years ago
Created attachment 217551 [details] [diff] [review]
Two further fixes

Even if you don't want this patch on the trunk I'd appreciate it on the branch, as it broke my branch tinderbox.
The perf cost of calling MultiByteToWideChar on even older Windows versions is probably offset by the improvement on 98/Me.
I also discovered an allocation bug.
Attachment #217551 - Flags: superreview?(wtchang)
Attachment #217551 - Flags: review?(wtchang)
Attachment #217551 - Flags: approval-branch-1.8.1?(wtchang)

Comment 38

11 years ago
Neil, thanks for the patch.  I understand the first change in
the patch (to fix the allocation bug), but I don't understand why
the second change is necessary, and why our patch broke your
branch tinderbox.

The reason we don't call WideCharToMultiByte(CP_UTF8, ...)
on Windows 95/98/Me is that Windows 95 does not support
the CP_UTF8 code page. Since we consider Windows 95/98/Me
as one category, we don't call WideCharToMultiByte(CP_UTF8, ...)
on Windows 98/Me either.

I think your patch assumes that WideCharToMultiByte(CP_UTF8, ...)
fails (returns 0) with the ERROR_INVALID_PARAMETER error on
Windows 95, right?  I am fine with this change, but I don't know
what problem it fixes.

Comment 39

11 years ago
Created attachment 218113 [details] [diff] [review]
Neil's fix for the allocation size bug

This is the first change in Neil's patch "Two further fixes".
The only change I made is to use sizeof(PRUnichar) instead of
2.  The code path in question is rarely taken (when the DLL's
pathname is longer than MAX_PATH), which is why we didn't
discover this bug during testing.

I checked in this patch on the NSPR trunk (NSPR 4.7),
NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.9 alpha),
NSPR_4_6_BRANCH (NSPR 4.6.2), and MOZILLA_1_8_BRANCH (Mozilla
1.8.1).
Attachment #218113 - Flags: approval-branch-1.8.1+

Comment 40

11 years ago
Comment on attachment 217551 [details] [diff] [review]
Two further fixes

jshin, what do you think of this patch?

I've already checked in the first change in this patch
(after changing 2 to sizeof(PRUnichar)).

I like our ability to use the environment variable
WINAPI_USE_ANSI to set _pr_useUnicode to false and
exercise the Win 9x code paths on Windows 2000/XP.  The
second change in this patch will make us lose that ability
partially.  Does that bother you?  If not, the only
change I may make is to change

+    if (utf8Len)
+        return utf8Len;

to

+    if (utf8Len || GetLastError() != ERROR_INVALID_PARAMETER)
+        return utf8Len;

because WideCharToMultiByte(CP_UTF8, ...) fails (returns 0)
with the error code ERROR_INVALID_PARAMETER on Windows 95
and NT 3.51.  Another potential improvement is to store
in a global variable the fact that the CP_UTF8 code page
isn't supported, so that we don't try
WideCharToMultiByte(CP_UTF8, ...) in subsequent calls,
but it may not be worth our time to optimize for Windows 95
and NT 3.51.
Attachment #217551 - Flags: review?(wtchang) → review?(jshin1987)

Comment 41

11 years ago
Comment on attachment 217551 [details] [diff] [review]
Two further fixes

Thanks for considering this.

>+    utf8Len = WideCharToMultiByte(CP_UTF8, 0, wname, -1, name, len, 
>+                                  NULL, NULL);
>+    if (utf8Len || GetLastError() != ERROR_INVALID_PARAMETER)
>+        return utf8Len;
> 
>     if (!wname || len < 0 || (len > 0 && !name)) {
>         SetLastError(ERROR_INVALID_PARAMETER);
wtc's change as shown techincally makes SetLastError unnecessary here.

By the way I double-checked on Windows 95 and older and they do both return 0 with error 87 for CP_UTF8.
(Assignee)

Comment 42

11 years ago
Comment on attachment 217551 [details] [diff] [review]
Two further fixes

wtc, if you're fine with that, I'm fine, too although I also like to be able to test Win9x/ME codepath on Win 2k/XP. As you wrote, it's only a partial loss which doesn't matter much.
Attachment #217551 - Flags: review?(jshin1987) → review+

Updated

11 years ago
Attachment #217551 - Flags: approval-branch-1.8.1?(wtchang) → approval1.8.1?

Comment 43

11 years ago
So, what is the final verdict on the second part of attachment 217551 [details] [diff] [review]?  Do we want it?  Time is ticking to get this in for FF2 if it is needed.  Please help inform us as to whether or not we should get this change in for FF2 beta1.  Thanks!

Comment 44

11 years ago
Comment on attachment 217551 [details] [diff] [review]
Two further fixes

Darin, please check in the second change in this patch,
with the change I described in comment 40 and a comment
like this: WideCharToMultiByte(CP_UTF8, ...) fails (returns 0)
with the error code ERROR_INVALID_PARAMETER on Windows 95
and NT 3.51.

Please keep MOZILLA_1_8_BRANCH and NSPR_4_6_BRANCH in
sync.  I can take care of the checkins on the NSPR trunk and
NSPRPUB_PRE_4_2_CLIENT_BRANCH when I return from vacation
on July 10.  Thanks a lot.
Attachment #217551 - Flags: superreview?(wtchang) → superreview+

Comment 45

11 years ago
Darin, you should also consider the change Neil described
in comment 41.  It is a correct change but makes the code
brittle.  Perhaps we should replace the SetLastError call
with a comment that the error code is ERROR_INVALID_PARAMETER.
Comment on attachment 217551 [details] [diff] [review]
Two further fixes

(In reply to comment #45)
> Darin, you should also consider the change Neil described
> in comment 41.  It is a correct change but makes the code
> brittle.  Perhaps we should replace the SetLastError call
> with a comment that the error code is ERROR_INVALID_PARAMETER.

Could we get a new patch for this change worked up, reviewed and landed on trunk before renominating it for 1.8.1 approval, please?
Attachment #217551 - Flags: approval1.8.1?

Comment 47

11 years ago
Created attachment 227586 [details] [diff] [review]
Patch as per comment 40
Attachment #217551 - Attachment is obsolete: true
Attachment #227586 - Flags: review?(wtchang)

Comment 48

11 years ago
Created attachment 227589 [details] [diff] [review]
Patch as per comment 41
Attachment #227589 - Flags: review?(wtchang)

Comment 49

11 years ago
Created attachment 228760 [details] [diff] [review]
Patch as per comment 40 (with a comment)

This is Neil's "patch as per comment 40" except that
I added a comment.

I've checked this patch in on the NSPR trunk (NSPR 4.7)
and NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla trunk 1.9 alpha).

Checking in prlink.c;
/cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v  <--  prlink.c
new revision: 3.91; previous revision: 3.90
done

Checking in prlink.c;
/cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v  <--  prlink.c
new revision: 3.51.2.36; previous revision: 3.51.2.35
done
Attachment #227586 - Attachment is obsolete: true
Attachment #227589 - Attachment is obsolete: true
Attachment #228760 - Flags: review+
Attachment #228760 - Flags: approval1.8.1?
Attachment #227586 - Flags: review?(wtchang)
Attachment #227589 - Flags: review?(wtchang)

Updated

11 years ago
Attachment #228760 - Flags: superreview?(darin)

Updated

11 years ago
Attachment #228760 - Flags: superreview?(darin) → superreview+

Updated

11 years ago
Attachment #228760 - Flags: approval1.8.1? → approval1.8.1+

Comment 50

11 years ago
Comment on attachment 228760 [details] [diff] [review]
Patch as per comment 40 (with a comment)

I checked in this patch on the MOZILLA_1_8_BRANCH (Mozilla 1.8.1 Beta 2)
and NSPR_4_6_BRANCH (NSPR 4.6.3 Beta).
Comment on attachment 228760 [details] [diff] [review]
Patch as per comment 40 (with a comment)

NSS cannot build the trunk NSPR source code since this checkin.  
Attempting to build with
   OS_TARGET=WIN95
   USE_DEBUG_RTL=1
the compile step for pr/src/linking/prlink.c fails with this error:

.../prlink.c(264) : error C2065: '_pr_useUnicode' : undeclared identifier
Regarding comment 51,
Forcibly cleaning the build by removing all built/installed NSPR directories
fixed this build problem.  Perhaps a dependency issue.  Not major.
You need to log in before you can comment on or make changes to this bug.