136729 - 64bit problems in CreateThread/PostEvent

Status: RESOLVED FIXED

(Core Graveyard :: Java: OJI, defect)

Description

[Philip K. Warren]

In nsJVMManager::CreateThread and nsJVMManager::PostEvent, there are problems
when compiled in 64-bit mode.

CreateThread takes a PRUint32 pointer as an argument. It currently creates a
PRThread, and copies the pointer to the PRThread into the PRUint32. This causes
problems in 64-bit mode because the pointer to the PRThread is 64-bit.

PostEvent is similar. It takes a PRUint32 argument, and casts that to a PRThread
pointer.

For the Java Plugin to work in 64-bit mode, I believe that both of these
functions should be altered to take a PRThread as a parameter instead of a
PRUint32.

Comment 1

[Philip K. Warren]

Attachment: Patch which changes PRUint32 to PRThread in CreateThread/PostEvent.

Comment 2

[Philip K. Warren]

Joe:

Have you had a chance to look at this patch?

Comment 3

[Jim Dunn]

Phil, is there a specific test/url/site that shows this problem?
Having a testcase that shows this issue would be extremely helpful
Or is it a build issue?  In which case having the error output
would be helpful.

Comment 4

[Joe Chou]

Phil, sorry for getting back to you so late. Currently I have over 200 bugzilla
bugs and sending me an email would catch my attention quicker.

About the changes, here is my comments:

1)this patch seems changing the interface, would it affect Java plugin?
2)how to verify that this patch works? 

Cc-ing some Java plugin folks.

Comment 5

[Joe Chou]

This may affect Mac's JRE too, cc-ing Patrick.

Comment 6

[Vinod S Patil]

There is no specific testcase to see this problem. Without the patch and with the 64-bit JavaPlugin enabled try accessing any remote applet - the browser fails to resolve the proxy address and will just hang saying "Loading applet....". With the patch the browser will successfully load the applet. Note: The browser has no problems loading an applet that is on the local m/c.   

Comment 7

[Joe Chou]

I applied the patch and tried with JRE 1.4.1, and it seemed working OK. Java
still worked, and no complaints in the Java console.
Still a bit concerned of the patch touched the interface. Please test more, and
make sure it would not break the interface to Java. Given that, 
r=joe.chou

Comment 8

[Philip K. Warren]

Another alternative (instead of using PRThread) is to change the parameter type 
to one of unsigned long, which will be the same in 32-bit compiles, but also 
work in 64-bit compiles.

Comment 9

[Joe Chou]

Could you provide a patch of that then, Phil? Also update the comments in a
place of explaining the parameters if parameter name(s) changed (i.e.,
"ThreadID" changed to "Thread").

Comment 10

[Shanmugavelu Shanmuganathan (gone)]

On a Tru64 UNIX system I applied this patch and rebuilt the liboji.so, 
but it didn't change the  behaviour. With or without this patch I am
not able to get the applet loaded on my mozilla browser. 

Comment 11

[Vinod S Patil]

What are the sizes of 'long' and a pointer on a Tru64 system?.and what are their
sizes on a 32-bit system?.
The patch suggested by Phil should work fine if longs and pointers are of the
same size on both 32-bit and 64-bit systems. On PowerPC32 a 'long' and a pointer
are of 32-bit size and on a PowerPC64 system they both are of 64-bit size. So
changing the parameter to a long should make it work on both 32-bit and 64-bit
system.

Comment 12

[Shanmugavelu Shanmuganathan (gone)]

On a Tru64 UNIX both long and pointers are 64 bit. 
But I applied this patch and rebuilt liboji.so and it doesn't seem
to work for me. 

Comment 13

[Vinod S Patil]

Did the earlier patch of changing the parameter to PRThread instead of PRUint32
work for you? 
and are the sizes of 'long' and 'pointer' same on your 32-bit system?

Comment 14

[Shanmugavelu Shanmuganathan (gone)]

I only applied the patch attached here, which didn't work.
If something else should work (changing it to long) can I have that patch, as
requested in comment #9. 
   BTW, "Tru64 UNIX" is a 64 bit OS and there is no existence of a 
32 bit Tru64 UNIX.

Comment 15

[Vinod S Patil]

I understand that Tru64 Unix is a 64-bit OS and I also didn't expect a 32-bit
version of Tru64 Unix either - What I wanted to know was the differences in
long and pointer sizes between Tru64 Unix and the Unix running on the 32-bit
Alpha processor. But anyway the patch(of changing PThread to 'unsigned long')
has not been made available.
Phil, could you please provide the patch of changing PRThread to 'unsigned
long'- thanks

Comment 16

[Vinod S Patil]

I understand that Tru64 Unix is a 64-bit OS and I also didn't expect a 32-bit
version of Tru64 Unix either - What I wanted to know was the differences in
long and pointer sizes between Tru64 Unix and the Unix running on the 32-bit
Alpha processor. But anyway the patch(of changing PThread to 'unsigned long')
has not been made available.
Phil, could you please provide the patch of changing PRThread to 'unsigned
long'- thanks

Comment 17

[Philip K. Warren]

Attachment: PRUint32 -> unsigned long

Here is the patch that uses unsigned long as opposed to PRUint32. This should
be functionally equivalent to what we have now, and should also work on 64-bit
platforms where an unsigned long (64-bit) can store an entire pointer address.

I still believe that the proper fix would be to use PRThread *, as we don't
know how this patch will affect platforms where unsigned long is not 32-bits in
32-bit mode, and is not 64-bits in 64-bit mode.

Comment 18

[Joe Chou]

Since Phil is working on this one, re-assign to phil.

Comment 19

[Shanmugavelu Shanmuganathan (gone)]

I tested this patch on a Tru64 UNIX system, but I didn't get the applet displayed.
(I tried http://java.sun.com/openstudio )
Has this been tested on another 64-bit OS?

Comment 20

[Vinod S Patil]

On IBM 64-bit AIX,I am able to load all the applets that are available at
http://java.sun.com/openstudio. 
Which JavaPlugin are you using?. Please note that changes to javaplugin code are
also needed apart from changes to the Mozilla code. We encountered the above
mentioned problems with Mozilla when we were porting Javaplugin to 64-bit AIX.

Phil, Thanks for the patch. I also agree with your last comment that the earlier
patch was better since we don't know about the sizes of 'unsigned long' on all
the 64-bit OSes. 

Comment 21

[Patty Mac]

Chris Petersen is a new QA contact for oji component. His email is:
[e-mail]

Comment 22

[Matthias Versen [:Matti]]

fixing small error for [e-mail] (filter with : SPAMMAILSUCKS)

Comment 23

[Joshua Xia]

reassign to me

Comment 24

[Joshua Xia]

I will review the first patch, "PRUint32 -> unsigned long" patch is not
appropriate, you should avoid to use native type.
32->64 porting -- Long Long (64-bit) Integers, see: 
http://www.mozilla.org/projects/nspr/reference/html/prlong.html

Comment 25

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

er, fyi: "unsigned long" in .idl files will end up as PRUint32 in the .h file
for the .idl. Do _not_ use unsigned long in the .cpp implementation of an
interface, even if that had "unsigned long".

Comment 26

[Joshua Xia]

Comment on attachment 78611 [details] [diff] [review]
Patch which changes PRUint32 to PRThread in CreateThread/PostEvent.

I reviewed this patch on Solaris, it works well

Comment 27

[Joshua Xia]

Comment on attachment 96193 [details] [diff] [review]
PRUint32 -> unsigned long

not better than the first one

Comment 28

[Simon Fraser [no longer active]]

Are these public APIs that are being changed?

Comment 29

[Philip K. Warren]

Yes I believe these are public APIs (Joshua can confirm).

Comment 30

[Joshua Xia]

CreateThread and PostEvent are exported to JPI side, so that JPI can create
thread and post event.

Comment 31

[Simon Fraser [no longer active]]

Since they are public, what will break if you change them?

Comment 32

[Joshua Xia]

I don't see any break on solaris

Comment 33

[Joshua Xia]

->kyle

Comment 34

[Kyle Yuan]

Comment on attachment 78611 [details] [diff] [review]
Patch which changes PRUint32 to PRThread in CreateThread/PostEvent.

Yes, nsIThreadManager.h is a public interface used by both OJI and JPI. But as
far as I know, Sun's Java plugin does not support 64-bit OS yet (JRE itself
does), and PRUint32 & PRThread * have the same size on 32-bit OS, so this patch
won't break anything on 32-bit OS. I don't have any 64-bit friendly Java plugin
in hand, AIX seems to be the only platform can run 64-bit java plugin and this
patch works well on it, so I'm okay with this patch.

Comment 35

[Brendan Eich [:brendan]]

Comment on attachment 78611 [details] [diff] [review]
Patch which changes PRUint32 to PRThread in CreateThread/PostEvent.

sr=[e-mail].  Anyone know why uint32 was used originally?

/be

Comment 36

[Kyle Yuan]

uint32 is there since its first version, don't know why.

checked in.

Comment 37

[Makoto Kato [:m_kato]]

Kyle, although these files aren't updated, should we file a new bug??

/modules/oji/tests/doc/OjiAPITestcases.html
/modules/oji/tests/script/OJIAPITestsDescr.html
/modules/oji/tests/src/TM/*

Comment 38

[Xiaobin Lu]

This change will break Java Plug-in. Here is how the current Java Plugin uses
the functions defined inside nsIThreadManager.idl. Java Plugin calls
GetCurrentThread to get the thread id. And then we use the returned value from
that thread id to post event to the Mozilla side by calling PostEvent.

So after this change, whatever we got from GetCurrentThread differs from what
PostEvent expects. This will stop Java Plug-in at least from working.

I think a better solution is to change the argument of PostEvent from PRUint32
to nsPluginThread since that is how the outside module is supposed to use this
interface usually.

Comment 39

[Kyle Yuan]

Actually, it won't break anything, see
http://lxr.mozilla.org/seamonkey/source/modules/oji/public/nsIThreadManager.h#60,
and http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/include/prthread.h#83
PRThread* == nsPluginThread* == void*

I'm wondering why we need |nsPluginThread*|? Casting PRThread* to
nsPluginThread* in nsJVMManager::GetCurrentThread and casting nsPluginThread*
back to PRThread* in nsJVMManager::PostEvent are totally unnecessary.

Comment 40

[Kyle Yuan]

Attachment: get rid of nsPluginThread

As I mentioned in comment 39, nsPluginThread is an unnecessary typedef.

Comment 41

[Kyle Yuan]

(In reply to comment #37)
> Kyle, although these files aren't updated, should we file a new bug??
> 
> /modules/oji/tests/doc/OjiAPITestcases.html
> /modules/oji/tests/script/OJIAPITestsDescr.html
> /modules/oji/tests/src/TM/*
> 

Makoto, nobody in maintaining the modules/oji/tests/ directory now, it's quite
out-of-date (the last valueable touch was in 2000). I don't think it's worth to
be updated.

Comment 42

[Brendan Eich [:brendan]]

Comment on attachment 148355 [details] [diff] [review]
get rid of nsPluginThread

Good to know that despite the pointer types differing, the actual pointer was
always to the PRThread.  I'm assuming this doesn't break any C++ ABI based on
typesafe name mangling.

/be