diff --git a/Common/System/Request.cpp b/Common/System/Request.cpp index 9b3a0ce080..858cf688f5 100644 --- a/Common/System/Request.cpp +++ b/Common/System/Request.cpp @@ -86,6 +86,7 @@ bool RequestManager::MakeSystemRequest(SystemRequestType type, RequesterToken to } void RequestManager::ForgetRequestsWithToken(RequesterToken token) { + std::lock_guard 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 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 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 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 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 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 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 guard(callbackMutex_); std::lock_guard responseGuard(responseMutex_); - pendingSuccesses_.clear(); - pendingFailures_.clear(); + pendingResponses_.clear(); callbackMap_.clear(); } diff --git a/Common/System/Request.h b/Common/System/Request.h index cbcd742fbf..41450f7355 100644 --- a/Common/System/Request.h +++ b/Common/System/Request.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include #include @@ -64,21 +65,19 @@ private: std::map 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 pendingSuccesses_; - std::vector pendingFailures_; + std::deque pendingResponses_; std::mutex responseMutex_; RequesterToken tokenGen_ = 20000; diff --git a/Common/UI/Screen.cpp b/Common/UI/Screen.cpp index e886ce5ab1..0e31b1f82e 100644 --- a/Common/UI/Screen.cpp +++ b/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) { diff --git a/Common/UI/Screen.h b/Common/UI/Screen.h index 03f4284021..485c28645d 100644 --- a/Common/UI/Screen.h +++ b/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); }; diff --git a/Common/UI/UIScreen.cpp b/Common/UI/UIScreen.cpp index 4817514066..d29d2dead9 100644 --- a/Common/UI/UIScreen.cpp +++ b/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) { diff --git a/unittest/UnitTest.cpp b/unittest/UnitTest.cpp index 51d92272a3..fd25357f62 100644 --- a/unittest/UnitTest.cpp +++ b/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 System_GetCameraDeviceList() { return std::vector cb) { cb(false, ""); } @@ -3270,6 +3279,48 @@ static bool TestQueueBuf() { return true; } +static bool TestRequestManager() { + RequestManager rm; + std::vector 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[]) {