840209 - txXPCOMExtensionFunction.cpp(190) : warning C4244: 'argument' : conversion from 'PRUnichar' to 'char', possible loss of data

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Daniel Holbert [:dholbert]]

We hit this build warning on Windows, when building txXPCOMExtensionFunction.cpp:
{
e:/builds/moz2_slave/m-cen-w32-dbg/build/content/xslt/src/xpath/txXPCOMExtensionFunction.cpp(190) : warning C4244: 'argument' : conversion from 'PRUnichar' to 'char', possible loss of data
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=19634130&tree=Firefox

The code:
> 183     PRUnichar letter;
[...]
> 190             methodName.Append(upperNext ? nsCRT::ToUpper(letter) : letter);

https://mxr.mozilla.org/mozilla-central/source/content/xslt/src/xpath/txXPCOMExtensionFunction.cpp?mark=183-183,190-190#181

As you can see there, |letter| is of type PRUnichar, whereas ToUpper() expects a char:

> 166   static char ToUpper(char aChar) { return NS_ToUpper(aChar); }
> 167   static char ToLower(char aChar) { return NS_ToLower(aChar); }
https://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsCRT.h?mark=166-167#166

Comment 1

[Daniel Holbert [:dholbert]]

This code was added in bug 278981.

sicking actually called out this chunk as a possible-issue in bug 278981 comment 32:
> >+    while ((letter = *name)) {
> >+        if (letter == '-') {
> >+            upperNext = PR_TRUE;
> >+        }
> >+        else {
> >+            methodName.Append(upperNext ? nsCRT::ToUpper(letter) : letter);
> >+            upperNext = PR_FALSE;
> >+        }
> >+        ++name;
> >+    }
> 
> You should probably make sure that all characters in the name are ascii
> characters. If there is an UTF8 char in there you could end up calling
> ToUpper on the first byte in a multibyte character.

...but peterv says 2 comments later that  'letter' is going to be ASCII and hence in-bounds for a character, so it shouldn't be a problem:
> AFAIK (XP)IDL only allows ASCII for the method name.


So, if peterv is right on that, this bug probably just needs a bounds-checking assertion and a static-cast to char.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 712605 [details] [diff] [review]
fix

Review of attachment 712605 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/xslt/src/xpath/txXPCOMExtensionFunction.cpp
@@ +186,5 @@
>          if (letter == '-') {
>              upperNext = true;
>          }
>          else {
> +            NS_ASSERTION(nsCRT::IsAscii(letter),

Make it a MOZ_ASSERT.

Comment 4

[Daniel Holbert [:dholbert]]

Switched to MOZ_ASSERT and pushed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/ae60b8d343e5

Comment 5

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/ae60b8d343e5