Added code to initiate controlled shutdown of plugins with timeouts for misbeahving plugin.

indra/llplugin/llpluginclassmedia.cpp

@@ -48,7 +48,6 @@ static int nextPowerOf2( int value )
 LLPluginClassMedia::LLPluginClassMedia(LLPluginClassMediaOwner *owner)
 {
 	mOwner = owner;
-	mPlugin = NULL;
 	reset();
 
 	//debug use
@@ -68,7 +67,7 @@ bool LLPluginClassMedia::init(const std::string &launcher_filename, const std::s
 	LL_DEBUGS("Plugin") << "dir: " << plugin_dir << LL_ENDL;
 	LL_DEBUGS("Plugin") << "plugin: " << plugin_filename << LL_ENDL;
 
-	mPlugin = new LLPluginProcessParent(this);
+	mPlugin = LLPluginProcessParent::create(this);
 	mPlugin->setSleepTime(mSleepTime);
 
 	// Queue up the media init message -- it will be sent after all the currently queued messages.
@@ -84,10 +83,10 @@ bool LLPluginClassMedia::init(const std::string &launcher_filename, const std::s
 
 void LLPluginClassMedia::reset()
 {
-	if(mPlugin != NULL)
+	if(mPlugin)
 	{
-		delete mPlugin;
-		mPlugin = NULL;
+        mPlugin->requestShutdown();
+        mPlugin.reset();
 	}
 
 	mTextureParamsReceived = false;

indra/llplugin/llpluginclassmedia.h

@@ -374,7 +374,7 @@ protected:
 	int			mPadding;
 	
 	
-	LLPluginProcessParent *mPlugin;
+	LLPluginProcessParent::ptr_t mPlugin;
 	
 	LLRect mDirtyRect;
 	

indra/llplugin/llpluginprocesschild.cpp

@@ -33,6 +33,7 @@
 #include "llpluginmessagepipe.h"
 #include "llpluginmessageclasses.h"
 
+static const F32 GOODBYE_SECONDS = 20.0f;
 static const F32 HEARTBEAT_SECONDS = 1.0f;
 static const F32 PLUGIN_IDLE_SECONDS = 1.0f / 100.0f;  // Each call to idle will give the plugin this much time.
 
@@ -194,33 +195,43 @@ void LLPluginProcessChild::idle(void)
 					}
 				}
 				// receivePluginMessage will transition to STATE_UNLOADING
-			break;
+			    break;
+
+            case STATE_SHUTDOWNREQ:
+                if (mInstance != NULL)
+                {
+                    sendMessageToPlugin(LLPluginMessage("base", "cleanup"));
+                    delete mInstance;
+                    mInstance = NULL;
+                }
+                setState(STATE_UNLOADING);
+                mWaitGoodbye.setTimerExpirySec(GOODBYE_SECONDS);
+                break;
 
 			case STATE_UNLOADING:
-				if(mInstance != NULL)
-				{
-					sendMessageToPlugin(LLPluginMessage("base", "cleanup"));
-					delete mInstance;
-					mInstance = NULL;
-				}
-				setState(STATE_UNLOADED);
-			break;
+                // waiting for goodbye from plugin.
+                if (mWaitGoodbye.hasExpired())
+                {
+                    LL_WARNS() << "Wait for goodbye expired.  Advancing to UNLOADED" << LL_ENDL;
+                    setState(STATE_UNLOADED);
+                }
+			    break;
 			
 			case STATE_UNLOADED:
 				killSockets();
 				setState(STATE_DONE);
-			break;
+			    break;
 
 			case STATE_ERROR:
 				// Close the socket to the launcher
 				killSockets();				
 				// TODO: Where do we go from here?  Just exit()?
 				setState(STATE_DONE);
-			break;
+			    break;
 			
 			case STATE_DONE:
 				// just sit here.
-			break;
+			    break;
 		}
 	
 	} while (idle_again);
@@ -350,6 +361,10 @@ void LLPluginProcessChild::receiveMessageRaw(const std::string &message)
 				mPluginFile = parsed.getValue("file");
 				mPluginDir = parsed.getValue("dir");
 			}
+            else if (message_name == "shutdown_plugin")
+            {
+                setState(STATE_SHUTDOWNREQ);
+            }
 			else if(message_name == "shm_add")
 			{
 				std::string name = parsed.getValue("name");
@@ -495,6 +510,10 @@ void LLPluginProcessChild::receivePluginMessage(const std::string &message)
 				// Let the parent know it's loaded and initialized.
 				sendMessageToParent(new_message);
 			}
+            else if (message_name == "goodbye")
+            {
+                setState(STATE_UNLOADED);
+            }
 			else if(message_name == "shm_remove_response")
 			{
 				// Don't pass this message up to the parent

indra/llplugin/llpluginprocesschild.h

@@ -80,6 +80,7 @@ private:
 		STATE_PLUGIN_LOADED,		// plugin library has been loaded
 		STATE_PLUGIN_INITIALIZING,	// plugin is processing init message
 		STATE_RUNNING,				// steady state (processing messages)
+        STATE_SHUTDOWNREQ,          // Parent has requested a shutdown.
 		STATE_UNLOADING,			// plugin has sent shutdown_response and needs to be unloaded
 		STATE_UNLOADED,				// plugin has been unloaded
 		STATE_ERROR,				// generic bailout state
@@ -101,12 +102,12 @@ private:
 	sharedMemoryRegionsType mSharedMemoryRegions;
 	
 	LLTimer mHeartbeat;
-	F64		mSleepTime;
-	F64		mCPUElapsed;
+    F64		mSleepTime;
+    F64		mCPUElapsed;
 	bool	mBlockingRequest;
 	bool	mBlockingResponseReceived;
 	std::queue<std::string> mMessageQueue;
-	
+    LLTimer mWaitGoodbye;
 	void deliverQueuedMessages();
 	
 };

indra/llplugin/llpluginprocessparent.cpp

@@ -46,7 +46,7 @@ bool LLPluginProcessParent::sUseReadThread = false;
 apr_pollset_t *LLPluginProcessParent::sPollSet = NULL;
 bool LLPluginProcessParent::sPollsetNeedsRebuild = false;
 LLMutex *LLPluginProcessParent::sInstancesMutex;
-std::list<LLPluginProcessParent*> LLPluginProcessParent::sInstances;
+LLPluginProcessParent::mapInstances_t LLPluginProcessParent::sInstances;
 LLThread *LLPluginProcessParent::sReadThread = NULL;
 
 
@@ -104,27 +104,12 @@ LLPluginProcessParent::LLPluginProcessParent(LLPluginProcessParentOwner *owner):
 	// Don't start the timer here -- start it when we actually launch the plugin process.
 	mHeartbeat.stop();
 	
-	// Don't add to the global list until fully constructed.
-	{
-		LLMutexLock lock(sInstancesMutex);
-		sInstances.push_back(this);
-	}
 }
 
 LLPluginProcessParent::~LLPluginProcessParent()
 {
 	LL_DEBUGS("Plugin") << "destructor" << LL_ENDL;
 
-	// Remove from the global list before beginning destruction.
-	{
-		// Make sure to get the global mutex _first_ here, to avoid a possible deadlock against LLPluginProcessParent::poll()
-		LLMutexLock lock(sInstancesMutex);
-		{
-			LLMutexLock lock2(&mIncomingQueueMutex);
-			sInstances.remove(this);
-		}
-	}
-
 	// Destroy any remaining shared memory regions
 	sharedMemoryRegionsType::iterator iter;
 	while((iter = mSharedMemoryRegions.begin()) != mSharedMemoryRegions.end())
@@ -139,9 +124,109 @@ LLPluginProcessParent::~LLPluginProcessParent()
 	}
 
 	LLProcess::kill(mProcess);
-	killSockets();
+    if (!LLApp::isQuitting())
+    {   // If we are quitting, the network sockets will already have been destroyed.
+        killSockets();
+    }
+}
+
+/*static*/
+LLPluginProcessParent::ptr_t LLPluginProcessParent::create(LLPluginProcessParentOwner *owner)
+{
+    ptr_t that(new LLPluginProcessParent(owner));
+
+    // Don't add to the global list until fully constructed.
+    {
+        LLMutexLock lock(sInstancesMutex);
+        sInstances.insert(mapInstances_t::value_type(that.get(), that));
+    }
+
+    return that;
+}
+
+/*static*/
+void LLPluginProcessParent::shutdown()
+{
+    LLMutexLock lock(sInstancesMutex);
+
+    mapInstances_t::iterator it;
+    for (it = sInstances.begin(); it != sInstances.end(); ++it)
+    {
+        (*it).second->setState(STATE_GOODBYE);
+        (*it).second->idle();
+    }
+    sInstances.clear();
+}
+
+
+void LLPluginProcessParent::requestShutdown()
+{
+    setState(STATE_GOODBYE);
+    mOwner = NULL;
+
+    if (LLApp::isQuitting())
+    {   // if we're quitting, run the idle once more
+        idle();
+        removeFromProcessing();
+        return;
+    }
+
+    static uint32_t count = 0;
+    std::stringstream namestream;
+
+    namestream << "LLPluginProcessParentListener" << ++count;
+
+    //*HACK!*//
+    // After requestShutdown has been called our previous owner will no longer call 
+    // our idle() method.  Tie into the event loop here to do that until we are good
+    // and finished.
+    LL_DEBUGS("LLPluginProcessParent") << "listening on \"mainloop\"" << LL_ENDL;
+    mPolling = LLEventPumps::instance().obtain("mainloop")
+        .listen(namestream.str(), boost::bind(&LLPluginProcessParent::pollTick, this));
+
+}
+
+bool LLPluginProcessParent::pollTick()
+{
+    if (isDone())
+    {
+        ptr_t that;
+        {
+            // this grabs a copy of the smart pointer to ourselves to ensure that we do not
+            // get destroyed until after this method returns.
+            LLMutexLock lock(sInstancesMutex);
+            mapInstances_t::iterator it = sInstances.find(this);
+            if (it != sInstances.end())
+                that = (*it).second;
+        }
+
+        removeFromProcessing();
+        return true;
+    }
+
+    idle();
+    return false;
 }
 
+void LLPluginProcessParent::removeFromProcessing()
+{
+    // Remove from the global list before beginning destruction.
+    {
+        // Make sure to get the global mutex _first_ here, to avoid a possible deadlock against LLPluginProcessParent::poll()
+        LLMutexLock lock(sInstancesMutex);
+        {
+            LLMutexLock lock2(&mIncomingQueueMutex);
+            sInstances.erase(this);
+        }
+    }
+}
+
+bool LLPluginProcessParent::wantsPolling() const
+{
+    return (mPollFD.client_data && (mState != STATE_DONE));
+}
+
+
 void LLPluginProcessParent::killSockets(void)
 {
 	{
@@ -371,48 +456,48 @@ void LLPluginProcessParent::idle(void)
 			break;
 			
 			case STATE_LISTENING:
-			{
-				// Launch the plugin process.
+			    {
+				    // Launch the plugin process.
 				
-				// Only argument to the launcher is the port number we're listening on
-				mProcessParams.args.add(stringize(mBoundPort));
-				if (! (mProcess = LLProcess::create(mProcessParams)))
-				{
-					errorState();
-				}
-				else
-				{
-					if(mDebug)
-					{
-						#if LL_DARWIN
-						// If we're set to debug, start up a gdb instance in a new terminal window and have it attach to the plugin process and continue.
+				    // Only argument to the launcher is the port number we're listening on
+				    mProcessParams.args.add(stringize(mBoundPort));
+				    if (! (mProcess = LLProcess::create(mProcessParams)))
+				    {
+					    errorState();
+				    }
+				    else
+				    {
+					    if(mDebug)
+					    {
+#if LL_DARWIN
+						    // If we're set to debug, start up a gdb instance in a new terminal window and have it attach to the plugin process and continue.
 						
-						// The command we're constructing would look like this on the command line:
-						// osascript -e 'tell application "Terminal"' -e 'set win to do script "gdb -pid 12345"' -e 'do script "continue" in win' -e 'end tell'
-
-						LLProcess::Params params;
-						params.executable = "/usr/bin/osascript";
-						params.args.add("-e");
-						params.args.add("tell application \"Terminal\"");
-						params.args.add("-e");
-						params.args.add(STRINGIZE("set win to do script \"gdb -pid "
-												  << mProcess->getProcessID() << "\""));
-						params.args.add("-e");
-						params.args.add("do script \"continue\" in win");
-						params.args.add("-e");
-						params.args.add("end tell");
-						mDebugger = LLProcess::create(params);
-
-						#endif
-					}
+						    // The command we're constructing would look like this on the command line:
+						    // osascript -e 'tell application "Terminal"' -e 'set win to do script "gdb -pid 12345"' -e 'do script "continue" in win' -e 'end tell'
+
+						    LLProcess::Params params;
+						    params.executable = "/usr/bin/osascript";
+						    params.args.add("-e");
+						    params.args.add("tell application \"Terminal\"");
+						    params.args.add("-e");
+						    params.args.add(STRINGIZE("set win to do script \"gdb -pid "
+												      << mProcess->getProcessID() << "\""));
+						    params.args.add("-e");
+						    params.args.add("do script \"continue\" in win");
+						    params.args.add("-e");
+						    params.args.add("end tell");
+						    mDebugger = LLProcess::create(params);
+
+#endif
+					    }
 					
-					// This will allow us to time out if the process never starts.
-					mHeartbeat.start();
-					mHeartbeat.setTimerExpirySec(mPluginLaunchTimeout);
-					setState(STATE_LAUNCHED);
-				}
-			}
-			break;
+					    // This will allow us to time out if the process never starts.
+					    mHeartbeat.start();
+					    mHeartbeat.setTimerExpirySec(mPluginLaunchTimeout);
+					    setState(STATE_LAUNCHED);
+				    }
+			    }
+			    break;
 
 			case STATE_LAUNCHED:
 				// waiting for the plugin to connect
@@ -430,7 +515,7 @@ void LLPluginProcessParent::idle(void)
 						setState(STATE_CONNECTED);
 					}
 				}
-			break;
+			    break;
 			
 			case STATE_CONNECTED:
 				// waiting for hello message from the plugin
@@ -439,7 +524,7 @@ void LLPluginProcessParent::idle(void)
 				{
 					errorState();
 				}
-			break;
+			    break;
 
 			case STATE_HELLO:
 				LL_DEBUGS("Plugin") << "received hello message" << LL_ENDL;
@@ -453,7 +538,7 @@ void LLPluginProcessParent::idle(void)
 				}
 
 				setState(STATE_LOADING);
-			break;
+			    break;
 			
 			case STATE_LOADING:
 				// The load_plugin_response message will kick us from here into STATE_RUNNING
@@ -461,15 +546,23 @@ void LLPluginProcessParent::idle(void)
 				{
 					errorState();
 				}
-			break;
+			    break;
 			
 			case STATE_RUNNING:
 				if(pluginLockedUpOrQuit())
 				{
 					errorState();
 				}
-			break;
+			    break;
 			
+            case STATE_GOODBYE:
+                {
+                    LLPluginMessage message(LLPLUGIN_MESSAGE_CLASS_INTERNAL, "shutdown_plugin");
+                    sendMessage(message);
+                }
+                setState(STATE_EXITING);
+                break;
+
 			case STATE_EXITING:
 				if (! LLProcess::isRunning(mProcess))
 				{
@@ -480,7 +573,7 @@ void LLPluginProcessParent::idle(void)
 					LL_WARNS("Plugin") << "timeout in exiting state, bailing out" << LL_ENDL;
 					errorState();
 				}
-			break;
+    			break;
 
 			case STATE_LAUNCH_FAILURE:
 				if(mOwner != NULL)
@@ -488,7 +581,7 @@ void LLPluginProcessParent::idle(void)
 					mOwner->pluginLaunchFailed();
 				}
 				setState(STATE_CLEANUP);
-			break;
+    			break;
 
 			case STATE_ERROR:
 				if(mOwner != NULL)
@@ -496,19 +589,18 @@ void LLPluginProcessParent::idle(void)
 					mOwner->pluginDied();
 				}
 				setState(STATE_CLEANUP);
-			break;
+			    break;
 			
 			case STATE_CLEANUP:
 				LLProcess::kill(mProcess);
 				killSockets();
 				setState(STATE_DONE);
-			break;
-			
+                dirtyPollSet();
+			    break;
 			
 			case STATE_DONE:
 				// just sit here.
-			break;
-			
+    			break;
 		}
 	
 	} while (idle_again);
@@ -651,14 +743,14 @@ void LLPluginProcessParent::updatePollset()
 		sPollSet = NULL;
 	}
 	
-	std::list<LLPluginProcessParent*>::iterator iter;
+    mapInstances_t::iterator iter;
 	int count = 0;
 	
 	// Count the number of instances that want to be in the pollset
 	for(iter = sInstances.begin(); iter != sInstances.end(); iter++)
 	{
-		(*iter)->mPolledInput = false;
-		if((*iter)->mPollFD.client_data)
+		(*iter).second->mPolledInput = false;
+        if ((*iter).second->wantsPolling())
 		{
 			// This instance has a socket that needs to be polled.
 			++count;
@@ -686,12 +778,12 @@ void LLPluginProcessParent::updatePollset()
 				// Pollset was created, add all instances to it.
 				for(iter = sInstances.begin(); iter != sInstances.end(); iter++)
 				{
-					if((*iter)->mPollFD.client_data)
+                    if ((*iter).second->wantsPolling())
 					{
-						status = apr_pollset_add(sPollSet, &((*iter)->mPollFD));
+						status = apr_pollset_add(sPollSet, &((*iter).second->mPollFD));
 						if(status == APR_SUCCESS)
 						{
-							(*iter)->mPolledInput = true;
+							(*iter).second->mPolledInput = true;
 						}
 						else
 						{
@@ -756,45 +848,27 @@ void LLPluginProcessParent::poll(F64 timeout)
 		if(status == APR_SUCCESS)
 		{
 			// One or more of the descriptors signalled.  Call them.
-			for(int i = 0; i < count; i++)
-			{
-				LLPluginProcessParent *self = (LLPluginProcessParent *)(descriptors[i].client_data);
-				// NOTE: the descriptor returned here is actually a COPY of the original (even though we create the pollset with APR_POLLSET_NOCOPY).
-				// This means that even if the parent has set its mPollFD.client_data to NULL, the old pointer may still there in this descriptor.
-				// It's even possible that the old pointer no longer points to a valid LLPluginProcessParent.
-				// This means that we can't safely dereference the 'self' pointer here without some extra steps...
-				if(self)
-				{
-					// Make sure this pointer is still in the instances list
-					bool valid = false;
-					{
-						LLMutexLock lock(sInstancesMutex);
-						for(std::list<LLPluginProcessParent*>::iterator iter = sInstances.begin(); iter != sInstances.end(); ++iter)
-						{
-							if(*iter == self)
-							{
-								// Lock the instance's mutex before unlocking the global mutex.  
-								// This avoids a possible race condition where the instance gets deleted between this check and the servicePoll() call.
-								self->mIncomingQueueMutex.lock();
-								valid = true;
-								break;
-							}
-						}
-					}
-					
-					if(valid)
-					{
-						// The instance is still valid.
-						// Pull incoming messages off the socket
-						self->servicePoll();
-						self->mIncomingQueueMutex.unlock();
-					}
-					else
-					{
-						LL_DEBUGS("PluginPoll") << "detected deleted instance " << self << LL_ENDL;
-					}
+            for (int i = 0; i < count; i++)
+            {
+                void *thatId = descriptors[i].client_data;
+
+                ptr_t that;
+                mapInstances_t::iterator it;
+
+                {
+                    LLMutexLock lock(sInstancesMutex);
+                    it = sInstances.find(thatId);
+                    if (it != sInstances.end())
+                        that = (*it).second;
+                }
+
+                if (that)
+                {
+                    that->mIncomingQueueMutex.lock();
+                    that->servicePoll();
+                    that->mIncomingQueueMutex.unlock();
+                }
 
-				}
 			}
 		}
 		else if(APR_STATUS_IS_TIMEUP(status))
@@ -812,6 +886,16 @@ void LLPluginProcessParent::poll(F64 timeout)
 			LL_WARNS("PluginPoll") << "apr_pollset_poll failed with status " << status << LL_ENDL;
 		}
 	}
+
+    // Remove instances in the done state from the sInstances map.
+    mapInstances_t::iterator itClean = sInstances.begin();
+    while (itClean != sInstances.end())
+    {
+        if ((*itClean).second->isDone())
+            sInstances.erase(itClean++);
+        else
+            ++itClean;
+    }
 }
 
 void LLPluginProcessParent::servicePoll()

indra/llplugin/llpluginprocessparent.h

@@ -30,6 +30,7 @@
 #define LL_LLPLUGINPROCESSPARENT_H
 
 #include <queue>
+#include <boost/enable_shared_from_this.hpp>
 
 #include "llapr.h"
 #include "llprocess.h"
@@ -40,8 +41,9 @@
 #include "lliosocket.h"
 #include "llthread.h"
 #include "llsd.h"
+#include "llevents.h"
 
-class LLPluginProcessParentOwner
+class LLPluginProcessParentOwner : public boost::enable_shared_from_this < LLPluginProcessParentOwner > 
 {
 public:
 	virtual ~LLPluginProcessParentOwner();
@@ -55,8 +57,11 @@ public:
 class LLPluginProcessParent : public LLPluginMessagePipeOwner
 {
 	LOG_CLASS(LLPluginProcessParent);
+
+    LLPluginProcessParent(LLPluginProcessParentOwner *owner);
 public:
-	LLPluginProcessParent(LLPluginProcessParentOwner *owner);
+    typedef boost::shared_ptr<LLPluginProcessParent> ptr_t;
+
 	~LLPluginProcessParent();
 		
 	void init(const std::string &launcher_filename, 
@@ -89,7 +94,10 @@ public:
 	void sendMessage(const LLPluginMessage &message);
 	
 	void receiveMessage(const LLPluginMessage &message);
-	
+
+    static ptr_t create(LLPluginProcessParentOwner *owner);
+    void requestShutdown();
+
 	// Inherited from LLPluginMessagePipeOwner
 	/*virtual*/ void receiveMessageRaw(const std::string &message);
 	/*virtual*/ void receiveMessageEarly(const LLPluginMessage &message);
@@ -121,7 +129,10 @@ public:
 	static bool canPollThreadRun() { return (sPollSet || sPollsetNeedsRebuild || sUseReadThread); };
 	static void setUseReadThread(bool use_read_thread);
 	static bool getUseReadThread() { return sUseReadThread; };
+
+    static void shutdown();
 private:
+    typedef std::map<void *, ptr_t> mapInstances_t;
 
 	enum EState
 	{
@@ -133,6 +144,7 @@ private:
 		STATE_HELLO,			// first message from the plugin process has been received
 		STATE_LOADING,			// process has been asked to load the plugin
 		STATE_RUNNING,			// 
+        STATE_GOODBYE,
 		STATE_LAUNCH_FAILURE,	// Failure before plugin loaded
 		STATE_ERROR,			// generic bailout state
 		STATE_CLEANUP,			// clean everything up
@@ -143,6 +155,9 @@ private:
 	EState mState;
 	void setState(EState state);
 
+    bool wantsPolling() const;
+    void removeFromProcessing();
+
 	bool pluginLockedUp();
 	bool pluginLockedUpOrQuit();
 
@@ -185,12 +200,15 @@ private:
 	static apr_pollset_t *sPollSet;
 	static bool sPollsetNeedsRebuild;
 	static LLMutex *sInstancesMutex;
-	static std::list<LLPluginProcessParent*> sInstances;
+    static mapInstances_t sInstances;
 	static void dirtyPollSet();
 	static void updatePollset();
 	void servicePoll();
 	static LLThread *sReadThread;
-	
+
+    LLTempBoundListener mPolling;
+    bool pollTick();
+
 	LLMutex mIncomingQueueMutex;
 	std::queue<LLPluginMessage> mIncomingQueue;
 };

indra/newview/llappviewer.cpp

@@ -1730,6 +1730,9 @@ bool LLAppViewer::cleanup()
 	// to ensure shutdown order
 	LLMortician::setZealous(TRUE);
 
+    // Give any remaining SLPlugin instances a chance to exit cleanly.
+    LLPluginProcessParent::shutdown();
+
 	LLVoiceClient::getInstance()->terminate();
 	
 	disconnectViewer();

indra/newview/tests/llmediadataclient_test.cpp

@@ -26,7 +26,7 @@
 
 #include "linden_common.h"
 #include "../llviewerprecompiledheaders.h"
- 
+
 #include <iostream>
 #include "../test/lltut.h"