132325 - LiveConnect holds stale MRJ Plugin references

Status: RESOLVED FIXED

(Core Graveyard :: Java: OJI, defect)

Description

[Brian Nesse (gone)]

I'm not really sure if this belongs in LiveConnect or OJI. I believe both
components may be at fault to some extent, but the initial fault lies with the
plugin so OJI wins.

We have lifetime issues with the connection between the MRJ Plugin and
LiveConnect which are causing stale references to be maintained to deleted objects.

Steps to reproduce.

1) Go to chat.yahoo.com.
2) Enter a chat room.
3) Go back to the previous page (you have to click back twice to do this).
4) Select a different chat room.
-> Crash.

I am fairly certain that this only happens on the Mac, and under both OS 9 and
OS X (though Yahoo Chat doesn't actually work under OS 9, the code paths are the
same.)

Comment 1

[Brian Nesse (gone)]

Attempting to summarize somewhat briefly...

InstantiateEmbededPlugin (called to create the plugin when you enter the chat
room) causes InitLiveConnectSupport() to be called. This saves a copy of the MRJ
Plugin into a global. The companion function, ShutdownLiveConnectSupport(),
however is only called at application shutdown via TryUnloadPlugin().
Unfortunately the plugin is deleted when you leave the page.

The above commentary is somewhat of an over simplification as the object saved
is an MRJPlugin, and the object released is an MRJPluginInstance. I don't have
the stack in front of me, but the crash occurs when accessing deleted members
(of the PluginInstance I expect) which are assumed to be valid somewhere along
the calling chain (I believe Java_netscape_javascript_JSObject_getWindow() is
the initiator.)

Comment 2

[Brian Nesse (gone)]

After a lot of digging, it turns out that the stale reference exists in the list 
of plugin instances held by the plugin. This reference is not removed from the 
list because some of the JS->Java connectivity routines are not releasing 
addrefed plugin instances properly.

Comment 3

[Brian Nesse (gone)]

Attachment: OS 9 patch

Patch to fix the classic plugin leaks.

Comment 4

[Brian Nesse (gone)]

Attachment: OS X patch

Patch to fix the Carbon plugin leaks.

Comment 5

[Brian Nesse (gone)]

The OS X patch also contains some bulletproofing code to replace exception 
handing code that was ifdefed out.

cc'ing Tony so he can get his feet wet in this stuff by reviewing these patchs. 
:)

Comment 6

[Patrick C. Beard]

Comment on attachment 75436 [details] [diff] [review]
OS X patch

I'm not sure why the throwing of a NullPointerException code was commented out.
I'd rather throw an exception than fail silently. It would make the patch
smaller to just bring that code back. :)

Comment 7

[Patrick C. Beard]

Comment on attachment 75435 [details] [diff] [review]
OS 9 patch

sr=beard

Comment 8

[Brian Nesse (gone)]

Attachment: Revised OS X patch

Patch as per beard's comments.

Comment 9

[Steve Dagley]

Comment on attachment 76002 [details] [diff] [review]
Revised OS X patch

r=sdagley

Comment 10

[Steve Dagley]

Comment on attachment 75435 [details] [diff] [review]
OS 9 patch

r=sdagley

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 75435 [details] [diff] [review]
OS 9 patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 12

[Patrick C. Beard]

Comment on attachment 76002 [details] [diff] [review]
Revised OS X patch

Ooops, I don't believe GetCurrentInstance(env) bumps increases the reference
count of the plugin instance, so the NS_RELEASE in ~EvalMessage() isn't
correct. Perhaps we should write another GetCurrentInstance(jsobject applet)
that doesn't bump the refCount for consistency.

Comment 13

[Brian Nesse (gone)]

Oh crap...you're right. I was confused by the commented out call to
getPluginInstance in the line above.

>Perhaps we should write another GetCurrentInstance(jsobject applet) that
>doesn't bump the refCount for consistency.


I'm sorry, you lost me here. Are you suggesting that we should just call
GetCurrentInstance instead of getPluginInstance in the ..._getWindow function?

Comment 14

[Patrick C. Beard]

I'm saying write another version of GetCurrentInstance() that walks the 
list, and doesnt' do any reference counting at all.

Comment 15

[Brian Nesse (gone)]

Attachment: New OS X patch

I think this is what Patrick is looking for. :)

Comment 16

[Patrick C. Beard]

Comment on attachment 76824 [details] [diff] [review]
New OS X patch

sr=beard

Comment 17

[Brian Nesse (gone)]

Fix checked in for OS X.

Comment 18

[Patty Mac]

Brian, I just tested on Mac OS 10.1.4, MRJ: 1.0fc1, using branch build
(2002-04-29-05-1.0.0)

BSteps to reproduce.

1) Go to http://chat.yahoo.com.
2) Enter a chat room.
3) Go back to the previous page (you have to click back twice to do this).
Sometimes the browser quits by itself and most of the time, it hangs.
I couldn't able go to step#4: Select another chat room.
---------------------------
Tested on Mac OS 10.1.4,MRJ: 1.0fc1, using commercial trunk build
(2002-04-29-08-TRUNK), it works fine without crashing/hanging!
Brian, is this one fixed only on trunk build only? If like this, should I 
mark "verified" instead? Please let me know. Thanks!

Comment 19

[Brian Nesse (gone)]

There is no branch/trunk build for the MRJ Plugin. If indeed it fails on the
branch, and works on the trunk, that is a different bug.