Properly lock UI mutex on accessing GUI state from network thread

client/CServerHandler.cpp

@@ -88,8 +88,6 @@ template<typename T> class CApplyOnLobby : public CBaseForLobbyApply
 public:
 	bool applyOnLobbyHandler(CServerHandler * handler, CPackForLobby & pack) const override
 	{
-		boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
-
 		auto & ref = static_cast<T&>(pack);
 		ApplyOnLobbyHandlerNetPackVisitor visitor(*handler);
 
@@ -278,6 +276,9 @@ void CServerHandler::connectToServer(const std::string & addr, const ui16 port)
 
 void CServerHandler::onConnectionFailed(const std::string & errorMessage)
 {
+	assert(state == EClientState::CONNECTING);
+	boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+
 	if (isServerLocal())
 	{
 		// retry - local server might be still starting up
@@ -294,6 +295,8 @@ void CServerHandler::onConnectionFailed(const std::string & errorMessage)
 
 void CServerHandler::onTimer()
 {
+	boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+
 	if(state == EClientState::CONNECTION_CANCELLED)
 	{
 		logNetwork->info("Connection aborted by player!");
@@ -306,6 +309,10 @@ void CServerHandler::onTimer()
 
 void CServerHandler::onConnectionEstablished(const NetworkConnectionPtr & netConnection)
 {
+	assert(state == EClientState::CONNECTING);
+
+	boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+
 	networkConnection = netConnection;
 
 	logNetwork->info("Connection established");
@@ -324,7 +331,6 @@ void CServerHandler::onConnectionEstablished(const NetworkConnectionPtr & netCon
 
 void CServerHandler::applyPackOnLobbyScreen(CPackForLobby & pack)
 {
-	boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 	const CBaseForLobbyApply * apply = applier->getApplier(CTypeList::getInstance().getTypeID(&pack)); //find the applier
 	apply->applyOnLobbyScreen(dynamic_cast<CLobbyScreen *>(SEL), this, pack);
 	GH.windows().totalRedraw();
@@ -428,7 +434,7 @@ void CServerHandler::sendClientDisconnecting()
 		logNetwork->info("Sent leaving signal to the server");
 	}
 	sendLobbyPack(lcd);
-	c->getConnection()->close();
+	networkConnection.reset();
 	c.reset();
 }
 
@@ -861,6 +867,8 @@ public:
 
 void CServerHandler::onPacketReceived(const std::shared_ptr<INetworkConnection> &, const std::vector<std::byte> & message)
 {
+	boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+
 	if(state == EClientState::DISCONNECTING)
 	{
 		assert(0); //Should not be possible - socket must be closed at this point
@@ -874,33 +882,35 @@ void CServerHandler::onPacketReceived(const std::shared_ptr<INetworkConnection>
 
 void CServerHandler::onDisconnected(const std::shared_ptr<INetworkConnection> & connection, const std::string & errorMessage)
 {
-	networkConnection.reset();
-
 	if(state == EClientState::DISCONNECTING)
 	{
-		logNetwork->info("Successfully closed connection to server, ending listening thread!");
+		assert(networkConnection == nullptr);
+		// Note: this branch can be reached on app shutdown, when main thread holds mutex till destruction
+		logNetwork->info("Successfully closed connection to server!");
+		return;
 	}
-	else
-	{
-		logNetwork->error("Lost connection to server, ending listening thread! Connection has been closed");
 
-		if(client)
-		{
-			state = EClientState::DISCONNECTING;
+	boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 
-			GH.dispatchMainThread([]()
-			{
-				CSH->endGameplay();
-				GH.defActionsDef = 63;
-				CMM->menu->switchToTab("main");
-			});
-		}
-		else
+	logNetwork->error("Lost connection to server! Connection has been closed");
+	networkConnection.reset();
+
+	if(client)
+	{
+		state = EClientState::DISCONNECTING;
+
+		GH.dispatchMainThread([]()
 		{
-			LobbyClientDisconnected lcd;
-			lcd.clientId = c->connectionID;
-			applyPackOnLobbyScreen(lcd);
-		}
+			CSH->endGameplay();
+			GH.defActionsDef = 63;
+			CMM->menu->switchToTab("main");
+		});
+	}
+	else
+	{
+		LobbyClientDisconnected lcd;
+		lcd.clientId = c->connectionID;
+		applyPackOnLobbyScreen(lcd);
 	}
 }
 

client/Client.cpp

@@ -521,7 +521,6 @@ void CClient::handlePack(CPack * pack)
 	CBaseForCLApply * apply = applier->getApplier(CTypeList::getInstance().getTypeID(pack)); //find the applier
 	if(apply)
 	{
-		boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 		apply->applyOnClBefore(this, pack);
 		logNetwork->trace("\tMade first apply on cl: %s", typeid(pack).name());
 		gs->apply(pack);

client/globalLobby/GlobalLobbyClient.cpp

@@ -207,6 +207,8 @@ void GlobalLobbyClient::receiveJoinRoomSuccess(const JsonNode & json)
 
 void GlobalLobbyClient::onConnectionEstablished(const std::shared_ptr<INetworkConnection> & connection)
 {
+	boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+
 	networkConnection = connection;
 
 	JsonNode toSend;
@@ -238,6 +240,8 @@ void GlobalLobbyClient::sendClientLogin()
 
 void GlobalLobbyClient::onConnectionFailed(const std::string & errorMessage)
 {
+	boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+
 	auto loginWindowPtr = loginWindow.lock();
 
 	if(!loginWindowPtr || !GH.windows().topWindow<GlobalLobbyLoginWindow>())
@@ -249,6 +253,8 @@ void GlobalLobbyClient::onConnectionFailed(const std::string & errorMessage)
 
 void GlobalLobbyClient::onDisconnected(const std::shared_ptr<INetworkConnection> & connection, const std::string & errorMessage)
 {
+	boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+
 	assert(connection == networkConnection);
 	networkConnection.reset();
 

server/CVCMIServer.cpp

@@ -261,10 +261,11 @@ bool CVCMIServer::prepareToStartGame()
 	Load::ProgressAccumulator progressTracking;
 	Load::Progress current(1);
 	progressTracking.include(current);
-	auto currentProgress = std::numeric_limits<Load::Type>::max();
-	
-	auto progressTrackingThread = boost::thread([this, &progressTracking, &currentProgress]()
+
+	auto progressTrackingThread = boost::thread([this, &progressTracking]()
 	{
+		auto currentProgress = std::numeric_limits<Load::Type>::max();
+
 		while(!progressTracking.finished())
 		{
 			if(progressTracking.get() != currentProgress)