Fix RecalculateSSRs race condition

this causes crashes and rare impossible scores on startup
at this point in song loading, usually a Steps's timingdata is empty. when you try to get the timingdata of a Steps in that state, it gives you the Song's timingdata. that's bad for when you rarely iterate through 2 highscores that are on different charts of the same song at the same time, because the Etaner of the timingdata gets cleared in one instance while messing with it in another function (we pass references around instead of copies, so messing with the Etaner is bad)

Author: poco0317

src/Etterna/Singletons/ScoreManager.cpp

@@ -13,6 +13,7 @@
 #include "Etterna/FileTypes/XmlFileUtil.h"
 #include "arch/LoadingWindow/LoadingWindow.h"
 #include "RageUtil/Misc/RageThreads.h"
+#include <cstdint>
 
 ScoreManager* SCOREMAN = NULL;
 
@@ -226,7 +227,8 @@ ScoresForChart::SetTopScores()
 			eligiblescores.emplace_back(hs);
 	}
 
-	// if there aren't 2 noccpbs in top scores we might as well use old cc scores -mina
+	// if there aren't 2 noccpbs in top scores we might as well use old cc
+	// scores -mina
 	if (eligiblescores.size() < 2) {
 		FOREACHM(int, ScoresAtRate, ScoresByRate, i)
 		{
@@ -237,7 +239,7 @@ ScoresForChart::SetTopScores()
 				eligiblescores.emplace_back(hs);
 		}
 	}
-	
+
 	if (eligiblescores.empty())
 		return;
 
@@ -351,50 +353,56 @@ ScoreManager::RecalculateSSRs(LoadingWindow* ld, const string& profileID)
 	}
 	int onePercent = std::max(static_cast<int>(scores.size() / 100 * 5), 1);
 	int scoreindex = 0;
-	mutex stepsMutex;
-	vector<string> currentSteps;
-	class StepsLock
+
+	mutex songVectorPtrMutex;
+	vector<std::uintptr_t> currentlyLockedSongs;
+	// This is meant to ensure mutual exclusion for a song
+	class SongLock
 	{
 	  public:
-		mutex& stepsMutex;
-		vector<string>& currentSteps;
-		string& ck;
-		StepsLock(vector<string>& vec, mutex& mut, string& k)
-		  : currentSteps(vec)
-		  , stepsMutex(mut)
-		  , ck(k)
+		mutex& songVectorPtrMutex; // This mutex guards the vector
+		vector<std::uintptr_t>&
+		  currentlyLockedSongs; // Vector of currently locked songs
+		std::uintptr_t song;	// The song for this lock
+		SongLock(vector<std::uintptr_t>& vec, mutex& mut, std::uintptr_t k)
+		  : currentlyLockedSongs(vec)
+		  , songVectorPtrMutex(mut)
+		  , song(k)
 		{
 			bool active = true;
 			{
-				lock_guard<mutex> lk(stepsMutex);
-				active = find(currentSteps.begin(), currentSteps.end(), ck) !=
-						 currentSteps.end();
+				lock_guard<mutex> lk(songVectorPtrMutex);
+				active = find(currentlyLockedSongs.begin(),
+							  currentlyLockedSongs.end(),
+							  song) != currentlyLockedSongs.end();
 				if (!active)
-					currentSteps.emplace_back(ck);
+					currentlyLockedSongs.emplace_back(song);
 			}
 			while (active) {
+				// TODO: Try to make this wake up from the destructor (CondVar's
+				// maybe)
 				std::this_thread::sleep_for(std::chrono::milliseconds(10));
 				{
-					lock_guard<mutex> lk(stepsMutex);
-					active =
-					  find(currentSteps.begin(), currentSteps.end(), ck) !=
-					  currentSteps.end();
+					lock_guard<mutex> lk(songVectorPtrMutex);
+					active = find(currentlyLockedSongs.begin(),
+								  currentlyLockedSongs.end(),
+								  song) != currentlyLockedSongs.end();
 					if (!active)
-						currentSteps.emplace_back(ck);
+						currentlyLockedSongs.emplace_back(song);
 				}
 			}
 		}
-		~StepsLock()
+		~SongLock()
 		{
-			lock_guard<mutex> lk(stepsMutex);
-			currentSteps.erase(
-			  find(currentSteps.begin(), currentSteps.end(), ck));
+			lock_guard<mutex> lk(songVectorPtrMutex);
+			currentlyLockedSongs.erase(find(
+			  currentlyLockedSongs.begin(), currentlyLockedSongs.end(), song));
 		}
 	};
 	function<void(std::pair<vectorIt<HighScore*>, vectorIt<HighScore*>>,
 				  ThreadData*)>
 	  callback =
-		[&stepsMutex, &currentSteps](
+		[&songVectorPtrMutex, &currentlyLockedSongs](
 		  std::pair<vectorIt<HighScore*>, vectorIt<HighScore*>> workload,
 		  ThreadData* data) {
 			auto pair =
@@ -414,12 +422,15 @@ ScoreManager::RecalculateSSRs(LoadingWindow* ld, const string& profileID)
 				if (hs->GetSSRCalcVersion() == GetCalcVersion())
 					continue;
 				string ck = hs->GetChartKey();
-				StepsLock lk(currentSteps, stepsMutex, ck);
 				Steps* steps = SONGMAN->GetStepsByChartkey(ck);
 
 				if (!steps)
 					continue;
 
+				SongLock lk(currentlyLockedSongs,
+							songVectorPtrMutex,
+							reinterpret_cast<std::uintptr_t>(steps->m_pSong));
+
 				if (!steps->IsRecalcValid()) {
 					hs->ResetSkillsets();
 					continue;
@@ -716,7 +727,8 @@ ScoresAtRate::LoadFromNode(const XNode* node,
 		SCOREMAN->RegisterScore(&scores.find(sk)->second);
 		SCOREMAN->AddToKeyedIndex(&scores.find(sk)->second);
 		SCOREMAN->RegisterScoreInProfile(&scores.find(sk)->second, profileID);
-		if (scores[sk].GetSSRCalcVersion() != GetCalcVersion() && SONGMAN->IsChartLoaded(ck))
+		if (scores[sk].GetSSRCalcVersion() != GetCalcVersion() &&
+			SONGMAN->IsChartLoaded(ck))
 			SCOREMAN->scorestorecalc.emplace_back(&scores[sk]);
 	}
 }