66413 - LiveConnect JavaScript to Java causes trap when closing window

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Thomas Lendacky]

From Bugzilla Helper:
User-Agent: Mozilla/4.76 [en] (WinNT; U)
BuildID:    0000000000

Start the browser, open a new window and then using the included test case, 
JavaScript to Java LiveConnect causes a trap in during JavaScript context 
cleanup.  During the processing of the nsJSContext destructor, a call is made 
back to the nsJSContext being destroyed which more often than not results in a 
trap.  I've checked this on Windows NT and it occasionally causes the browser to 
hang.  Here is the call trace for the problem:

When destroying the window that was created the nsDocShell::Destroy method is 
invoked.  In this method, the class variable mScriptContext is set to null.  
This causes the RefCnt to go to zero and causes the nsJSContext object to be 
deleted.  The object deletion begins with nsJSContext::~nsJSContext 
(mozilla/dom/src/base/nsJSEnvironment.cpp).  During the context destruction, a 
method of the nsJSContext object that is being deleted is invoked (because of 
the use of nsCOMPtr).  Sometimes the method is invoked successfully only to have 
an assertion
generated and sometimes the invocation of the method results in a call to 
address 00000000.  The stack trace is as follows:

~nsJSContext invokes JS_DestroyContext (mozilla/js/src/jsapi.c)
invokes js_DestroyContext (mozilla/js/src/jscntxt.c)
invokes js_ForceGC (mozilla/js/src/jsgc.c)
invokes js_GC (mozilla/js/src/jsgc.c)
invokes js_FinalizeObject (mozilla/js/src/jsobj.c)
invokes JavaObject_finalize (mozilla/js/src/liveconnect/jsj_JavaObject.c)
invokes remove_java_object_reflection_from_hashtable 
(mozilla/js/src/liveconnect/jsj_JavaObject.c)
invokes jsj_HashJavaObject (mozilla/js/src/liveconnect/jsj_utils.c)
invokes CallStaticIntMethod (mozilla/modules/oji/src/ProxyJNI.cpp)
invokes InvokeStaticMethod (mozilla/modules/oji/src/ProxyJNI.cpp)
invokes InvokeStaticMethod (mozilla/modules/oji/src/ProxyJNI.cpp)
invokes CallStaticMethod (CSecureJNIEnv.cpp in OJI Plugin)
invokes SecureCallStaticMethod (remotejni.h in OJI Plugin)
invokes jni_SecureCallStaticMethod (remotejni.cpp in OJI Plugin)
invokes getAndPackSecurityInfo (remotejni.cpp in OJI Plugin)
invokes GetOrigin (mozilla/modules/oji/src/nsCSecurityContext.cpp)
invokes JS_GetContextPrivate (mozilla/js/src/jsapi.c)

JS_GetContextPrivate returns the nsJSContext object that is being destroyed and 
assigns it to an nsCOMPtr<nsIScriptContext> variable.  One of two things happens 
in this situation:

1. nsCSecurityContext.cpp uses an nsCOMPtr to contain the nsJSContext 
   returned by JS_GetContextPrivate.  Invoking the AddRef function
   results in a call to address 00000000 because it looks like some of 
   the function table information has already been trashed.

or

2. The assignment to the nsCOMPtr is successful, but an assertion is
   generated because the nsJSContext object does not have a global
   object (scriptContext->GetGlobalObject() returns null).

I don't know if this is the proper resolution, but if ~nsJSContext were to null 
out the private context data (JS_SetContextPrivate method) before all the Java 
reflection cleanup is performed, the trap and the assertion are not generated.


Reproducible: Sometimes
Steps to Reproduce:
1. Start the browser (be sure to have the Java plugin installed)
2. Create a new window (File -> New Navigator Window)
3. Open the supplied HTML file
4. Click on the Test and Test1 buttons
5. Close the newly created window

Actual Results:  The browser traps.

Expected Results:  The window closes.

OS/2 tends to show these problems more than Windows.  I've included the call 
trace to show what is occurring because this does not consistently occur on 
Windows.  The call trace shows what is happening - the assignment of an 
nsJSContext object to the nsCOMPtr<nsIScriptContext> variable in 
nsCSecurityContext.cpp, the same nsJSContext whose destructor is running.

Comment 1

[Thomas Lendacky]

Attachment: HTML Testcase

Comment 2

[Thomas Lendacky]

Attachment: Java Testcase

Comment 3

[Thomas Lendacky]

Attachment: Compiled Java Testcase

Comment 4

[Thomas Lendacky]

Attachment: Java Testcase (second applet)

Comment 5

[Thomas Lendacky]

Attachment: Compiled Java Testcase (second applet)

Comment 6

[Thomas Lendacky]

I've created a fix for this problem.  It removes the nsJSContext object from the 
JSContext during destruction.  Can you please review and let me know if this is 
acceptable?  I've tested it and haven't seen any problems that I'm aware of.

I'm attaching a diff for mozilla/dom/src/base/nsJSEnvironment.cpp

Comment 7

[Thomas Lendacky]

Attachment: Diff of nsJSEnvironment.cpp

Comment 8

[Phil Schwartau]

cc'ing Brendan, jband on this one - 

Comment 9

[Phil Schwartau]

cc'ing mstoltz, jst, and Jeff - not sure of the correct component for this one.
 

Comment 10

[John Bandhauer]

This sounds reasonable to me. But I haven't looked close or tested it.

nsJSEnvironment.cpp is in jst's module. I'd say he should verify the fix and 
check it in.

I think it would be wise to look at all the callers of JS_GetContextPrivate to 
see that none of them are doing anything stupid if it is null...
http://lxr.mozilla.org/seamonkey/search?string=JS_GetContextPrivate

Comment 11

[Thomas Lendacky]

I took a look through LXR and found the following callers that are not checking 
for null returned from JS_GetContextPrivate:

  mozilla/silentdl/nsJSSilentDownloadTask.cpp
  mozilla/dom/src/html/nsJSHTMLBlockquoteElement.cpp
  mozilla/layout/html/document/src/nsHTMLDocument.cpp
     which calls into:
       mozilla/dom/src/html/nsJSHTMLDocument.cpp

I don't know if these are areas that we need to worry about or not.  Could the 
routines in these files get called during nsJSContext destruction?

Comment 12

[Phil Schwartau]

Since this is an embedding issue rather than JS Engine, sending this to 
DOM Level 0; although it looks like other components are involved, too - 

Comment 13

[Xiaobin Lu]

Thomas:
   Could you make your fix super reviewed and got approved as soon as possible?
   Thanks!

Comment 14

[Thomas Lendacky]

Xiaobin, what do I need to do to get that to happen?

Comment 15

[Johnny Stenback (:jst)]

I haven't had a chance to dig into this yet but from what I can tell from
looking at the patch this could be checked in as is. Out of the
JS_GetContextPrivate() callers that don't check for null (thanks for collecting
the list, Thomas) nsJSSilentDownloadTask.cpp and nsHTMLDocument.cpp are the only
ones that could need null checks, the other caller are either not even build at
this point or they're about to be removed form mozilla. Could someone file
separate bugs on the two callers that need null checks (file the
nsHTMLDocument.cpp one against me and the other one against the silent download
component)?

I'm all for getting this cheked in and if someone is willing to do the actual
checkin then go for it, if not I will do it once I get sr= for this. r&a=jst if
someone wants to check this in, brendan, sr=?

Comment 16

[John Bandhauer]

sr=jband

Comment 17

[Johnny Stenback (:jst)]

Thanks John, so I'll try to land this later today unless someone beats me to it :-)

Comment 18

[Johnny Stenback (:jst)]

Fix checked in.

Comment 19

[Xiaobin Lu]

Thanks, Johnny!

Comment 20

[Prashant Desale]

Verified with 2001-020608.