Fixed a few memory leaks as well as the CPlayer/SColour crash-on-exit bug

This was SVN commit r1756.

source/lib/res/snd.cpp

@@ -178,10 +178,13 @@ static ALCdevice* alc_dev=NULL;
 
 static void alc_shutdown()
 {
-	if (alc_ctx && alc_dev)
+	if (alc_dev)
 	{
-		alcMakeContextCurrent(0);
+		if (alc_ctx)
-		alcDestroyContext(alc_ctx);
+		{
+			alcMakeContextCurrent(0);
+			alcDestroyContext(alc_ctx);
+		}
 		alcCloseDevice(alc_dev);
 	}
 }

source/main.cpp

@@ -422,6 +422,17 @@ static int handler(const SDL_Event* ev)
 
 void EndGame()
 {
+	if (g_NetServer)
+	{
+		delete g_NetServer;
+		g_NetServer=NULL;
+	}
+	else if (g_NetClient)
+	{
+		delete g_NetClient;
+		g_NetServer=NULL;
+	}
+
 	delete g_Game;
 	g_Game=NULL;
 }
@@ -921,7 +932,7 @@ static void Shutdown()
 	psShutdown(); // Must delete g_GUI before g_ScriptingHost
 
 	if (g_Game)
-		delete g_Game;
+		EndGame();
 
 	delete &g_Scheduler;
 

source/ps/ConfigDB.cpp

@@ -276,14 +276,15 @@ bool CConfigDB::Reload(EConfigNamespace ns)
 	TConfigMap newMap;
 	
 	char *filebuf=(char *)buffer;
+	char *filebufend=filebuf+buflen;
 	
 	// Read file line by line
 	char *next=filebuf-1;
 	do
 	{
 		char *pos=next+1;
-		next=strchr(pos, '\n');
+		next=(char *)memchr(pos, '\n', filebufend-pos);
-		if (!next) next=filebuf+buflen;
+		if (!next) next=filebufend;
 
 		char *lend=next;
 		if (*(lend-1) == '\r') lend--;
@@ -315,7 +316,7 @@ bool CConfigDB::Reload(EConfigNamespace ns)
 			}
 		}
 	}
-	while (next < filebuf+buflen);
+	while (next < filebufend);
 	
 	m_Map[ns].swap(newMap);
 	

source/ps/Game.cpp

@@ -16,9 +16,17 @@ namespace PlayerArray_JS
 		if (!JSVAL_IS_INT(id))
 			return JS_FALSE;
 		uint index=g_ScriptingHost.ValueToInt(id);
-		uint numPlayers=pInstance->m_NumPlayers;
+		
+		// Clamp to a preset upper bound.
+		// FIXME I guess we'll ultimately have max players as a config variable
+		if (pInstance->m_NumPlayers > PS_MAX_PLAYERS)
+			pInstance->m_NumPlayers = PS_MAX_PLAYERS;
 
-		if (numPlayers+1 > pInstance->m_Players.size())
+		// Resize array according to new number of players (note that the array
+		// size will go up to PS_MAX_PLAYERS, but will never be made smaller -
+		// this to avoid losing player pointers and make sure they are all
+		// reclaimed in the end - it's just simpler that way ;-) )
+		if (pInstance->m_NumPlayers+1 > pInstance->m_Players.size())
 		{
 			for (size_t i=pInstance->m_Players.size();i<=index;i++)
 			{
@@ -28,7 +36,7 @@ namespace PlayerArray_JS
 			}
 		}
 
-		if (index > numPlayers)
+		if (index > pInstance->m_NumPlayers)
 			return JS_FALSE;
 
 		*vp=OBJECT_TO_JSVAL(pInstance->m_Players[index]->GetScript());
@@ -185,9 +193,6 @@ CGame::CGame():
 
 CGame::~CGame()
 {
-	for (size_t i=0; i<m_Players.size(); ++i)
-		delete m_Players[i];
-
 	debug_out("CGame::~CGame(): Game object DESTROYED\n");
 }
 
@@ -195,19 +200,13 @@ PSRETURN CGame::StartGame(CGameAttributes *pAttribs)
 {
 	try
 	{
-		// Free old memory
-		for (size_t i=0; i<m_Players.size(); ++i)
-			delete m_Players[i];
-
-		//m_NumPlayers=pAttribs->GetValue("numPlayers").ToUInt();
 		m_NumPlayers=pAttribs->m_NumPlayers;
 
 		// Note: If m_Players is resized after this point (causing a reallocation)
 		// various bits of code will still contain pointers to data at the original
 		// locations. This is seldom a good thing. Make it big enough here.
 
-		// Player 0 = Gaia
+		// Player 0 = Gaia - allocate one extra
-
 		m_Players.resize(m_NumPlayers + 1);
 
 		for (uint i=0;i <= m_NumPlayers;i++)

source/ps/Network/Client.cpp

@@ -21,6 +21,7 @@ enum CClientEvents
 	CLIENT_EVENT_START_GAME,
 	CLIENT_EVENT_CHAT,
 	CLIENT_EVENT_CONNECT_COMPLETE,
+	CLIENT_EVENT_DISCONNECT,
 	CLIENT_EVENT_LAST
 };
 
@@ -36,6 +37,7 @@ class CChatEvent: public CScriptEvent
 {
 	CStrW m_Sender;
 	CStrW m_Message;
+	
 public:
 	CChatEvent(CStrW sender, CStrW message):
 		CScriptEvent(L"chat", false, CLIENT_EVENT_CHAT),
@@ -51,6 +53,7 @@ class CConnectCompleteEvent: public CScriptEvent
 {
 	CStrW m_Message;
 	bool m_Success;
+	
 public:
 	CConnectCompleteEvent(CStrW message, bool success):
 		CScriptEvent(L"connectComplete", false, CLIENT_EVENT_CONNECT_COMPLETE),
@@ -62,6 +65,19 @@ public:
 	}
 };
 
+class CDisconnectEvent: public CScriptEvent
+{
+	CStrW m_Message;
+	
+public:
+	CDisconnectEvent(CStrW message):
+		CScriptEvent(L"disconnect", false, CLIENT_EVENT_DISCONNECT),
+		m_Message(message)
+	{
+		AddReadOnlyProperty(L"message", &m_Message);
+	}
+};
+
 CNetClient::CNetClient(CGame *pGame, CGameAttributes *pGameAttribs):
 	CNetSession(ConnectHandler),
 	m_pLocalPlayer(NULL),
@@ -137,6 +153,9 @@ bool CNetClient::<X>Handler(CNetMessage *pMsg, CNetSession *pSession)
 #define UNHANDLED(_pMsg) return false;
 #define HANDLED(_pMsg) delete _pMsg; return true;
 #define TAKEN(_pMsg) return true;
+// Uglily assumes the arguments are called pMsg and pSession (which they are
+// all through this file)
+#define CHAIN(_chainHandler) STMT(if (_chainHandler(pMsg, pSession)) return true;)
 
 bool CNetClient::ConnectHandler(CNetMessage *pMsg, CNetSession *pSession)
 {
@@ -149,7 +168,7 @@ bool CNetClient::ConnectHandler(CNetMessage *pMsg, CNetSession *pSession)
 		pClient->m_pMessageHandler=HandshakeHandler;
 		if (pClient->m_OnConnectComplete.Defined())
 		{
-			CConnectCompleteEvent evt=CConnectCompleteEvent(CStr(PS_OK), true);
+			CConnectCompleteEvent evt(CStr((char *)PS_OK), true);
 			pClient->m_OnConnectComplete.DispatchEvent(pClient->GetScript(), &evt);
 		}
 		break;
@@ -159,7 +178,7 @@ bool CNetClient::ConnectHandler(CNetMessage *pMsg, CNetSession *pSession)
 		LOG(ERROR, LOG_CAT_NET, "CNetClient::ConnectHandler(): Connect Failed: %s", msg->m_Error);
 		if (pClient->m_OnConnectComplete.Defined())
 		{
-			CConnectCompleteEvent evt=CConnectCompleteEvent(CStr(msg->m_Error), false);
+			CConnectCompleteEvent evt(CStr(msg->m_Error), false);
 			pClient->m_OnConnectComplete.DispatchEvent(pClient->GetScript(), &evt);
 		}
 		break;
@@ -170,9 +189,36 @@ bool CNetClient::ConnectHandler(CNetMessage *pMsg, CNetSession *pSession)
 	HANDLED(pMsg);
 }
 
+bool CNetClient::BaseHandler(CNetMessage *pMsg, CNetSession *pSession)
+{
+	CNetClient *pClient=(CNetClient *)pSession;
+	LOG(NORMAL, LOG_CAT_NET, "CNetClient::BaseHandler(): %s.", pMsg->GetString().c_str());
+	switch (pMsg->GetType())
+	{
+	case NMT_ERROR:
+	{
+		CNetErrorMessage *msg=(CNetErrorMessage *)pMsg;
+		if (msg->m_State == SS_UNCONNECTED)
+		{
+			CStr message=msg->m_Error;
+			CDisconnectEvent evt(message);
+			if (pClient->m_OnDisconnect.Defined())
+				pClient->m_OnDisconnect.DispatchEvent(pClient->GetScript(), &evt);
+		}
+		break;
+	}
+	default:
+		UNHANDLED(pMsg);
+	}
+	HANDLED(pMsg);
+}
+
 bool CNetClient::HandshakeHandler(CNetMessage *pMsg, CNetSession *pSession)
 {
 	CNetClient *pClient=(CNetClient *)pSession;
+	
+	CHAIN(BaseHandler);
+	
 	LOG(NORMAL, LOG_CAT_NET, "CNetClient::HandshakeHandler(): %s.", pMsg->GetString().c_str());
 	switch (pMsg->GetType())
 	{
@@ -205,6 +251,9 @@ bool CNetClient::HandshakeHandler(CNetMessage *pMsg, CNetSession *pSession)
 bool CNetClient::AuthenticateHandler(CNetMessage *pMsg, CNetSession *pSession)
 {
 	CNetClient *pClient=(CNetClient *)pSession;
+	
+	CHAIN(BaseHandler);
+	
 	LOG(NORMAL, LOG_CAT_NET, "CNetClient::AuthenticateHandler(): %s.", pMsg->GetString().c_str());
 	switch (pMsg->GetType())
 	{
@@ -231,11 +280,12 @@ bool CNetClient::AuthenticateHandler(CNetMessage *pMsg, CNetSession *pSession)
 bool CNetClient::PreGameHandler(CNetMessage *pMsg, CNetSession *pSession)
 {
 	CNetClient *pClient=(CNetClient *)pSession;
+	
+	CHAIN(BaseHandler);
+	CHAIN(ChatHandler);
+	
 	LOG(NORMAL, LOG_CAT_NET, "CNetClient::PreGameHandler(): %s.", pMsg->GetString().c_str());
 
-	if (ChatHandler(pMsg, pSession))
-		return true;
-
 	switch (pMsg->GetType())
 	{
 		case NMT_StartGame:
@@ -267,6 +317,9 @@ bool CNetClient::InGameHandler(CNetMessage *pMsg, CNetSession *pSession)
 	CNetClient *pClient=(CNetClient *)pSession;
 	ENetMessageType msgType=pMsg->GetType();
 
+	CHAIN(BaseHandler);
+	CHAIN(ChatHandler);
+
 	if (msgType != NMT_EndCommandBatch)
 		LOG(NORMAL, LOG_CAT_NET, "CNetClient::InGameHandler(): %s.", pMsg->GetString().c_str());
 
@@ -296,9 +349,6 @@ bool CNetClient::InGameHandler(CNetMessage *pMsg, CNetSession *pSession)
 		HANDLED(pMsg);
 	}
 
-	if (ChatHandler(pMsg, pSession))
-		return true;
-
 	UNHANDLED(pMsg);
 }
 

source/ps/Network/Client.h

@@ -22,6 +22,7 @@ class CNetClient: public CNetSession, protected CTurnManager, public CJSObject<C
 	CScriptObject m_OnStartGame;
 	CScriptObject m_OnChat;
 	CScriptObject m_OnConnectComplete;
+	CScriptObject m_OnDisconnect;
 
 protected:
 	virtual void NewTurn();
@@ -40,10 +41,13 @@ public:
 	}
 	
 	static MessageHandler ConnectHandler;
+
+	static MessageHandler BaseHandler; // Common to all connected states
 	static MessageHandler HandshakeHandler;
 	static MessageHandler AuthenticateHandler;
+
+	static MessageHandler ChatHandler; // Common to pre-game and later
 	static MessageHandler PreGameHandler;
-	static MessageHandler ChatHandler;
 	static MessageHandler InGameHandler;
 	
 	bool JSI_BeginConnect(JSContext *cx, uintN argc, jsval *argv);

source/ps/Network/Server.cpp

@@ -42,6 +42,7 @@ void CNetServer::OnAccept(const CSocketAddress &addr)
 CNetServer::CNetServer(CGame *pGame, CGameAttributes *pGameAttribs):
 	m_pGame(pGame),
 	m_pGameAttributes(pGameAttribs),
+	m_NumPlayers(pGameAttribs->m_NumPlayers),
 	m_MaxObservers(5),
 	m_ServerPlayerName(L"Noname Server Player"),
 	m_ServerName(L"Noname Server"),
@@ -62,7 +63,6 @@ CNetServer::CNetServer(CGame *pGame, CGameAttributes *pGameAttribs):
 
 	m_pGameAttributes->SetUpdateCallback(AttributeUpdate, this);
 	m_pGameAttributes->SetPlayerUpdateCallback(PlayerAttributeUpdate, this);
-	m_NumPlayers=m_pGameAttributes->m_NumPlayers;
 
 	m_pGame->GetSimulation()->SetTurnManager(this);
 	// Set an incredibly long turn length - less command batch spam that way
@@ -72,6 +72,11 @@ CNetServer::CNetServer(CGame *pGame, CGameAttributes *pGameAttribs):
 	g_ScriptingHost.SetGlobal("g_NetServer", OBJECT_TO_JSVAL(GetScript()));
 }
 
+CNetServer::~CNetServer()
+{
+	g_ScriptingHost.SetGlobal("g_NetServer", JSVAL_NULL);
+}
+
 bool CNetServer::JSI_Open(JSContext *cx, uintN argc, jsval *argv)
 {
 	CSocketAddress addr;
@@ -120,7 +125,12 @@ bool CNetServer::AddNewPlayer(CNetServerSession *pSession)
 
 	if (m_PlayerSessions.size() < m_NumPlayers-1)
 	{
+		// First two players are Gaia and Server player, so assign new player
+		// ID's starting from 2
+		uint newPlayerID=2+m_PlayerSessions.size();
 		m_PlayerSessions.push_back(pSession);
+		pSession->m_pPlayer=m_pGame->GetPlayer(newPlayerID);
+		pSession->m_pPlayer->SetName(pSession->GetName());
 
 		// Broadcast a message for the newly added player session
 		CPlayerConnect *pMsg=new CPlayerConnect();
@@ -133,14 +143,14 @@ bool CNetServer::AddNewPlayer(CNetServerSession *pSession)
 		
 		// Server Player
 		pMsg->m_Players.resize(1);
-		pMsg->m_Players.back().m_PlayerID=m_pServerPlayer->GetPlayerID();
+		pMsg->m_Players.back().m_PlayerID=1; // Server is always 1
 		pMsg->m_Players.back().m_Nick=m_ServerPlayerName;
 		
 		// All the other players
 		for (uint i=0;i<m_PlayerSessions.size()-1;i++)
 		{
 			pMsg->m_Players.resize(i+2);
-			pMsg->m_Players.back().m_PlayerID=i+1;
+			pMsg->m_Players.back().m_PlayerID=i+2;
 			pMsg->m_Players.back().m_Nick=m_PlayerSessions[i]->GetName();
 		}
 		pSession->Push(pMsg);
@@ -204,7 +214,10 @@ void CNetServer::RemoveSession(CNetServerSession *pSession)
 void CNetServer::Broadcast(CNetMessage *pMsg)
 {
 	if (m_Sessions.empty())
+	{
+		delete pMsg;
 		return;
+	}
 
 	size_t i=0;
 	for (;i<m_Sessions.size()-1;i++)
@@ -216,8 +229,10 @@ void CNetServer::Broadcast(CNetMessage *pMsg)
 
 int CNetServer::StartGame()
 {
-	// TODO Check for the case where we haven't yet filled all player slots
+	if (m_PlayerSessions.size() < m_pGameAttributes->m_NumPlayers-1)
-	// CGame expects to have numPlayer players when it starts the game...
+	{
+		return -1;
+	}
 
 	Broadcast(new CStartGame());
 

source/ps/Network/Server.h

@@ -106,6 +106,7 @@ protected:
 
 public:
 	CNetServer(CGame *pGame, CGameAttributes *pGameAttribs);
+	virtual ~CNetServer();
 
 	static void GetDefaultListenAddress(CSocketAddress &address);
 	PS_RESULT Bind(const CSocketAddress &address);

source/scripting/ScriptCustomTypes.cpp

@@ -85,3 +85,16 @@ JSBool SColour::Construct( JSContext* cx, JSObject* obj, unsigned int argc, jsva
 
 	return( JS_TRUE );
 }
+
+// (Simon) Added this to prevent a deep copy, which evidently makes direct
+// copies of the heap allocated objects within CJSObject, which eventually
+// goes boom
+SColour &SColour::operator = (const SColour &o)
+{
+	r=o.r;
+	g=o.g;
+	b=o.b;
+	a=o.a;
+
+	return *this;
+}

source/scripting/ScriptCustomTypes.h

@@ -22,6 +22,8 @@ public:
 	SColour( float r, float g, float b ) { SColourInit( r, g, b, 1.0f ); }
 	SColour( float r, float g, float b, float a ) { SColourInit( r, g, b, a ); }
 	void SColourInit( float r, float g, float b, float a );
+	
+	SColour &operator = (const SColour &o);
 
 	jsval ToString( JSContext* cx, uintN argc, jsval* argv );
 	static void ScriptingInit();

source/scripting/ScriptableObject.h

@@ -71,6 +71,13 @@ public:
 
 class IJSObject
 {
+	// Make copy constructor and assignment operator private - since copying of
+	// these objects is unsafe unless done specially.
+	// These will never be implemented (they are, after all, here to *prevent*
+	// copying)
+	IJSObject(const IJSObject &other);
+	IJSObject& operator=(const IJSObject &other);
+
 public:
 	typedef STL_HASH_MAP<CStrW, IJSProperty*, CStrW_hash_compare> PropertyTable;
 	typedef std::vector<IJSObject*> InheritorsList;
@@ -106,6 +113,8 @@ public:
 	// Add a property (with immediate value)
 	virtual void AddProperty( CStrW PropertyName, jsval Value ) = 0;
 	virtual void AddProperty( CStrW PropertyName, CStrW Value ) = 0;
+	
+	inline IJSObject() {}
 };
 
 template<typename T, bool ReadOnly = false> class CJSObject;