Calling top.window.close() from Java (via JSObject) does not work [REQUESTOR: Arun Ranganathan (aruner@netscape.com)]

RESOLVED INCOMPLETE

Status

Core Graveyard
Java: Live Connect
P1
normal
RESOLVED INCOMPLETE
18 years ago
7 years ago

People

(Reporter: Arun Ranganathan, Unassigned)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [oji_escalation] oji_working)

Attachments

(8 attachments)

(Reporter)

Description

18 years ago
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.
(Reporter)

Comment 1

18 years ago
Created attachment 29543 [details]
Java Source File -- Compile it!
(Reporter)

Comment 2

18 years ago
Created attachment 29544 [details]
HTML file
(Reporter)

Comment 3

18 years ago
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

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

Comment 5

18 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 6

18 years ago
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

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 7

18 years ago
Ro
Assignee: rogerl → xiaobin.lu
Status: ASSIGNED → NEW

Comment 8

18 years ago
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

18 years ago
Created attachment 30180 [details] [diff] [review]
patch for modules/oji/src/lcglue.cpp to fix this bug

Comment 10

18 years ago
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

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

Comment 12

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

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 13

18 years ago
To compile this, you need:

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

Comment 14

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

Comment 15

18 years ago
Xiaobin, I see that you asked Rogerl in the bug for sr.  Did you also email him 
and cc reviewers@mozilla.org?

Comment 16

18 years ago
I will do it!

Comment 17

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

Comment 18

18 years ago
Thanks Rogerl!

Comment 19

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

Comment 20

18 years ago
Use NS_REINTERPRET_CAST instead of straight old-style casts.

Comment 21

18 years ago
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.
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
This shouldn't change with the XPCDOM branch changes.

Comment 24

18 years ago
Created attachment 31338 [details] [diff] [review]
Use NS_REINTERPRET_CAST

Comment 25

18 years ago
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
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

18 years ago
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

18 years ago
Terry:
   Would you please test this in Mac?

Xiaobin

Comment 29

18 years ago
ok, gimme a few hours...

Comment 30

18 years ago
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

18 years ago
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

18 years ago
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

18 years ago
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

18 years ago
 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

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

Comment 36

17 years ago
  Will check Nikolay's patch to the trunk after liveconnect resumes working.
AFAIK liveconnect does work now.

Comment 38

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

Comment 39

17 years ago
Marking as one of the OJI group's hot bugs.
Whiteboard: [oji_escalation]

Updated

17 years ago
Summary: Calling top.window.close() from Java (via JSObject) does not work → Calling top.window.close() from Java (via JSObject) does not work [REQUESTOR: Arun Ranganathan (aruner@netscape.com)]

Comment 40

17 years ago
Adding dependency
Depends on: 77600

Updated

17 years ago
Priority: -- → P1

Comment 41

17 years ago
Updating status on this bug to indicate active working status on Xiaobin's part,
modulo it being blocked on 77600.
Whiteboard: [oji_escalation] → [oji_escalation] oji_working

Comment 42

17 years ago
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

17 years ago
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

17 years ago
Created attachment 37659 [details] [diff] [review]
Use JSContext from jsj_Exit_js avoid using applet_obj to get a Context

Comment 45

17 years ago
  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

17 years ago
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

17 years ago
Created attachment 37909 [details] [diff] [review]
New version of patch based on Patrick's suggestion

Comment 48

17 years ago
Created attachment 37910 [details] [diff] [review]
New version of patch based on Patrick's suggestions.

Comment 49

17 years ago
r=beard
sr=brendan@mozilla.org, 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

17 years ago
Created attachment 38101 [details] [diff] [review]
Comment has been changed

Comment 52

17 years ago
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

17 years ago
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

17 years ago
a= asa@mozilla.org for checkin to the trunk.
(on behalf of drivers)
Blocks: 83989

Comment 55

17 years ago
Fix has been checked in!

Comment 56

17 years ago
Please verify!
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 57

17 years ago
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? 

 
(Reporter)

Comment 58

17 years ago
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

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

Comment 60

17 years ago
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

17 years ago
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 -  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 62

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

Comment 63

17 years ago
  Since this bug now only exists on Mac. I would like to reassign to
beard@netscape.com and Move this to 0.9.3
Assignee: xiaobin.lu → beard
Status: REOPENED → NEW
Target Milestone: --- → mozilla0.9.3

Comment 64

17 years ago
Doesn't look like this is getting fixed before the freeze tonight.
Pushing out a milestone.  Please correct if I'm mistaken.
Target Milestone: mozilla0.9.3 → mozilla0.9.4

Updated

17 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Comment 65

17 years ago
--> Future
Target Milestone: mozilla0.9.5 → Future

Comment 66

12 years ago
-> default assignee for old netscape assigned bugs.
Assignee: beard → live-connect
Target Milestone: Future → ---

Updated

8 years ago
Component: Java: Live Connect → Java: Live Connect
Product: Core → Core Graveyard

Comment 67

7 years ago
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".
Status: NEW → RESOLVED
Last Resolved: 17 years ago7 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.