124160 - warning Converting NULL to non-pointer type |jvalue outValue = { NULL };|

Status: RESOLVED FIXED

(Core Graveyard :: Java: OJI, defect)

Description

[timeless]

jvalue is a union. http://lxr.mozilla.org/seamonkey/source/embedding/browser/activex/src/pluginhostctrl/pluginsdk_include/jni.h#118

it appears you're trying to clobber the jobject l (pointer to _jobject). Is there a reason not to do jvalue outValue={0}?

4.
modules/oji/src/ProxyJNI.cpp:529 (See build log excerpt)Converting NULL 
to non-pointer type
527 static jvalue InvokeMethod(JNIEnv *env, jobject obj, JNIMethod* 
method, jvalue* args) 528 { 529 jvalue outValue = { NULL }; 530 
ProxyJNIEnv& proxyEnv = GetProxyEnv(env); 531 nsISecureEnv* secureEnv = 
GetSecureEnv(env);5.
modules/oji/src/ProxyJNI.cpp:637 (See build log excerpt)Converting NULL 
to non-pointer type
635 static jvalue InvokeNonVirtualMethod(JNIEnv *env, jobject obj, 
jclass clazz, JNIMethod* method, jvalue* args) 636 { 637 jvalue outValue 
= { NULL }; 638 ProxyJNIEnv& proxyEnv = GetProxyEnv(env); 639 
nsISecureEnv* secureEnv = GetSecureEnv(env);6.
modules/oji/src/ProxyJNI.cpp:741 (See build log excerpt)Converting NULL 
to non-pointer type
739 static jvalue GetField(JNIEnv* env, jobject obj, JNIField* field) 
740 { 741 jvalue outValue = { NULL }; 742 ProxyJNIEnv& proxyEnv = 
GetProxyEnv(env); 743 nsISecureEnv* secureEnv = GetSecureEnv(env);7.
modules/oji/src/ProxyJNI.cpp:824 (See build log excerpt)Converting NULL 
to non-pointer type
822 static jvalue InvokeStaticMethod(JNIEnv *env, jclass clazz, 
JNIMethod* method, jvalue* args) 823 { 824 jvalue outValue = { NULL };
 825 ProxyJNIEnv& proxyEnv = GetProxyEnv(env); 826 nsISecureEnv* 
secureEnv = GetSecureEnv(env);8.
modules/oji/src/ProxyJNI.cpp:928 (See build log excerpt)Converting NULL 
to non-pointer type
926 static jvalue GetStaticField(JNIEnv* env, jclass clazz, JNIField* 
field) 927 { 928 jvalue outValue = { NULL }; 929 ProxyJNIEnv& proxyEnv = 
GetProxyEnv(env); 930 nsISecureEnv* secureEnv = GetSecureEnv(env);

Comment 1

[Igor Nekrestyanov]

-> me

Comment 2

[Igor Nekrestyanov]

Attachment: Proposed patch

We can not just use 
  outValue = {0}
because it likely to be the same as 
  outValue.z = 0;
z is the first field of the union and it is the jboolean!
As a result part of the union will be uninitialized.

I proposed to clear double field (because it is longest one).

Please, review.

Comment 3

[Igor Nekrestyanov]

joe: could you please review proposed change?

Comment 4

[timeless]

I can't remember if OJI uses sr's...

Comment 5

[Igor Nekrestyanov]

Back to Joe.
Patch exists but needs to be reviewed.

Comment 6

[Joshua Xia]

reassign to me

Comment 7

[Pete Zha]

Comment on attachment 68533 [details] [diff] [review]
Proposed patch

Joshua,
Can you review this patch?

Comment 8

[Pete Zha]

Comment on attachment 68533 [details] [diff] [review]
Proposed patch

[e-mail], [e-mail]
Can you review this patch?

Comment 9

[Patrick C. Beard]

Comment on attachment 68533 [details] [diff] [review]
Proposed patch

I think the best thing to do would be to define a static const global,

static const jvalue kErrorValue;

and in each method that has a jvalue return type, if the call to the underlying
nsISecureEnv fails, then return that value. Patch coming up.

Comment 10

[Patrick C. Beard]

Attachment: Alternative Patch

This uses the return value of the call to the nsISecureEnv to decide whether to
return the local jvalue, or a const error (bottom) value. This removes the need
for initializing the local jvalue altogether.

Comment 11

[Patrick C. Beard]

The original rational for initializing the jvalue to NULL was to ensure that any
jobject (pointer) fields of the jvalue are 0. I don't recall if the bit pattern
of a jdouble (IEEE 64-bit floating point) is all zeroes. If it is, then my
alternative patch should be safe.

Comment 12

[Patrick C. Beard]

Just defining kErrorValue as:

static jvalue kErrorValue;

will result in it being initialized to all zeroes.

Comment 13

[Brendan Eich [:brendan]]

IEEE positive zero is all-zeros in 64 bits.

Beard, you wanna use the patch mgr to request r and sr?

/be

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 109135 [details] [diff] [review]
Alternative Patch

Seems ok to me, but is this bug well-assigned still?  Timeless, can you find
out?

/be

Comment 15

[timeless]

checked in per beard