From fb0fc07579359c0055fc4ad92a005aa89dc9b817 Mon Sep 17 00:00:00 2001
From: Mattes D <github@xoft.cz>
Date: Fri, 13 Jan 2017 10:31:05 +0100
Subject: Fixed race conditions and forgotten clear in Lua ref tracking.
 (#3530)

This fixes occasional crashes on plugin reload.
---
 src/Bindings/LuaState.cpp | 11 +++++++----
 src/Bindings/LuaState.h   | 24 +++++++++++++++---------
 2 files changed, 22 insertions(+), 13 deletions(-)

(limited to 'src/Bindings')

diff --git a/src/Bindings/LuaState.cpp b/src/Bindings/LuaState.cpp
index 61d206109..b768b21a3 100644
--- a/src/Bindings/LuaState.cpp
+++ b/src/Bindings/LuaState.cpp
@@ -218,7 +218,7 @@ void cLuaState::cTrackedRef::Clear(void)
 	// Free the reference:
 	lua_State * luaState = nullptr;
 	{
-		auto cs = m_CS;
+		auto cs = m_CS.load();
 		if (cs != nullptr)
 		{
 			cCSLock Lock(*cs);
@@ -246,7 +246,7 @@ void cLuaState::cTrackedRef::Clear(void)
 
 bool cLuaState::cTrackedRef::IsValid(void)
 {
-	auto cs = m_CS;
+	auto cs = m_CS.load();
 	if (cs == nullptr)
 	{
 		return false;
@@ -261,7 +261,7 @@ bool cLuaState::cTrackedRef::IsValid(void)
 
 bool cLuaState::cTrackedRef::IsSameLuaState(cLuaState & a_LuaState)
 {
-	auto cs = m_CS;
+	auto cs = m_CS.load();
 	if (cs == nullptr)
 	{
 		return false;
@@ -285,7 +285,7 @@ bool cLuaState::cTrackedRef::IsSameLuaState(cLuaState & a_LuaState)
 
 void cLuaState::cTrackedRef::Invalidate(void)
 {
-	auto cs = m_CS;
+	auto cs = m_CS.load();
 	if (cs == nullptr)
 	{
 		// Already invalid
@@ -548,6 +548,7 @@ void cLuaState::Close(void)
 		{
 			r->Invalidate();
 		}
+		m_TrackedRefs.clear();
 	}
 
 	cCanonLuaStates::Remove(*this);
@@ -2357,6 +2358,7 @@ void cLuaState::cRef::RefStack(cLuaState & a_LuaState, int a_StackPos)
 	{
 		UnRef();
 	}
+	ASSERT(cCanonLuaStates::GetCanonState(a_LuaState)->m_CS.IsLockedByCurrentThread());
 	m_LuaState = a_LuaState;
 	lua_pushvalue(a_LuaState, a_StackPos);  // Push a copy of the value at a_StackPos onto the stack
 	m_Ref = luaL_ref(a_LuaState, LUA_REGISTRYINDEX);
@@ -2370,6 +2372,7 @@ void cLuaState::cRef::UnRef(void)
 {
 	if (IsValid())
 	{
+		ASSERT(cCanonLuaStates::GetCanonState(m_LuaState)->m_CS.IsLockedByCurrentThread());
 		luaL_unref(m_LuaState, LUA_REGISTRYINDEX, m_Ref);
 	}
 	m_LuaState = nullptr;
diff --git a/src/Bindings/LuaState.h b/src/Bindings/LuaState.h
index 01faf2c26..80f5c3cc6 100644
--- a/src/Bindings/LuaState.h
+++ b/src/Bindings/LuaState.h
@@ -35,6 +35,7 @@ extern "C"
 	#include "lua/src/lauxlib.h"
 }
 
+#include <atomic>
 #include "../Vector3.h"
 #include "../Defines.h"
 #include "PluginManager.h"
@@ -189,24 +190,29 @@ public:
 		}
 
 		/** Set the contained reference to the object at the specified Lua state's stack position.
-		If another reference has been previously contained, it is freed first. */
+		If another reference has been previously contained, it is Clear()-ed first. */
 		bool RefStack(cLuaState & a_LuaState, int a_StackPos);
 
-		/** Frees the contained reference, if any. */
+		/** Frees the contained reference, if any.
+		Untracks the reference from its canon Lua state. */
 		void Clear(void);
 
-		/** Returns true if the contained reference is valid. */
+		/** Returns true if the contained reference is valid.
+		(Note that depending on this value is not thread-safe, another thread may invalidate the ref in the meantime. It is meant for quick ASSERTs only). */
 		bool IsValid(void);
 
 		/** Returns true if the reference resides in the specified Lua state.
-		Internally, compares the reference's canon Lua state. */
+		Internally, compares the reference's canon Lua state.
+		(Note that depending on this value is not thread-safe, another thread may modify the ref in the meantime. It is meant for quick ASSERTs only). */
 		bool IsSameLuaState(cLuaState & a_LuaState);
 
 	protected:
 		friend class cLuaState;
 
-		/** The mutex protecting m_Ref against multithreaded access */
-		cCriticalSection * m_CS;
+		/** The mutex protecting m_Ref against multithreaded access.
+		Actually points to the canon Lua state's m_CriticalSection.
+		Is nullptr when ref is empty (not bound). */
+		std::atomic<cCriticalSection *> m_CS;
 
 		/** Reference to the Lua callback */
 		cRef m_Ref;
@@ -254,7 +260,7 @@ public:
 		template <typename... Args>
 		bool Call(Args &&... args)
 		{
-			auto cs = m_CS;
+			auto cs = m_CS.load();
 			if (cs == nullptr)
 			{
 				return false;
@@ -336,7 +342,7 @@ public:
 		template <typename... Args>
 		bool CallTableFn(const char * a_FnName, Args &&... args)
 		{
-			auto cs = m_CS;
+			auto cs = m_CS.load();
 			if (cs == nullptr)
 			{
 				return false;
@@ -356,7 +362,7 @@ public:
 		template <typename... Args>
 		bool CallTableFnWithSelf(const char * a_FnName, Args &&... args)
 		{
-			auto cs = m_CS;
+			auto cs = m_CS.load();
 			if (cs == nullptr)
 			{
 				return false;
-- 
cgit v1.2.3