148889 - crash cleaning up scriptable plugin object after unload plugin [@ nsCOMPtr_base::~nsCOMPtr_base][@nsCOMPtr<nsIClassInfo>::~nsCOMPtr<nsIClassInfo>]

Status: VERIFIED FIXED

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

Description

[shrirang khanzode]

installed viewpoint plugin from here 
http://elwood.mcom.com/arun/plugins/viewpoint/viewpoint-demo.html

used 0603 brnch build on NT

steps:
go to above url
click on 'Launch demo'
Observe that nothing laods in the left pane, close the window
again, click on 'Launch demo', browser should crash

stack:
nsCOMPtr_base::~nsCOMPtr_base 
[d:\builds\seamonkey\mozilla\xpcom\glue\nsCOMPtr.cpp, line 64] 
XPCWrappedNativeProto::~XPCWrappedNativeProto 
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativeproto.cpp, 
line 89] 
DyingProtoKiller 
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcjsruntime.cpp, line 201] 
JS_DHashTableEnumerate [d:\builds\seamonkey\mozilla\js\src\jsdhash.c, line 600] 
XPCJSRuntime::GCCallback 
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcjsruntime.cpp, line 543] 
DOMGCCallback [d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp, 
line 1623] 
js_GC [d:\builds\seamonkey\mozilla\js\src\jsgc.c, line 1349] 
js_ForceGC [d:\builds\seamonkey\mozilla\js\src\jsgc.c, line 981] 
JS_GC [d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 1657] 
nsJSContext::Notify 
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp, line 1573] 
nsTimerManager::FireNextIdleTimer 
[d:\builds\seamonkey\mozilla\xpcom\threads\nsTimerImpl.cpp, line 593] 
nsAppShell::Run [d:\builds\seamonkey\mozilla\widget\src\windows\nsAppShell.cpp, 
line 134] 
nsAppShellService::Run 
[d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsAppShellService.cpp, line 451] 
main1 [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1473] 
main [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1809] 
WinMain [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1827] 
WinMainCRTStartup() 
KERNEL32.dll + 0x1ba06 (0x77f1ba06)

Comment 1

[rubydoo123]

assigning to AV

Comment 2

[Peter Lubczynski]

The stack in this bug looks oftly similar to the one in bug 144838. Andrei, do
you think it's a dup?

The root cause may really be bug 148458 as I think a
navigator.plugins.refresh(1) is involved.

Comment 3

[av (gone)]

Well, maybe. The stack is not exactly the same. I see similar stack on
assertions quite often by the way, when reloading pages with plugins.

Comment 4

[Peter Lubczynski]

I'm crashing a lot more reliably closing/opening the demo on this page:
http://www.geometrix.com/facevision/applications/virtual.html

Comment 5

[serge (gone)]

Attachment: patch proposal

this patch fixes this crash, but probably it is not suitable for bug 144838
or for situation when plugin replaces itself with new dll and force to reload
the page by calling javascript:navigator.plugins.refresh(true)
though we need a test case for that.

Comment 6

[av (gone)]

XPconnected plugins are supposed to delay unload without special call to
SetCache. I am wondering why |nsPluginHostImpl::SetIsScriptableInstance| is not
called.

Comment 7

[Peter Lubczynski]

I like Serge's patch but I'm worried if some upgrade process will be broken
because we don't unload the DLL.

Since these stacks seem to go through garbage collection, if we could force a GC
before we unload the library, would that stop the crash?

Comment 8

[serge (gone)]

av:|nsPluginHostImpl::SetIsScriptableInstance| is getting called

Comment 9

[serge (gone)]

peterl: do you know any good axample outside of js code how to force GC?

Comment 10

[Peter Lubczynski]

I haven't tried this out and I'm just _totally_ guessing here...but:

It looks like nsIScriptContext interface has a public, synchronous |GC| method:
http://lxr.mozilla.org/mozilla/source/dom/public/nsIScriptContext.h#270

..and if you have a document, it looks like you can do a |GetScriptContext| by
doing something like this:
221   nsCOMPtr<nsIDocument> doc ( do_QueryInterface(aDOMDocument) );
222   if (!doc)
223     return NS_ERROR_FAILURE;
224 
225   nsCOMPtr<nsIScriptGlobalObject> scriptGlobalObject;
226   result = doc->GetScriptGlobalObject(getter_AddRefs(scriptGlobalObject));
227 
228   if (NS_FAILED(result) || !scriptGlobalObject)
229     return NS_ERROR_FAILURE;
230 
231   return scriptGlobalObject->GetContext(aScriptContext);

Comment 11

[Peter Lubczynski]

*** Bug 144838 has been marked as a duplicate of this bug. ***

Comment 12

[Peter Lubczynski]

*** Bug 109281 has been marked as a duplicate of this bug. ***

Comment 13

[Peter Lubczynski]

*** Bug 150764 has been marked as a duplicate of this bug. ***

Comment 14

[Peter Lubczynski]

in bug 144838, jband brings up why forcing a GC is not a good solution:

> ...would it be possible to fix this crash by forcing a GC... ?

No, that would be a very bad bet. It assumes that doing a gc will for sure
release all references. But if the JS code happens to hold a reference then that
assumption will be incorrect. e.g...

var foo = some_plugin_interface;

// do the stuff that unloads with a forced gc. xpconnect does not release 
// references into plugin here because the JS code holds a ref. 

var foo = null;

// gc happens sometime later and xpconnect does try to release. bad things happen

The above noted bugs are clearly dups - and should be marked as such. 

Either there is a refcounting error - some code is releasing refs held by other
code - or you are unloading a dll for which there are still live referenced
objects. You need to figure out how to fix the real problem. I don't buy the
idea that the occurance of gc is the source of the problem or that invoking fgc
at a particular time is a viable solution.

-----

Ari, how would Viewpoint's upgrade process be effected if we didn't unload the
DLL until the browser was shut down like is done with the patch in this bug?

Comment 15

[av (gone)]

I don't see |nsPluginHostImpl::SetIsScriptableInstance| getting called on 0603
branch which is the version of the original report. However, I can see it on the
trunk. In this call we set a flag not to unload the library from memory
immediately but rather do it later. Maybe we should look at the definition of
'later' which is currently just next time _any_ plugin is needed.

Observing GC events if any?

Comment 16

[Peter Lubczynski]

If we observed GC, we still don't know if there are any outstanding references
to our plugin object. Could we use some sort of weak pointer?

Comment 17

[Ari Berger]

As far as I can tell, we shouldn't care when we are unloaded.
For example, I know that IE makes no guarentees of when it unloads our plugin- 
it can hold it around for 4-5 pages before unloading it.

Comment 18

[Peter Lubczynski]

Comment on attachment 87323 [details] [diff] [review]
patch proposal

r=peterl since this doesn't negativly effect Viewpoint, I think this may be the
right thing to do now.

Comment 19

[shrirang khanzode]

fyi: am crashing hard with this same trace on the latest flash scriptable 
plugin provided by macromedia.

Comment 20

[scottputterman]

Let's get a super review and then let it bake on the trunk.

Comment 21

[serge (gone)]

Patrick, could you sr= on attachment 87323 [details] [diff] [review]?

Comment 22

[scottputterman]

Serge, how much do you expect this to add to bloat.  

Comment 23

[serge (gone)]

npViewpoint.dll [MetaStream 3 Plugin r4] on w2k occupied 163839 bytes of code 
memory

Comment 24

[av (gone)]

This will be the case for XPConnected plugins, of which we currently have just
few. Besides, once a dll is loaded, the next time it is needed it will be used
from memory, so the bloat will only grow when we hit a new for the current
session plugin.

Comment 25

[Peter Lubczynski]

Yeah, it's like 'caching' the plugin in memory once used. 

Note: This is already being done for true XPCOM plugins like Real and Java. This
fix will extend the domain to any 'scriptable' plugin.

Comment 26

[Frank Tang]

The cache approach should be a short term solution. But I think it cannot be
fixed untill xpcom include DllCanLoadNow facility ( see LockServer in the page
156-157 of "Inside COM" for details). The cache won't have more problem by the
current way that we cache all the mozilla dll untill we shutdown xpcom. It is
not a nice solution, but it is worst than all other dll we have in mozilla
today. I file bug 151928 for that. And untill 151928 got implemented, I don't
see a better way to solve this bug in a nicer way.

Comment 27

[av (gone)]

Another way to address this bug is to make it clear for the plugin developers
that the current situation requires the plugin dll to remain in memory if the
plugin is scriptable, which can be achieved by the plugin itself setting this
flag. In this case we will need to update samples, articles and redirect the bug
to evangelism. The advantage is one less hack and cleaner code. The disadvantage
is indeed that plugins need to be rewritten, but ViewPoint is still in the stage
when it can be done. QT?

Comment 28

[Jeff Price]

How does locking a plugin into memory effect hot patching of a plugin?  IE 
essentialy does the same thing, they release an instance of the ActiveX control 
but don't unload the dll until browser shutdown.  This causes all kinds of 
problems around patching the plugin on the fly...

Comment 29

[av (gone)]

Are not ActiveX controls upgradable? With the plugin API the flag to keep plugin
in memory can be set or reset by plugin at any time. I would assume this can be
used to hot patch plugins.

Comment 30

[Jeff Price]

When using IE's patching system the dll is unloaded automatically.  But I'd 
rather stay away from the "IE did it this way" argument (seems flawed).  Of 
course the more similar the browsers are the simpler the web/plugin developers 
life will be.  I'm most concerned about keeping patching of plugins simple.  It 
makes my codebase much cleaner and most importantly, it is smoother for the 
user.  My questions is if we lock a plugin dll in memory, and then use the 
SmartUpdate(still the right name?)/XPI patching suite will we run into a 
install failure due to a locked dll?  It sounds like the other option is to 
script to the plugin to tell it that it is being patched...which unsets the 
lock...then navigate to another page with no scriptable plugins in it (to avoid 
the crash)...then navigate to a page to perform the patch...  which seems like 
a bit of a kludge we'd all regret.

Comment 31

[serge (gone)]

av: we have to lock scriptable plugin dll in memory regardless of what flags 
plugin itself is setting, and for the future we have to recommend plugin 
developers to add nsSupportsWeakReference into their  class nsScriptablePeer 
implementation, in this case we'll able to determine when all references to 
nsSupportsWeakReference are gone and plugin dll can be safely unload/replace, 
there is only one disadvantage I see in this approach plugin dll should be 
explicitly linked against xpcom.lib.
What I'm trying to say is we always have to lock scriptable plugin in memory to 
prevent the crashes, so the patch (attachment 87323 [details] [diff] [review]) should be checked in 
anyway, that we have to implement the mechanism to unload plugin if it 
nsScriptablePeer supports nsISupportsWeakReference. 

Comment 32

[av (gone)]

serge: I agree with you. Should we still leave the possibility for a plugin to
set cache FALSE? Looks like the present patch will allow plugins to reset the flag.

Comment 33

[serge (gone)]

yes, plugins developers can reset this flag, but in this case they know what 
they do, is anything wrong with this?

Comment 34

[av (gone)]

Well, for XPCOM plugins we don't look at this flag at all. Just keep them loaded.

Also, the patch has no effect for me on the trunk. The browser hangs with or
without it. Anybody else sees it?

Comment 35

[Patrick C. Beard]

Comment on attachment 87323 [details] [diff] [review]
patch proposal

sr=beard

Comment 36

[av (gone)]

In other words, maybe it would be better to do something like this in
|TryUnloadPlugin|:

-  if (isXPCOM && !aForceShutdown) return;
+  if (mXPConnected && isXPCOM && !aForceShutdown) return;

Comment 37

[serge (gone)]

av: with your proposal I crashed on test case with slightly different 
stack trace:
XPCWrappedNative::FlatJSObjectFinalized(JSContext * 0x048ef140, JSObject * 
0x03cd5628) line 838 + 3 bytes
XPC_WN_NoHelper_Finalize(JSContext * 0x048ef140, JSObject * 0x03cd5628) line 615
js_FinalizeObject(JSContext * 0x048ef140, JSObject * 0x03cd5628) line 1816 + 106 
bytes
js_GC(JSContext * 0x048ef140, unsigned int 0) line 1283 + 11 bytes
js_ForceGC(JSContext * 0x048ef140, unsigned int 0) line 980 + 13 bytes
JS_GC(JSContext * 0x048ef140) line 1656 + 11 bytes
nsJSContext::Notify(nsJSContext * const 0x048f2f10, nsITimer * 0x049c0a58) line 
1595 + 13 bytes
nsTimerImpl::Fire() line 348
nsTimerManager::FireNextIdleTimer(nsTimerManager * const 0x01c2b5e8) line 593
...

Comment 38

[av (gone)]

This was not a patch, just an idea to use a flag we already have, not to set
another one. The complete patch will require some cleaning as we no longer need
this special case -- (old_school && XPConnected). I need to wait till my branch
build finishes to come up with the complete patch, but looking at the code I can
see that we should also not set |aUnloadLibraryLater| in
|nsActivePluginList::remove|.

Comment 39

[Michael Buckland]

av:  would it be possible for you to let me know when you can resolve this bug?
 There are other bugs which we need to resolve quickly that are currently
blocked.  Let me know if you need details. 

Comment 40

[Peter Lubczynski]

I just talked with AV about this and we both agree that Serge's patch does the
right thing.

Basically, Serge's patch will make NPAPI+XPConnected plugins act exactly like
XPCOM plugins -- they will not get a NP_Shutdown call nor get their library
unloaded (see exception to rule below).

However, Serge's patch does not remove the dead code from the the
'UnloadLibraryLater' list. We've already hit this problem and tried to avoid
this bug before in bug 61388. See, we already had support for NOT unloading a
plugin library that is marked as scriptable UNTIL the next time is is used in
|nsPluginHostImpl::GetPluginFactory|  where we unload it. But unloading it there
may be too soon as this bug has shown us.

Does this crash also go away if |CleanUnusedLibraries| is removed from
|GetPluginFactory|?

A side note: We will not keep the plugins loaded in memory forever! Looking back
through the code, I now recall that we will only keep a fixed number of plugins
in memory before beginning to unload the oldest ones we stopped. Currently, this
number is set pretty high at 10 plugins (not individual plugin instances) or can
be override by setting the number via a pref called:
"browser.plugins.max_num_cached_plugins"

Comment 41

[Daniel Veditz [:dveditz]]

how can this be adt1 and not even plussed? I don't see how it can have a
priority if we haven't even agreed we need to fix it.

Comment 42

[Syd Logan]

Fixed checked into NETSCAPE_7_0_PR1_BRANCH by [e-mail]

Comment 43

[serge (gone)]

Thanks Syd.
Peter, we cannot unload scriptable plugin dll unless we are 100% sure js
releases all pointers it holds to nsIScriptablePeer, we cannot control js_GC,
however as I explained in comment 31 adding nsSupportsWeakReference can help us
to keep track of such refs and unload plugin when it safe, and this has to be
implemented on both side plugin & mozilla.
So, to prevent the crashes in scriptable plugins we *MUST* lock it's dll into
memory. Of course, there is several way to implement the patch for this, I think
I've proposed just easy one.
I'm not quiet sure what you mean by "the dead code from the the
'UnloadLibraryLater'".

Comment 44

[Jeff Price]

What impact will any of these solutions have on patching plugins from within 
the browser?

Comment 45

[av (gone)]

Attachment: patch v.2

The kind of patch I was thinking of. I think we should reconsider the approach
for the trunk at least.

Comment 46

[av (gone)]

serge: nobody is arguing that XPConneceted plugins should stay in memory. As I
understood from what Peter said, with the patch we can still hit the situation
when we unload it. Namely, when the number of cached plugins reaches the limit,
because the patch does not plain prevent the dll from unloading but rather
instructs plugin host to keep the plugin in the list of active plugin instances.
Which is still not a full guarantee because of abovementioned limitation on the
maximum number of such plugins.

Comment 47

[av (gone)]

We need to decide which patch should go to the trunk and 1.0 branch. Please
review and give your opinions.

Briefly, the outline of the two approaches.

First patch (87323)
  - instance |Destroy| method is called
  - instance remains in the active instance list after the job is finished
  - instance can be later kicked off the active list in which case the dll can
get unloaded from memory
  - in a special case of refresh(1) plugins |Shutdown| method is called and the
dll gets unloaded from memory
  - the patch is simple, safe and works for most cases

Second patch (88050)
  - instance |Destroy| method is called
  - instance removed from the active instance list
  - unloading code checks for a flag and bypasses UnloadLibrary, so the dll stays
  - in a special case of refresh(1) plugins |Shutdown| method is called and the
dll stays in memory (this can be changed, I just did it this way to be exactly
like XPCOM plugins)
  - the patch eliminates some code which made distinction between XPCOM plugins
and NPAPI XPConnected plugins, they are now treated the same in the destroy business

Comment 48

[Frank Tang]

>------- Additional Comment #43 From [e-mail] 2002-06-17 18:07 -------
>
>Thanks Syd.
>Peter, we cannot unload scriptable plugin dll unless we are 100% sure js
>releases all pointers it holds to nsIScriptablePeer, we cannot control js_GC
I take my early comment back. I think there are a better fix than the first patch. 

If those plugin dll implement NSGetModule and correctly implement
nsIModule->canUnload . then we can call it's canUnload to tell it can be unload
or not. In theory, the DLL should maintain a global ref count of all the object
created / released by any class it implemented and based on that dll global ref
count to answer that question. If any code create an object from that dll, the
global ref count will increase, if javascript reference to it (I hope javascript
does addref) then the global ref count will also increase one. And when all the
C++ release it and JavaScript release it, the global ref count will go to zero.
And in that time when we ask canUnload it should answer TRUE.

I know a lot of our dll currently do not implement this correctly and therefore
we can not TRUST the canUnload of most of our own dll. However, since the
mechanism EXIST, we can use it if all the plugin dll implement it correctly, and
we can apply that rule to all the plugin dll only without apply it to other
mozilla dll internally. If canUnload does not implemented correctly by the
plugin, then it is a plugin implementation issue instead of a interface design
issue and we can lobby the plugin vendor to fix it for us.

Comment 49

[shrirang khanzode]

tested the pr1 respin bits 06/18 on NT, 98, 2k and XP. Am still crashing with 
the same trace on NT. However, XP, 2k, 98 seem just fine, I cannnot crash on 
those. will do more detailed testing and comment

Comment 50

[Jeff Price]

This is of course exactly what CanUnload is there for, but for plugins you are 
currently recommending tweaking a NS4 plugin to expose an XPCOM interface, so 
there is no NSGetModule().  So for plugins there is probably not a can unload 
method.  Unless of course you make a new GetValue enum for can unload...

Comment 51

[serge (gone)]

shrir: I found only one TB report
http://climate.netscape.com/reports/SingleIncidentInfo.cfm?dynamicBBID=7469862
from respined NetscapeGecko1.0Win322002061802
int's w2k and stack trace is not the same:(

Comment 52

[shrirang khanzode]

talked with serge and he has found the NT crasher traces that I submitted. This 
win2k crasher was very random. I could see it only once after I opened 25-30 
windows of the demo (test url) above and tried to close them .until then I could 
not crash on w2k. However, NT crashes reliably.

Comment 53

[serge (gone)]

shrir: please provide TB ID for NT crashes

Comment 54

[scottputterman]

Should we try Andrei's patch?  Can we get that reviewed and tested?  

Comment 55

[shrirang khanzode]

av: traces for NT crashers are here:

http://climate.netscape.com/reports/SingleIncidentInfo.cfm?dynamicBBID=7466584
http://climate.netscape.com/reports/SingleIncidentInfo.cfm?dynamicBBID=7469587
http://climate.netscape.com/reports/SingleIncidentInfo.cfm?dynamicBBID=7469634

Comment 56

[av (gone)]

Both patches are supposed to do the same thing: keep dll in memory after plugin
is done. Chances are that if serge's patch fails in some cases, the problem is
beyond just preventing the dll from unloading, and my patch will probably fail
too. Is it possible to find somebody with NT40 debug environment and try the
patch there first?

Comment 57

[serge (gone)]

shrir: all incidents have build ID 2002051220, it's a bit old:)
the correct build id should be 2002061802

Comment 58

[shrirang khanzode]

serge, maybe since it's a the PR1 respin..the build id is that of the date for 
pr1 (original). I am positive it's the correct bits...

Comment 59

[shrirang khanzode]

err..I had used the nssetup.exe on nt and nssetupB.exe on other platforms. 

Indded, the bits fetched on NT were from 0512 (without the patch). I just 
downloaded the respin bits with the blob and see the correct build id 
(2002061802)and *don't see any crash*  this is fixed indeed. Sorry for the 
confusion...

Comment 60

[serge (gone)]

Comment on attachment 88050 [details] [diff] [review]
patch v.2


>@@ -1049,11 +1028,11 @@
>   if (!(mFlags & NS_PLUGIN_FLAG_OLDSCHOOL))
>     isXPCOM = PR_TRUE;
> 
>-  if (isXPCOM && !aForceShutdown) return;
>+  if ((isXPCOM || mXPConnected) && !aForceShutdown) return;
> 
we'll never call ns4xPlugin::Shutdown()wich actually calls NPP_ShutdownProc
is this intend, do not we leak here?
>   if (mEntryPoint)
>   {
>    mEntryPoint->Shutdown();
>    mEntryPoint->Release();
>    mEntryPoint = nsnull;
>  }

Comment 61

[av (gone)]

Why? We call it when the corresponding |nsPluginTag::~nsPluginTag| is called.
This happens on refresh(1) and on exit.

Comment 62

[serge (gone)]

exactly it happens only on refresh(1) and on exit, but when the last plugin
instance is gone, we are not calling NP_Shutdown(), well we never know what
process resources plugins vendors can allocate in NP_Initialize(), which suppose
to be released on NP_Shutdown(), so all those resources will remain allocated in
addition to address space consumed by plugins dll untill refresh(1) or exit,
which in common case means during whole client life time.  

Comment 63

[av (gone)]

The same story with XPCOM stuff. Do we worry about that? If so, and it is going
to be fixed then this case will be fixed too.

Comment 64

[Peter Lubczynski]

Serge, is it the same case with the SetCached(1) patch that with this flag on,
NPAPI plugins will not be removed from the "active" list of plugins and therfore
Shutdown is not called either. Isn't it in "remove" that we call |TryUnloadPlugin|?

Comment 65

[serge (gone)]

with SetCached(1) NP_Shutdown() will be called
here is the code, my comments start with ***
>+PRBool nsActivePluginList::remove(nsActivePlugin * plugin)
> {
>   if(mFirst == nsnull)
>     return PR_FALSE;
>@@ -550,19 +546,7 @@
>         delete p; // plugin instance is destroyed here> 
>         if(pluginTag)
>-        {
>-          // xpconnected plugins from the old world should postpone unloading
library 
>-          // to avoid crash check, if so add library to the list of unloaded
libraries
>-          if(pluginTag->mXPConnected && (pluginTag->mFlags &
NS_PLUGIN_FLAG_OLDSCHOOL))
>-          {
*** We set it to 0 that's important
>-          pluginTag->mCanUnloadLibrary = PR_FALSE;
>-
>-            if(aUnloadLibraryLater)
>-              *aUnloadLibraryLater = PR_TRUE;
>-          }
>-        
>           pluginTag->TryUnloadPlugin();
>
>@@ -1049,11 +1028,11 @@
>   if (!(mFlags & NS_PLUGIN_FLAG_OLDSCHOOL))
>     isXPCOM = PR_TRUE;
> 
*** We do not return from here
>-  if (isXPCOM && !aForceShutdown) return;
>+  if ((isXPCOM || mXPConnected) && !aForceShutdown) return;
>
>   if (mEntryPoint)
>   {
*** We call it
>    mEntryPoint->Shutdown();
>    mEntryPoint->Release();
>    mEntryPoint = nsnull;
>    }
>   // before we unload check if we are allowed to, see bug #61388
>   // also, never unload an XPCOM plugin library

*** We fail here because mCanUnloadLibrary == 0
>-  if (mLibrary && mCanUnloadLibrary && !isXPCOM)
>+  if (mLibrary && mCanUnloadLibrary && !(isXPCOM || mXPConnected))
>     PostPluginUnloadEvent(mLibrary); // unload the plugin asynchronously by
posting a PLEvent 

Comment 66

[av (gone)]

But |SetCached| causes |nsActivePluginList::remove| never get called, does not it?

Comment 67

[Peter Lubczynski]

Comment on attachment 88050 [details] [diff] [review]
patch v.2

NP_Shutdown will be called when the browser is shutting down or at
plugins.refresh(1) time, but it will not be called when the user leave a page
with a plugin and there are no more plugin instances in other windows. Is this
what we want? I thought we just wanted to keep from unloading the library?
Perhaps we could evangelize plugin developers to return error from NP_Shutdown
if there are outstanding refcounts and it is not safe to unload?

Comment 68

[serge (gone)]

I apologize, my rough review of the source was wrong:(
with SetCached(1) NP_Shutdown() is never getting called

Comment 69

[av (gone)]

Attachment: patch v.3

Even more clean up. As Peter rightfully pointed out, the original reason for
introducing |UnusedLibraryList| (refresh(1) with running plugins) is no longer
valid after unloading libraries was made asynchronous. So, I beleive we can now
completely get rid of this code.

Serge, would you please apply? You seem to be the only engineer who can see the
original bug in the debug environment.

Please review.

Comment 70

[av (gone)]

Peter, there is one more (almost) ghost left -- |mCanUnloadLibrary|. Here, in
one of |nsPluginTag| constructors:

#if TARGET_CARBON
  mCanUnloadLibrary = !aPluginInfo->fBundle;
#endif

Comment 71

[Doug Turner (:dougt)]

Comment on attachment 88346 [details] [diff] [review]
patch v.3

This patch is going the wrong way!  We want to be able to unload plugins - all
plugins.

Comment 72

[av (gone)]

Attachment: patch v.4

- keeping track of unused libs is back
- shutdown is called for NPAPI XPConnected plugins
- unloaded libs cleared on XPCOM shutdown event

Please review.

Comment 73

[rubydoo123]

this one should have been nsbeta1+ already

Comment 74

[Jaime Rodriguez, Jr.]

lowering impact to adt2 RTM.

Comment 75

[rubydoo123]

bumping back up to ADT1, this affects every scriptable plug-in, this is also the
patch that was checked into the beta branch and respun. This is necessary for
ViewPoint to work.

Comment 76

[Peter Lubczynski]

Comment on attachment 88562 [details] [diff] [review]
patch v.4

r=peterl but open a new bug about unloading XPCOM plugins at XPCOM shutdown
time.

Comment 77

[av (gone)]

Bug 153975 now filed on unloading XPCOM plugins.

Comment 78

[Jaime Rodriguez, Jr.]

back to adt2. pls do not change ADT impact markings, until you have talked to
the ADT. thanks!

Comment 79

[scottputterman]

changing to adt1.

Comment 80

[Patrick C. Beard]

Comment on attachment 88562 [details] [diff] [review]
patch v.4

My only concern is the lifetime of mPluginHost. Presumably it outlives all
other data structures, and thus it is safe to keep a weak reference to it?
Otherwise, sr=beard

Comment 81

[av (gone)]

Thanks Patrick. Yes, plugin host is supposed to outlive anything else. In fact,
this struct I add the reference to, has child-parent relationship with the
plugin host, the hosd being parent. I don't really see how it can live without
the host.

Comment 82

[av (gone)]

In the trunk. Marking fixed, nominating for the branch.

Comment 83

[Jaime Rodriguez, Jr.]

shrir - can you pls verify this fix on the trunk? thanks!

Comment 84

[Ari Berger]

144838 (marked a dup of this bug) seems to work now. Thanks!

Comment 85

[shrirang khanzode]

checked out this fix on trunk 0626 on NT..works great. No crash seen. 

Comment 86

[scottputterman]

adding adt1.0.1+.  Please get drivers approval before checking into the branch.

Comment 87

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 88

[av (gone)]

On the branch.

Comment 89

[shrirang khanzode]

verifyd on branch 0718 NT.