104038 - Browser calls "Destroy" twice for some XPCOM plugins

Status: VERIFIED FIXED

(Core Graveyard :: Java: OJI, defect, P3)

Description

[Steven Katz]

The browser appears to be making two requests to the JavaPlugin to "Destroy" a
given applet.

To reproduce:

1) Install JavaPlugin
2) Go to any applet
3) Press the back button

Note that an error will be reported in the Java Plugin Console.

I have looked at this in a debugger and it is the result of the Browser calling
our Destroy method twice.

Comment 1

[Igor Nekrestyanov]

Works for me on Linux RedHat 6.2.
Could it be solaris-specific problem?

Comment 2

[Steven Katz]

When you say that it "works for you" what do you mean?  Do you see the error
reported in the Java Console?  That is the only observable effect outside of a
debugger.

Comment 3

[Igor Nekrestyanov]

ok, let me clarify this. I do following
  1. start ./dist/bin/mozilla www.mozilla.org
  2. open Java console and set it's debug level to 5
  3. load http://java.sun.com (both applets on this page work)
  4. press Back

I see no errors in java console. Moreover only two instances of
 each type of DESTROY, DELETE or QUIT events are reported.

My plugin version is 1.3.0_01.

Comment 4

[Steven Katz]

Ah, let me clarify as well.

This problem is with the 1.4 release of the Plugin/JRE currently in beta.  This
version uses a different scheme (based on nsIModule) to connect with the
browser.  I believe this scheme causes the browser to exersise different code
paths then the previous scheme (based on NSGetFactory).

Comment 5

[Peter Lubczynski]

I see this on Win32 1.4 JRE too. It looks like the plugin isn't setting
nsPluginInstanceVariable_DoCacheBool to true and therefore the second call to
nsIPluginInstance::Destroy(). If the plugin returns true for this variable, is
there a second call to destroy?

Andrei, this may be a bug on our side that effects all XPCOM plugins in general.
It may be helpful to see what's going on in the sample.

Comment 6

[Steven Katz]

What does this variable do?  When would we (the plugin) want to set it to true?

Comment 7

[av (gone)]

If I remember correctly it was introduced earlier by Sun people. The thing is 
that plugins may stay in memory after the page is left. This is by default for 
XPCOM plugins, right Peter? And this may be controlled by responding when 
browser calls NPP_GetValue asking for this variable.

Comment 8

[Peter Lubczynski]

The extra nsIPluginInstance::Destroy() is in ~nsActivePlugin(). Should we be
calling this here?

Comment 9

[av (gone)]

And the latter comes when we remove item from the nsActivePlugin list. This can 
happen without nsObjectFrame::Destroy so we still need instance::Destroy there. 
The same situation can also happen in nsPluginHostImpl::Destroy() when we do 
this in nsActivePluginList.stopRunning() if some conditions are met (those 
obscure variables are set) and right after this in nsActivePluginList.shut() 
which causes ~nsActivePlugin. So, looks like a bug on our side. Don't see now 
what the best way to fix it would be. Flag indicating the active instance has 
already been destroyed?

Comment 10

[Peter Lubczynski]

Hm....I have an idea. Would it work if we moved the call to
nsIPluginInstance::Destroy() from ~nsActivePlugin to the 'else' in stopRunning()
so it looks more like the same code segment in nsObjectFrame::Destroy()? It
should then be skipped when stopRunning() is called because mStopped would have
been set by StopPluginInstance() which is called right after first call to
nsIPluginInstance::Destroy() in nsObjectFrame.cpp. 

Actually, the single nsIPluginInstance::Destroy() in ~nsActivePlugin may be for
plugins that desire to be cached. If the above idea worked and a plugin
requested to be cached, the code path through nsObjectFrame::Destroy() would
cause us to never call nsIPluginInstance::Destroy(). Maybe in addition to the
above idea, also check nsPluginInstanceVariable_DoCacheBool and call
nsIPluginInstance::Destroy() from ~nsActivePlugin if set?

Comment 11

[Peter Lubczynski]

mine

Comment 12

[Peter Lubczynski]

Attachment: possible patch

Comment 13

[Peter Lubczynski]

Here's a possible patch to fix this, but I havn't tired it yet, especially with
1.3. Basically, we only call the Destroy() in the ActivePlugin destructor if
it's a cached plugin. In all other cases, we should have already done this
either in ObjectFrame.cpp if the plugin was made through layout or in remove()
if it's not.

Please test.

Comment 14

[Peter Lubczynski]

Comment on attachment 55596 [details] [diff] [review]
possible patch

wrong patch :-/

Comment 15

[Peter Lubczynski]

Attachment: possible fix

Comment 16

[Peter Lubczynski]

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

Comment 17

[av (gone)]

Comment on attachment 55597 [details] [diff] [review]
possible fix

r=av, I have tested with Real which also displayed the problem.

Comment 18

[Marc Attinasi]

Comment on attachment 55597 [details] [diff] [review]
possible fix

uh, OK. Seems a little kinky, but that's plugins! sr=attinasi

Comment 19

[Peter Lubczynski]

patch checked in, marking FIXED.

I double checked this in the debugger with the JRE 1.3.0, 1.3.1, and 1.4. I set
breakpoints on the Destroy() call sites and the instance was always destroyed,
only once, when the frame went away.

Comment 20

[Patty Mac]

Verified on trunk build (2001-11-09-08-trunk). There's no error in Java Console 
window.

Comment 21

[Judson Valeski]

brent, can you verify this (installing 1.3, 1.3.1, and 1.4b3)?

Comment 22

[Judson Valeski]

minusing for 0.9.4.

Steve, are you seeing this in the 1.4 windows plugin too?

Comment 23

[Steven Katz]

Yes.

Ref: 10-10-2001 posting by me