nsIPlugin::Shutdown called too often

VERIFIED FIXED in mozilla0.9

Status

()

Core
Plug-ins
P2
normal
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: sean echevarria, Assigned: av (gone))

Tracking

Trunk
mozilla0.9
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: regression, URL)

Attachments

(7 attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
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;
        }
      }
(Reporter)

Comment 2

17 years ago
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?

(Assignee)

Comment 3

17 years ago
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.
Status: NEW → ASSIGNED
OS: Windows 2000 → All
Priority: -- → P2
Whiteboard: regression
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 4

17 years ago
Created attachment 29571 [details] [diff] [review]
possible fix
(Assignee)

Comment 5

17 years ago
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.
(Reporter)

Comment 6

17 years ago
I'll look at it tomorrow also - agree it's best to handle shutdown, unload, etc
from one place.
(Reporter)

Comment 7

17 years ago
Created attachment 29629 [details] [diff] [review]
updated patch for nsPluginHostImpl.cpp
(Assignee)

Comment 8

17 years ago
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?
(Reporter)

Comment 9

17 years ago
Created attachment 29637 [details] [diff] [review]
one more time
(Reporter)

Comment 10

17 years ago
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.
(Assignee)

Comment 11

17 years ago
Ah! You just got me. This is right thing and I was just going to implement it. 
I'll take a look.
(Assignee)

Comment 12

17 years ago
Created attachment 29734 [details] [diff] [review]
final candidate -- first try
(Assignee)

Comment 13

17 years ago
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.
(Reporter)

Comment 14

17 years ago
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).
(Assignee)

Comment 15

17 years ago
>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.
(Assignee)

Comment 16

17 years ago
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))
(Assignee)

Comment 17

17 years ago
Created attachment 29758 [details] [diff] [review]
final candidate -- second try
(Assignee)

Comment 18

17 years ago
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)
(Reporter)

Comment 19

17 years ago
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.
(Reporter)

Comment 20

17 years ago
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.
(Assignee)

Comment 21

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

Comment 22

17 years ago
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=sean@beatnik.com.  Might want to get Patrick to take a look too.
(Assignee)

Comment 23

17 years ago
Created attachment 29840 [details] [diff] [review]
final candidate -- third try, sean's previous comment taken

Updated

17 years ago
Blocks: 65661

Comment 24

17 years ago
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.
(Reporter)

Comment 25

17 years ago
>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

17 years ago
sr=waterson

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

Comment 27

17 years ago
Created attachment 30488 [details] [diff] [review]
final candidate -- forth try, waterson suggestions taken, more comments in/out
(Assignee)

Comment 28

17 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Reporter)

Comment 29

17 years ago
vrfy
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.