74485 - nsIPlugin::Shutdown called too often

Status: VERIFIED FIXED

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

Description

[sean echevarria]

Due to the fix for bug http://bugzilla.mozilla.org/show_bug.cgi?id=73071, 
nsIPlugin::Shutdown() is now called multiple times for plugins.  

It is first called when the activePluginList is being destroyed.  It is then 
called by the patch for bug 73071 in nsPluginHostImpl::Destroy.

First time:
BeatnikPlugin::Shutdown
nsActivePluginList::remove
nsActivePluginList::shut
nsPluginHostImpl::Destroy
nsPluginHostImpl::Observe
nsObserverService::Notify
NS_ShutdownXPCOM
main
mainCRTStartup()

Second:
BeatnikPlugin::Shutdown
nsPluginHostImpl::Destroy
nsPluginHostImpl::Observe
nsObserverService::Notify
NS_ShutdownXPCOM
main
mainCRTStartup

The new PR_UnloadLibrary is also a problem.  PR_UnloadLibrary is explicitly 
called from nsPluginHostImpl::Destroy() and then again from the dtor of 
mPlugins when nsPluginHostImpl::Destroy() calls delete mPlugins.

Comment 1

[sean echevarria]

Maybe the problem with bug 73017 is in nsPluginHostImpl::StopPluginInstance() 
where uncached or old school plugins are unloaded without a call to Shutdown:

      if(lastInstance)
      {
+  // XXX need to call pluginTag->mEntryPoint->Shutdown() here?
        pluginTag->mEntryPoint = nsnull;
        if ((nsnull != pluginTag->mLibrary) && pluginTag->mCanUnloadLibrary)
        {
          PR_UnloadLibrary(pluginTag->mLibrary);
          pluginTag->mLibrary = nsnull;
        }
      }

Comment 2

[sean echevarria]

scratch the last comment - mActivePluginList.remove(plugin, &lastInstance) does 
the right thing.

Under what circumstances is nsIPlugin::Initialize() called without any 
nsIPluginInstances getting created?  That's the cause of bug 73071, correct?  
Maybe nsPluginHostImpl::SetUpPluginInstance() needs to be modified to Shutdown 
and Release the plugin factory if it created and initialized it but then failed 
to create a pluginInstance?

Comment 3

[av (gone)]

This was added by request of Patric Beard. From what he told me I learnes that 
the MRJ plugin on Mac doesn't create instances. It is not that it failed to 
initialize during SetUpPluginInstance, it is just how it works.

I'll try to get it fixed as soon as I can. If this is blocking you now just 
comment out this shutdown line in nsPluginHostImpl::Destroy which I added in 
the mPlugin list demolition loop.

Comment 4

[av (gone)]

Attachment: possible fix

Comment 5

[av (gone)]

Sean, this patch should fix it. I basically moved last instance business so it 
is hidden in nsActivePluginList::remove. Now it will take care of zeroing 
nsPluginTag's mEntryPoint and mLibrary members, so they will not be used more 
than once.

I did not try it yet. Will do tomorrow as soon as I get back in the office.

Comment 6

[sean echevarria]

I'll look at it tomorrow also - agree it's best to handle shutdown, unload, etc
from one place.

Comment 7

[sean echevarria]

Attachment: updated patch for nsPluginHostImpl.cpp

Comment 8

[av (gone)]

Why do you think it is not safe to have shut() in the destructor? If it all goes 
by scenario this shut() is not going to do anything and thus is not needed. But 
why not to have it just in case as a bulletproofing mechanism for potential 
bugs?

Comment 9

[sean echevarria]

Attachment: one more time

Comment 10

[sean echevarria]

The original patch does not compile.  One problem is that shut() now takes an 
argument and the argument, mPlugins, is not accessible from nsActivePluginList.

Added a new patch that fixes nsIPlugin refcnt leak.  It isolates the Shutdown, 
Release and PR_UnloadLibrary calls to a single new method - 
nsPluginTag::ShutdownPluginModule.

Comment 11

[av (gone)]

Ah! You just got me. This is right thing and I was just going to implement it. 
I'll take a look.

Comment 12

[av (gone)]

Attachment: final candidate -- first try

Comment 13

[av (gone)]

I attached the patch which I think could be what we want. I tried to make the 
code cleaner and all shutdown related logic clearer so the code should look less 
messy now.

Sean, Peter, please apply and let it run to see I didn't introduce any 
regressions and the actual bug is really fixed. You will probably need to update 
the tree before apply the patch.

Comment 14

[sean echevarria]

This line (1904) can be removed:
      p->TryShutdownPlugin();
since the next line calls TryShutdownPlugin:
      delete p;


I'm hitting this assert pretty often at shutdown:
    NS_ASSERTION(!unloadLibLater, "Plugin doesn't want to be unloaded");


It seems like the new (but not new in this patch) mXPConnected vars are only 
useful for dealing with 4x plugins.  It looks like that's the reason I'm 
getting the assert at shutdown.

XPCOM plugins are loaded by the componentManager, so it's safe for the plugin 
host to 'unload' xpcom libraries it has 'loaded' (since the loads and unloads 
are refcnted).

Comment 15

[av (gone)]

>This line (1904) can be removed:
>      p->TryShutdownPlugin();

Right, I noticed that after I posted the patch.

As to unloading asserion, I'll do it so it will do the 'safe' unload only for 
4.x plugins. Thanks for pointing this out.

Comment 16

[av (gone)]

OK, to reflect Sean's comment one can just quickly do the following:

In nsPluginHostImpl::ReloadPlugins

-  if(IsRunningPlugin(p))
+  if(IsRunningPlugin(p) && (p->mFlags & NS_PLUGIN_FLAG_OLDSCHOOL))

-  p->TryShutdownPlugin();

In nsActivePluginList::remove

-  if(pluginTag->mXPConnected)
+  if(pluginTag->mXPConnected && (pluginTag->mFlags & NS_PLUGIN_FLAG_OLDSCHOOL))

Comment 17

[av (gone)]

Attachment: final candidate -- second try

Comment 18

[av (gone)]

Full patch with those small chages above. Now latering library unload is 
strictly limited to the 4.x style plugins.

Sean, if you have a minute would you please do the following with your plugin 
and see if everything goes fine:
0. have npnull32.dll -- the default plugin
1. have more than one plugin running on a page (with xpconnect in use)
2. go to any other page
3. come back
4. type javascript:navigator.plugins.refresh(1)
5. remove the actual plugin dll 
6. type javascript:navigator.plugins.refresh(1)
7. get the dll back
8. type javascript:navigator.plugins.refresh(1)

Comment 19

[sean echevarria]

The latest patch is looking good.  I tried your steps, but it is impossible to 
remove the plugin dll since it is a component.  The component Manager owns 
references to its nsIModule and nsIFactory.

javascript:navigator.plugins.refresh(1) does stop and destroy all plugin 
instances.  The library and nsIPlugin references are released by the plugin 
host but since the component manager still has a hold of the module and 
factory, the dll is not actually unloaded.

Comment 20

[sean echevarria]

One more note about javascript:navigator.plugins.refresh(1): although this 
causes the plugin host to stop and release plugin instances, the instances 
themselves are not destroyed because the docshell sometimes continues to hold 
references to xpconnected plugin instances.  They get garbage collected - 
sometimes on page transitions and sometimes at shutdown.

Comment 21

[av (gone)]

OK, so looks like everything works as expected. When you feel comfortable with 
this patch, would you give me your r= ?

Comment 22

[sean echevarria]

Takes care of the issue and makes plugin management cleaner.  Just noticed that 
in nsActivePluginList::removeAllStopped(), these lines can be removed since 
prev isn't used anymore:
  nsActivePlugin * prev = nsnull;
    prev = p;

r=[e-mail].  Might want to get Patrick to take a look too.

Comment 23

[av (gone)]

Attachment: final candidate -- third try, sean's previous comment taken

Comment 24

[shrirang khanzode]

Is this problem causing the plugin to continuep playing even when I leave the 
page on *mac* ? Or would this be a seperate problem ? Pls let me know.

Comment 25

[sean echevarria]

>Is this problem causing the plugin to continuep playing even when I leave 
>the page on *mac* ? Or would this be a seperate problem ? Pls let me know.

No - not related.  Browser support of full-page plugins on mac is very new.  
Sounds like the plugin is not getting a nsIPluginInstance::Stop call from the 
plugin host.

Comment 26

[Chris Waterson]

sr=waterson

Maybe rename ``unloaded library list'' to ``unused library list'',
``TryShutdownPlugin()'' to ``TryUnloadPlugin()''.

Comment 27

[av (gone)]

Attachment: final candidate -- forth try, waterson suggestions taken, more comments in/out

Comment 28

[av (gone)]

Checked in.

Comment 29

[sean echevarria]

vrfy