63466 - Build 2000122004 Breaks LiveConnect!

Status: VERIFIED FIXED

(Core Graveyard :: Java: OJI, defect)

Description

[gary.kind]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; m18) Gecko/20001220
BuildID:    2000122004

I am running an Applet that uses liveConnect to get the javascript window object
and then to call in to Javascript from java via "window.call(<method name>,
<method arg array>);  This worked fine until today's build.  I put returned to
my previous build of 12/12 and
it works fine.  There are NO error messages in the java or javascript consoles.
 It's as though the MAYSCRIPT parameter of the applet is not being recognized

Reproducible: Always
Steps to Reproduce:
Write a quick applet.  In the init() method do:
JSObject domWin = (JSObject) JSObject.getWindow(this);
String args[] = new String[1];
args[0] = "From Applet Init method";
domWin.call("showAlert", args);

In your html file, create a javascript function as follows:

function showAlert(txt)
{
    alert("JS showAlert: being called " + txt);
}

Actual Results:  Nothing appears and no error message is seen in the java or
javascript console windows.

Expected Results:  An alert should appear indicating that Java successfully
called into Javascript.

Comment 1

[gary.kind]

Correction.  The build id of the working version that I have is 2000121104

Comment 2

[Phil Schwartau]

Reassigning to OJI component -

Comment 3

[Thomas Lendacky]

I found this problem when I moved to the current level of the code from the 
"NS6" level.  I did some investigation and found that bug 49839 removed the 
registration for LiveConnect. (See 
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&fi
le=nsSetupRegistry.cpp&root=/cvsroot&subdir=mozilla/webshell/tests/viewer&comman
d=DIFF_FRAMESET&rev1=1.182&rev2=1.183).

The LiveConnect code does not register itself and needs to be updated to do 
that (The JVM Manager registration code was also removed by 49839 but is 
registers itself and was therefore unaffected).

Comment 4

[gary.kind]

Is this fixed?  I am completely amazed that liveconnect has been completely
broken since the 12/20 build and it has not been fixed yet!  If it has been
fixed, then great, but somebody ought to update the bug?

Comment 5

[edburns]

Hi Xiaobin, can you please see if this is still occurring with 0.7?

Thanks,

Ed

Comment 6

[Xiaobin Lu]

Attachment: An Test case

Comment 7

[Xiaobin Lu]

  I provided a test case and lauched the applet, after that I took a look at 
the java console, it told me:

netscape.javascript.JSException
  at netscape.javascript.JSObject.getWindow(JSObject.java:141)
  at TestApplet.init(TestApplet.java:8)
  at sun.applet.AppletPanel.run(Unknown Source)
  at java.lang.Thread.run(Unknown Source)
Lauch the same applet with Netscape 4.75, the java console shows:
   netscape.javascript.JSException: MAYSCRIPT is not enabled for this applet.
Gary:
    Could you confirm my test case to see that applet source code is correct? 
Thanks!

Xiaobin 

Comment 8

[edburns]

Adding CC.

Comment 9

[gary.kind]

Xiaobin has asked me to review her code in the attachment she created.  I am
sorry but I don't know how to get her attachment.  I clicked on it and up came a
Browser window with a bunch of garbage in it.  Could somebody e-mail me
instructions on how to get an attachment?  Obviously, I am doing something
wrong.  Now be nice...

Comment 10

[gary.kind]

Got the attachment from MSIE, sorry.  It would not work in Netscape/Mozilla.
Anyway I reviewed (his) Xiaobin's code and it was missing the MAYSCRIPT applet
tag parameter.  I fixed it and tried it on the 12/11 build and it works ok.  I
then tried it on today's build, id 2000012404 and it STILL does not work!

It does not give me great confidence when the engineer assigned to work on
liveconnect does not even know about the MAYSCRIPT parameter.

The only thing I can assume is that nobody at Mozilla cares about liveconnect at
ll.  It has been broken for well over a month now and nobody is doing anything
about it.  How is anybody supposed to do any development using java and
javascript?  Fortunately, I have an old version that still works I would be
completely hosed...

The reason I am getting a little "sharp" in my comments is that I am getting
very frustrated with the lack of activity on Mozilla and Netscape's part in
dealing with liveconnect issues.  I have several blocking issues and there is
almost NO activity on these bugs.  Liveconnect is not usable and it is essential
to Oracle Products, if we are going to develop for Netscape.

Comment 11

[Xiaobin Lu]

Gary:
    Thanks very much for bringing this issue up. I will try to understand the 
problem and fix it as soon as possible.
    Sorry for inconviniece we brought to the user like you.
Xiaobin 

Comment 12

[Patrick C. Beard]

I confirm that the LiveConnect class isn't getting autoregistered. I am going to 
look at the autoregistration code in OJI right now to see what the problem is.

Comment 13

[Patrick C. Beard]

I believe Jud's checkin to mozilla/webshell/tests/viewer/nsSetupRegistry.cpp was 
the cause of this. He explicitly removed the registration of the 
NS_CLIVECONNECT_CID factory, which as it turns out *ISN'T* registered but perhaps 
should be by the OJI module. The implementation of this CID is in fact part of 
the LiveConnect DLL, which isn't a component, but a shared library.

Comment 14

[Judson Valeski]

I applied the patch in http://bugzilla.mozilla.org/show_bug.cgi?id=49839 (r=ed 
burns, a=brendan). Ed, I thought oji was handling this registration. Backing out 
my change is not the fix for this, oji needs to do the registration itself.

Comment 15

[Xiaobin Lu]

I changed mozilla/webshell/tests/viewer/nsSetupRegistry.cpp back and run that 
my testcase and alert box still does not come up. There is definitely something 
wrong somewhere else.

Comment 16

[Patrick C. Beard]

Here's a patch that moves the explicit registration to a routine exported by the 
LiveConnect DLL, and puts the responsibility for this registration back in the 
OJI component. Please review so we can get this checked in as soon as possible.

Comment 17

[Patrick C. Beard]

Attachment: Patch to register the nsCLiveConnectFactory for use by OJI plugins.

Comment 18

[Mitchell Stoltz (not reading bugmail)]

Looks good to me. r=mstoltz

Comment 19

[Patrick C. Beard]

This patch definitely fixes the first part of the problem, which is finding the 
nsILiveConnect service, but there appears to be a problem with 
JSObject.getWindow() always returning null to Java right now. I'll look into this 
part of the problem tomorrow morning.

Comment 20

[edburns]

Thanks Patrick.

Comment 21

[edburns]

Patrick, can you please get SR for this and get it checked in?

Comment 22

[gary.kind]

Sorry if I jump the gun, but the last comment in this description about checking 
in is premature if the JSObject.getWindow(myApplet) does not work.  This MUST 
work before closing the bug.  LiveConnect cannot be used unless it is fixed.

Comment 23

[Xiaobin Lu]

Patrik:
   I have confirmed that getWindow stuff and found that it returns null. This 
fix is very important to Liveconnect to make it work.
   Would you let me know if you got some idea about it? Thanks very much!

Comment 24

[Thomas Lendacky]

I have a build from code pulled on 01/24 where I modified 
mozilla/webshell/tests/viewer/nsSetupRegistry.cpp to register the LiveConnect 
service.  With this build, I receive the JavaScript alert when I run the test 
case (attachment id 23213) with the Test.html modification to add the mayscript 
parameter to the applet tag.

When I make the changes specified in the patch from attachment id 23492, oji.dll 
fails to link successfully because JSJ_RegisterLiveConnectFactory is an 
unresolved external.  This makes sense since it lives in jsj.dll.  The reason 
you're seeing the failure could be that the LiveConnect code is still not 
registered.

Why not just take the lines related to LiveConnect that were removed from 
mozilla/webshell/tests/viewer/nsSetupRegistry.cpp and add them in the OJI code 
somewhere.

Comment 25

[Judson Valeski]

I like tom's idea.

Comment 26

[Xiaobin Lu]

Tom:
    Could you tell me how did you change  
mozilla/webshell/tests/viewer/nsSetupRegistry.cpp file? I ran the test case and 
found that the returned value of call JSObject.getWindow(this) is not null 
although I saw it is null from Java console ( this lloks like java console has 
a bug). 
   I think the domWin.alert() call has some promblem. At least, it does not 
work in my mozilla build.

Comment 27

[Thomas Lendacky]

Attachment: Temporary work around to register LiveConnect

Comment 28

[Xiaobin Lu]

Beard:
    Could you let me know how do you think about this bug because I am also 
debugging to see what's wrong?
     Thanks! 

Comment 29

[Xiaobin Lu]

Beard:
     Would you mind updating the status of bug 63466? I noticed that you are now
doing investigation to this bug. I would like to let you know some of my
observation:
     * JSObject.getWindow(applet) actually does not return null even though I
saw it is null from Java console. I think there must be something wrong with
java console. I verified it both in plugin side and in the application.
    
     * native implementation of JSObjectCall ( when it calls
JSObject.call("showAlert","...")) actually dispatch the JS call from the current
java thread  to the browser's javascipt thread to carry the call action.Actually
the below is what I am debugging result:
     
     Dispatch_JSObjectCall (java plugin side native code)--->
nsJVMManager::PostEvent((PRUint32)jsThreadID,pIRunnable,FALSE)--->nsEventQueueImpl::PostSynchronous(runnableEvent,nsnull)--->PL_PostSynchronousEvent(nsEventQueue,
aEvent)-->PL_PostEvent(self,event)->__pl_NativeNotify(PLEventQueue* self):here
it use window system call PostMessage to deliver the event.

   Actually something should happen after this, but nothing happens. I suspect
that the event was lost. How do you think about this?

Comment 30

[Xiaobin Lu]

 Beard:
     I applied your patch and tested in my build and it works now. Thanks for 
your patch!

 Tom:
     You also proposed a nice patch and it also works in my build now(last 
time, I told you it does not work because I made a mistake, sorry!).

    I think Beard's patch is workable. I wish someone can confirm this for me 
and if it is OK, I suggest to check it in.

Comment 31

[Judson Valeski]

r=valeski on patrick's 01/25/01 16:10 patch.

Comment 32

[Xiaobin Lu]

Hi Patrick:
   Would you mind to get this patch approved and checked into the trunk as soon 
as possible because I can see that patch is especially to make livecoonect work 
from Java to JS side. I know you are very busy and we would be greatly 
appreciate you help to this bug!

   Thanks again!

Comment 33

[Brendan Eich [:brendan]]

Can we get this bug targeted?

/be

Comment 34

[Patrick C. Beard]

I am within striking range of fixing JSObject.eval(), and will submit an 
additional patch hopefully later today. So, I'd definitely consider targeting 
this for 0.8.

Comment 35

[gary.kind]

Patrick, if you are fixing JSObject.eval, there is another bug I filed where
JSObject.getMember() does not work either.  Perhaps you can fix this at the same
time?  That would seem to be easier.

Comment 36

[Patrick C. Beard]

OK, what follows is a patch that touches OJI, Caps, and LiveConnect.

The changes to OJI include the previous patch, which fixes registration of the 
NS_CLIVECONNECT_CID class, but also includes new callback code to convert an 
nsISecurityContext to a JSPrincipals object, using the nsIScriptSecurityManager 
service (thanks Mitch!). With this new code, a Java applet can call 
JSObject.getWindow(this).eval("alert('foo')"), and it works correctly.

The changes to Caps are quite simple, and amount to short circuiting the same-
origin test in nsCodebasePrincipal::Equals() for URIs with scheme == "file". 
Mitch removed this short-circuit test a while back, and it turns out that it is 
necessary to work around a bug in nsStdURL (see bug # 68335).

Finally, the changes to LiveConnect amount to changing the type of the 
pNSISecurityContext parameter passed to most of the nsILiveconnect methods, from 
void* to nsISupports*. This is to imply that the parameter can be QueryInterfaced 
to be whatever is appropriate to the OJI callbacks. For now, this parameter is 
only going to be an nsISecurityContext, but the changes to OJI allow for a newer 
OJI plugin to pass an nsIPrincipal directly, which will simplify the 
implementation. In addition, there is some cleanup of unused variables, and the 
addition of the JSJ_RegisterLiveConnectFactory() that was in the previous patch.

Please review and approve for Mozilla 0.8 checkin.

Comment 37

[Patrick C. Beard]

Attachment: Grand unified patch to fix LiveConnect problems

Comment 38

[Christopher Blizzard (:blizzard)]

Is this a 0.8 patch?  I don't see any reviews or anything yet.  We need to get
on this so we can get it into 0.8.

Comment 39

[Patrick C. Beard]

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

Comment 40

[Patrick C. Beard]

Attachment: Regenerated patch with -u for easier comprehension.

Comment 41

[Christopher Blizzard (:blizzard)]

This looks fine to me but I'm just a layman here.  Mitch + brendan should really
look at this too just to make sure.

Comment 42

[Brendan Eich [:brendan]]

Code looks ok to me.  A couple of thoughts, maybe spin-off bugs:

- Should the caps origin check use PL_strcasecmp?  For that matter, could it use
gagan's souped-up SchemeIs method instead?  mstoltz?

- The nsISecurityContext::GetOrigin method (maybe others, I didn't check) is
highly non-XPCOM and bogus, wherefore the 512-byte buffer passed in by the
patch.  Can't it use a property out string (IDL speak; char ** in C++) param? 
Or can we really fix the length of an origin?

/be

Comment 43

[Mitchell Stoltz (not reading bugmail)]

r=mstoltz

Comment 44

[Brendan Eich [:brendan]]

sr=[e-mail]

beard, leaf is gonna branch soon.  Please get this one, and the MRJ fix if you
get it r='d and you answer my questions about code details, in the trunk first.

/be

Comment 45

[Joe Chou]

Will the interface changes in nsILiveconnect.h be affecting Java plugin?

Comment 46

[Xiaobin Lu]

Yes, nsILiveconnect.h is used by java plugin side and I think we'd better to 
add another interface like nsILiveconnect2.h instead of changing the current 
interface file.

Comment 47

[Patrick C. Beard]

The change to nsILiveconnect is binary compatible. The type of the parameter 
before was void* now it's nsISupports*.

Comment 48

[Joe Chou]

I ran a simple test case with the code below:

try1.h:

#ifndef TRY1_H
#define TRY1_H
void try1(void *p);
#endif // TRY1_H

try1.cpp:

#include <stdio.h>
#include <stdlib.h>
#include <strings.h>
#include "try1.h"

void try1(void *vp)
{
        char *sp;

        sp = (char*)vp;
        printf("%s\n", sp);
}

int main()
{
        char s1[50];

        strcpy(s1, "Hello, there.");
        try1(s1);
        exit(0);
}

It was compiled OK and ran OK. But if I change the parameter type of try1 from
void* to char* in the prototype of try1.h:

...
void try1(char *p);
...

Then when compiled again, I got undefined symble, try1(char *), error.


The test case may not be saying much, but do we really need to touch the
interface here?

Comment 49

[Patrick C. Beard]

The reason it isn't linking is because of type-safe linkage. This doesn't apply 
with interfaces. I wasn't making an argument that this would be source 
compatible, simply that it IS binary compatible, since the OJI module currently 
statically casts a void* parameter to be an nsISecurityContext*. So, we're 
gaining in type-safety with this change, but not committing to which interface is 
being passed.

Comment 50

[Patrick C. Beard]

Changes landed on trunk. Will land on 0.8 branch tomorrow.

Comment 51

[Patrick C. Beard]

Note the fix for this also involves changing the usage pattern of the 
nsILiveconnect interface from a singleton service, to a one nsILiveconnect 
instance per plugin instance. For examples of this usage, see:

http://lxr.mozilla.org/seamonkey/source/plugin/oji/MRJ/plugin/Source/
LiveConnectNativeMethods.cpp#205

Comment 52

[Asa Dotzler [:asa]]

I see this was checked into the branch. rmoving status whiteboard 0.8 noise.  
Was bu 68338 also fixed on the branch?

Comment 53

[Asa Dotzler [:asa]]

sorry for all the spam, cleaning up whiteboard, patches for both of these bugs
did make it into the 0.8 branch.

Comment 54

[Patrick C. Beard]

Bug is now fixed.

Comment 55

[stanley.ho]

> Note the fix for this also involves changing the usage pattern of the 
> nsILiveconnect interface from a singleton service, to a one nsILiveconnect 
> instance per plugin instance. For examples of this usage, see:

This change will definitely break the Java Plug-in on all platforms because we 
assume nsILiveConnect is a singleton and cache it around. What's the side 
effect if we use this cached nsILiveConnect instead of getting a new one per 
plugin instance?

Comment 56

[shrirang khanzode]

testcase works now on the windows trunk and 0.8 branch builds( 0219, 0215 
respectively). marking VERIFIED !

Comment 57

[Joe Chou]

Patrick, since the Java plugin does not have a chance to update its code to
adapt the change of nsILiveconnect instance vs. plugin instances(from 1-to-many
to 1-to-1), and the code freeze date for the up coming Java plugin release is
the end of this week. To avoid the potential problem (as Stanley explained early
in the comment above that currently Java plugin is expecting the 1-to-many
case), is it possible to only back out the 1-to-1 change of nsILiveconnect
instance in the patch for now, and put it back along with Java plugin's update
later?