74482 - Calling top.window.close() from Java (via JSObject) does not work [REQUESTOR: Arun Ranganathan ([e-mail])]

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Java: Live Connect, defect, P1)

Description

[Arun Ranganathan]

2001031904 all the way up to 2001032804 exhibit this problem.
1. Using a Java Applet
2. Using netscape.javascript.* package
3. Using JSObject (netscape.javascript.JSObject)
4. Using Java Applet to invoke JavaScript (via JSObject)
5. Calling a function in JavaScript that simply does top.window.close()
End Result: Window does not close, unlike the behavior on IE 5.x and
Communicator 4.x.  NO security errors are thrown in the Java console.  No clues
given in JavaScript console.
Attachments to follow.

Comment 1

[Arun Ranganathan]

Attachment: Java Source File -- Compile it!

Comment 2

[Arun Ranganathan]

Attachment: HTML file

Comment 3

[Arun Ranganathan]

Essentially, you can't call top.window.close() from within Java (invoking
JavaScript function that calls top.window.close).  A workaround is to call a
JavaScript function from Java that sets a 'setTimeout("test()", 100).
Inside test() you can call top.window.close().  So this bypasses the quirky
behavior fairly easily, but if a fix is feasible, it ought to be put in :-)
I'll help in any way I can.
-- AKR

Comment 4

[edburns]

Hi Xiaobin, can you please take a quick look at this bug and see if you 
understand it?

Comment 5

[Xiaobin Lu]

Reporter:
   The browser should work without calling  setTimeout("test()",100), right? 
   How did you figure to use the setTimeout function to make this work?

Xiaobin

Comment 6

[Arun Ranganathan]

Hi edburns and xiaobin,
Firstly I'm really sorry if I was unclear.  I tried to be as lucid as possible 
:-(
Secondly, yes xiaobin, it should work without the setTimeout.  It was actually 
Marcio Galli (now on CC list) who figured out that you needed to use setTimeout 
-- the logic is as follows:
Using setTimeout transfers control (calling context) of the call 
'top.window.close()' over to a JavaScript function.  Without setTimeout, you're 
calling the method directly from Java and this doesn't work under any 
circumstances.  So using setTimeout is a workaround, and the customer seems to 
be happy chewing on it for the moment.  Ideally, you shouldn't have to use such 
workarounds to bypass the direct Java-JavaScript connection.  Try on 
Communicator 4.x without setTimeout -- it works, as it does on IE.  Workaround 
using setTimeout is only for Mozilla (and Netscape 6)
-- AKR

Comment 7

[Xiaobin Lu]

Ro

Comment 8

[Nikolay Igotti]

I found reason of this bug - it's pretty funny. In some cases (like window
closing) 
Mozilla DOM JS decides not to execute request immediatly, but deffer it instead. 
So to fix this bug we just need to add deffered call execution when Java JS
request completed. In attachment I'll add quick and dirty fix, which just calls
proper finalization request in OJI js_exit_impl().

Comment 9

[Nikolay Igotti]

Attachment: patch for modules/oji/src/lcglue.cpp to fix this bug

Comment 10

[edburns]

Xiaobin, can you please apply Igotti's patch and see if it works?  If it does,
let's get the approval process rolling.

Comment 11

[Xiaobin Lu]

  With this patch, it works with Apr 9's build.
  Ed, please review this patch. Thanks!

Comment 12

[Xiaobin Lu]

Rogerl:
   Would you like to give a super review to the patch? Thanks in advance!

Comment 13

[edburns]

To compile this, you need:

javac -g -classpath C:\jdk1.3.0_01\jre\lib\jaws.jar -d . Close.java

Comment 14

[edburns]

Tested the patch with win32 trunk.  It works.  ra=edburns.  (I'm the OJI Module 
owner).

Comment 15

[edburns]

Xiaobin, I see that you asked Rogerl in the bug for sr.  Did you also email him 
and cc [e-mail]?

Comment 16

[Xiaobin Lu]

I will do it!

Comment 17

[rogerl (gone)]

Sorry Xiaobin, but I don't feel at all qualified to super review this, I'm 
passing the buck to Patrick...

Comment 18

[Xiaobin Lu]

Thanks Rogerl!

Comment 19

[Patrick C. Beard]

Adding vidur and jst to ask for their input on this patch.

Comment 20

[Patrick C. Beard]

Use NS_REINTERPRET_CAST instead of straight old-style casts.

Comment 21

[vidur (gone)]

The patch should be fairly benign, but I'm concerned that all of this is going
to change when Johnny lands the XPCDOM branch in the next couple of weeks. I'll
make sure he reviews this and suggests an alternative (if one exists) for the
new DOM world.

Comment 22

[Johnny Stenback (:jst)]

Are we guaranteed that the JSContext's private data really is a Mozilla DOM
context (i.e. that the type of the private data really is nsIScriptContext *);?
Could we end up running a script through this path on a XPConnect context? Maybe
the assumption should be that the private data is nsISupports and relying on
QueryInterface() (with the help of an nsCOMPtr) to check that the private data
really is of type nsIScriptContext.

If the above is guaranteed (I see that the same assumption is made in other
places in this same file), this looks fine to me, calling ScriptEvaluated() on
the DOM JSContext after a script is executed is a good thing to do. Oh, and this
*is* always called on the main thread, right?

Assuming none of the above questions trigger required changes in this patch, sr=jst

Comment 23

[Johnny Stenback (:jst)]

This shouldn't change with the XPCDOM branch changes.

Comment 24

[Xiaobin Lu]

Attachment: Use NS_REINTERPRET_CAST

Comment 25

[Xiaobin Lu]

Johnny:
   Thank you for your suggestions! I found it is not going to be possible to 
use do_QueryInterface to get a nsIScriptContext pointer because the 
JS_GetContextPrivate returns a void*. So I changed it to use 
NS_REINTERPRET_CAST.
   Would you mind reviewing this patch and we are going to check this patch 
into trunk as soon as possible? Thanks in advance!

Xiaobin

Comment 26

[Johnny Stenback (:jst)]

Xiaobin, to use QI you should cast it nsISupports and then call the QI, that
would be safer, but still not bullet proof.

What you're doing in the patch is ok with me since we're doing the same thing in
one other place in the same file, sr=jst.

Comment 27

[edburns]

ra=edburns.  

I tried this on solaris, but when pressing exit, the dreaded 75070 manifested 
itself.  However, I'm confident that the fix for this bug is correct.

Ed

Comment 28

[Xiaobin Lu]

Terry:
   Would you please test this in Mac?

Xiaobin

Comment 29

[Terry Noyes]

ok, gimme a few hours...

Comment 30

[Xiaobin Lu]

Terry: 
   I noticed that there are something wrong with the JVM start now. Please see 
detail of bug 76677. Until that bug has been solved, I think it is good time to 
test and check in this patch.
   Thanks!

Comment 31

[Terry Noyes]

Using a pull from about 5pm, pushing the 'Exit' button gives me in the Java
Console an endless run of:
"JNI panic: JNI array element type mismatch
   at java.lang.Object.wait(Object.java:307)" and
"JNI panic: JNI array operation received a non array
    at java.lang.Object.wait(Object.java:307)"

whether I've patched lcglue.cpp and recompiled oji.shlb or not.  I'm not sure of
the value of this information as Mozilla itself seems REALLY shaky right now.  I
had lots of crashes and anomolies trying to put this together.

Comment 32

[Patrick C. Beard]

Are you using the proper <jni.h>? There's another bug that says the one that's in 
the tree is bogus. I've fixed my LiveConnect project's access paths to point at 
{Compiler}Mac OS Support:JNIHeaders: ahead of dist. I'll try this patch as well 
to verify your results.

Comment 33

[Terry Noyes]

No, I was using the standard build environment as documented on
http://mozilla.org/build/mac.html with the one exception: you have to create
that 'JNIHeaders' folder and place into it an alias to StdCLib to avoid breaking
the MRJPlugin.mcp build.

So I put an alias of the UnivHdrs jni.h (v1.30) into this JNIHeaders ?, removed
the jni.h from the tree, recompiled oji.mcp and the LiveConnect.mcp project
(because there are two) which creates LiveConnect.shlb.  Was that correct?

With the same results.  I should add that the "hi!" comes up and then the
endless string of JNI panics.

Comment 34

[Xiaobin Lu]

 This is what I can see from the current windows build. If you go to the 
testcase right away after you start the browser, the build with the patch will 
not work. However, after you visit a page with an applet like java.sun.com and 
then visit the testcase, it works. I think this is maybe the same thing as bug 
76677.

Comment 35

[Xiaobin Lu]

  I would like to check this into the trunk after some applet crash bug fixed 
when browser starts.

Comment 36

[Xiaobin Lu]

  Will check Nikolay's patch to the trunk after liveconnect resumes working.

Comment 37

[Johnny Stenback (:jst)]

AFAIK liveconnect does work now.

Comment 38

[Xiaobin Lu]

Thanks Johnny! Liveconnect will  resume working with 77600 has been fixed.

Comment 39

[drapeau]

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

Comment 40

[drapeau]

Adding dependency

Comment 41

[drapeau]

Updating status on this bug to indicate active working status on Xiaobin's part,
modulo it being blocked on 77600.

Comment 42

[Patrick C. Beard]

One problem with our architecture, is that a call like window.close() will cause 
the plugin instance to be destroyed while a reference to it is dangling in the 
nsCLiveconnect instance. OJI Java plugins need to ensure that the lifetime of an 
active plugin instance is made to be longer than any call into JavaScript, 
perhaps by grabbing an additional reference to the plugin instance when calling 
into nsILiveconnect interface methods.

In the future, the nsILiveconnect interface would do this, because it would 
reference the plugin as an nsISupports, but currently it only keeps an opaque 
pointer that we have to be very careful with. If we use this patch, we have to 
make some changes to our OJI plugin implementations to reflect this lifetime 
issue.

Comment 43

[drapeau]

Copying Java Plug-In folks as a heads-up for future changes needed (Jim,
Stanley, you'll find beard's comment just before this one informative and
important).

Comment 44

[Xiaobin Lu]

Attachment: Use JSContext from jsj_Exit_js avoid using applet_obj to get a Context

Comment 45

[Xiaobin Lu]

  In order not to use applet_obj to get JSContext, modify exit_js_impl to use a 
JSContext passed down from the caller (jsj_Enter_js & jsj_exit_js). The reason 
of avoiding using applet_obj to get JSContext is explained in Patrick's last 
comment.

Ed and Patrick:
   Would you please review the patch? Thanks!

Comment 46

[Patrick C. Beard]

Going back to what jst said about an earlier patch, what he really was getting at 
is that you should be certain that the pointer you are casting to be a 
nsIScriptContext* you should instead be more conservative about and instead cast 
to an nsISupports, and then use do_QueryInterface to make sure it truly is that 
interface. Here's how you'd rewrite the last part of the patch:

+    if (cx)
+    {
+        nsISupports* supports = NS_REINTERPRET_CAST(nsISupports*,
+                                                    JS_GetContextPrivate(cx));
+        nsCOMPtr<nsIScriptContext> scriptContext = do_QueryInterface(supports);
+        if (scriptContext)
+            scriptContext->ScriptEvaluated(PR_TRUE);
+    }

This is probably the most conservative way to write this. With this change, r=
beard.

Comment 47

[Xiaobin Lu]

Attachment: New version of patch based on Patrick's suggestion

Comment 48

[Xiaobin Lu]

Attachment: New version of patch based on Patrick's suggestions.

Comment 49

[Patrick C. Beard]

r=beard

Comment 50

[Brendan Eich [:brendan]]

sr=[e-mail], except this comment could be worded better:

+    // The main idea here is to process some scripts right away in JS stack
+    if (cx)

What's happened is that some JS has already just executed, and we are exiting
the JS engine back to Java.  In similar cases where the DOM code runs scripts,
it wants to call ScriptEvaluated after each such unit of execution, so the
LiveConnect code should do likewise.  The above comment makes it sound as though
the ScriptEvaluated call is processing some scripts, as in running them.  It's
not -- it is merely noting that one such script evaluation has just completed.

/be

Comment 51

[Xiaobin Lu]

Attachment: Comment has been changed

Comment 52

[stanley.ho]

Wouldn't it be easier to just AddRef()/Release() the plugin instance pointer in 
the nsILiveConnect implementation? Or are we saying that the plugin instance 
pointer is already invalid before calling nsILiveConnect?

Comment 53

[Xiaobin Lu]

Actually the nsCLiveconnect implementation use void* type to pluginInstance. 
Clearly it is a design misconsideratin(or not considering that far like 
situation about document.write()) so that the interface and data structure is 
not well defined. 

The pluginInstance is valid before calling Liveconnect. Anyway, this problem has 
already been addressed. 

Comment 54

[Asa Dotzler [:asa]]

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

Comment 55

[Xiaobin Lu]

Fix has been checked in!

Comment 56

[Xiaobin Lu]

Please verify!

Comment 57

[Phil Schwartau]

I get these errors when I try to compile the Java source file
at attachment (id=29543) above:


[ ] javac Close.java
Close.java:7: package netscape.javascript does not exist
import netscape.javascript.*;
^
Close.java:63: cannot resolve symbol
symbol  : class JSObject
location: class Close
        JSObject appletJS = null;
        ^
Close.java:81: cannot resolve symbol
symbol  : class JSObject
location: class Close
                JSObject js = null;
                ^
Close.java:89: cannot resolve symbol
symbol  : variable JSObject
location: class Close
                        js = JSObject.getWindow(this);
                             ^
4 errors



How do I get the package netscape.javascript to be seen?
Is this a CLASSPATH problem? 

 

Comment 58

[Arun Ranganathan]

Do something like this:

javac -classpath [path to java40.jar in Communicator installation OR path to
jaws.jar in Java 2 Plugin installation] fileName.java

Comment 59

[Phil Schwartau]

OK, that worked. Now I can pull the new code and verify the fix, thanks -

Comment 60

[Phil Schwartau]

I have to reopen the bug, because the testcase doesn't working on my 
Mac Mozilla binary. Here are the results on WinNT, Linux, and Mac. 
Using nightly trunk binaries 20010613xx in each case:

WinNT: testcase passes. When I click the Exit button, everything exits cleanly.
Linux: Same thing -
Mac:   When I click the Exit button, the button disappears, the cursor
       changes to the "wristwatch" icon, and Mozilla hangs forever -



Here is info I got from "about:plugins" on the Mac build:

File name: MRJPlugin
Runs Java applets using an <EMBED> tag. 
For more information, please visit the MRJ Plugin web site. 

Mime Type                                 Description        Suffixes   Enabled 
application/x-java-vm                    Embedded JVM          xjv        Yes 
application/x-java-applet                Embedded Java Applet  xja        Yes 
application/x-java-applet;version=1.1                          xja11      Yes 
application/x-java-applet;version=1.1.2                        xja112     Yes

Comment 61

[Phil Schwartau]

Notes:

1. I compiled the testcase on my WinNT box and emailed it to myself.
   That is how I tested it on my Linux box - successfully. I hope
   this is valid to do on the Mac as well -

2. After I load the testcase in Mozilla (but before I click the Exit
   button) the Mozilla status bar says "Applet Loaded". Whereas on WinNT 
   it says "Applet Close started", and on Linux it says "Applet started". 
   Just in case that is relevant to anything - 


Reopening bug -  

Comment 62

[Xiaobin Lu]

Phil:
   Can you recompile the source file in Mac and see it is OK or not?

Comment 63

[Xiaobin Lu]

  Since this bug now only exists on Mac. I would like to reassign to
[e-mail] and Move this to 0.9.3

Comment 64

[Blake Ross]

Doesn't look like this is getting fixed before the freeze tonight.
Pushing out a milestone.  Please correct if I'm mistaken.

Comment 65

[Patrick C. Beard]

--> Future

Comment 66

[Bob Clary [:bc] (inactive)]

-> default assignee for old netscape assigned bugs.

Comment 67

[Benjamin Smedberg]

Firefox code moved from custom Liveconnect code to the NPAPI/NPRuntime bridge a while back. Mass-closing the bugs in the liveconnect component which are likely invalid. If you believe that this bug is still relevant to modern versions of Firefox, please reopen it and move it the "Core" product, component "Plug-Ins".