bug 525792: fix leaking PluginModules, call NP_Shutdown() on plugins. minor, no r=
authorChris Jones <jones.chris.g@gmail.com>
Tue, 03 Nov 2009 15:37:07 -0600
changeset 36039 06a506e6870014a2e06a4a1ef729779efd74a672
parent 36038 777af746b2942935e4fb1a5e5c73bbd765ac62ab
child 36040 b323e3f3af614f7e7138436c30fb8398342382e2
push id10694
push userbsmedberg@mozilla.com
push dateMon, 14 Dec 2009 15:23:10 +0000
treeherdermozilla-central@683dfdc4adf0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs525792
milestone1.9.3a1pre
bug 525792: fix leaking PluginModules, call NP_Shutdown() on plugins. minor, no r=
dom/plugins/PPluginModule.ipdl
dom/plugins/PluginModuleChild.cpp
dom/plugins/PluginModuleChild.h
dom/plugins/PluginModuleParent.cpp
dom/plugins/PluginProcessParent.h
ipc/glue/AsyncChannel.cpp
ipc/glue/AsyncChannel.h
ipc/ipdl/ipdl/lower.py
modules/plugin/base/src/nsNPAPIPlugin.cpp
modules/plugin/base/src/nsNPAPIPlugin.h
--- a/dom/plugins/PPluginModule.ipdl
+++ b/dom/plugins/PPluginModule.ipdl
@@ -60,16 +60,19 @@ child:
                       uint16_t aMode,
                       nsCString[] aNames,
                       nsCString[] aValues)
     returns (NPError rv);
 
   rpc ~PPluginInstance()
     returns (NPError rv);
 
+  rpc NP_Shutdown()
+    returns (NPError rv);
+
 parent:
   // XXX does NPN_UserAgent need to be RPC? certainly hope not, but to
   // XXX be safe ...
   rpc NPN_UserAgent()
     returns (nsCString userAgent);
 
   sync NPN_GetStringIdentifier(nsCString aString)
     returns (NPRemoteIdentifier aId);
--- a/dom/plugins/PluginModuleChild.cpp
+++ b/dom/plugins/PluginModuleChild.cpp
@@ -136,16 +136,17 @@ PluginModuleChild::Init(const std::strin
     NS_ASSERTION(mLibrary, "couldn't open shared object");
 
     if (!Open(aChannel, aParentProcessHandle, aIOLoop))
         return false;
 
     memset((void*) &mFunctions, 0, sizeof(mFunctions));
     mFunctions.size = sizeof(mFunctions);
 
+    // TODO: use PluginPRLibrary here
 
 #if defined(OS_LINUX)
     mShutdownFunc =
         (NP_PLUGINSHUTDOWN) PR_FindFunctionSymbol(mLibrary, "NP_Shutdown");
 
     // create the new plugin handler
 
     mInitializeFunc =
@@ -160,17 +161,17 @@ PluginModuleChild::Init(const std::strin
         (NP_GETENTRYPOINTS)PR_FindSymbol(mLibrary, "NP_GetEntryPoints");
     NS_ENSURE_TRUE(mGetEntryPointsFunc, false);
 
     mInitializeFunc =
         (NP_PLUGININIT)PR_FindFunctionSymbol(mLibrary, "NP_Initialize");
     NS_ENSURE_TRUE(mInitializeFunc, false);
 #else
 
-#error Please copy the initialization code from nsNPAPIPlugin.cpp
+#  error Please copy the initialization code from nsNPAPIPlugin.cpp
 
 #endif
     return true;
 }
 
 bool
 PluginModuleChild::InitGraphics()
 {
@@ -179,20 +180,32 @@ PluginModuleChild::InitGraphics()
     gtk_init(0, 0);
 #else
     // may not be necessary on all platforms
 #endif
 
     return true;
 }
 
+bool
+PluginModuleChild::AnswerNP_Shutdown(NPError *rv)
+{
+    AssertPluginThread();
+
+    // FIXME/cjones: should all instances be dead by now?
+
+    if (mShutdownFunc)
+        *rv = mShutdownFunc();
+
+    return true;
+}
+
 void
 PluginModuleChild::CleanUp()
 {
-    // FIXME/cjones: destroy all instances
 }
 
 bool
 PluginModuleChild::RegisterNPObject(NPObject* aObject,
                                     PluginScriptableObjectChild* aActor)
 {
     AssertPluginThread();
     NS_ASSERTION(mObjectMap.IsInitialized(), "Not initialized!");
--- a/dom/plugins/PluginModuleChild.h
+++ b/dom/plugins/PluginModuleChild.h
@@ -118,16 +118,18 @@ protected:
 
     virtual bool
     AnswerPPluginInstanceConstructor(PPluginInstanceChild* aActor,
                                      const nsCString& aMimeType,
                                      const uint16_t& aMode,
                                      const nsTArray<nsCString>& aNames,
                                      const nsTArray<nsCString>& aValues,
                                      NPError* rv);
+    virtual bool
+    AnswerNP_Shutdown(NPError *rv);
 
 public:
     PluginModuleChild();
     virtual ~PluginModuleChild();
 
     bool Init(const std::string& aPluginFilename,
               base::ProcessHandle aParentProcessHandle,
               MessageLoop* aIOLoop,
--- a/dom/plugins/PluginModuleParent.cpp
+++ b/dom/plugins/PluginModuleParent.cpp
@@ -461,21 +461,22 @@ PluginModuleParent::NP_Initialize(NPNets
 }
 #endif
 
 nsresult
 PluginModuleParent::NP_Shutdown(NPError* error)
 {
     _MOZ_LOG(__FUNCTION__);
 
-    // FIXME/cjones: shut down all our instances, and kill
-    // off the child process
+    // FIXME/cjones: should all sub-actors be dead by now?
 
-    *error = NPERR_NO_ERROR;
-    return NS_OK;
+    bool ok = CallNP_Shutdown(error);
+    Close();
+
+    return ok ? NS_OK : NS_ERROR_FAILURE;
 }
 
 nsresult
 PluginModuleParent::NP_GetMIMEDescription(char** mimeDesc)
 {
     _MOZ_LOG(__FUNCTION__);
 
     *mimeDesc = (char*)"application/x-foobar";
--- a/dom/plugins/PluginProcessParent.h
+++ b/dom/plugins/PluginProcessParent.h
@@ -59,17 +59,17 @@ namespace plugins {
 
 class PluginProcessParent : mozilla::ipc::GeckoChildProcessHost
 {
 public:
     PluginProcessParent(const std::string& aPluginFilePath);
     ~PluginProcessParent();
 
     /**
-     * Asynchronously launch the plugin process.
+     * Synchronously launch the plugin process.
      */
     bool Launch();
 
     virtual bool CanShutdown()
     {
         return true;
     }
 
--- a/ipc/glue/AsyncChannel.cpp
+++ b/ipc/glue/AsyncChannel.cpp
@@ -101,19 +101,22 @@ AsyncChannel::Open(Transport* aTransport
     }
 
     return true;
 }
 
 void
 AsyncChannel::Close()
 {
-    // FIXME impl
+    mTransport->Close();
+    // don't lose error-state information
+    if (ChannelError != mChannelState)
+        mChannelState = ChannelClosed;
 
-    mChannelState = ChannelClosed;
+    mTransport = NULL;
 }
 
 bool
 AsyncChannel::Send(Message* msg)
 {
     AssertWorkerThread();
     mMutex.AssertNotCurrentThreadOwns();
     NS_ABORT_IF_FALSE(MSG_ROUTING_NONE != msg->routing_id(), "need a route");
--- a/ipc/glue/AsyncChannel.h
+++ b/ipc/glue/AsyncChannel.h
@@ -100,28 +100,29 @@ public:
         mWorkerLoop()
     {
     }
 
     virtual ~AsyncChannel()
     {
         if (mTransport)
             Close();
+        // we only hold a weak ref to the transport, which is "owned"
+        // by GeckoChildProcess/GeckoThread
         mTransport = 0;
     }
 
     // Open  from the perspective of the transport layer; the underlying
     // socketpair/pipe should already be created.
     //
     // Returns true iff the transport layer was successfully connected,
     // i.e., mChannelState == ChannelConnected.
     bool Open(Transport* aTransport, MessageLoop* aIOLoop=0);
     
-    // Close from the perspective of the RPC layer; leaves the
-    // underlying transport channel open, however.
+    // Close the underlying transport channel.
     void Close();
 
     // Asynchronously send a message to the other side of the channel
     bool Send(Message* msg);
 
     // Implement the IPC::Channel::Listener interface
     NS_OVERRIDE virtual void OnMessageReceived(const Message& msg);
     NS_OVERRIDE virtual void OnChannelConnected(int32 peer_pid);
--- a/ipc/ipdl/ipdl/lower.py
+++ b/ipc/ipdl/ipdl/lower.py
@@ -2195,16 +2195,17 @@ class _GenerateProtocolActorHeader(ipdl.
                                        [ ExprVar(self.clsname) ])))
         self.cls.addstmts([ ctor, Whitespace.NL ])
 
         # ~Actor()
         dtor = DestructorDefn(
             DestructorDecl(self.clsname, virtual=True))
         dtor.addstmt(StmtExpr(ExprCall(ExprVar('MOZ_COUNT_DTOR'),
                                                [ ExprVar(self.clsname) ])))
+
         self.cls.addstmts([ dtor, Whitespace.NL ])
 
         if p.decl.type.isToplevel():
             # Open()
             aTransportVar = ExprVar('aTransport')
             aThreadVar = ExprVar('aThread')
             processvar = ExprVar('aOtherProcess')
             openmeth = MethodDefn(
--- a/modules/plugin/base/src/nsNPAPIPlugin.cpp
+++ b/modules/plugin/base/src/nsNPAPIPlugin.cpp
@@ -352,16 +352,18 @@ nsNPAPIPlugin::nsNPAPIPlugin(NPPluginFun
 
   fLibrary = aLibrary;
 }
 
 nsNPAPIPlugin::~nsNPAPIPlugin()
 {
   // reset the callbacks list
   memset((void*) &fCallbacks, 0, sizeof(fCallbacks));
+  delete fLibrary;
+  fLibrary = NULL;
 }
 
 
 #if defined(XP_MACOSX)
 void
 nsNPAPIPlugin::SetPluginRefNum(short aRefNum)
 {
   fPluginRefNum = aRefNum;
@@ -475,18 +477,20 @@ nsNPAPIPlugin::CreatePlugin(const char* 
 
   NPError initError;
   nsresult initResult = pluginLib->NP_Initialize(&(nsNPAPIPlugin::CALLBACKS), &initError);
   if (initResult != NS_OK || initError != NPERR_NO_ERROR)
     return NS_ERROR_FAILURE;
 #endif
 
 #ifdef XP_OS2
+  PluginLibrary* pluginLib = GetNewPluginLibrary(aFilePath, aLibrary);
+
   // create the new plugin handler
-  *aResult = new nsNPAPIPlugin(nsnull, aLibrary);
+  *aResult = new nsNPAPIPlugin(nsnull, pluginLib);
 
   if (*aResult == NULL)
     return NS_ERROR_OUT_OF_MEMORY;
 
   NS_ADDREF(*aResult);
 
   // we must init here because the plugin may call NPN functions
   // when we call into the NP_Initialize entry point - NPN functions
@@ -595,16 +599,18 @@ nsNPAPIPlugin::CreatePlugin(const char* 
   plugin->SetPluginRefNum(pluginRefNum);
 #endif
 #endif
 
 #ifdef XP_BEOS
   // I just copied UNIX version.
   // Makoto Hamanaka <VYA04230@nifty.com>
 
+  // XXX this code won't compile with the new e10s changes
+
   nsNPAPIPlugin *plptr;
 
   NPPluginFuncs callbacks;
   memset((void*) &callbacks, 0, sizeof(callbacks));
   callbacks.size = sizeof(callbacks);
 
   NP_PLUGINSHUTDOWN pfnShutdown =
     (NP_PLUGINSHUTDOWN)PR_FindSymbol(aLibrary, "NP_Shutdown");
--- a/modules/plugin/base/src/nsNPAPIPlugin.h
+++ b/modules/plugin/base/src/nsNPAPIPlugin.h
@@ -75,17 +75,17 @@ typedef NS_NPAPIPLUGIN_CALLBACK(NPError,
 
 class nsNPAPIPlugin : public nsIPlugin
 {
 private:
   typedef mozilla::PluginLibrary PluginLibrary;
 
 public:
   nsNPAPIPlugin(NPPluginFuncs* callbacks,
-                PluginLibrary* aLibrary);
+                PluginLibrary* aLibrary /*assume ownership*/);
   virtual ~nsNPAPIPlugin();
 
   NS_DECL_ISUPPORTS
   NS_DECL_NSIPLUGIN
 
   // Constructs and initializes an nsNPAPIPlugin object. A NULL file path
   // will prevent this from calling NP_Initialize.
   static nsresult CreatePlugin(const char* aFilePath, PRLibrary* aLibrary,