Delay unloading of plugins until it is absolutely safe (bug 5034, r=ds).

core/metamod_console.cpp

@@ -428,6 +428,12 @@ bool Command_Meta(IMetamodSourceCommandInfo *info)
 				bool already;
 				CPluginManager::CPlugin *pl;
 
+				// If we've recently tried to unload plugins, they might still
+				// be in the unload queue. Force them out now. This is not
+				// lowered to CPluginManager because it's not strictly safe
+				// there.
+				g_SourceHook.ResolvePendingUnloads(true);
+
 				PluginId id = g_PluginMngr.Load(full_path, Pl_Console, already, error, sizeof(error));
 				pl = g_PluginMngr.FindById(id);
 				if (!pl || id < Pl_MinId || (pl->m_Status < Pl_Paused))

core/metamod_plugins.cpp

@@ -382,6 +382,36 @@ const char *CPluginManager::GetStatusText(CPlugin *pl)
 	}
 }
 
+struct Unloader : public SourceHook::Impl::UnloadListener
+{
+	CPluginManager::CPlugin *plugin_;
+	bool destroy_;
+
+	Unloader(CPluginManager::CPlugin *plugin, bool destroy)
+	  : plugin_(plugin), destroy_(destroy)
+	{ }
+
+	void ReadyToUnload(SourceHook::Plugin plug)
+	{
+		if (plugin_->m_UnloadFn != NULL)
+			plugin_->m_UnloadFn();
+
+		dlclose(plugin_->m_Lib);
+
+		if (destroy_)
+		{
+			delete plugin_;
+		}
+		else
+		{
+			plugin_->m_Lib = NULL;
+			plugin_->m_API = NULL;
+		}
+
+		delete this;
+	}
+};
+
 CPluginManager::CPlugin *CPluginManager::_Load(const char *file, PluginId source, char *error, size_t maxlen)
 {
 	FILE *fp;
@@ -552,17 +582,8 @@ CPluginManager::CPlugin *CPluginManager::_Load(const char *file, PluginId source
 	if (pl->m_Lib && (pl->m_Status < Pl_Paused))
 	{
 		pl->m_Events.clear();
-		g_SourceHook.UnloadPlugin(pl->m_Id);
 		UnregAllConCmds(pl);
-
-		if (pl->m_UnloadFn != NULL)
-		{
-			pl->m_UnloadFn();
-		}
-
-		dlclose(pl->m_Lib);
-		pl->m_Lib = NULL;
-		pl->m_API = NULL;
+		g_SourceHook.UnloadPlugin(pl->m_Id, new Unloader(pl, false));
 	}
 
 	return pl;
@@ -580,23 +601,9 @@ bool CPluginManager::_Unload(CPluginManager::CPlugin *pl, bool force, char *erro
 		//Note, we'll always tell the plugin it will be unloading...
 		if (pl->m_API->Unload(error, maxlen) || force)
 		{
-			//Make sure to detach it from sourcehook!
-			g_SourceHook.UnloadPlugin(pl->m_Id);
-
 			pl->m_Events.clear();
-
 			UnregAllConCmds(pl);
 
-			if (pl->m_UnloadFn != NULL)
-			{
-				pl->m_UnloadFn();
-			}
-
-			//Clean up the DLL
-			dlclose(pl->m_Lib);
-			pl->m_Lib = NULL;
-			pl->m_API = NULL;
-
 			//Remove the plugin from the list
 			PluginIter i;
 			for (i=m_Plugins.begin(); i!=m_Plugins.end(); i++)
@@ -607,9 +614,9 @@ bool CPluginManager::_Unload(CPluginManager::CPlugin *pl, bool force, char *erro
 					break;
 				}
 			}
-			//Free its memory
-			delete pl;
 
+			//Make sure to detach it from sourcehook!
+			g_SourceHook.UnloadPlugin(pl->m_Id, new Unloader(pl, true));
 			return true;
 		}
 	} else {

core/sourcehook/sourcehook.cpp

@@ -1,4 +1,5 @@
 /* ======== SourceHook ========
+* vim: set ts=4 sw=4 tw=99 noet:
 * Copyright (C) 2004-2010 Metamod:Source Development Team
 * No warranties of any kind
 *
@@ -357,7 +358,7 @@ namespace SourceHook
 			return m_ContextStack.front().pOverrideRet;
 		}
 
-		void CSourceHookImpl::UnloadPlugin(Plugin plug)
+		void CSourceHookImpl::UnloadPlugin(Plugin plug, UnloadListener *listener)
 		{
 			// 1) Remove all hooks by this plugin
 
@@ -375,6 +376,20 @@ namespace SourceHook
 				else
 					++iter;
 			}
+
+			// Fix for bug 5034. It's pretty tricky to find whether hookmanagers owned by this
+			// plugin are active. We could change how hook managers are tracked in SH, and lazily
+			// free them as the context stack drops to 0, or we could change the pubfunc API to
+			// know whether it's active or not. Rather than deal with this extra complexity, we
+			// just conservatively wait until the context stack hits 0 before unloading.
+			if (m_ContextStack.size() == 0)
+			{
+				listener->ReadyToUnload(plug);
+			}
+			else
+			{
+				m_PendingUnloads.push_back(new PendingUnload(listener, plug));
+			}
 		}
 
 		CHookManList::iterator CSourceHookImpl::RemoveHookManager(CHookManList::iterator hookman_iter)
@@ -571,12 +586,41 @@ namespace SourceHook
 			return pCtx;
 		}
 
+		void CSourceHookImpl::ResolvePendingUnloads(bool force)
+		{
+			List<PendingUnload *>::iterator iter = m_PendingUnloads.begin();
+			while (iter != m_PendingUnloads.end())
+			{
+				PendingUnload *unload = *iter;
+
+				if (!force && !unload->deactivated())
+				{
+					// Unless being forced, wait one drop of the context stack
+					// before actually unloading. Otherwise, we'll still return
+					// to unloaded memory.
+					unload->deactivate();
+					iter++;
+				}
+				else
+				{
+					unload->listener()->ReadyToUnload(unload->plugin());
+					delete unload;
+					iter = m_PendingUnloads.erase(iter);
+				}
+			}
+		}
+
 		void CSourceHookImpl::EndContext(IHookContext *pCtx)
 		{
 			// Do clean up task, if any is associated with this context
 			m_ContextStack.front().DoCleanupTaskAndDeleteIt();
 			// Then remove it
 			m_ContextStack.pop();
+
+			// If we've reached 0 contexts and there are pending unloads,
+			// resolve them now.
+			if (m_ContextStack.size() == 0 && m_PendingUnloads.size() != 0)
+				ResolvePendingUnloads();
 		}
 
 		void CSourceHookImpl::CompleteShutdown()

core/sourcehook/sourcehook_impl.h

@@ -1,4 +1,5 @@
 /* ======== SourceHook ========
+* vim: set ts=4 sw=4 tw=99 noet:
 * Copyright (C) 2004-2010 Metamod:Source Development Team
 * No warranties of any kind
 *
@@ -254,7 +255,43 @@ namespace SourceHook
 		};
 
 		typedef CStack<CHookContext> HookContextStack;
-		
+
+		class UnloadListener
+		{
+		public:
+			virtual void ReadyToUnload(Plugin plug) = 0;
+		};
+
+		class PendingUnload
+		{
+			UnloadListener *listener_;
+			Plugin plug_;
+			bool deactivated_;
+
+		public:
+			PendingUnload(UnloadListener *listener, Plugin plug)
+			  : listener_(listener), plug_(plug), deactivated_(false)
+			{ }
+
+			Plugin plugin() const
+			{
+				return plug_;
+			}
+
+			UnloadListener *listener() const
+			{
+				return listener_;
+			}
+
+			void deactivate()
+			{
+				deactivated_ = true;
+			}
+			bool deactivated() const
+			{
+				return deactivated_;
+			}
+		};
 
 		class CSourceHookImpl : public ISourceHook
 		{
@@ -263,6 +300,7 @@ namespace SourceHook
 			CVfnPtrList m_VfnPtrs;
 			CHookIDManager m_HookIDMan;
 			HookContextStack m_ContextStack;
+			List<PendingUnload *> m_PendingUnloads;
 
 			bool SetHookPaused(int hookid, bool paused);
 			CHookManList::iterator RemoveHookManager(CHookManList::iterator iter);
@@ -304,10 +342,16 @@ namespace SourceHook
 
 			void *GetOverrideRetPtr();				//!< Used for setting the override return value
 
-			/**
-			*	@brief Make sure that a plugin is not used by any other plugins anymore, and unregister all its hook managers
-			*/
-			void UnloadPlugin(Plugin plug);
+			/* 
+			 * @brief Make sure that a plugin is not used by any
+			 * other plugins anymore, and unregister all its hook
+			 * managers. If any hooks owned by this plugin are
+			 * still on the callstack, defers notifying the listener
+			 * until the count has dropped to 0.
+			 */
+			void UnloadPlugin(Plugin plug, UnloadListener *listener);
+
+			void ResolvePendingUnloads(bool force = false);
 
 			void RemoveHookManager(Plugin plug, HookManagerPubFunc pubFunc);