116357 - Old school plug-ins need the ability to be cached

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[TaylorToddK]

Since old school plug-ins can access XPCOM functionality, old school plug-ins 
need the ability to be cached or prolong their lifetime until they have 
completed cleanup.

The plug-in may have registered a listener with an channel.  Due to 
asynchronous behaviour of a channel, a plug-in cannot guarantee all references 
to its objects are released when the plug-in's destroy and shutdown methods are 
called.  Since NPAPI plug-in are unloaded from memory, this would result in the 
channel holding an invalid listener memory address and crashing in subsequent 
calls.

Comment 1

[Peter Lubczynski]

Andrei, isn't that what nsPluginInstanceVariable_DoCacheBool in NPP_GetValue was
for? 

Maybe it has regressed?

Comment 2

[Peter Lubczynski]

nominating nsbeta1

Comment 3

[Arun Ranganathan]

Nominating nsbeta1+ per ADT triage.

Comment 4

[av (gone)]

For some reason Mozilla does not want to attach a file, so I am cut'n'pasting
the patch.

This patch removes a check for being 'old school' before unloading the plugin
but rather only relies on the plugin response. Please review.

Comment 5

[av (gone)]

Index: nsPluginHostImpl.cpp
===================================================================
RCS file: /cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v
retrieving revision 1.366
diff -u -w -r1.366 nsPluginHostImpl.cpp
--- nsPluginHostImpl.cpp	16 Mar 2002 15:58:54 -0000	1.366
+++ nsPluginHostImpl.cpp	17 Mar 2002 16:07:46 -0000
@@ -5712,6 +5712,15 @@
   return rv;
 }
 
+static BOOL IsCachedPlugin(nsIPluginInstance * aInstance)
+{
+  PRBool doCache = PR_FALSE;
+  aInstance->GetValue(nsPluginInstanceVariable_DoCacheBool, (void *) &doCache);
+
+  // I have to write it this ugly way because due to interference between
+  // nsplugindefs.h and npapi.h value 5 may be used for java class either
+  return (doCache == PR_TRUE);
+}
 
 ////////////////////////////////////////////////////////////////////////
 NS_IMETHODIMP
@@ -5727,15 +5736,10 @@
     plugin->setStopped(PR_TRUE);  // be sure we set the "stop" bit
 
     // if the plugin does not want to be 'cached' just remove it
-    PRBool doCache = PR_TRUE;
-    aInstance->GetValue(nsPluginInstanceVariable_DoCacheBool, (void *) &doCache);
-
-    // we also do that for 4x plugin, shall we? Let's see if it is
-    PRBool oldSchool = PR_TRUE;
-    if(plugin->mPluginTag)
-      oldSchool = plugin->mPluginTag->mFlags & NS_PLUGIN_FLAG_OLDSCHOOL ?
PR_TRUE : PR_FALSE;
-
-    if (!doCache || oldSchool)
+    // we should also keep in mind that some new 4x plugins may use XPCOM
+    // services and thus may want to be cached too. We assume that they
+    // don't in many cases but do not check for being old school explicitely
+    if (!IsCachedPlugin(aInstance)) 
       {
       PRLibrary * library = nsnull;
       if(plugin->mPluginTag)

Comment 6

[Peter Lubczynski]

Attachment: another possible patch? (creating NPPVpluginKeepLibraryInMemory)

I was also looking at this last night and noticed the descrepency in variables.
I don't think we should be calling into the plugin with a wrong variables and
twice no less (once in nsObjectFrame::Destroy). With this mix-up, a plugin may
tell us the wrong thing.

So, in this patch, I created a new variable, NPPVpluginKeepLibraryInMemory,
that the plugin can set with a NPP_SetValue and prevent us from calling in with
wrong variables. Works similar to how the plugin tells us it's transparent and
we can leave the the misnamed variables behind.

Comment 7

[av (gone)]

Adding a new value for NPPvariable is a good idea. However, I am not sure that
we want to do it similar to the case of transparancy. We promoted doCacheBool
for quite a time and can break whatever plugins already using this mechanism.
Java comes to mind at once.

Comment 8

[av (gone)]

Attachment: patch v.3

Comment 9

[Peter Lubczynski]

Andrei, your patch doesn't catch the "extra" calls to NPP_GetValue in
nsObjectFrame::Destory for variable NPPVjavaClass aka DoCacheBool and
CallSetWindowAfterDestroyBool. We call multiple times.

Wasn't this caching only done for XPCOM plugins? Have we ever cached 4.x-style
plugins? nsplugindefs.h isn't even part of the NPAPI SDK. These variables have
gotten really screwed up and I don't think we should attempt to use them
interchangably anymore.

My patch doesn't change anything for XPCOM plugins such as Java. They will
continue to use nsPluginInstanceVariable_DoCacheBool and it will function just
like before. :) With 4.x plugins, however, we will no longer make calls for
PPVjavaClass and NPPVpluginWindowSize
(nsPluginInstanceVariable_CallSetWindowAfterDestroyBool) which are meaningless
and may trigger unwanted side-effects. Instead, I just remapped an exsisting
XPCOM nsPluginInstanceVariable to a newly invent NPPV one. Also, since this is
new with 4.x plugins, why not save us a call into the plugin and have it call us
to set the value? I think that's cleaner and matches the way Patrick recently
implemented javascriptPushCallerBool. Fixing the bad calls to
NPPVpluginWindowSize may one day lead to implementing the feature.

Comment 10

[av (gone)]

Is not Java plugin supposed to respond to this GetValue call? This was my only
concern so far. Otherwise I agree with what you are saying.

Comment 11

[av (gone)]

Peter, I was under impression that Java is not an XPCOM plugin in full sense.
Otherwise, there would be no need for DoCacheBool. If my memory does not mislead
me this variable was 'invented' specifically for the Java plugin. In your patch
you remove this GetValue call to plugin. Please correct me if I am wrong. In all
other aspects I agree: we should not promote nsplugindefs.h anymore, and given
all this is brand new the mechanism similar to setting transparancy makes
perfect sense.

Comment 12

[Patrick C. Beard]

1. Don't change anything in the nsplugindefs.h file, it's effectively frozen 
and deprecated. We don't want to add anything new here.

2. Java plugins are XPCOM plugins in the sense that they have a longer 
lifetime than a given web page, since we can use Java in LiveConnect, 
and create new threads that outlive the web page that spawned them.

Comment 13

[Winnie Lam]

adding adt2 nomination in status whiteboard.

Comment 14

[av (gone)]

Comment on attachment 74619 [details] [diff] [review]
another possible patch? (creating NPPVpluginKeepLibraryInMemory)

After detailed discussion with Peter, I agree that this approach would address
our problem and will not cause any problems with Java plugin. r=av

Comment 15

[Patrick C. Beard]

Comment on attachment 74619 [details] [diff] [review]
another possible patch? (creating NPPVpluginKeepLibraryInMemory)

sr=beard

Comment 16

[av (gone)]

asa wrote in e-mail response to my approval request:
>How well has this been tested? What kinds of risk is this patch? 

This has been tested to make sure all major plugins work as expected. The risk
is minimal, this fix adds a possibility for a plugin to use the new behaviour,
if plugin remains silent there will be no effect at all. As plugin manufacturers
are not aware of this possibility yet, there are no plugins using this feature
on the market. So no changes are expected for the existing plugins.

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 74619 [details] [diff] [review]
another possible patch? (creating NPPVpluginKeepLibraryInMemory)

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 18

[av (gone)]

Checked in, thanks, Peter.

Comment 19

[av (gone)]

Shrirang, I don't think it can be verified any way other than just making sure
the code is in. There are no yet plugins using this feature.

Comment 20

[Peter Lubczynski]

Actually, can the NPAPI tester plugin be used to verify this? It might be kinda
tricky.

Comment 21

[av (gone)]

It should be updated by the way to be able to set those new variables. After
that... Any utilities showing dlls in memory?

Comment 22

[shrirang khanzode]

verif patch is in.

Comment 23

[av (gone)]

I think I'll come with a way to verify the behaviour soon. It will be good for
bug 133651 too.

Comment 24

[av (gone)]

See bug 133651, comments #9 and #10 for steps to reproduce and verify.

Comment 25

[shrirang khanzode]

Tested with steps mentioned in bug 133651, Thanks, Andrei !