Merge pull request #22511 from hrydgard/android-inputbox-fixes

Claude code review: Android inputbox fixes

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,8 @@
 #pragma once
 
+#include <atomic>
 #include <vector>
+#include <deque>
 #include <mutex>
 #include <map>
 #include <functional>
@@ -64,24 +66,23 @@ 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_;
+	// Atomic since requests are made from several threads (the CPU thread for the OSK and the microphone).
+	std::atomic<int> idCounter_{ 10 };
+	std::deque<PendingResponse> pendingResponses_;
 	std::mutex responseMutex_;
 
-	RequesterToken tokenGen_ = 20000;
+	std::atomic<RequesterToken> tokenGen_{ 20000 };
 };
 
 const char *RequestTypeAsString(SystemRequestType type);

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) {