bug 526626: band-aids for shutdown assertions
authorPhineas T. Farnsworth
Mon, 09 Nov 2009 16:56:55 -0600
changeset 36063 8058ec0fe278f1d03d71358f635bead3ec0002f4
parent 36062 a03c00114c66070d5768ff111abf69ef215b7f1d
child 36064 8212015ff29d47dd436692a72cd9e2e29163d84d
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)
bugs526626
milestone1.9.3a1pre
bug 526626: band-aids for shutdown assertions
dom/plugins/PluginModuleParent.cpp
dom/plugins/PluginModuleParent.h
dom/plugins/PluginProcessParent.cpp
dom/plugins/PluginProcessParent.h
ipc/glue/AsyncChannel.cpp
ipc/glue/AsyncChannel.h
ipc/glue/GeckoChildProcessHost.cpp
ipc/glue/GeckoChildProcessHost.h
ipc/ipdl/test/cxx/IPDLUnitTestThreadChild.cpp
ipc/ipdl/test/cxx/IPDLUnitTests.h
ipc/ipdl/test/cxx/IPDLUnitTests.template.cpp
ipc/ipdl/test/cxx/genIPDLUnitTests.py
--- a/dom/plugins/PluginModuleParent.cpp
+++ b/dom/plugins/PluginModuleParent.cpp
@@ -44,43 +44,50 @@
 using mozilla::PluginLibrary;
 
 using mozilla::ipc::NPRemoteIdentifier;
 
 using namespace mozilla::plugins;
 
 PR_STATIC_ASSERT(sizeof(NPIdentifier) == sizeof(void*));
 
+// static
 PluginLibrary*
 PluginModuleParent::LoadModule(const char* aFilePath)
 {
     _MOZ_LOG(__FUNCTION__);
 
     // Block on the child process being launched and initialized.
     PluginModuleParent* parent = new PluginModuleParent(aFilePath);
-    parent->mSubprocess.Launch();
-    parent->Open(parent->mSubprocess.GetChannel(),
-                 parent->mSubprocess.GetChildProcessHandle());
+    parent->mSubprocess->Launch();
+    parent->Open(parent->mSubprocess->GetChannel(),
+                 parent->mSubprocess->GetChildProcessHandle());
 
     return parent;
 }
 
 
-PluginModuleParent::PluginModuleParent(const char* aFilePath) :
-    mSubprocess(aFilePath)
+PluginModuleParent::PluginModuleParent(const char* aFilePath)
 {
+    mSubprocess = new PluginProcessParent(aFilePath);
+    NS_ASSERTION(mSubprocess, "Out of memory!");
+
 #ifdef DEBUG
     PRBool ok =
 #endif
     mValidIdentifiers.Init();
     NS_ASSERTION(ok, "Out of memory!");
 }
 
 PluginModuleParent::~PluginModuleParent()
 {
+    if (mSubprocess) {
+        mSubprocess->Delete();
+        mSubprocess = nsnull;
+    }
 }
 
 PPluginInstanceParent*
 PluginModuleParent::AllocPPluginInstance(const nsCString& aMimeType,
                                          const uint16_t& aMode,
                                          const nsTArray<nsCString>& aNames,
                                          const nsTArray<nsCString>& aValues,
                                          NPError* rv)
--- a/dom/plugins/PluginModuleParent.h
+++ b/dom/plugins/PluginModuleParent.h
@@ -195,17 +195,17 @@ private:
 #if defined(XP_WIN) || defined(XP_MACOSX)
     virtual nsresult NP_GetEntryPoints(NPPluginFuncs* pFuncs, NPError* error);
 #endif
     virtual nsresult NPP_New(NPMIMEType pluginType, NPP instance,
                              uint16_t mode, int16_t argc, char* argn[],
                              char* argv[], NPSavedData* saved,
                              NPError* error);
 private:
-    PluginProcessParent mSubprocess;
+    PluginProcessParent* mSubprocess;
     const NPNetscapeFuncs* mNPNIface;
     nsTHashtable<nsVoidPtrHashKey> mValidIdentifiers;
 };
 
 } // namespace plugins
 } // namespace mozilla
 
 #endif  // ifndef dom_plugins_PluginModuleParent_h
--- a/dom/plugins/PluginProcessParent.cpp
+++ b/dom/plugins/PluginProcessParent.cpp
@@ -35,22 +35,28 @@
  * the provisions above, a recipient may use your version of this file under
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */
 
 #include "mozilla/plugins/PluginProcessParent.h"
 
 #include "base/string_util.h"
+#include "mozilla/ipc/GeckoThread.h"
 
+using mozilla::ipc::BrowserProcessSubThread;
 using mozilla::ipc::GeckoChildProcessHost;
+using mozilla::plugins::PluginProcessParent;
 
-namespace mozilla {
-namespace plugins {
-
+template<>
+struct RunnableMethodTraits<PluginProcessParent>
+{
+    static void RetainCallee(PluginProcessParent* obj) { }
+    static void ReleaseCallee(PluginProcessParent* obj) { }
+};
 
 PluginProcessParent::PluginProcessParent(const std::string& aPluginFilePath) :
     GeckoChildProcessHost(GeckoProcessType_Plugin),
     mPluginFilePath(aPluginFilePath)
 {
   // XXXbent Need to catch crashing plugins by watching the process event!
 }
 
@@ -61,11 +67,23 @@ PluginProcessParent::~PluginProcessParen
 bool
 PluginProcessParent::Launch()
 {
     std::vector<std::string> args;
     args.push_back(mPluginFilePath);
     return SyncLaunch(args);
 }
 
+void
+PluginProcessParent::Delete()
+{
+  MessageLoop* currentLoop = MessageLoop::current();
+  MessageLoop* ioLoop = 
+    BrowserProcessSubThread::GetMessageLoop(BrowserProcessSubThread::IO);
 
-} // namespace plugins
-} // namespace mozilla
+  if (currentLoop == ioLoop) {
+      delete this;
+      return;
+  }
+
+  ioLoop->PostTask(FROM_HERE,
+                   NewRunnableMethod(this, &PluginProcessParent::Delete));
+}
--- a/dom/plugins/PluginProcessParent.h
+++ b/dom/plugins/PluginProcessParent.h
@@ -63,16 +63,18 @@ public:
     PluginProcessParent(const std::string& aPluginFilePath);
     ~PluginProcessParent();
 
     /**
      * Synchronously launch the plugin process.
      */
     bool Launch();
 
+    void Delete();
+
     virtual bool CanShutdown()
     {
         return true;
     }
 
     using mozilla::ipc::GeckoChildProcessHost::GetShutDownEvent;
     using mozilla::ipc::GeckoChildProcessHost::GetChannel;
     using mozilla::ipc::GeckoChildProcessHost::GetChildProcessHandle;
--- a/ipc/glue/AsyncChannel.cpp
+++ b/ipc/glue/AsyncChannel.cpp
@@ -101,20 +101,31 @@ AsyncChannel::Open(Transport* aTransport
     }
 
     return true;
 }
 
 void
 AsyncChannel::Close()
 {
-    mTransport->Close();
-    // don't lose error-state information
-    if (ChannelError != mChannelState)
-        mChannelState = ChannelClosed;
+    // XXXcjones pretty much stuck with this bad parent/child
+    // dichotomy as long as the child's IO thread is the "main"
+    // thread and we don't have proper shutdown handling implemented.
+    if (mChild)
+        return OnClose();
+
+    AssertWorkerThread();
+
+    MutexAutoLock lock(mMutex);
+
+    mIOLoop->PostTask(
+        FROM_HERE, NewRunnableMethod(this, &AsyncChannel::OnClose));
+
+    while (ChannelConnected == mChannelState)
+        mCvar.Wait();
 
     mTransport = NULL;
 }
 
 bool
 AsyncChannel::Send(Message* msg)
 {
     AssertWorkerThread();
@@ -269,11 +280,28 @@ AsyncChannel::OnChannelOpened()
 void
 AsyncChannel::OnSend(Message* aMsg)
 {
     AssertIOThread();
     mTransport->Send(aMsg);
     // mTransport deletes aMsg
 }
 
+void
+AsyncChannel::OnClose()
+{
+    AssertIOThread();
+
+    mTransport->Close();
+
+    // don't lose error-state information
+    if (ChannelError != mChannelState)
+        mChannelState = ChannelClosed;
+
+    if (!mChild) {
+        MutexAutoLock lock(mMutex);
+        mCvar.Notify();
+    }
+}
+
 
 } // namespace ipc
 } // namespace mozilla
--- a/ipc/glue/AsyncChannel.h
+++ b/ipc/glue/AsyncChannel.h
@@ -156,16 +156,17 @@ protected:
     {
         fprintf(stderr, "\n###!!! [%s][%s] Error: %s\n\n",
                 mChild ? "Child" : "Parent", channelName, msg);
     }
 
     // Run on the IO thread
     void OnChannelOpened();
     void OnSend(Message* aMsg);
+    void OnClose();
 
     Transport* mTransport;
     AsyncListener* mListener;
     ChannelState mChannelState;
     Mutex mMutex;
     CondVar mCvar;
     MessageLoop* mIOLoop;       // thread where IO happens
     MessageLoop* mWorkerLoop;   // thread where work is done
--- a/ipc/glue/GeckoChildProcessHost.cpp
+++ b/ipc/glue/GeckoChildProcessHost.cpp
@@ -59,16 +59,22 @@ GeckoChildProcessHost::GeckoChildProcess
                                              base::WaitableEventWatcher::Delegate* aDelegate)
   : ChildProcessHost(RENDER_PROCESS), // FIXME/cjones: we should own this enum
     mProcessType(aProcessType),
     mMonitor("mozilla.ipc.GeckChildProcessHost.mMonitor"),
     mLaunched(false),
     mDelegate(aDelegate),
     mChildProcessHandle(0)
 {
+    MOZ_COUNT_CTOR(GeckoChildProcessHost);
+}
+
+GeckoChildProcessHost::~GeckoChildProcessHost()
+{
+    MOZ_COUNT_DTOR(GeckoChildProcessHost);
 }
 
 bool
 GeckoChildProcessHost::SyncLaunch(std::vector<std::string> aExtraOpts)
 {
   MessageLoop* ioLoop = 
     BrowserProcessSubThread::GetMessageLoop(BrowserProcessSubThread::IO);
   NS_ASSERTION(MessageLoop::current() != ioLoop, "sync launch from the IO thread NYI");
--- a/ipc/glue/GeckoChildProcessHost.h
+++ b/ipc/glue/GeckoChildProcessHost.h
@@ -56,16 +56,18 @@ protected:
   typedef mozilla::Monitor Monitor;
 
 public:
   typedef base::ProcessHandle ProcessHandle;
 
   GeckoChildProcessHost(GeckoProcessType aProcessType=GeckoProcessType_Default,
                         base::WaitableEventWatcher::Delegate* aDelegate=nsnull);
 
+  ~GeckoChildProcessHost();
+
   bool SyncLaunch(std::vector<std::string> aExtraOpts=std::vector<std::string>());
   bool AsyncLaunch(std::vector<std::string> aExtraOpts=std::vector<std::string>());
 
   virtual void OnChannelConnected(int32 peer_pid);
   virtual void OnMessageReceived(const IPC::Message& aMsg);
   virtual void OnChannelError();
 
   virtual bool CanShutdown() { return true; }
--- a/ipc/ipdl/test/cxx/IPDLUnitTestThreadChild.cpp
+++ b/ipc/ipdl/test/cxx/IPDLUnitTestThreadChild.cpp
@@ -60,13 +60,12 @@ IPDLUnitTestThreadChild::Init()
     GeckoThread::Init();
     IPDLUnitTestChildInit(channel(), GetParentProcessHandle(), owner_loop());
 }
 
 void
 IPDLUnitTestThreadChild::CleanUp()
 {
     GeckoThread::CleanUp();
-    IPDLUnitTestChildCleanUp();
 }
 
 } // namespace _ipdltest
 } // namespace mozilla
--- a/ipc/ipdl/test/cxx/IPDLUnitTests.h
+++ b/ipc/ipdl/test/cxx/IPDLUnitTests.h
@@ -97,16 +97,14 @@ void IPDLUnitTestMain(void* aData);
 
 
 //-----------------------------------------------------------------------------
 // child process only
 
 void IPDLUnitTestChildInit(IPC::Channel* transport,
                            base::ProcessHandle parent,
                            MessageLoop* worker);
-void IPDLUnitTestChildCleanUp();
-
 
 } // namespace _ipdltest
 } // namespace mozilla
 
 
 #endif // ifndef mozilla__ipdltest_IPDLUnitTests_h
--- a/ipc/ipdl/test/cxx/IPDLUnitTests.template.cpp
+++ b/ipc/ipdl/test/cxx/IPDLUnitTests.template.cpp
@@ -183,27 +183,10 @@ IPDLUnitTestChildInit(IPC::Channel* tran
 //-----------------------------------------------------------------------------
 
     default:
         fail("not reached");
         return;                 // unreached
     }
 }
 
-void IPDLUnitTestChildCleanUp()
-{
-    if (!gChildActor)
-        return;
-
-    switch (IPDLUnitTest()) {
-//-----------------------------------------------------------------------------
-//===== TEMPLATED =====
-${CHILD_CLEANUP_CASES}
-//-----------------------------------------------------------------------------
-
-    default:
-        fail("not reached");
-        return;                 // unreached
-    }
-}
-
 } // namespace _ipdltest
 } // namespace mozilla
--- a/ipc/ipdl/test/cxx/genIPDLUnitTests.py
+++ b/ipc/ipdl/test/cxx/genIPDLUnitTests.py
@@ -75,33 +75,21 @@ def main(argv):
         %sChild** child =
             reinterpret_cast<%sChild**>(&gChildActor);
         *child = new %sChild();
         (*child)->Open(transport, parent, worker);
         return;
     }
 '''% (t, t, t, t) for t in unittests ])
 
-    child_cleanup_cases = '\n'.join([
-'''    case %s: {
-        %sChild** child =
-            reinterpret_cast<%sChild**>(&gChildActor);
-        delete *child;
-        *child = 0;
-        return;
-    }
-'''% (t, t, t) for t in unittests ])
-
-
     templatefile = open(template, 'r')
     sys.stdout.write(
         string.Template(templatefile.read()).substitute(
             INCLUDES=includes,
             ENUM_VALUES=enum_values, LAST_ENUM=last_enum,
             STRING_TO_ENUMS=string_to_enums,
             ENUM_TO_STRINGS=enum_to_strings,
             PARENT_MAIN_CASES=parent_main_cases,
-            CHILD_INIT_CASES=child_init_cases,
-            CHILD_CLEANUP_CASES=child_cleanup_cases))
+            CHILD_INIT_CASES=child_init_cases))
     templatefile.close()
 
 if __name__ == '__main__':
     main(sys.argv)