44667 - Incorrect assumption about lifetime of nsPluginInstancePeerImpl, which can lead to crash of Mozilla with some plugins on exit.

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Nikolay Igotti]

I'm bugfixing Java Plugin for Unix and have implemented thread-safe calls of
some Mozilla core functions (for example updating status string from plugin).
I've used for it interthread XPCOM proxy and have the following problem:
 when Mozilla exits and applet is running it coredumps. Simple debugging shows
that the problem is in modules/plugin/nglsrc/nsPluginInstancePeerImpl.cpp
nsresult nsPluginInstancePeerImpl::Initialize(nsIPluginInstanceOwner *aOwner,
                                                const nsMIMEType aMIMEType)
{
  //don't add a ref to prevent circular references... MMP
  mOwner = aOwner;

So, when shutdown is in progress, aOwner can be destroyed (as class doesn't hold
reference), but when applet is stopped (as part of shutdown process), it sets
status to applet stopped. Interthread proxy puts request to do call of
showStatus() into event queue of GUI thread and in some moment GUI thread tryes
to do the call, but fails. The problem is incorrect skipping of NS_ADDREF().
Problem with circular references should be solved in another manner.

Comment 1

[leger]

Adding crash keyword.

Comment 2

[Andreas Franke (gone)]

Setting to NEW. Well-written report. Needs to be looked at.

Comment 3

[ekrock's old account (dead)]

Not a Netscape 6 RTM blocker. FUTURE. This bug has been marked Future because
the Netscape engineer it is assigned to is overburdened.

Comment 4

[karnaze (gone)]

Changing milestone from future to m1.0.

Comment 5

[Mikhail Voitenko]

This bug is critical for the Sun One Webtop plugin.
The problem is that in case our plugin works in 'fullpage' mode
the browser crashes during the exit.
In the 'embedded' case all works ok.

The debugging shows that the problem should be the same for the all
Mozilla fullpage plugins. Because of the NS_ADDREF() absence
aOwner is destroyed as mentioned in the submitter description
and during the nsActivePlugin destruction it leads to the browser crash. 
The function call stack follows.

ns_if_addref(nsIPluginInstanceOwner * 0x03f91240) line 1195 + 15 bytes
nsPluginInstancePeerImpl::GetOwner(nsIPluginInstanceOwner * & 0x03f91240) line 
932 + 21 bytes
nsActivePlugin::~nsActivePlugin() line 344 + 46 bytes
nsActivePlugin::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsActivePluginList::remove(nsActivePlugin * 0x03f4bac0, int * 0x0012fc50) line 
494 + 28 bytes
nsActivePluginList::shut() line 403
nsPluginHostImpl::Destroy(nsPluginHostImpl * const 0x00e5d1d4) line 2778
nsPluginHostImpl::Observe(nsPluginHostImpl * const 0x00e5d1e0, nsISupports * 
0x004841c0, const unsigned short * 0x0012feb4, const unsigned short * 
0x00000000) line 4920
nsObserverService::Notify(nsObserverService * const 0x00d73720, nsISupports * 
0x004841c0, const unsigned short * 0x0012feb4, const unsigned short * 
0x00000000) line 238
NS_ShutdownXPCOM(nsIServiceManager * 0x00000000) line 457
main(int 1, char * * 0x00484550) line 1607 + 8 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903()

Comment 6

[av (gone)]

89672? Mikhail, is your plugin xpcom?

Comment 7

[Mikhail Voitenko]

We use Mozilla plugin API. 
We have some problems with xpcom because of our special build envinronment.
But it still possible to use it in case we really need it.

Comment 8

[Nikolay Igotti]

Hmm, I'm not sure that Michail's problem is exactly one described in original bug, as this bug aplies to any multithreaded plugin(full-page or embedded).But plugin lifecycle should be - cleaned up - specifiedAnyway workaround could be to add mAlive boolean var to nsActivePluginset it to PR_FALSE on destroy() of plugin and change test in nsActivePlugin::~nsActivePlugin() to if(mPeer && mAlive).

Comment 9

[av (gone)]

I think I addressed the issue in the patch to bug 89672. Please try it and 
comment.

Comment 10

[av (gone)]

Gentlemen, if everybody agrees this is a dup of bug 89672 I would mark it as 
such.

Comment 11

[Mikhail Voitenko]

Sorry for a delay.
Patch for the bug 89672 relates to this problem but does not help.
It fixes only
    modules/plugin/base/src/nsPluginInstancePeer.cpp
    modules/plugin/base/src/nsPluginViewer.cpp

Applying of the same fix for the following files helped
    modules/plugin/nglsrc/nsPluginInstancePeer.cpp
    modules/plugin/nglsrc/nsPluginViewer.cpp

Comment 12

[Peter Lubczynski]

The nglsrc directory has been removed from CVS on the trunk. Please update your
tree.

Comment 13

[Mikhail Voitenko]

Sorry!
Patch for the bug 89672 fixes our problem.
Thanks alot!

Comment 14

[av (gone)]

OK, marking fixed. Please redo if disagree.

Comment 15

[shrirang khanzode]

verif patch is in.