78840 - Browser crashes when the liveconnect call invoked using File->open

Status: VERIFIED FIXED

(Core Graveyard :: Java: OJI, defect, P1)

Description

[Xiaobin Lu]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.01; Windows NT)
BuildID:    



Reproducible: Always
Steps to Reproduce:
1.Download the testcase, lauch Java2JS.html
2.click the button


Actual Results:  Browser crashes!

Expected Results:  An alert box should come up.

Drag the html page and drop it into the browser, it will not crash. However, 
crash happens when using File->open. Also remember to reproduce this bug, don't 
go to any applet page first.

Comment 1

[Xiaobin Lu]

Reassign to myself!

Comment 2

[shrirang khanzode]

merking NEW

Comment 3

[Xiaobin Lu]

Attachment: Testcase in Zip format

Comment 4

[Xiaobin Lu]

Attachment: Patch (Iteration 1)

Comment 5

[Xiaobin Lu]

Beard and Ed:
   Finally, we found out the reason why this problem exists. Actually it is 
caused by lazy loading fix. After lazy loading fix, the responsiblity of JVM 
start up was delayed until a Java application/applet are run. So, when the JVM 
started, a system class loader will be created to load the system class such as 
java.lang.System. There is a system properties called "usr.dir" which is used 
with "java.class.path" by the system classloader to find the class to load. When 
the user uses File->Open to open an applet page, the user.dir is set to the 
directory where is the class file is. This is why we always see that the applet 
was loaded by the system classloader which may cause liveconnect call from Java 
to JS crash.
  My fix is to save the current directory to a place and then reset the current 
directory to the java plugin lib directory. After the JVM is started up, we may 
just restore the directory to the original one. 
  Unix plugin implementation does not have this problem. So this is why I use 
#ifdef. Also a static variable was used to indicate that the JVM was started up 
or not. This is out of the performance reason.
  The fix is fairly simple and please give it a review and sr. Thanks in 
advance!

Comment 6

[Xiaobin Lu]

Attachment: Correct one

Comment 7

[Xiaobin Lu]

Sorry for attching a wrong diff. Please use the last patch call " Correct one" 
foe reviewing.
Thanks!

Comment 8

[Xiaobin Lu]

With this patch, it is still not enough to make the testcase work. It just avoid 
crash when you bring up the testcase after the browser start up.

Comment 9

[edburns]

With applied patch, no crash on win32, but no alert either.

r=edburns.

Comment 10

[sean echevarria]

Shouldn't this be fixed in the plugin itself?

Comment 11

[Xiaobin Lu]

Sean:
   Actually this is not wrong with Java Plugin. It is caused by the JVM start 
up behavior change in browser side. Please refer my 5/16(13:12) comment for the 
reason.

Comment 12

[sean echevarria]

I guess I don't follow the explanation of the cause.  

But from looking at the patch, it appears that you're just reading the Java
plugin's Windows registry information to temporarily set the current working
directory to "JavaHome" while a plugin instance is created.  

Why should that code be in the browser?  Why doesn't the plugin read it's own
registry information itself to see where the system classloader can find the
class to load?

Comment 13

[Xiaobin Lu]

sean: 
    Sorry if I made you confused. Actually it is possible to make change in the 
plugin code. However, the change will be much extensive than here. Java plugin 
can not control the system class loader to do things. It is part of jdk code 
which we do not have much control. Basically the lazy loading fix does something 
sort of breaking the assumption which Java plugin used to design. 

Comment 14

[drapeau]

Adding status to indicate that it's currently being worked on by Xiaobin.

Comment 15

[Xiaobin Lu]

Attachment: Using DirectoryService instead of using registry service

Comment 16

[Xiaobin Lu]

Patrick:
   Please sr the patch. Tested in Windows and Linux, works fine!

Comment 17

[Patrick C. Beard]

Comments on the patch, you shouldn't put your "static classLoaded" variable at 
top-level in the file, but only inside the function, right next to the 
declaration of origDir. Consider renaming the variable to something like 
firstPluginInstance. Also, you appear to have a memory leak where you call 
"binDirectory->GetPath(&path);" because path is dynamically allocated. You need 
to free the memory. Might as well use an nsXPIDLCString for this, and the 
getter_Copies() pattern. Fix these problems, and then you'll my r=beard.

Comment 18

[Xiaobin Lu]

Attachment: Put static variable inside function

Comment 19

[Xiaobin Lu]

Patrick:
    It is better to use char* instead of nsXPIDLString because the parameter to 
pass into the SetCurrentDirectory is char*, I am trying to avoid using cast 
here. Thanks for your suggestion anyway.

Comment 20

[Brendan Eich [:brendan]]

xiaobin, beard recommended nsXPIDLCString, and you don't need to cast when
passing one (call it s) wrapped by getter_Copies(s) -- don't cast away compiler
errors in cases like this -- there's a helper class to use.  See
xpcom/ds/nsXPIDLString.h's comments.

/be

Comment 21

[Xiaobin Lu]

Brendan:
    I think the reason I don't want to use nsXPIDLString is the parameter 
GetPath accepts is a char** instead of char*. I don't know if there is any way 
to use nsXPIDLString in that case.
 

Comment 22

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

getter_Copies (the usual case) and getter_Shares (the unusual case) create
temporary objects with implicit conversions to char** and correctly manipulate
the internals of the nsXPIDL[C]String.  See
http://lxr.mozilla.org/seamonkey/source/string/obsolete/nsXPIDLString.h#33

Comment 23

[Xiaobin Lu]

Attachment: use nsXPIDLCString

Comment 24

[Patrick C. Beard]

r=beard

Comment 25

[Brendan Eich [:brendan]]

So if I understand correctly, you need to change current process directory only
for the first time, and only on Windows, because after the first time, the JVM
will be already started, running in its own process, with the right current
working directory?

If so, sr=[e-mail].

/be

Comment 26

[Xiaobin Lu]

Yes, exactly like what you said here. This fix should be considered as part of 
Lazy loading fix(Beard).

Comment 27

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 28

[Xiaobin Lu]

Fix checked in!

Comment 29

[edburns]

This still works on win32 with Wednesday's trunk.

Comment 30

[shrirang khanzode]

qa->pmac

Comment 31

[Patty Mac]

Verified ok without crash on windows 98 (commercial trunk: 2001-08-29-11-trunk)
and Java(TM) Plug-in: Version 1.3.0_01.