268266 - profile and appstartup code should use NS_CopyNativeToUnicode

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Jungshik Shin]

While working on bug 267980, I found this problem in the profile code.

nsProfile.cpp has the following pattern used at a few places:

if (is_ascii(profilename))
   inflate to UTF-16
else
   get platform charset using a helper function defined in nsProfileAccess.cpp
   call a helper function ConvertStringToUnicode (with platform charset) 
   defined in nsProfileAccess.cpp

In addition, there are a couple of misuse of '*WithConversion' for non-ASCII
strings. 

Patch coming up.

Comment 1

[Jungshik Shin]

Attachment: patch 0.7 (not yet tested well)

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

thank you!

do you think it's time to remove the "only use CopyNativeToUnicode for
filenames" comment?

Comment 3

[Jungshik Shin]

I think it's to be noted that those two functions can (should) be used for
filenames, reigstries (on Windows), command line arguments (on all platforms)
and environment variables. For environment variables, we already have
nsIEnvironment. For registries on Windows, we can just use Windows APIs.
However, command line arguments have to be handled with these two functions
unless we change nsICmdLineService to return 'wstring/AString/AUTF8String'.  

I think it's also a good idea to add this to the string guide. As I noted
elsewhere (where you seconded), there are many places where "*WithConversion" or
friends are misused.

Btw, the error-handling in the iconv-based implementation has room for improvement. 

Comment 4

[Jungshik Shin]

Attachment: patch 0.9

nsAppStartup.cpp has the same problem.

Comment 5

[Jungshik Shin]

Attachment: patch (tested on Linux)

I fixed a mistake in XP_WIN code path (NS4 profile migration).

Comment 6

[Jungshik Shin]

Comment on attachment 165811 [details] [diff] [review]
patch (tested on Linux)

asking for r/sr

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 165811 [details] [diff] [review]
patch (tested on Linux)

profile/src/nsProfile.cpp
+	 nsCAutoString uiLocale, contentLocale;
+	 LossyCopyUTF16toASCII(aUILocale, uiLocale);
+	 LossyCopyUTF16toASCII(aContentLocale, contentLocale);

could make this:
NS_LossyConvertUTF16toASCII uiLocale(aUILocale);
and similar for contentLocale

-	 if ((!aUILocale || !aUILocale[0]) && packageRegistry) {

hmm... this line scares me a bit... AssignWithConversion is null-safe; is
LossyCopy... too?

profile/src/nsProfileAccess.cpp
-	 nsAutoString convertedProfLoc;
...
	 CFStringRef pathStrRef = ::CFStringCreateWithCharacters(NULL,
				       convertedProfLoc.get(),
convertedProfLoc.Length())

won't this break the build?

xpfe/components/startup/src/nsAppStartup.cpp
+      NS_ASSERTION(0, "Failed to convert commandline url to unicode");

can you make this NS_ERROR while you are here?


Thanks for doing this!

Comment 8

[Jungshik Shin]

Attachment: patch (now include toolkit)

I changed |NS_ASSERTION(0,...)| to |NS_ERROR(....)|. 

As for the Mac compilation problem,  |convertedProfLoc| was kept on Mac OS X
while it's removed on other platforms. So, there was no problem (there was not
enough context for you to see that) :-). BTW, I combined two
#ifdef-#else-#endif blocks to one.

|NS_LossyConvertUTF16toASCII uiLocale(aUILocale)| doesn't work (I had already
tried that before uploading attachment 165811 [details] [diff] [review] :-)) because |uiLocale =
currentUILocaleName| doesn't get compiled. (I expected it to work, but it
didn't)
Finally, I confirmed that |LossyCopyUTF16toASCII| does 'null check'

Comment 9

[Jungshik Shin]

Comment on attachment 165883 [details] [diff] [review]
patch (now include toolkit)

thanks for r and carrying over cbie's review flag here.

toolkit/components/startup patch is virtually identical to
xpfe/components/startup patch

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

you probably need approval for the toolkit change from a toolkit peer, cc'ing
bsmedberg

Comment 11

[Benjamin Smedberg]

a=me, pending superreview

Comment 12

[Darin Fisher]

Comment on attachment 165883 [details] [diff] [review]
patch (now include toolkit)

>Index: profile/src/nsProfile.cpp

>+            rv = NS_CopyNativeToUnicode(
>+                 nsDependentCString(strtok(cmdResult.BeginWriting(), " ")), currProfileName);
>+            NS_ASSERTION(NS_SUCCEEDED(rv), "failed to convert ProfileName to unicode");
>+
>+            char *currProfileDirString = strtok(NULL, " "); 

Perhaps it would be better to use the threadsafe nsCRT::strtok instead (though
I guess it's not likely for there to be multiple threads at this point).

Also, strtok may return NULL, and that could cause nsDependentCString to crash.


Otherwise, nice patch!	sr=darin

Comment 13

[Simon Montagu :smontagu]

Comment on attachment 165883 [details] [diff] [review]
patch (now include toolkit)

>+        NS_ConvertUnicodeToNative(profile, temp);

This should be NS_CopyUnicodeToNative, right? The patch as is caused tinderbox
bustage.

Comment 14

[Jungshik Shin]

yeah, sorry for bustage. bz already fixed it. thanks.

Comment 15

[Jungshik Shin]

thanks for r/sr.

I replaced strtok with nsCRT::strtok. As for nsDependentCString, we don't have
to worry because cmdResult is already checked for null (by
nsICmdLineService::get..Value) so that strtok will not return null. 

Comment 16

[Boris Zbarsky [:bzbarsky]]

Note that this patch didn't compile on Windows...

Comment 17

[Darin Fisher]

> ... As for nsDependentCString, we don't have
> to worry because cmdResult is already checked for null (by
> nsICmdLineService::get..Value) so that strtok will not return null. 

hmm?  what if the token is not found?  strtok returns null if there are no more
tokens.  (e.g., what if the given value is the empty string?)