Braden Reporter
	Description • 25 years ago

Per the docs, nsIPlugin::Initialize() should be called when a plugin is first
loaded into memory, and nsIPlugin::Shutdown() should be called when it is
unloaded. Neither of these events appear to be occurring.

	sean echevarria
	Comment 1 • 25 years ago

Doesn't happen on windows either.  changing platform to all.

	Patrick C. Beard
	Comment 2 • 24 years ago

This has significant ramifications for Java plugins. Nominating for nsbeta3.

	Marc Attinasi
	Comment 3 • 24 years ago

Andrei, what is the status of this bug? How serious and how involved to fix? 
Thanks.

	karnaze (gone)
	Comment 4 • 24 years ago

Adding nsbeta3+ to status field.

	av (gone) Assignee
	Comment 5 • 24 years ago

I looked thru the code and got an impression that as it currently goes xpcom 
plugins don't need to implement nsIPlugin at all. When it is first created in 
nsPluginHostImpl::SetUpPluginInstance the CreateInstance method of the 
ComponentManager takes nsIPluginInstance::GetIID(). Isn't this wrong? Shouldn't 
it be nsIPlugin::GetIID() here? We probably need to create a plugin instance 
here (not PluginInstance but rather an instance of nsPlugin) and then, on 
success, call plugin->CreateInstance.
Sean, Rusty, what do you thionk?

	sean echevarria
	Comment 6 • 24 years ago

Correct - the way things are right now, there doesn't need to be an nsIPlugin - 
just an nsIFactory (which nsIPlugin is derived from).

In my plugin factory I implement nsIPlugin just in case it gets asked for - but 
it never gets QI'd.  Once I noticed that Initialize and Shutdown were never 
called, I just added calls to them in my factory dtor and ctor - and made sure 
that multiple calls to them would be safe in case they ever do start getting 
called by the plugin host.

Regarding CreateInstance(), I think there are a few issues outstanding 
regarding multiple mimetypes, etc. that come into play here.  What you suggest 
was probably right when the API was first created (create a plugin, then call 
CreatePluginInstance on the plugin).  I think the issue of XPCOM plugin 
registration has clouded or strayed from what was originally intended (I think 
waterson even removed nsIPlugin from the sample plugin at 
http://lxr.mozilla.org/seamonkey/source/modules/plugin/test/npsimple.cpp during 
the xpcom plugin registration creation).

	lchiang
	Comment 7 • 24 years ago

per PDT: P3-P5 priority bugs changed from nsbeta3+ to nsbeta3- since we have
more important work to do for Seamonkey. If you disagree, please state your case
in the bug report and nominate for rtm. Thanks.

	karnaze (gone)
	Comment 8 • 24 years ago

Adding rtm+.

	Phil Peterson
	Comment 9 • 24 years ago

PDT marking [rtm-] since it isn't clear what the impact of not fixing this would 
be. 

	ekrock's old account (dead)
	Comment 10 • 24 years ago

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

	Kathleen :Brade
	Comment 11 • 24 years ago

removing target milestone (Future) in the hopes that this bug will be
re-evaluated and fixed for mozilla 1.0 or sooner.

This is particular annoying on Macintosh where 2 asserts show up (see blocking
bug #49510 I think) on every exit of the application.

	Peter Grandmaison
	Comment 12 • 24 years ago

The shockwave plugin isn't receiving this NPP_Shutdown() ... this is a crash 
when using the windows debug plugin, but cannot be confirmed to be the source 
of any problem with the release build.

	ekrock's old account (dead)
	Comment 13 • 24 years ago

Nom. nsbeta1 as this blocks 49510 which may cause crashes on shutdown. However,
Andrei, since 45009 is currently only believed to be the cause of shutdown
crashes (at worst) in the release build, if you run out of time, fix other
plug-in bugs before this one. The correctness of our plug-in support while
running is more important than embarrassing (but otherwise harmless) crashes on
exit.

	edburns
	Comment 14 • 24 years ago

bug=45009
author=edburns

This fix makes it so nsIPlugin::Shutdown() is called when the app exits.

I have found that nsIPlugin::Initialize was getting correctly called
after the plugin was loaded.  I have implemented this by adding a
parameter for nsCOMPtr<nsIPlugin> to AddInstanceToActiveList and the
nsActivePlugin ctor.  

I have also modified all the find* methods in nsActivePluginList to not
find plugins that do not want to be cached.

	av (gone) Assignee
	Comment 17 • 24 years ago

Ed, could you please give some verbal explanation? I do not really understand 
what is the code supposed to do in nsActivePluginList::shut(). 

1. I see shutSent array allocated but I do not see it being populated with any 
data before you first use it. 

2. nsActivePluginList is essentialy a list of nsIPluginInstance instances which 
may correspond to the same nsIPlugin object so mCount might be excessive for the 
size of the shutSent array. Probably cleaner solution would be to form a list 
rather than array.

3. I am not sure this is OK to call nsIPlugin->Shutdown() before we destroy all 
the plugin nsIPluginInstance objects. We call mInstance->Destroy() in 
nsActivePlugin::~nsActivePligin which happens after your proposed 
nsIPlugin->Shutdown(). Perhaps we should create the list of nsIPlugins _pending_ 
shutdown and do it after the for cycle.

	edburns
	Comment 20 • 24 years ago

Andrei Volkov wrote:

> Ed, could you please give some verbal explanation? I do
> not really understand what is the code supposed to do in
> nsActivePluginList::shut().

This code is supposed to ensure that each existing nsPlugin
instance receives a ::Shutdown() call once.  Granted, the
first iteration code did not do that.  The second iteration
code does.

> 1. I see shutSent array allocated but I do not see it being
> populated with any data before you first use it.

Good point, fixed in second iteration patch.

> 2. nsActivePluginList is essentialy a list of
> nsIPluginInstance instances which may correspond to the
> same nsIPlugin object so mCount might be excessive for the
> size of the shutSent array. Probably cleaner solution
> would be to form a list rather than array.

True, mCount may be excessive, but it's just a local pointer
array that gets quickly destroyed.  I don't think the added
complexity would be worth the memory savings.

> 3. I am not sure this is OK to call nsIPlugin->Shutdown()
> before we destroy all the plugin nsIPluginInstance
> objects. We call mInstance->Destroy() in
> nsActivePlugin::~nsActivePligin which happens after your
> proposed nsIPlugin->Shutdown(). Perhaps we should create
> the list of nsIPlugins _pending_ shutdown and do it after
> the for cycle.

True, I have fixed this in the second iteration patch.

Please review the second iteration patch.

	av (gone) Assignee
	Comment 21 • 24 years ago

Couple of questions so far. 

1. what is the point in checking DoCache in find methods of nsActivePluginList? 
Currently, plugins that do not want to be cached are not present in the list 
anyway if they are not really running.

2. is it safe to call nsPluginHostImpl::Destroy from at least two places

3. if we do nsIPlugin::Shutdown in nsActivePluginList::shut (which makes sense), 
should we remove it from nsPluginHostImpl::Destroy? And

4. we probably should do this in nsActivePluginList::remove too, here writing a 
new method something like nsActivePluginList::IsLastInstance would be useful

	edburns
	Comment 24 • 24 years ago

Av wrote:
> Couple of questions so far. 

> 1. what is the point in checking DoCache in find methods
> of nsActivePluginList? Currently, plugins that do not want
> to be cached are not present in the list anyway if they
> are not really running.

Looking at the current implementation of
AddInstanceToActiveList, it seems they are indeed in the
list.  The purpose is to have them be in the list, but not
be findable.

> 2. is it safe to call nsPluginHostImpl::Destroy from at
> least two places

Probably not.  I have addressed this in the third iteration
patch.

> 3. if we do nsIPlugin::Shutdown in
> nsActivePluginList::shut (which makes sense), should we
> remove it from nsPluginHostImpl::Destroy? 

Yes, I have done this in the third iteration patch.
However, I think you need to de-allocate the mPlugins list.
This is another bug.

> 4. we probably should do this in
> nsActivePluginList::remove too, here writing a new method
> something like nsActivePluginList::IsLastInstance would be
> useful

Do what in nsActivePluginList::remove()?

	av (gone) Assignee
	Comment 25 • 24 years ago

>Looking at the current implementation of AddInstanceToActiveList, it seems they 
>are indeed in the list. The purpose is to have them be in the list, but not be 
>findable.

Plugins that say don't want to be cached are removed from the list in 
StopPluginInstance. They are in the list _only_ if they are currently running.

>Do what in nsActivePluginList::remove()?

nsIPlugin::Shutdown(). In fact, this seems to be the only needed place to call 
shutdown because remove is called on each element in nsActivePluginList::shut.

	av (gone) Assignee
	Comment 27 • 24 years ago

Ed, please have a look at nsActivePlugin::remove method as I see it can be. 
Looks like we don't need modifications to nsActivePlugin::shut at all.

	edburns
	Comment 28 • 24 years ago

Ok that looks good. We can leave ::shut() unmodified and modify ::remove() and 
add ::IsLastInstance().  Av can you finish up on this: merge the patch, get sr= 
and check it in?  If not, I can do it.  It's up to you.  I'd just like to see 
it happen soon.

Thanks,

Ed

	av (gone) Assignee
	Comment 29 • 24 years ago

I'll be happy to do this. But there is still a thing you wanted in the fix which 
I don't fully understand -- plugins that don't want to be cached, see my comment 
on 2001-02-15 18:04 

	edburns
	Comment 30 • 24 years ago

> Plugins that say don't want to be cached are removed from the list in 
> StopPluginInstance. They are in the list _only_ if they are currently running.

If you can guarantee via an assertion that plugins are in the list only if they 
are running, then we can get rid of the modifications to the ::find*() methods.

Is that possible?

	av (gone) Assignee
	Comment 31 • 24 years ago

When we leave the page with running plugin we are always supposed to call 
nsPluginHostImpl::StopPluginInstance, we there is a check whether or not a 
plugin wants to stay in the active instance list, and if not, we do 
nsActivePluginList.remove. Isn't it sufficient garantee?

	av (gone) Assignee
	Comment 33 • 24 years ago

Ed, Peter, please review the forth iteration of the fix. I hope it will satisfy 
everybody. Peter, would be great if you apply and test this patch.

	Peter Lubczynski
	Comment 34 • 24 years ago

I'm having all sorts of stability problems with the "forth iteration" patch on 
Win32 (other platforms yet to try).

1) Tons of asserts. This one on closing a page with flash on it:
###!!! ASSERTION: This plugin is not supposed to be cached!: 'p->mStopped && 
!doCache', file S:\mozilla\modules\plugin\nglsrc\nsPluginHostImpl.cpp, line 502

2) Crashes upon exit in Shockwave.com or any game inside there.

3) Crashes when going from one page with a plugin to another or back to same one 
(Example: quicktime.com --> mozilla.org --> quicktime.com --> crash)

Haven't tried java. Where are the testcases for this bug? Am I missing something 
besides just applying the last patch?

	av (gone) Assignee
	Comment 35 • 24 years ago

Sorry, my fault. The assertions should use the reverse of the condition. Oh, 
man... You can just quickly change it in four ::find methods of 
nsActivePluginList, so p->mStopped && !doCache becomes !p->mStopped || doCache.

Now looking at crashes...

	av (gone) Assignee
	Comment 36 • 24 years ago

Peter, I cannot reproduce #3 in your previous comment.

	Peter Lubczynski
	Comment 38 • 24 years ago

Re-tested 5th iteration, looks good on Windows so far. Will continue to run with 
the patch and try Mac.

	av (gone) Assignee
	Comment 39 • 24 years ago

ALERT!!! I noticed that nsPluginHostImpl::~nsPluginHostImpl is no longer called 
on exit. Therefore, we don't have nsPluginHostImpl::Destroy called nor do we 
shut nsActivePluginList.

Anybody has an idea why?

	Peter Lubczynski
	Comment 40 • 24 years ago

Something is still holding on to it

	av (gone) Assignee
	Comment 42 • 24 years ago

The sixth attempt addresses crashes mentioned by peterl in his 2001-02-19 15:18 
comment. Please try.

	Peter Lubczynski
	Comment 43 • 24 years ago

6th patch works well on Windows. I could not reproduce any sort of crash. I will 
try on the Mac. This patch does NOT fix bug 58128 :( 

It would still be nice to run some regression tests as this change could effect 
a lot of plugins.

	edburns
	Comment 44 • 24 years ago

AV, for some reason, I can't get this patch to apply.  Can you please help me?

D:\Projects\trunk\mozilla\modules\plugin\nglsrc>patch -u -i 
nsPluginHostImpl.h.patch nsPluginHostImpl.h
patch -u -i nsPluginHostImpl.h.patch nsPluginHostImpl.h
patching file `nsPluginHostImpl.h'
Hunk #1 FAILED at 27.
Hunk #2 FAILED at 81.
Hunk #3 FAILED at 119.
Hunk #4 FAILED at 298.
Hunk #5 FAILED at 342.
5 out of 5 hunks FAILED -- saving rejects to nsPluginHostImpl.h.rej

	Peter Lubczynski
	Comment 47 • 24 years ago

Patch (#25660) is good on Mac. All checks out and Shutdown is properly being 
called when the browser shuts down.

It is still unclear if the Shutdown() should be called when "the last instance 
of a plugin is Destroyed." However, if the cache is set to 0 then, then shutdown 
should always be called?

	Peter Lubczynski
	Comment 48 • 24 years ago

Oh and r=peterl if the Java folks approve

	av (gone) Assignee
	Comment 49 • 24 years ago

peterl wrote:
> It is still unclear if the Shutdown() should be called when "the last instance 
> of a plugin is Destroyed."

The spec says:
'Communicator calls this function once after the last instance of your plug-in 
is destroyed, before unloading the plug-in library itself. Use NPP_Shutdown to 
delete any data allocated in NPP_Initialize to be shared by all instances of a 
plug-in.'

> However, if the cache is set to 0 then, then shutdown should always be called?

Again, only on the destruction of last instance. If we, say, have two embeds on 
the page and leave it, we deatroy instances one after another, and Shutdown on 
the nsIPlugin object is supposed to be called after the last instance is 
destroyed.

	av (gone) Assignee
	Comment 50 • 24 years ago

BTW, Peter, do you see nsPluginHostImpl::Observe reliably called?

	Peter Lubczynski
	Comment 51 • 24 years ago

Observe is being called. I like this patch. 

But, according to the spec, I don't think Shutdown is being called when the last 
instanstance is destroyed. I set a breakpoint in Shutdown() and visited a page 
with Flash. When I left the page, ns4xPlugin::Shutdown() was never called. I 
tried the same with two of the same plugin on the page, and it was not called. 
When I left the page, all instances should have been Destroyed and therefore 
Shutdown should have been called, if I am understanding the spec correctly?

	edburns
	Comment 52 • 24 years ago

I'm planning on trying this out today.

	av (gone) Assignee
	Comment 53 • 24 years ago

peter: when we leave the page we don't destroy the instance but rather cache it 
setting Stop flag. This is the whole point of having nsActivePluginList which is 
the list of plugin instance objects, some of them may be not running.

	edburns
	Comment 54 • 24 years ago

I have applied these files to my trunk build.  This looks good to me.  
r=edburns.

	edburns
	Comment 55 • 24 years ago

Av what's the status?

	av (gone) Assignee
	Comment 56 • 24 years ago

Waiting for sr from waterson.

	Chris Waterson
	Comment 57 • 24 years ago

You never set mDestroyed to PR_TRUE; I think you probably want to do that
immediately after checking in in nsPluginHostImpl::Destroy()? (Do you want to
botch-assert if somebody tries to destroy the nsPluginHostImpl twice? Or use the
nsPluginHostImpl after it's been destroyed?)

At a minimum, be sure to set mIsDestroyed, and sr=waterson

	av (gone) Assignee
	Comment 59 • 24 years ago

Checked in. Marking fixed.

	sean echevarria
	Comment 60 • 24 years ago

OUCH!!!!

XPCOM plugins now get wrapped with the 4x legacy classes because the call to 
GetPluginFactory moved up before the nsComponentManager::CreateInstance() 
call.  As implemented, GetPluginFactory() should only be called if the 
componentManager failed to create an xpcom plugin.

Looks like GetPluginFactory call was moved in order to get a nsIPlugin* to pass 
into the call to AddInstanceToActiveList().

Either need to move GetPluginFactory() back, or modify it so that it is xpcom 
savvy.  Looks like the call to 
   PR_FindSymbol(pluginTag->mLibrary, "NSGetFactory");
is not satisfactory for determining if a plugin module is an xpcom plugin or a 
4x plugin.

	sean echevarria
	Comment 61 • 24 years ago

bug 72017 has been opened to address the problems (in last comment) introduced 
by the patch for this bug.

	shrirang khanzode
	Comment 62 • 23 years ago

verif (stamp)