64bit safe code for liveconnect (WinXP AMD64)

RESOLVED FIXED

Status

enhancement
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: m_kato, Assigned: yuanyi21)

Tracking

({64bit})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040219
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040219

According to
http://java.sun.com/j2se/1.4.2/docs/guide/jni/spec/types.html#wp428, jint is 32
bit and jlong is 64 bit.  But current LiveConnect code defined jobject as jint.
 So this may cause bad casting issue for 64bit architicture.

Reproducible: Always
Steps to Reproduce:
N/A.  This is porting issue.
Actual Results:  
N/A.  This is porting issue.

Expected Results:  
N/A.  This is porting issue.
Summary: 64bit safe code for liveconnect (WinXP AMD64) part 2 → 64bit safe code for liveconnect (WinXP AMD64)

Updated

15 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: 64bit
(Assignee)

Comment 3

15 years ago
Comment on attachment 145401 [details] [diff] [review]
a patch for mozilla/js/src/liveconnect

1. Please don't use PRWord here. see the comment in prtypes.h about PRWord.
2. Whenever you want to change the data type of jsobject, please also change
its name. see bug 227170 comment 4.
3. In bug 227170, we all intend to use a pointer type for jsobject, rather than
just define it as an integer type on 32/64 bit platform.
4. Please make all patches together next time. That would be much easier for
reviewing.
Attachment #145401 - Flags: review-
OK.  I will post new patch next time.

And, about discussion for pointer type for WinXP AMD64, please see bug 226094.

Comment 5

15 years ago
*** Bug 248209 has been marked as a duplicate of this bug. ***

Comment 6

15 years ago
Add myself in the list.

Comment 7

15 years ago
This patch is only to make Windows & Linux platform 64 bit liveconnect ready. I
don't know the story about MRJ. 

The patch has been tested on Windows 32bit, Solaris 32bit & Linux AMD 64 bit
platforms. Java Plug-in hasn't been able to be built on Windows AMD64 bit
platform, but the change should make the current liveconnect & oji module to be
ready for 64 bit platforms.

Updated

15 years ago
Attachment #154366 - Flags: superreview?(jst)
Attachment #154366 - Flags: review?(kyle.yuan)
(Assignee)

Comment 8

15 years ago
Comment on attachment 154366 [details] [diff] [review]
The fix for porting Liveconnect to 64 bit platforms (Windows & Linux).

Some thoughts about the patch:

1) bug 227170 comment 4 suggests that you should avoid using jsobject in
liveconnect code, because it belongs to js engine namespace;

2) since you already defined a new type |jsobject|, why don't just use it as
argument/return type, rather than use #if PR_BYTES_PER_LONG == 8 everywhere?
for example, use

+    jsobject obj = 0;

instead of

+#if JS_BYTES_PER_LONG == 8
+    jlong obj = 0;
+#else
+	jint  obj = 0;
+#endif

3) please use spaces instead of tabs for indention.

4) Could you point a place where JS_BYTES_PER_LONG is defined as 8 in 64-bit
mode? I think it should be, but I just can't find the place.

Comment 9

15 years ago
JS_BYTES_PER_LONG defined in:
http://lxr.mozilla.org/seamonkey/source/js/src/jscpucfg.h

I didn't notice that this bug is a dup of the other bug. I would suggest Kyle to
repatch my fix and post it into the other bug.

Updated

15 years ago
Attachment #154366 - Flags: superreview?(jst)
Attachment #154366 - Flags: review?(kyle.yuan)
(Assignee)

Comment 10

15 years ago
(In reply to comment #9)
> JS_BYTES_PER_LONG defined in:
> http://lxr.mozilla.org/seamonkey/source/js/src/jscpucfg.h

My question is where JS_BYTES_PER_LONG is defined to 8? In jscpucfg.h,
JS_BYTES_PER_LONG is always 4.

> I didn't notice that this bug is a dup of the other bug. I would suggest Kyle to
> repatch my fix and post it into the other bug.

I already have a patch in bug 227170 for months and it needs your review :)

My basic idea is defining jsobject as a pointer type instead of int/long. That
could handle both 32-bit & 64-bit correctly. I also did some code clean work in
my patch which makes it much longer than necessary.

Comment 11

15 years ago
JS_BYTES_PER_LONG is defined in jsautocfg.h which is automatically generated
during compile time (look at js/src/Makefile.in to see how that works).

I don't understand why using jsobject is not a good idea. Anyway, the current
liveconnect module is made like a extension of js module. 

I am choosing to keep maxim compatibilty just to add what 64-bit porting needs.
I believe many people outisde rely on nsILiveconnect.h and I don't think it is a
good idea to change it even we are sure the source/binary compatibility is
there. Also define lcobject as a pointer to JSObjectHandle removes the
flexibility of the interface (1. Why do the outside people using the interface
care about JSObjectHandle which is a implementation detail ? 2. How about if
later on, we want to point to something else other than JSObjectHandle...).

Last but not least, it is always good idea to do defensive programming. So I
don't think we should remove jEnv == NULL check in the patch of bug 227170. We
could define a MACRO to avoid repeatedly duplicate the same code. But leave that
there. You never know what happens if something is wrong :-).
(Assignee)

Comment 12

15 years ago
(In reply to comment #11)
> JS_BYTES_PER_LONG is defined in jsautocfg.h which is automatically generated
> during compile time (look at js/src/Makefile.in to see how that works).

Ok, bow i see how it works, thanks.

> I don't understand why using jsobject is not a good idea. Anyway, the current
> liveconnect module is made like a extension of js module. 

Brendan should be the right person to answer this question.
 
> I am choosing to keep maxim compatibilty just to add what 64-bit porting needs.
> I believe many people outisde rely on nsILiveconnect.h and I don't think it is a
> good idea to change it even we are sure the source/binary compatibility is
> there. Also define lcobject as a pointer to JSObjectHandle removes the
> flexibility of the interface (1. Why do the outside people using the interface
> care about JSObjectHandle which is a implementation detail ? 2. How about if
> later on, we want to point to something else other than JSObjectHandle...).

Ok, if you don't want to define it as a pointer, what is wrong with always
defining it as jlong?
 
> Last but not least, it is always good idea to do defensive programming. So I
> don't think we should remove jEnv == NULL check in the patch of bug 227170. We
> could define a MACRO to avoid repeatedly duplicate the same code. But leave that
> there. You never know what happens if something is wrong :-).

I'm not removing the NULL check, I just removed the brace to match the coding style.

Comment 13

15 years ago
>>Ok, if you don't want to define it as a pointer, what is wrong with always
>> defining it as jlong?

Defining it as jlong is bad for 2 reasons:
1. Not compatible with the current jint
2. Not necessary for 32bit platforms since jlong is always 8 bits.
(Assignee)

Comment 14

15 years ago
> Defining it as jlong is bad for 2 reasons:
> 1. Not compatible with the current jint
> 2. Not necessary for 32bit platforms since jlong is always 8 bits.

Oh, sorry, I was confused by jlong and JS_BYTES_PER_LONG. But it still looks
weird to me that use a js engine macro (JS_BYTES_PER_LONG) to define a java data
type. It should be done in the java side, i.e. java header should define jint to
32/64-bit accroding to the platform. I bet you will face this problem while
porting the plugin code.

Comment 15

15 years ago
In my patch, I used PR_BYTES_PER_LONG (a NSPR macro, not JS_BYTES_PER_LONG) to
define jsobject in "nsILiveconnect.h". We are not defining a java type, but
"jsobject". I agree with you it is a bad design choice to use jint to define
jsobject. But to keep backward compatibility ( interface are not supposed to
change if you agree with me), I think this is the best thing I can do.
(Assignee)

Comment 16

15 years ago
Posted patch new patch (obsolete) — Splinter Review
I renamed jsobject to lcjsobject and move its definition from nsILiveconnect.h
to jsjava.h so that it can be used in c file. Other changes are based on
Xiaobin's patch.

One thing I'm not sure is the following 2 lines:
jfieldID fid = (*jEnv)->GetFieldID(jEnv, cid, "nativeJSObject", "J");
handle = (JSObjectHandle*)((*jEnv)->GetLongField(jEnv, java_wrapper_obj, fid));


Is Java going to always use long for nativeJSObject no matter what the platform
is? Xiaobin, can you confirm that?
(Assignee)

Updated

15 years ago
Assignee: live-connect → kyle.yuan
Status: NEW → ASSIGNED
(Assignee)

Updated

15 years ago
Attachment #155053 - Flags: review?(Xiaobin.Lu)
(Assignee)

Comment 17

15 years ago
*** Bug 227170 has been marked as a duplicate of this bug. ***

Comment 18

15 years ago
It is my bad. We should probably treat that segment of code similar as before.
Just add 
#if JS_BYTE_PER_LONG == 8 
jfieldID fid = (*jEnv)->GetFieldID(jEnv, cid, "nativeJSObject", "J");
handle = (JSObjectHandle*)((*jEnv)->GetLongField(jEnv, java_wrapper_obj, fid));
#else
jfieldID fid = (*jEnv)->GetFieldID(jEnv, cid, "nativeJSObject", "I");
handle = (JSObjectHandle*)((*jEnv)->GetIntField(jEnv, java_wrapper_obj, fid));
#endif

This requires the other implementation of Java Plugin(e.g MRJ) to do similar
change in JSObject.java if they want to port to 64 bit platform.
 
Thanks for pointing that out. With that change, I will give a "go" to that patch.
(Assignee)

Comment 19

15 years ago
Attachment #145401 - Attachment is obsolete: true
Attachment #145402 - Attachment is obsolete: true
Attachment #154366 - Attachment is obsolete: true
Attachment #155053 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #155053 - Flags: review?(Xiaobin.Lu)
(Assignee)

Updated

15 years ago
Attachment #155056 - Flags: review?(Xiaobin.Lu)

Updated

15 years ago
Attachment #155056 - Flags: review?(Xiaobin.Lu) → review+
(Assignee)

Updated

15 years ago
Attachment #155056 - Flags: superreview?(brendan)

Comment 20

15 years ago
Could we get any attention on this bug for superreview? Thanks.
Comment on attachment 155056 [details] [diff] [review]
revised per Xiaobin's comment

>@@ -317,16 +311,21 @@ jsj_UnwrapJSObjectWrapper(JNIEnv *jEnv, 
> 	   case should be removed as soon as the unwrap function is supported by the Sun JPI. */
> 
>     if (JSJ_callbacks && JSJ_callbacks->unwrap_java_wrapper != NULL) {
>         handle = (JSObjectHandle*)JSJ_callbacks->unwrap_java_wrapper(jEnv, java_wrapper_obj);
>     }
>     else {
>         jclass   cid = (*jEnv)->GetObjectClass(jEnv, java_wrapper_obj);
>+#if JS_BYTES_PER_LONG == 8

Compare with:

>+++ js/src/liveconnect/jsjava.h	3 Aug 2004 02:19:43 -0000
>@@ -52,14 +52,20 @@
> #endif
> 
> JS_BEGIN_EXTERN_C
> 
> #include "jni.h"             /* Java Native Interface */
> #include "jsapi.h"           /* JavaScript engine API */
> 
>+#if PR_BYTES_PER_LONG == 8

Please use JS_BYTES_PER_LONG (it should be the same as PR_BYTES_PER_LONG -- if
not, we have a bad bug!), and you're in js/src/liveconnect, so it seems ok to
me to use JS_* rather than PR_* here.  If you prefer PR_*, use that in both
places.  sr=brendan@mozilla.org with this change.

/be
Attachment #155056 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 22

15 years ago
Fixed.

Checking in js/src/liveconnect/jsj_JSObject.c;
/cvsroot/mozilla/js/src/liveconnect/jsj_JSObject.c,v  <--  jsj_JSObject.c
new revision: 1.29; previous revision: 1.28
done
Checking in js/src/liveconnect/jsjava.h;
/cvsroot/mozilla/js/src/liveconnect/jsjava.h,v  <--  jsjava.h
new revision: 1.26; previous revision: 1.25
done
Checking in js/src/liveconnect/nsCLiveconnect.cpp;
/cvsroot/mozilla/js/src/liveconnect/nsCLiveconnect.cpp,v  <--  nsCLiveconnect.cpp
new revision: 1.38; previous revision: 1.37
done
Checking in js/src/liveconnect/nsCLiveconnect.h;
/cvsroot/mozilla/js/src/liveconnect/nsCLiveconnect.h,v  <--  nsCLiveconnect.h
new revision: 1.13; previous revision: 1.12
done
Checking in js/src/liveconnect/nsILiveconnect.h;
/cvsroot/mozilla/js/src/liveconnect/nsILiveconnect.h,v  <--  nsILiveconnect.h
new revision: 1.14; previous revision: 1.13
done
Checking in modules/oji/public/nsIJVMPlugin.h;
/cvsroot/mozilla/modules/oji/public/nsIJVMPlugin.h,v  <--  nsIJVMPlugin.h
new revision: 1.18; previous revision: 1.17
done
Checking in modules/oji/src/lcglue.cpp;
/cvsroot/mozilla/modules/oji/src/lcglue.cpp,v  <--  lcglue.cpp
new revision: 1.57; previous revision: 1.56
done
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 23

15 years ago
kyle you torched most builds. please fix. and please get on irc.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 24

15 years ago
Comment on attachment 156477 [details] [diff] [review]
fix the bustage

What's wrong with using jlong unconditionally?
Comment on attachment 156477 [details] [diff] [review]
fix the bustage

>-#include "jsjava.h" // for lcjsobject

OK, fixes bustage

>+#if PR_BYTES_PER_LONG == 8
>+	GetJavaWrapper(JNIEnv* jenv, jlong obj, jobject *jobj) = 0;
>+#else
>+	GetJavaWrapper(JNIEnv* jenv, jint obj, jobject *jobj) = 0;
>+#endif

Why couldn't you make this jlong unconditionally?
Attachment #156477 - Flags: superreview?(dveditz)
Comment on attachment 156477 [details] [diff] [review]
fix the bustage

Oh, sorry, I was thinking jlong was a C++ long, not a Java long.  Why not just
use JSWord?
Comment on attachment 156477 [details] [diff] [review]
fix the bustage

r=dveditz for bustage fix, the ifdef looks like the best thing for now since
platform "long" is broken on some 64-bit compilers
Attachment #156477 - Flags: superreview?(dveditz) → review+
(Assignee)

Comment 29

15 years ago
I have checked in attachment 156477 [details] [diff] [review], though it won't work in LLP64 model.
Brendan mentioned some tricky methods to work around this issue, such as
ptrdiff_t, intptr_t (C99 only), but I still prefer a ultimate solution for the
LLP64 issue, and I'll be happy to make a new patch then.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Updated

9 years ago
Component: Java: Live Connect → Java: Live Connect
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.