Last Comment Bug 326168 - Add a UTF-16 API to load a library
: Add a UTF-16 API to load a library
Status: RESOLVED FIXED
: fixed1.8.1, intl
Product: NSPR
Classification: Components
Component: NSPR (show other bugs)
: other
: x86 Windows 2000
: -- normal (vote)
: 4.6.2
Assigned To: Jungshik Shin
: Wan-Teh Chang
Mentors:
Depends on:
Blocks: 162361 330150
  Show dependency treegraph
 
Reported: 2006-02-06 17:51 PST by Jungshik Shin
Modified: 2006-08-01 19:44 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (7.48 KB, patch)
2006-02-07 00:56 PST, Jungshik Shin
no flags Details | Diff | Review
patch update (7.95 KB, patch)
2006-02-12 04:34 PST, Jungshik Shin
no flags Details | Diff | Review
patch update (16.40 KB, patch)
2006-02-17 15:11 PST, Jungshik Shin
no flags Details | Diff | Review
patch (14.27 KB, patch)
2006-02-20 03:50 PST, Jungshik Shin
no flags Details | Diff | Review
patch addressing wtc's concerns (14.86 KB, patch)
2006-02-23 10:49 PST, Jungshik Shin
darin.moz: superreview+
Details | Diff | Review
patch v6 (19.40 KB, patch)
2006-02-27 19:11 PST, Wan-Teh Chang
jshin1987: review+
darin.moz: superreview+
Details | Diff | Review
Back out the incorrect change to PRUnichar typedef (613 bytes, patch)
2006-03-12 18:17 PST, Wan-Teh Chang
no flags Details | Diff | Review
Minor tweaks (7.24 KB, patch)
2006-03-13 16:13 PST, Wan-Teh Chang
jshin1987: review+
darin.moz: superreview+
Details | Diff | Review
patch for 1.8.x branch with wtc's minor tweaks (18.73 KB, patch)
2006-03-21 15:34 PST, Jungshik Shin
no flags Details | Diff | Review
patch for 1.8.x branch with wtc's minor tweaks (as checked in) (18.24 KB, patch)
2006-04-05 14:53 PDT, Wan-Teh Chang
wtc: approval‑branch‑1.8.1+
Details | Diff | Review
Two further fixes (1.38 KB, patch)
2006-04-07 05:51 PDT, neil@parkwaycc.co.uk
jshin1987: review+
wtc: superreview+
Details | Diff | Review
Neil's fix for the allocation size bug (827 bytes, patch)
2006-04-11 17:40 PDT, Wan-Teh Chang
wtc: approval‑branch‑1.8.1+
Details | Diff | Review
Patch as per comment 40 (1.01 KB, patch)
2006-06-29 12:59 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Review
Patch as per comment 41 (1.21 KB, patch)
2006-06-29 13:07 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Review
Patch as per comment 40 (with a comment) (1.13 KB, patch)
2006-07-10 18:01 PDT, Wan-Teh Chang
wtc: review+
darin.moz: superreview+
mtschrep: approval1.8.1+
Details | Diff | Review

Description Jungshik Shin 2006-02-06 17:51:19 PST
spun off from bug 162361.
Comment 1 Jungshik Shin 2006-02-07 00:56:26 PST
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?
Comment 2 Jungshik Shin 2006-02-12 04:34:48 PST
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).
Comment 3 Jungshik Shin 2006-02-12 07:47:13 PST
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);
Comment 4 Jungshik Shin 2006-02-17 15:11:09 PST
Created attachment 212262 [details] [diff] [review]
patch update

the previous patch has a couple of mistakes.
Comment 5 Jungshik Shin 2006-02-17 15:13:09 PST
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. 
Comment 6 Jungshik Shin 2006-02-20 03:50:17 PST
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.
Comment 7 Darin Fisher 2006-02-22 19:16:35 PST
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.
Comment 8 Jungshik Shin 2006-02-23 10:49:51 PST
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.
Comment 9 Darin Fisher 2006-02-23 11:21:11 PST
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
Comment 10 Jungshik Shin 2006-02-23 19:09:48 PST
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 Darin Fisher 2006-02-23 22:14:01 PST
OK
Comment 12 Wan-Teh Chang 2006-02-27 19:11:27 PST
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.
Comment 13 Wan-Teh Chang 2006-02-27 19:14:46 PST
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 Darin Fisher 2006-02-27 19:24:30 PST
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.
Comment 15 Darin Fisher 2006-02-27 19:31:22 PST
> 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.
Comment 16 Jungshik Shin 2006-03-02 08:20:29 PST
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.
Comment 17 Jungshik Shin 2006-03-02 08:51:35 PST
(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'. 
Comment 18 Jungshik Shin 2006-03-02 21:06:49 PST
(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 Wan-Teh Chang 2006-03-02 22:01:31 PST
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.
Comment 20 Jungshik Shin 2006-03-02 22:33:50 PST
(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. 
Comment 21 Jungshik Shin 2006-03-05 20:12:02 PST
(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.


Comment 22 Jungshik Shin 2006-03-07 05:47:00 PST
Wan-Teh, would you please land the latest patch (possibly with the code snippet in comment #20)? 
Comment 23 Wan-Teh Chang 2006-03-08 22:53:37 PST
jshin: I plan to check in the patch on Friday or this
weekend.  Darin, feel free to check it in.
Comment 24 Darin Fisher 2006-03-09 21:40:35 PST
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!
Comment 25 David G King 2006-03-11 21:53:35 PST
This patch created bug #330150
Comment 26 Henrik Gemal 2006-03-12 01:47:15 PST
unable to build under windows with gcc due to this patch!
Comment 27 Wan-Teh Chang 2006-03-12 18:17:02 PST
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 Wan-Teh Chang 2006-03-13 16:13:17 PST
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.
Comment 29 Wan-Teh Chang 2006-03-13 16:37:50 PST
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 Wan-Teh Chang 2006-03-13 16:46:06 PST
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.
Comment 31 Jungshik Shin 2006-03-14 20:35:26 PST
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.
Comment 32 Jungshik Shin 2006-03-14 20:45:10 PST
(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? 
Comment 33 Jungshik Shin 2006-03-21 15:34:07 PST
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.
Comment 34 Jungshik Shin 2006-04-04 18:50:03 PDT
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.
Comment 35 Wan-Teh Chang 2006-04-05 14:53:28 PDT
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.
Comment 36 Wan-Teh Chang 2006-04-05 14:54:58 PDT
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).
Comment 37 neil@parkwaycc.co.uk 2006-04-07 05:51:28 PDT
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.
Comment 38 Wan-Teh Chang 2006-04-07 14:59:05 PDT
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 Wan-Teh Chang 2006-04-11 17:40:19 PDT
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).
Comment 40 Wan-Teh Chang 2006-04-11 17:53:26 PDT
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.
Comment 41 neil@parkwaycc.co.uk 2006-04-12 07:45:13 PDT
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.
Comment 42 Jungshik Shin 2006-04-12 09:42:07 PDT
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.
Comment 43 Darin Fisher 2006-06-23 11:21:43 PDT
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 Wan-Teh Chang 2006-06-29 07:00:24 PDT
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.
Comment 45 Wan-Teh Chang 2006-06-29 07:12:57 PDT
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 46 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-29 11:19:31 PDT
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?
Comment 47 neil@parkwaycc.co.uk 2006-06-29 12:59:42 PDT
Created attachment 227586 [details] [diff] [review]
Patch as per comment 40
Comment 48 neil@parkwaycc.co.uk 2006-06-29 13:07:36 PDT
Created attachment 227589 [details] [diff] [review]
Patch as per comment 41
Comment 49 Wan-Teh Chang 2006-07-10 18:01:37 PDT
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
Comment 50 Wan-Teh Chang 2006-07-14 15:19:05 PDT
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 51 Nelson Bolyard (seldom reads bugmail) 2006-08-01 19:34:45 PDT
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
Comment 52 Nelson Bolyard (seldom reads bugmail) 2006-08-01 19:44:59 PDT
Regarding comment 51,
Forcibly cleaning the build by removing all built/installed NSPR directories
fixed this build problem.  Perhaps a dependency issue.  Not major.

Note You need to log in before you can comment on or make changes to this bug.