77194 - bug 60018: LiveConnect does not work with JavaObjects in no applet case - JSObject.getMembers does not work, returns null [EQUESTOR: Oracle]

Status: VERIFIED FIXED

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

Description

[edburns]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 4.0)
BuildID:    Netscape 6, build date 11/05/2000

Using liveconnect, Oracle Tools products need the ability to use a JavaScript 
Object to set 2 of its members to the DOM window and DOM document.  Our code 
can then can then create a JavaObject by doing the following:
var OracleObj = new Packages.<classname>();
We then would pass in the JavaScript object with our parameters, e.g.
OracleObj.load(ourParams);
In the load method for the JavaObject, we need to do the following:
JSObject win = (JSObject) ourParams.getMember("windowMember");
JSObject doc = (JSObject) ourParams.getMember("documentMember");
In Netscape 6, both of these calls fail, whereas in Netscape 4.7,
my testcase works perfectly.
We can NOT use an Applet.   Our applications are large and complex.
If we use an Applet and a user goes to another URL, then returns to our 
Application, it would reload the entire application.  This is unacceptable to 
our customers.  To avoid this problem we use a JavaObject (as I described 
above), which does not get unloaded as the user changes pages and comes back.
Being able to use a JavaObject instead of an Applet AND do a getMember on any 
JSObject is critical for Oracle Tools products.  I consider this a showstopper 
and it needs to be addressed as quickly as your schedule permits.

Reproducible: Always
Steps to Reproduce:
1. Use the jotest.html that is included in "Additional information" below.
2. Take the Cherokee.java file included in "Additional information" below, 
place it in a directory c:\java\lang\Cherokee.java.
3. Compile it into c:\java\lang\Cherokee.class
4. Zip \java\lang\Cherokee.class into your JDK 1.3 rt.jar file. Using Winzip, 
verify that Cherokee.class is in your rt.jar file and its directory is 
java\lang.
5. Bring up the Netscape 6 browser and open the java console.
6. run the jotest.html file provided.
7. In the console, you will see that the Cherokee JavaObject is created, but 
the calls in the load method to params.getMember both return null.  NOTE:  This 
works fine under Netscape 4.7
*
IMPORTANT NOTE:  The reason that Cherokee.class is put into rt.jar under 
java/lang is that Netscape 6/JDK 1.3 does not use the classpath the way it did 
under JDK 1.1 and I can't figure out a way to tell javascript how to find the 
Cherokee.class file.

Actual Results:  The System.out.println statements in the JavaObject's load 
method indicate the dom win and doc are both null:
***********
Starting jotest.html

*** Cherokee constructor: ***Hello

*** jotest.html: calling Cherokee.load() ****


*** cherokee.load: Start of method ***

*** cherokee.load: msg is Cherokee.load

*** cherokee.load: win is null ***

java.lang.NullPointerException

	at java.lang.Cherokee.load(Cherokee.java:55)

	at java.lang.reflect.Method.invoke(Native Method)

	at sun.plugin.liveconnect.PrivilegedCallMethodAction.run(Unknown Source)

	at java.security.AccessController.doPrivileged(Native Method)

	at sun.plugin.liveconnect.SecureInvocation.CallMethod(Unknown Source)

*** cherokee.load: doc is null ***

*** cherokee.load: Using DOC to write html ***

java.security.PrivilegedActionException: 
java.lang.reflect.InvocationTargetException: java.lang.NullPointerException

	at java.lang.Cherokee.load(Cherokee.java:81)

	at java.lang.reflect.Method.invoke(Native Method)

	at sun.plugin.liveconnect.PrivilegedCallMethodAction.run(Unknown Source)

	at java.security.AccessController.doPrivileged(Native Method)

	at sun.plugin.liveconnect.SecureInvocation.CallMethod(Unknown Source)



Expected Results:  I would have expected no errors and a valid DOM window and 
document returned to the applet's method as JSObjects.  The calls win.call
("showAlert", msg); and doc.call("write", msg);  should both have worked 
correctly.

Source code to jotest.html:
********
<html>

<head>
<title>Live Connect using a JavaObject</title>
</head>
<body>
<form ID="form1" name=myForm>
<input ID="field1" Type=text value="Live Connect using a JavaObject" 
name="fieldOne">
<input ID="button1" Type=button value="Set Cookie" name="buttonOne">
</form>

<script>


	function showAlert(txt)
	{
	    alert ("JavaScript 'showAlert': called from " + txt);
	}

   var params =
        {
		    domwin: window,
			domdoc: document,
			serverhost: "tkrtc-server.us.oracle.com"
		};

   java.lang.System.out.println("Starting jotest.html");

   var htmlForm = new java.lang.Cherokee("Hello");

   java.lang.System.out.println("*** jotest.html: calling Cherokee.load() 
****\n");
   htmlForm.load(params);
   java.lang.System.out.println("*** jotest.html: returned from Cherokee.load() 
****\n");

</script>
</body>
</html>
*************************************************************
Source to Cherokee.java
*************************************************************
package java.lang;

import netscape.javascript.*; 

public class Cherokee
{ 
    JSObject win = null;
	JSObject doc = null;

	//-----------------------------------------------------------------
	// Constructor
	//-----------------------------------------------------------------

    public Cherokee(String txt)
	{
 	   System.out.println("*** Cherokee constructor: ***" + txt);
    }   
    

    //--------------------------------------------------------------------
	// Public Methods
	//--------------------------------------------------------------------

    public void load(JSObject params)
	{
        System.out.println("*** cherokee.load: Start of method ***");

        String msg[];
        msg = new String[1];

        // Create a local copy of the cookie text
        msg[0] = "Cherokee.load";
        
        System.out.println("*** cherokee.load: msg is " + msg[0]);

        try
		{
			// Get the DOM window through params             
			win = (JSObject) params.getMember("domwin");

			if (win == null)
			{
			   System.out.println("*** cherokee.load: win is null 
***");
			}
		}
        catch (Exception e)
		{
            // Don't throw exception information away, print it.
            e.printStackTrace();
        }

        try
		{
			// Call the JavaScript function to set the 
cookie             
			win.call("showAlert", msg);
		}
        catch (Exception e)
		{
            // Don't throw exception information away, print it.
            e.printStackTrace();
        }

        try
		{
			// Get the DOM window through params             
			doc = (JSObject) params.getMember("domdoc");

			if (doc == null)
			{
			   System.out.println("*** cherokee.load: doc is null 
***");
			}
		}
        catch (Exception e)
		{
            // Don't throw exception information away, print it.
            e.printStackTrace();
        }

        System.out.println("*** cherokee.load: Using DOC to write html ***");
        msg[0] = "<H1><I>Cherokee.load</I><H1>";
		doc.call("write", msg);

        System.out.println("*** cherokee.load: End of method ***");
    }
}

Comment 1

[edburns]

This is created from bug 60018 to address the fact that it doesn't work on Unix 
and Win32.

Comment 2

[edburns]

Attachment: Packaged test case as jotest.zip

Comment 3

[Phil Schwartau]

Changing component and QA to OJI to match bug 60018 -

Comment 4

[Xiaobin Lu]

*** Bug 77041 has been marked as a duplicate of this bug. ***

Comment 5

[Patrick C. Beard]

So, the remaining issue for all platforms is the fact that LiveConnect has no 
mechanism to load a class file that happens to be stored in the same directory as 
an HTML document that references it. To implement this, we need to provide 
LiveConnect with at least the URL of the document that is referencing the class, 
so that it can attempt to download the class. For some time now, I've wanted to 
expose some sort of class loading interface to Rhino and Spidermonkey, so that JS 
code can dynamically reference packages without having to install them in the 
JVM's actual CLASSPATH.

Comment 6

[Patrick C. Beard]

I have checked into the tree, an initial implementation of the idea I had to 
extend the ProxyJNI::FindClass() method to load classes from the same URL as the 
document of the currently executing script. This will need a thorough security 
review, and there are additional changes in my tree to make this work. I'm happy 
to report, that I don't have to put the Cherokee.class file in my class path to 
get it to run, I am able to load it via class loader from both a file URL and an 
http URL. The new files I added to the there are here:

mozilla/modules/oji/src/classes/netscape/oji/ProxyClassLoader.java
mozilla/modules/oji/src/ProxyClassLoader.cpp
mozilla/modules/oji/src/ProxyClassLoader.h

I will also enclose the patches to my tree needed to uses these new files. You 
will have to do some build system work to make these work on the other platforms.

Comment 7

[Patrick C. Beard]

Attachment: Patches to use new ProxyClassLoader.

Comment 8

[drapeau]

Marking as one of the OJI group's current hot bugs.

Comment 9

[drapeau]

Updating bug dependencies.  Also, completion of this bug is also dependent on
the next release of the Java Plug-In (JDK 1.4), due later this year.

Comment 10

[edburns]

I have filed sun internal bugtraq bug: 4470652 to cover the sun side of the 
work.

Comment 11

[Xiaobin Lu]

Attachment: Patch for Cherokee case

Comment 12

[Xiaobin Lu]

Attachment: The patch for browser side

Comment 13

[Xiaobin Lu]

Attachment: A tar ball of new files added

Comment 14

[edburns]

I've tried this with JDK1.3.1 and Monday's trunk on WINNT.  I have found that 
java still works, and liveconnect functionality that is known to work still 
works after applying the patch.  Here are my code review comments for Xiaobin.  
By the way, great work on this!

Notes: Summary: overall ra=edburns, but you must address each of my 

question:

comments below.

Index: lcglue.cpp: Summary: ra=edburns
--------------------------------------

Do some fancy accounting for the java_applet_obj pointer.  Looks ok to me.

Index: ProxyClassLoader.cpp: Summary: ra=edburns IF questions are addressed.
----------------------------------------------------------------------------
Includes ProxyClassloaderFactory.java: ra=edburns

function getScriptClassLoader(): 
{

<<<<<<< variant A
    if (!spec) {
>>>>>>> variant B
    if (!jspec) {
======= end

This change looks like a simple typo fix, but nice catch.

Here we're just loading and using the new ProxyClassLoaderFactory java
class instead of loading the ProxyClassLoader directly.

Then we create an instance of the new nsCSecurityContext2().

question: If nsCSecurityContext2 is only used for this "null context"
purpose, perhaps it should be renamed to nsCNullSecurityContext?

We save the existing security context, call
ProxyClassLoaderFactory.createClassLoader(), then restore the old
security context.

ProxyClassLoaderFactory.createClassLoader()
{
     Just creates a java.net.URLClassLoader instance around jspec.  I'm
     glad to see that you catch the exception and proceed as normal if
     the URL is invalid.  

     question: can we print out an error message or something in the
     event of a malformed URL?
}

question: It looks like there is a leak of your nsCSecurityContext2
instance.  True?

}

function ProxyFindClass()
{

I like that you check for sucessful return from getScriptClassLoader().

Looks like you get rid of a spurious Z in the method signature for
loadClass:

javap -s java.lang.ClassLoader:

	/*   (Ljava/lang/String;)Ljava/lang/Class;   */
    protected synchronized java.lang.Class loadClass(java.lang.String, boolean) 
throws java.lang.ClassNotFoundException;

}

Index: ProxyJNI.cpp: summary: ra=edburns
-----------------------------------------

Yes, you call your modified ProxyFindClass().  Yes, you make a wrapper
around getContext().

Index: ProxyJNI.h: summary: ra=edburns
--------------------------------------

Yes, you add a new method.

Index: nsCSecurityContext2.{h,cpp}: Summary ra=edburns if you change the
name to something descriptive, like nsCNullSecurityContext.

Comment 15

[Xiaobin Lu]

Attachment: Patch for 77194, modified based on Ed's suggestions

Comment 16

[shrirang khanzode]

SPAM: reassigning all OJI bugs to new OJI QA, pmac ( 227 bugs)

Comment 17

[Xiaobin Lu]

Attachment: 1st iteration for sr

Comment 18

[Brendan Eich [:brendan]]

Please make sure no tabs get checked in.  There are some here, for instance:

         result = secureEnv->FindClass(name, &outClass);
+       if (NS_FAILED(result) || !outClass)
+           outClass = ProxyFindClass(env, name);

and elsewhere, including the next fragment I cite.  Use vim, :set expandtabs
(www.vim.org), or if you use Emacs, get it to respect the modeline comment at
the top of each file.

No need for else after return below (expand all tabs, too, please):

+    nsresult GetSecurityContext(nsISecurityContext **context) {

+
if (!context)

+
    return NS_ERROR_FAILURE;

+
else {

+
    *context = getContext();

+
    return NS_OK;

+
}

+    }

Indentation in nsCNullSecurityContext::GetOrigin is way off, three spaces for
the if line, then a tab followed by four spaces for the next line, etc.

ProxyClassLoaderFactory.java uses tabs throughout -- please expand to four-space
equivalent indentation units.

Why are you using the NPL license header comment for new files?  Why not use the
MPL instead?

Are these new files being added to the Mac build?

I don't see an r=, so can't yet sr= -- can you get [e-mail] to do the
code review?  Thanks,

/be

Comment 19

[Xiaobin Lu]

Attachment: 2nd iteration for sr

Comment 20

[Xiaobin Lu]

Two mistakes were correted:
 . Tab and space
 . Using MPL instead of NPL for new file

Patrick:
  Would you please also review my patch to see if it works in Mac? Thank you 
very much!

Comment 21

[Patrick C. Beard]

As written, the patch to ProxyClassLoader.cpp appears to leak the
nsCNullSecurityContext instance |nullContext|:

+    nsISecurityContext* nullContext = new nsCNullSecurityContext();
+    SetSecurityContext(env, nullContext);
+    *classloader =
env->CallStaticObjectMethod(netscape_oji_ProxyClassLoaderFac, staticMethodID,
jspec);
     if (!*classloader) {
         env->ExceptionClear();
+        SetSecurityContext(env, origContext);
         return NS_ERROR_FAILURE;
     }
 
+    SetSecurityContext(env, origContext);
+    if (nullContext)
+
nullContext = NULL;

However, I'm guessing that |SetSecurityContext(env, nullContext)| actually
brings the refCount of |nullContext| to 1, and then calling
|SetSecurityContext(env, origContext)| reduces it back to 0, which is why you
just NULL out |nullContext|. For clarity, consider using
|nsCOMPtr<nsISecurityContext> nullContext(new nsCNullSecurityContext())|. 

You always need to restore |origContext| so I'd rearrange the patch to always
call |SetSecurityContext(env, origContext)| before checking for the error. Also,
 you're not checking to see if the allocation of |nullContext| succeeds.

|nsCNullSecurityContext::GetOrigin(char* buf, int len)| isn't safe, as it isn't
using the len parameter to do range checking on the buffer. You should add a
check for that.

With these changes, I am happy to makr r=beard.

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Fixing database corruption caused by bug 95743.  Pay no attention to the man
behind the curtain.

Comment 23

[Xiaobin Lu]

Attachment: 3rd iteration

Comment 24

[Xiaobin Lu]

Thank you, Patrick! I have adapt the patch for your suggestion.

Brendan:
   Would you mind give me a sr=? Thank you in advance!

Comment 25

[Brendan Eich [:brendan]]

I think you want to fail with NS_ERROR_OUT_OF_MEMORY if you can't allocate
nullContext.  Therefore (and anyway, given that you've switched nullContext from
a raw pointer to an nsCOMPtr), you don't need the

    if (nullContext)
        nullContext = nsnull;

any longer.

Please use NS_IMPL_ISUPPORTS1(nsCNullSecurityContext, nsISecurityContext)
instead of writing your own QueryInterface method for that class, and calling
the NS_IMPL_ADDREF/RELEASE macros.  Then you don't need kISecurityContextIID or
kISupportsIID either, so you should remove those.

/be

Comment 26

[Xiaobin Lu]

Attachment: 4th iteration

Comment 27

[Xiaobin Lu]

Thanks, Brendan! Please review the change and give a sr.

Comment 28

[Brendan Eich [:brendan]]

This control flow makes no sense:

+    nsCOMPtr<nsISecurityContext> nullContext(new nsCNullSecurityContext());
+    if (nullContext) {
+        SetSecurityContext(env, nullContext);
+    }
+    else {
+
return NS_ERROR_OUT_OF_MEMORY;
+    }
+    *classloader =
env->CallStaticObjectMethod(netscape_oji_ProxyClassLoaderFac, staticMethodID,
jspec);
+    SetSecurityContext(env, origContext);
     if (!*classloader) {

It's formally correct, but why is the SetSecurityContext(env, nullContext) done
in a 'then' statement that must preceded the *classloader = ...;
SetSecurityContext(env, origContext); if (!*classloader) {...} rest of the
method?  Instead, just check for a null return from new and return early:

+    nsCOMPtr<nsISecurityContext> nullContext(new nsCNullSecurityContext());
+    if (!nullContext) {
+
return NS_ERROR_OUT_OF_MEMORY;
+    }
+    SetSecurityContext(env, nullContext);
+    *classloader =
env->CallStaticObjectMethod(netscape_oji_ProxyClassLoaderFac, staticMethodID,
jspec);
+    SetSecurityContext(env, origContext);
     if (!*classloader) {

This way, you don't arbitrarily put part of the sequential control flow that
must occur if new succeeds in the 'then' part of an if statement, and the rest
of that flow after the 'else' part that returns early.

Fix this, and sr=[e-mail].

/be

Comment 29

[Xiaobin Lu]

Attachment: 5th ieteration

Comment 30

[Xiaobin Lu]

Problem fixed! 
Will check in today at 6:35.
Thank you very much, Brendan!

Comment 31

[Brendan Eich [:brendan]]

What was checked in was *not* what I reviewed.  In particular, see

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/modules/oji/src&command=DIFF_FRAMESET&file=ProxyJNI.cpp&rev1=1.18&rev2=1.19&root=/cvsroot

where the #include "ProxyClassLoader.h" added in the first diff for ProxyJNI.cpp
in attachment 46519 [details] [diff] [review] is missing, and the second diff for ProxyJNI.cpp from that
attachment has been mangled to have bad indentation of the

    if (NS_FAILED(result) || !outClass)

line.

The missing #include set tinderbox afire, and Xiaobin, you were nowhere to be
found.  Waterson made the fix, you owe him.  Also, you checked in related files
over several commit commands, spread over a 20 minute interval.  That is just
poor practice, it makes a red tinderbox cycle likely for no good reason.  Did
you have network troubles, or what?

[e-mail] are considering what to do about this bad cvs citizenship.

/be

Comment 32

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

This patch caused bustage for (at least -- Tinderbox isn't green yet) three
reasons (the third hadn't been discovered when Brendan made his comment above):

1) The checkin was spread over 26 minutes, causing many of the builds that
pulled during that time to turn red.

2) The missing #include that brendan mentioned.

3) There were no Mac project changes corresponding to the makefile changes.

Comment 33

[(not reading, please use [e-mail] instead)]

Attachment: build log (for warnings)

Comment 34

[(not reading, please use [e-mail] instead)]

two other things:

I changed #include <jni.h> to #include "jni.h" for the mac, and some additional 
access paths.

the new file also has some warnings, I've attach the build log so you can look 
into them.

ProxyJNI.cpp:458: warning: initialization to non-pointer type
ProxyJNI.cpp:458: warning: argument to non-pointer type `unsigned char'
ProxyJNI.cpp:566: warning: initialization to non-pointer type
ProxyJNI.cpp:566: warning: argument to non-pointer type `unsigned char'
ProxyJNI.cpp:670: warning: initialization to non-pointer type
ProxyJNI.cpp:670: warning: argument to non-pointer type `unsigned char'
ProxyJNI.cpp:753: warning: initialization to non-pointer type
ProxyJNI.cpp:753: warning: argument to non-pointer type `unsigned char'
ProxyJNI.cpp:857: warning: initialization to non-pointer type
ProxyJNI.cpp:857: warning: argument to non-pointer type `unsigned char'

Comment 35

[Xiaobin Lu]

Brendan:
   I am really sorry for this! I think I am so careless and accidently removed 
one file in my working directory which caused this. I feel shame for myself.

Waterson:
   Thank you very much for your fix! 

Xiaobin 

Comment 36

[Xiaobin Lu]

Seth:
   I think that is not caused by my fix. I will look at that soon.

Comment 37

[Xiaobin Lu]

  I do appreciate the help from all of your guys. The browser side fix has been 
done. In order to make all thing work, we still need to wait for JRE1.4 release 
which will happen soon maybe.

Comment 38

[Patty Mac]

Marking verified.