UI: Forget the views' pending requests when recreating them

WipeRequesterToken() only reset the screen's token, after CreateViews() had
already handed it to the new views, so nothing was ever forgotten - not even
in ~Screen(), which saw a token of -1. A text input dialog open across a
RecreateViews() (on the RetroAchievements screen, a slow login failing) would
then write through a deleted PopupTextInputChoice, possibly into the field
that took its place: #21708.

Requests made while creating views now get their own token, forgotten
before the views are deleted. Screen-level requests keep the screen's token.

RequestManager: ForgetRequestsWithToken() now locks, and also drops
responses that arrived but weren't processed yet. ProcessRequests() calls
the callbacks one at a time without holding the lock, so a callback that
closes a screen drops the rest of that screen's responses.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Common/System/Request.cpp

@@ -86,6 +86,7 @@ bool RequestManager::MakeSystemRequest(SystemRequestType type, RequesterToken to
 }
 
 void RequestManager::ForgetRequestsWithToken(RequesterToken token) {
+	std::lock_guard<std::mutex> guard(callbackMutex_);
 	for (auto &iter : callbackMap_) {
 		if (iter.second.token == token) {
 			INFO_LOG(Log::System, "Forgetting about requester with token %d", token);
@@ -93,23 +94,28 @@ void RequestManager::ForgetRequestsWithToken(RequesterToken token) {
 			iter.second.failedCallback = nullptr;
 		}
 	}
+	// Also the ones that finished but haven't been processed yet.
+	std::lock_guard<std::mutex> responseGuard(responseMutex_);
+	for (auto &response : pendingResponses_) {
+		if (response.token == token) {
+			response.callback = nullptr;
+			response.failedCallback = nullptr;
+		}
+	}
 }
 
+// Responses can contain passwords, so we don't log them.
 void RequestManager::PostSystemSuccess(int requestId, std::string_view responseString, int responseValue) {
 	std::lock_guard<std::mutex> guard(callbackMutex_);
 	auto iter = callbackMap_.find(requestId);
 	if (iter == callbackMap_.end()) {
-		ERROR_LOG(Log::System, "PostSystemSuccess: Unexpected request ID %d (responseString=%.*s)", requestId, (int)responseString.size(), responseString.data());
+		ERROR_LOG(Log::System, "PostSystemSuccess: Unexpected request ID %d", requestId);
 		return;
 	}
 
 	std::lock_guard<std::mutex> responseGuard(responseMutex_);
-	PendingSuccess response;
-	response.callback = iter->second.callback;
-	response.responseString = responseString;
-	response.responseValue = responseValue;
-	pendingSuccesses_.push_back(response);
-	DEBUG_LOG(Log::System, "PostSystemSuccess: Request %d (%.*s, %d)", requestId, (int)responseString.size(), responseString.data(), responseValue);
+	pendingResponses_.push_back({ true, std::string(responseString), responseValue, iter->second.callback, nullptr, iter->second.token });
+	DEBUG_LOG(Log::System, "PostSystemSuccess: Request %d (%d chars, %d)", requestId, (int)responseString.size(), responseValue);
 	callbackMap_.erase(iter);
 }
 
@@ -124,35 +130,38 @@ void RequestManager::PostSystemFailure(int requestId, int responseValue) {
 	WARN_LOG(Log::System, "PostSystemFailure: Request %d failed", requestId);
 
 	std::lock_guard<std::mutex> responseGuard(responseMutex_);
-	PendingFailure response;
-	response.failedCallback = iter->second.failedCallback;
-	response.responseValue = responseValue;
-	pendingFailures_.push_back(response);
+	pendingResponses_.push_back({ false, std::string(), responseValue, nullptr, iter->second.failedCallback, iter->second.token });
 	callbackMap_.erase(iter);
 }
 
 void RequestManager::ProcessRequests() {
-	std::lock_guard<std::mutex> guard(responseMutex_);
-	for (auto &iter : pendingSuccesses_) {
-		if (iter.callback) {
-			iter.callback(iter.responseString.c_str(), iter.responseValue);
+	// One at a time, without holding the lock while calling. A callback can make new requests, or close a screen,
+	// which forgets the callbacks of the responses still in the queue.
+	while (true) {
+		PendingResponse response;
+		{
+			std::lock_guard<std::mutex> guard(responseMutex_);
+			if (pendingResponses_.empty()) {
+				break;
+			}
+			response = std::move(pendingResponses_.front());
+			pendingResponses_.pop_front();
+		}
+		if (response.success) {
+			if (response.callback) {
+				response.callback(response.responseString, response.responseValue);
+			}
+		} else if (response.failedCallback) {
+			response.failedCallback(response.responseValue);
 		}
 	}
-	pendingSuccesses_.clear();
-	for (auto &iter : pendingFailures_) {
-		if (iter.failedCallback) {
-			iter.failedCallback(iter.responseValue);
-		}
-	}
-	pendingFailures_.clear();
 }
 
 void RequestManager::Clear() {
 	std::lock_guard<std::mutex> guard(callbackMutex_);
 	std::lock_guard<std::mutex> responseGuard(responseMutex_);
 
-	pendingSuccesses_.clear();
-	pendingFailures_.clear();
+	pendingResponses_.clear();
 	callbackMap_.clear();
 }
 

Common/System/Request.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include <vector>
+#include <deque>
 #include <mutex>
 #include <map>
 #include <functional>
@@ -64,21 +65,19 @@ private:
 	std::map<int, CallbackPair> callbackMap_;
 	std::mutex callbackMutex_;
 
-	struct PendingSuccess {
+	// A finished request, waiting for ProcessRequests to call its callback.
+	struct PendingResponse {
+		bool success;
 		std::string responseString;
-		int responseValue;
-		RequestCallback callback;
-	};
-
-	struct PendingFailure {
-		RequestFailedCallback failedCallback;
 		int responseValue;  // can have error codes for example.
+		RequestCallback callback;
+		RequestFailedCallback failedCallback;
+		RequesterToken token;
 	};
 
 	// Let's start at 10 to get a recognizably valid ID in logs.
 	int idCounter_ = 10;
-	std::vector<PendingSuccess> pendingSuccesses_;
-	std::vector<PendingFailure> pendingFailures_;
+	std::deque<PendingResponse> pendingResponses_;
 	std::mutex responseMutex_;
 
 	RequesterToken tokenGen_ = 20000;

Common/UI/Screen.cpp

@@ -27,18 +27,27 @@ void Screen::focusChanged(ScreenFocusChange focusChange) {
 }
 
 int Screen::GetRequesterToken() {
-	if (token_ < 0) {
-		token_ = g_requestManager.GenerateRequesterToken();
+	int &token = creatingViews_ ? viewsToken_ : token_;
+	if (token < 0) {
+		token = g_requestManager.GenerateRequesterToken();
+	}
+	return token;
+}
+
+void Screen::ForgetViewRequests() {
+	if (viewsToken_ >= 0) {
+		g_requestManager.ForgetRequestsWithToken(viewsToken_);
+		viewsToken_ = -1;
 	}
-	return token_;
 }
 
 Screen::~Screen() {
 	screenManager_ = nullptr;
+	// To avoid expired callbacks getting called.
 	if (token_ >= 0) {
-		// To avoid expired callbacks getting called.
 		g_requestManager.ForgetRequestsWithToken(token_);
 	}
+	ForgetViewRequests();
 }
 
 const char *DialogResultToString(DialogResult result) {

Common/UI/Screen.h

@@ -138,14 +138,18 @@ public:
 	virtual bool WantsTextInput() const { return false; }
 
 protected:
+	// Requests made while creating views (by the views themselves, or with callbacks that capture them) get a separate
+	// token, so they can be forgotten when the views are deleted, while the screen's own requests stay alive.
 	int GetRequesterToken();
-	void WipeRequesterToken() {
-		token_ = -1;
-	}
+	void SetCreatingViews(bool creating) { creatingViews_ = creating; }
+	// Drops the callbacks of pending requests made by the views, which are about to be deleted.
+	void ForgetViewRequests();
 
 private:
 	ScreenManager *screenManager_ = nullptr;
 	int token_ = -1;
+	int viewsToken_ = -1;
+	bool creatingViews_ = false;
 
 	DISALLOW_COPY_AND_ASSIGN(Screen);
 };

Common/UI/UIScreen.cpp

@@ -39,9 +39,14 @@ void UIScreen::DoRecreateViews() {
 		root_->PersistData(UI::PERSIST_SAVE, "root", persisted);
 	}
 
+	// Pending requests from the views (like an open text input dialog) would call back into deleted views.
+	ForgetViewRequests();
+
 	delete root_;
 	root_ = nullptr;
+	SetCreatingViews(true);
 	CreateViews();
+	SetCreatingViews(false);
 	UI::View *defaultView = root_ ? root_->GetDefaultFocusView() : nullptr;
 	if (defaultView && defaultView->GetVisibility() == UI::V_VISIBLE) {
 		defaultView->SetFocus(UI::FocusFlags::CAUSE_OTHER);
@@ -59,9 +64,6 @@ void UIScreen::DoRecreateViews() {
 			root_->SubviewFocused(focused);
 		}
 	}
-
-	// NOTE: We also wipe the requester token. It's possible that views were created with the old token, so any pending requests from them must be invalidated.
-	WipeRequesterToken();
 }
 
 bool UIScreen::touch(const TouchInput &touch) {

unittest/UnitTest.cpp

@@ -122,6 +122,7 @@
 #include "GPU/Math3D.h"
 
 #include "Common/File/AndroidContentURI.h"
+#include "Common/System/Request.h"
 
 #include "unittest/JitHarness.h"
 #include "unittest/TestVertexJit.h"
@@ -158,7 +159,15 @@ std::vector<std::string> System_GetCameraDeviceList() { return std::vector<std::
 void NativeFrame(GraphicsContext *graphicsContext) {}
 void NativeResized() {}
 
-bool System_MakeRequest(SystemRequestType type, int requestId, const std::string &param1, const std::string &param2, int64_t param3, int64_t param4) { return false; }
+// TestRequestManager answers these itself.
+static int g_lastInputBoxRequestId = -1;
+bool System_MakeRequest(SystemRequestType type, int requestId, const std::string &param1, const std::string &param2, int64_t param3, int64_t param4) {
+	if (type == SystemRequestType::INPUT_TEXT_MODAL) {
+		g_lastInputBoxRequestId = requestId;
+		return true;
+	}
+	return false;
+}
 // Pulled in via Core/WebServer.cpp's OpenWebDebugger(), which CmdLine.cpp now references.
 void System_LaunchUrl(LaunchUrlType urlType, std::string_view url) {}
 void System_InputBoxGetString(const std::string &title, const std::string &defaultValue, std::function<void(bool, const std::string &)> cb) { cb(false, ""); }
@@ -3270,6 +3279,48 @@ static bool TestQueueBuf() {
 	return true;
 }
 
+static bool TestRequestManager() {
+	RequestManager rm;
+	std::vector<std::string> results;
+	auto ask = [&](RequesterToken token, std::string name) {
+		rm.MakeSystemRequest(SystemRequestType::INPUT_TEXT_MODAL, token, [&results, name](std::string_view value, int) {
+			results.push_back(name + "=" + std::string(value));
+		}, nullptr, "", "", 0);
+		return g_lastInputBoxRequestId;
+	};
+
+	RequesterToken tokenA = rm.GenerateRequesterToken();
+	RequesterToken tokenB = rm.GenerateRequesterToken();
+
+	// Forgotten before the answer arrives.
+	int a1 = ask(tokenA, "a1");
+	rm.ForgetRequestsWithToken(tokenA);
+	rm.PostSystemSuccess(a1, "x");
+	// Forgotten after the answer arrived, but before it was processed.
+	int a2 = ask(tokenA, "a2");
+	int b1 = ask(tokenB, "b1");
+	rm.PostSystemSuccess(a2, "y");
+	rm.PostSystemSuccess(b1, "z");
+	rm.ForgetRequestsWithToken(tokenA);
+	rm.ProcessRequests();
+	EXPECT_EQ_INT((int)results.size(), 1);
+	EXPECT_TRUE(results[0] == "b1=z");
+
+	// A callback that forgets its requester drops the rest of its responses in the same batch.
+	results.clear();
+	int b2 = rm.MakeSystemRequest(SystemRequestType::INPUT_TEXT_MODAL, tokenB, [&](std::string_view, int) {
+		results.push_back("b2");
+		rm.ForgetRequestsWithToken(tokenB);
+	}, nullptr, "", "", 0) ? g_lastInputBoxRequestId : -1;
+	int b3 = ask(tokenB, "b3");
+	rm.PostSystemSuccess(b2, "");
+	rm.PostSystemSuccess(b3, "");
+	rm.ProcessRequests();
+	EXPECT_EQ_INT((int)results.size(), 1);
+	EXPECT_TRUE(results[0] == "b2");
+	return true;
+}
+
 TestItem availableTests[] = {
 #if PPSSPP_ARCH(ARM64) || PPSSPP_ARCH(AMD64) || PPSSPP_ARCH(X86)
 	TEST_ITEM(Arm64Emitter),
@@ -3347,6 +3398,7 @@ TestItem availableTests[] = {
 	TEST_ITEM(FatShortNames),
 	TEST_ITEM(GEDumpGameID),
 	TEST_ITEM(QueueBuf),
+	TEST_ITEM(RequestManager),
 };
 
 int main(int argc, const char *argv[]) {