45009 - nsIPlugin::Initialize() and ::Shutdown aren't called

Status: VERIFIED FIXED

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

Description

[Braden]

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.

Comment 1

[sean echevarria]

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

Comment 2

[Patrick C. Beard]

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

Comment 3

[Marc Attinasi]

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

Comment 4

[karnaze (gone)]

Adding nsbeta3+ to status field.

Comment 5

[av (gone)]

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?

Comment 6

[sean echevarria]

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).

Comment 7

[lchiang]

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.

Comment 8

[karnaze (gone)]

Adding rtm+.

Comment 9

[Phil Peterson]

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

Comment 10

[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 11

[Kathleen :Brade]

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.

Comment 12

[Peter Grandmaison]

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.

Comment 13

[ekrock's old account (dead)]

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.

Comment 14

[edburns]

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.

Comment 15

[edburns]

Attachment: cvs diff -u of fix for this bug, first iteration, diff from trunk.

Comment 16

[edburns]

Attachment: tar.gz of files in fix for this bug. first iteration, mod from trunk

Comment 17

[av (gone)]

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.

Comment 18

[edburns]

Attachment: cvs diff -u of files in fix for this bug, second iteration

Comment 19

[edburns]

Attachment: tar.gz of files in fix for this bug, second iteration

Comment 20

[edburns]

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.

Comment 21

[av (gone)]

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

Comment 22

[edburns]

Attachment: cvs diff -u of fix for this bug, third iteration

Comment 23

[edburns]

Attachment: tar.gz of files in fix for this bug, third iteration

Comment 24

[edburns]

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()?

Comment 25

[av (gone)]

>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.

Comment 26

[av (gone)]

Attachment: nsActivePluginList::remove

Comment 27

[av (gone)]

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.

Comment 28

[edburns]

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

Comment 29

[av (gone)]

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 

Comment 30

[edburns]

> 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?

Comment 31

[av (gone)]

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?

Comment 32

[av (gone)]

Attachment: Forth iteration

Comment 33

[av (gone)]

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.

Comment 34

[Peter Lubczynski]

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?

Comment 35

[av (gone)]

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...

Comment 36

[av (gone)]

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

Comment 37

[av (gone)]

Attachment: fifth iteration. Assertion condition fixed.

Comment 38

[Peter Lubczynski]

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

Comment 39

[av (gone)]

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?

Comment 40

[Peter Lubczynski]

Something is still holding on to it

Comment 41

[av (gone)]

Attachment: sixth iteration

Comment 42

[av (gone)]

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

Comment 43

[Peter Lubczynski]

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.

Comment 44

[edburns]

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

Comment 45

[av (gone)]

Attachment: patch applied to nsPluginHostImpl.cpp, v. 1.202

Comment 46

[av (gone)]

Attachment: patch applied to nsPluginHostImpl.h, v. 1.47

Comment 47

[Peter Lubczynski]

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?

Comment 48

[Peter Lubczynski]

Oh and r=peterl if the Java folks approve

Comment 49

[av (gone)]

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.

Comment 50

[av (gone)]

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

Comment 51

[Peter Lubczynski]

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?

Comment 52

[edburns]

I'm planning on trying this out today.

Comment 53

[av (gone)]

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.

Comment 54

[edburns]

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

Comment 55

[edburns]

Av what's the status?

Comment 56

[av (gone)]

Waiting for sr from waterson.

Comment 57

[Chris Waterson]

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

Comment 58

[av (gone)]

Attachment: seventh iteration -- corrected for waterson's comments

Comment 59

[av (gone)]

Checked in. Marking fixed.

Comment 60

[sean echevarria]

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.

Comment 61

[sean echevarria]

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

Comment 62

[shrirang khanzode]

verif (stamp)