116243 - Incorrect type conversion of Java Boolean to Javascript String

Status: RESOLVED FIXED

(Core Graveyard :: Java: Live Connect, defect)

Description

[Scott Anderson]

According to the LiveConnect documentation, a Java Boolean object should be
converted to a Javascript boolean.

We're calling a Javascript function with two parameters:

Java:
Object[] args = new Object[2];
args[0]
	  = "a string";
args[1]
	  = new Boolean( false );
String s = (String)window.call( "foo", args );

Javascript:
function foo( arg1, arg2 )
{
  /* Do some stuff with the arg1 string */

  if( !arg2 )
    alert( 'arg2 was false' );
  else
    alert( 'arg2 was true' );
}

The above works fine in Internet Explorer. However, in Mozilla arg2 is being
sent as the string "false" instead of the boolean false. As a result, the wrong
part of the if statement is being exercised.

If we change the test to:

if (!arg2 || arg2 == "false")

Then the operation works correctly in both Mozilla and Internet Explorer.

This is Mozilla build 2001112012 on SuSE Linux.

Comment 1

[Phil Schwartau]

Reassigning to Patrick - 

Comment 2

[Patrick C. Beard]

I didn't realize we would attempt to convert java.lang.Boolean objects to Java
script boolean values.

Comment 3

[Scott Anderson]

From http://home.netscape.com/eng/mozilla/3.0/handbook/javascript/livecon.htm at
the very bottom:

Data type conversion
Values passed from Java to JavaScript are converted as follows:

    * Java byte, char, short, int, long, float, and double are converted to
JavaScript numbers.
    * A Java boolean is converted to a JavaScript boolean.
    * An object of class JSObject is converted to the original JavaScript
object.
    * An object of any other class is converted to a JavaScript wrapper, which
can be used to access methods and fields of the Java object. Converting this
wrapper to a string will call the toString method on the original object;
converting to a number will call the floatValue method if possible and fail
otherwise. Converting to a boolean will attempt to call the booleanValue method
in the same way.
    * Java arrays are converted to a JavaScript pseudo-Array object; this object
behaves just like a JavaScript Array object: you can access it with the syntax
arrayName[index] (where index is an integer), and determine its length with
arrayName.length.

Comment 4

[Bob Clary [:bc] (inactive)]

-> default assignee for old netscape assigned bugs.

Comment 5

[Alfred Peng]

Attachment: Patch v1

Add the type conversion from Object to Boolean in jsbool.c.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 254306 [details] [diff] [review]
Patch v1

ECMA-262 Editions 1-3 all say that ToBoolean on an object results in true, no matter what. This change is in the wrong layer, if LiveConnect needs something like its effect, the patch should be to js/src/liveconnect/* code.

/be

Comment 7

[Alfred Peng]

Attachment: Patch v2

Add the type conversion in jsj_convert.c.

The added conversions include:
Java Boolean->boolean
Java Double->number
Java String->jsstring

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 255118 [details] [diff] [review]
Patch v2

Don't you want to avoid creating js_obj in all but the default: case of the switch?

/be

Comment 9

[Alfred Peng]

Attachment: Patch v3

Define a separated function for JavaObject=>JSObject.

Some issues not so sure about:
1. Is it necessary to call DeleteLocalRef manually? It seems that the reference will be released at the end of the function scope.
2. jsj_ReleaseJavaClassDescriptor doesn't work as expected: http://lxr.mozilla.org/seamonkey/source/js/src/liveconnect/jsj_class.c#513. The comment shows that the reference-count doesn't work well and it's disabled.
3. Remove the commented part for "NULL" value of "js_obj".

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 255182 [details] [diff] [review]
Patch v3

>+static JSBool
>+jsj_ConvertJavaObjectToJSObject(JSContext *cx, JNIEnv *jEnv,
>+                                JavaClassDescriptor *class_descriptor,
>+                                jobject java_obj, jsval *vp)

Just wondering whether the jsj_ prefix is needed given the static. On the other hand, it makes the name and function future-proofed against becoming non-static (library-private).

>+    jclass java_class = class_descriptor->java_class;

This variable is single-use and used only in the else below. It's not necessary, or at least it should not be initialized early as above.

> 
>     /*
>      * If it's an instance of netscape.javascript.JSObject, i.e. a wrapper
>      * around a JS object that has been passed into the Java world, unwrap
>      * it to obtain the original JS object.
>      */
>-     if (njJSObject && (*jEnv)->IsInstanceOf(jEnv, java_obj, njJSObject)) {
>+    if (njJSObject && (*jEnv)->IsInstanceOf(jEnv, java_obj, njJSObject)) {
> #ifdef PRESERVE_JSOBJECT_IDENTITY
> #if JS_BYTES_PER_LONG == 8
>         js_obj = (JSObject *)((*jEnv)->GetLongField(jEnv, java_obj, njJSObject_long_internal));
> #else
>         js_obj = (JSObject *)((*jEnv)->GetIntField(jEnv, java_obj, njJSObject_internal));
> #endif
> #else
>         js_obj = jsj_UnwrapJSObjectWrapper(jEnv, java_obj);
> #endif
>-		/* NULL is actually a valid value. It means 'null'.
>-
>-        JS_ASSERT(js_obj);
>+    } else {
>+        /* otherwise, wrap it inside a JavaObject */
>+        js_obj = jsj_WrapJavaObject(cx, jEnv, java_obj, java_class);

Only java_class use in new static helper function is here.

/be

Comment 11

[Alfred Peng]

Attachment: Patch v4

> This variable is single-use and used only in the else below. It's not
> necessary, or at least it should not be initialized early as above.

My mistake. The DeleteLocalRefs were deleted and there is only one place to use this variable.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 255187 [details] [diff] [review]
Patch v4

> /*
>- * Reflect a Java object into a JS value.  The source object, java_obj, must
>- * be of type java.lang.Object or a subclass and may, therefore, be an array.
>+ * Convert a Java object to a JSObject.
>  */
>-JSBool
>-jsj_ConvertJavaObjectToJSValue(JSContext *cx, JNIEnv *jEnv,
>-                               jobject java_obj, jsval *vp)
>+static JSBool
>+convertJavaObjectToJSObject(JSContext *cx, JNIEnv *jEnv,
>+                            JavaClassDescriptor *class_descriptor,
>+                            jobject java_obj, jsval *vp)

I reminded myself of liveconnect "prevailing code style", which was due to fur (Scott Furman). He used all_lowercase_underscore_separated names for static functions. That's been upheld, modulo static function names that seem ready for jsj_StudlyCaps library-private style, such as jsj_ConnectToJavaVM. So I say use all_lowercase_underscore_separated or else stick with your jsj_Convert... style.

Looks great otherwise, r=me with the style change that seems better to you.

/be

Comment 13

[Alfred Peng]

Attachment: Patch v5

I'm for the former one, to make them all lowercase underscore separated. Thanks for the review and Happy Chinese new year :-)

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 255318 [details] [diff] [review]
Patch v5

r+sr=jst

Comment 15

[Alfred Peng]

Checking in js/src/liveconnect/jsj_convert.c;
/cvsroot/mozilla/js/src/liveconnect/jsj_convert.c,v  <--  jsj_convert.c
new revision: 1.19; previous revision: 1.18
done

=>FIXED

Comment 16

[Alfred Peng]

Some testcases in http://www.mozilla.org/quality/browser/front-end/testcases/oji/javatojstests.html are broken due to this patch and need to be updated.