Skip to content

Commit

Permalink
Merge pull request #18723 from hrydgard/requester-tokens
Browse files Browse the repository at this point in the history 
Fix a particular type of race condition in file dialog requests
  • Loading branch information
@hrydgard
hrydgard committed Jan 18, 2024
2 parents b4122ef + 1304d04 commit 7918e49
Show file tree
Hide file tree
Showing 24 changed files with 174 additions and 124 deletions.
32 changes: 25 additions & 7 deletions Common/System/Request.cpp
View file
Expand Up @@ -31,13 +31,21 @@ const char *RequestTypeAsString(SystemRequestType type) {
}
}

bool RequestManager::MakeSystemRequest(SystemRequestType type, RequestCallback callback, RequestFailedCallback failedCallback, const std::string &param1, const std::string &param2, int param3) {
bool RequestManager::MakeSystemRequest(SystemRequestType type, RequesterToken token, RequestCallback callback, RequestFailedCallback failedCallback, const std::string &param1, const std::string &param2, int param3) {
if (token == NO_REQUESTER_TOKEN) {
_dbg_assert_(!callback);
_dbg_assert_(!failedCallback);
}
if (callback || failedCallback) {
_dbg_assert_(token != NO_REQUESTER_TOKEN);
}

int requestId = idCounter_++;

// NOTE: We need to register immediately, in order to support synchronous implementations.
if (callback || failedCallback) {
std::lock_guard<std::mutex> guard(callbackMutex_);
callbackMap_[requestId] = { callback, failedCallback };
callbackMap_[requestId] = { callback, failedCallback, token };
}

VERBOSE_LOG(SYSTEM, "Making system request %s: id %d", RequestTypeAsString(type), requestId);
Expand All @@ -52,6 +60,16 @@ bool RequestManager::MakeSystemRequest(SystemRequestType type, RequestCallback c
return true;
}

void RequestManager::ForgetRequestsWithToken(RequesterToken token) {
for (auto &iter : callbackMap_) {
if (iter.second.token == token) {
INFO_LOG(SYSTEM, "Forgetting about requester with token %d", token);
iter.second.callback = nullptr;
iter.second.failedCallback = nullptr;
}
}
}

void RequestManager::PostSystemSuccess(int requestId, const char *responseString, int responseValue) {
std::lock_guard<std::mutex> guard(callbackMutex_);
auto iter = callbackMap_.find(requestId);
Expand Down Expand Up @@ -82,7 +100,7 @@ void RequestManager::PostSystemFailure(int requestId) {

std::lock_guard<std::mutex> responseGuard(responseMutex_);
PendingFailure response;
response.callback = iter->second.failedCallback;
response.failedCallback = iter->second.failedCallback;
pendingFailures_.push_back(response);
callbackMap_.erase(iter);
}
Expand All @@ -96,8 +114,8 @@ void RequestManager::ProcessRequests() {
}
pendingSuccesses_.clear();
for (auto &iter : pendingFailures_) {
if (iter.callback) {
iter.callback();
if (iter.failedCallback) {
iter.failedCallback();
}
}
pendingFailures_.clear();
Expand All @@ -113,9 +131,9 @@ void RequestManager::Clear() {
}

void System_CreateGameShortcut(const Path &path, const std::string &title) {
g_requestManager.MakeSystemRequest(SystemRequestType::CREATE_GAME_SHORTCUT, nullptr, nullptr, path.ToString(), title, 0);
g_requestManager.MakeSystemRequest(SystemRequestType::CREATE_GAME_SHORTCUT, NO_REQUESTER_TOKEN, nullptr, nullptr, path.ToString(), title, 0);
}

void System_ShowFileInFolder(const Path &path) {
g_requestManager.MakeSystemRequest(SystemRequestType::SHOW_FILE_IN_FOLDER, nullptr, nullptr, path.ToString(), "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::SHOW_FILE_IN_FOLDER, NO_REQUESTER_TOKEN, nullptr, nullptr, path.ToString(), "", 0);
}
74 changes: 41 additions & 33 deletions Common/System/Request.h
View file
Expand Up @@ -12,6 +12,10 @@ class Path;
typedef std::function<void(const char *responseString, int responseValue)> RequestCallback;
typedef std::function<void()> RequestFailedCallback;

typedef int RequesterToken;
#define NO_REQUESTER_TOKEN -1
#define NON_EPHEMERAL_TOKEN -2

// Platforms often have to process requests asynchronously, on wildly different threads,
// and then somehow pass a response back to the main thread (especially Android...)
// This acts as bridge and buffer.
Expand All @@ -23,7 +27,7 @@ class RequestManager {
// The callback you pass in will be called on the main thread later.
// Params are at the end since it's the part most likely to recieve additions in the future,
// now that we have both callbacks.
bool MakeSystemRequest(SystemRequestType type, RequestCallback callback, RequestFailedCallback failedCallback, const std::string &param1, const std::string &param2, int param3);
bool MakeSystemRequest(SystemRequestType type, RequesterToken token, RequestCallback callback, RequestFailedCallback failedCallback, const std::string &param1, const std::string &param2, int param3);

// Called by the platform implementation, when it's finished with a request.
void PostSystemSuccess(int requestId, const char *responseString, int responseValue = 0);
Expand All @@ -33,19 +37,21 @@ class RequestManager {
// This will call the callback of any finished requests.
void ProcessRequests();

RequesterToken GenerateRequesterToken() {
int token = tokenGen_++;
return token;
}

void ForgetRequestsWithToken(RequesterToken token);

// Unclear if we need this...
void Clear();

private:
struct PendingRequest {
SystemRequestType type;
RequestCallback callback;
RequestFailedCallback failedCallback;
};

struct CallbackPair {
RequestCallback callback;
RequestFailedCallback failedCallback;
RequesterToken token;
};

std::map<int, CallbackPair> callbackMap_;
Expand All @@ -58,14 +64,16 @@ class RequestManager {
};

struct PendingFailure {
RequestFailedCallback callback;
RequestFailedCallback failedCallback;
};

// 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::mutex responseMutex_;

RequesterToken tokenGen_ = 20000;
};

const char *RequestTypeAsString(SystemRequestType type);
Expand All @@ -75,14 +83,14 @@ extern RequestManager g_requestManager;
// Wrappers for easy requests.
// NOTE: Semantics have changed - this no longer calls the callback on cancellation, instead you
// can specify a different callback for that.
inline void System_InputBoxGetString(const std::string &title, const std::string &defaultValue, RequestCallback callback, RequestFailedCallback failedCallback = nullptr) {
g_requestManager.MakeSystemRequest(SystemRequestType::INPUT_TEXT_MODAL, callback, failedCallback, title, defaultValue, 0);
inline void System_InputBoxGetString(RequesterToken token, const std::string &title, const std::string &defaultValue, RequestCallback callback, RequestFailedCallback failedCallback = nullptr) {
g_requestManager.MakeSystemRequest(SystemRequestType::INPUT_TEXT_MODAL, token, callback, failedCallback, title, defaultValue, 0);
}

// This one will pop up a special image browser if available. You can also pick
// images with the file browser below.
inline void System_BrowseForImage(const std::string &title, RequestCallback callback, RequestFailedCallback failedCallback = nullptr) {
g_requestManager.MakeSystemRequest(SystemRequestType::BROWSE_FOR_IMAGE, callback, failedCallback, title, "", 0);
inline void System_BrowseForImage(RequesterToken token, const std::string &title, RequestCallback callback, RequestFailedCallback failedCallback = nullptr) {
g_requestManager.MakeSystemRequest(SystemRequestType::BROWSE_FOR_IMAGE, token, callback, failedCallback, title, "", 0);
}

enum class BrowseFileType {
Expand All @@ -95,78 +103,78 @@ enum class BrowseFileType {
ANY,
};

inline void System_BrowseForFile(const std::string &title, BrowseFileType type, RequestCallback callback, RequestFailedCallback failedCallback = nullptr) {
g_requestManager.MakeSystemRequest(SystemRequestType::BROWSE_FOR_FILE, callback, failedCallback, title, "", (int)type);
inline void System_BrowseForFile(RequesterToken token, const std::string &title, BrowseFileType type, RequestCallback callback, RequestFailedCallback failedCallback = nullptr) {
g_requestManager.MakeSystemRequest(SystemRequestType::BROWSE_FOR_FILE, token, callback, failedCallback, title, "", (int)type);
}

inline void System_BrowseForFolder(const std::string &title, RequestCallback callback, RequestFailedCallback failedCallback = nullptr) {
g_requestManager.MakeSystemRequest(SystemRequestType::BROWSE_FOR_FOLDER, callback, failedCallback, title, "", 0);
inline void System_BrowseForFolder(RequesterToken token, const std::string &title, RequestCallback callback, RequestFailedCallback failedCallback = nullptr) {
g_requestManager.MakeSystemRequest(SystemRequestType::BROWSE_FOR_FOLDER, token, callback, failedCallback, title, "", 0);
}

// The returned string is username + '\n' + password.
inline void System_AskUsernamePassword(const std::string &title, RequestCallback callback, RequestFailedCallback failedCallback = nullptr) {
g_requestManager.MakeSystemRequest(SystemRequestType::ASK_USERNAME_PASSWORD, callback, failedCallback, title, "", 0);
inline void System_AskUsernamePassword(RequesterToken token, const std::string &title, RequestCallback callback, RequestFailedCallback failedCallback = nullptr) {
g_requestManager.MakeSystemRequest(SystemRequestType::ASK_USERNAME_PASSWORD, token, callback, failedCallback, title, "", 0);
}

inline void System_CopyStringToClipboard(const std::string &string) {
g_requestManager.MakeSystemRequest(SystemRequestType::COPY_TO_CLIPBOARD, nullptr, nullptr, string, "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::COPY_TO_CLIPBOARD, NO_REQUESTER_TOKEN, nullptr, nullptr, string, "", 0);
}

inline void System_ExitApp() {
g_requestManager.MakeSystemRequest(SystemRequestType::EXIT_APP, nullptr, nullptr, "", "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::EXIT_APP, NO_REQUESTER_TOKEN, nullptr, nullptr, "", "", 0);
}

inline void System_RestartApp(const std::string &params) {
g_requestManager.MakeSystemRequest(SystemRequestType::RESTART_APP, nullptr, nullptr, params, "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::RESTART_APP, NO_REQUESTER_TOKEN, nullptr, nullptr, params, "", 0);
}

inline void System_RecreateActivity() {
g_requestManager.MakeSystemRequest(SystemRequestType::RECREATE_ACTIVITY, nullptr, nullptr, "", "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::RECREATE_ACTIVITY, NO_REQUESTER_TOKEN, nullptr, nullptr, "", "", 0);
}

// The design is a little weird, just a holdover from the old message. Can either toggle or set to on or off.
inline void System_ToggleFullscreenState(const std::string &param) {
g_requestManager.MakeSystemRequest(SystemRequestType::TOGGLE_FULLSCREEN_STATE, nullptr, nullptr, param, "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::TOGGLE_FULLSCREEN_STATE, NO_REQUESTER_TOKEN, nullptr, nullptr, param, "", 0);
}

inline void System_GraphicsBackendFailedAlert(const std::string &param) {
g_requestManager.MakeSystemRequest(SystemRequestType::GRAPHICS_BACKEND_FAILED_ALERT, nullptr, nullptr, param, "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::GRAPHICS_BACKEND_FAILED_ALERT, NO_REQUESTER_TOKEN, nullptr, nullptr, param, "", 0);
}

inline void System_CameraCommand(const std::string &command) {
g_requestManager.MakeSystemRequest(SystemRequestType::CAMERA_COMMAND, nullptr, nullptr, command, "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::CAMERA_COMMAND, NO_REQUESTER_TOKEN, nullptr, nullptr, command, "", 0);
}

inline void System_GPSCommand(const std::string &command) {
g_requestManager.MakeSystemRequest(SystemRequestType::GPS_COMMAND, nullptr, nullptr, command, "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::GPS_COMMAND, NO_REQUESTER_TOKEN, nullptr, nullptr, command, "", 0);
}

inline void System_InfraredCommand(const std::string &command) {
g_requestManager.MakeSystemRequest(SystemRequestType::INFRARED_COMMAND, nullptr, nullptr, command, "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::INFRARED_COMMAND, NO_REQUESTER_TOKEN, nullptr, nullptr, command, "", 0);
}

inline void System_MicrophoneCommand(const std::string &command) {
g_requestManager.MakeSystemRequest(SystemRequestType::MICROPHONE_COMMAND, nullptr, nullptr, command, "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::MICROPHONE_COMMAND, NO_REQUESTER_TOKEN, nullptr, nullptr, command, "", 0);
}

inline void System_ShareText(const std::string &text) {
g_requestManager.MakeSystemRequest(SystemRequestType::SHARE_TEXT, nullptr, nullptr, text, "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::SHARE_TEXT, NO_REQUESTER_TOKEN, nullptr, nullptr, text, "", 0);
}

inline void System_NotifyUIState(const std::string &state) {
g_requestManager.MakeSystemRequest(SystemRequestType::NOTIFY_UI_STATE, nullptr, nullptr, state, "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::NOTIFY_UI_STATE, NO_REQUESTER_TOKEN, nullptr, nullptr, state, "", 0);
}

inline void System_SetWindowTitle(const std::string &param) {
g_requestManager.MakeSystemRequest(SystemRequestType::SET_WINDOW_TITLE, nullptr, nullptr, param, "", 0);
g_requestManager.MakeSystemRequest(SystemRequestType::SET_WINDOW_TITLE, NO_REQUESTER_TOKEN, nullptr, nullptr, param, "", 0);
}

inline bool System_SendDebugOutput(const std::string &string) {
return g_requestManager.MakeSystemRequest(SystemRequestType::SEND_DEBUG_OUTPUT, nullptr, nullptr, string, "", 0);
return g_requestManager.MakeSystemRequest(SystemRequestType::SEND_DEBUG_OUTPUT, NO_REQUESTER_TOKEN, nullptr, nullptr, string, "", 0);
}

inline void System_SendDebugScreenshot(const std::string &data, int height) {
g_requestManager.MakeSystemRequest(SystemRequestType::SEND_DEBUG_SCREENSHOT, nullptr, nullptr, data, "", height);
g_requestManager.MakeSystemRequest(SystemRequestType::SEND_DEBUG_SCREENSHOT, NO_REQUESTER_TOKEN, nullptr, nullptr, data, "", height);
}

// Non-inline to avoid including Path.h
Expand Down
18 changes: 9 additions & 9 deletions Common/UI/PopupScreens.cpp
View file
Expand Up @@ -500,8 +500,8 @@ void SliderFloatPopupScreen::OnCompleted(DialogResult result) {
}
}

PopupTextInputChoice::PopupTextInputChoice(std::string *value, const std::string &title, const std::string &placeholder, int maxLen, ScreenManager *screenManager, LayoutParams *layoutParams)
: AbstractChoiceWithValueDisplay(title, layoutParams), screenManager_(screenManager), value_(value), placeHolder_(placeholder), maxLen_(maxLen) {
PopupTextInputChoice::PopupTextInputChoice(RequesterToken token, std::string *value, const std::string &title, const std::string &placeholder, int maxLen, ScreenManager *screenManager, LayoutParams *layoutParams)
: AbstractChoiceWithValueDisplay(title, layoutParams), screenManager_(screenManager), value_(value), placeHolder_(placeholder), maxLen_(maxLen), token_(token) {
OnClick.Handle(this, &PopupTextInputChoice::HandleClick);
}

Expand All @@ -510,7 +510,7 @@ EventReturn PopupTextInputChoice::HandleClick(EventParams &e) {

// Choose method depending on platform capabilities.
if (System_GetPropertyBool(SYSPROP_HAS_TEXT_INPUT_DIALOG)) {
System_InputBoxGetString(text_, *value_ , [=](const std::string &enteredValue, int) {
System_InputBoxGetString(token_, text_, *value_ , [=](const std::string &enteredValue, int) {
*value_ = StripSpaces(enteredValue);
EventParams params{};
OnChange.Trigger(params);
Expand Down Expand Up @@ -668,10 +668,10 @@ std::string ChoiceWithValueDisplay::ValueText() const {
return valueText.str();
}

FileChooserChoice::FileChooserChoice(std::string *value, const std::string &text, BrowseFileType fileType, LayoutParams *layoutParams)
: AbstractChoiceWithValueDisplay(text, layoutParams), value_(value), fileType_(fileType) {
FileChooserChoice::FileChooserChoice(RequesterToken token, std::string *value, const std::string &text, BrowseFileType fileType, LayoutParams *layoutParams)
: AbstractChoiceWithValueDisplay(text, layoutParams), value_(value), fileType_(fileType), token_(token) {
OnClick.Add([=](UI::EventParams &) {
System_BrowseForFile(text_, fileType, [=](const std::string &returnValue, int) {
System_BrowseForFile(token, text_, fileType, [=](const std::string &returnValue, int) {
if (*value_ != returnValue) {
*value = returnValue;
UI::EventParams e{};
Expand All @@ -692,10 +692,10 @@ std::string FileChooserChoice::ValueText() const {
return path.GetFilename();
}

FolderChooserChoice::FolderChooserChoice(std::string *value, const std::string &text, LayoutParams *layoutParams)
: AbstractChoiceWithValueDisplay(text, layoutParams), value_(value) {
FolderChooserChoice::FolderChooserChoice(RequesterToken token, std::string *value, const std::string &text, LayoutParams *layoutParams)
: AbstractChoiceWithValueDisplay(text, layoutParams), value_(value), token_(token) {
OnClick.Add([=](UI::EventParams &) {
System_BrowseForFolder(text_, [=](const std::string &returnValue, int) {
System_BrowseForFolder(token_, text_, [=](const std::string &returnValue, int) {
if (*value_ != returnValue) {
*value = returnValue;
UI::EventParams e{};
Expand Down
10 changes: 6 additions & 4 deletions Common/UI/PopupScreens.h
View file
Expand Up @@ -360,7 +360,7 @@ class PopupSliderChoiceFloat : public AbstractChoiceWithValueDisplay {
// NOTE: This one will defer to a system-native dialog if possible.
class PopupTextInputChoice : public AbstractChoiceWithValueDisplay {
public:
PopupTextInputChoice(std::string *value, const std::string &title, const std::string &placeholder, int maxLen, ScreenManager *screenManager, LayoutParams *layoutParams = 0);
PopupTextInputChoice(RequesterToken token, std::string *value, const std::string &title, const std::string &placeholder, int maxLen, ScreenManager *screenManager, LayoutParams *layoutParams = 0);

Event OnChange;

Expand All @@ -370,6 +370,7 @@ class PopupTextInputChoice : public AbstractChoiceWithValueDisplay {
private:
EventReturn HandleClick(EventParams &e);
EventReturn HandleChange(EventParams &e);
RequesterToken token_;
ScreenManager *screenManager_;
std::string *value_;
std::string placeHolder_;
Expand Down Expand Up @@ -405,27 +406,28 @@ enum class FileChooserFileType {

class FileChooserChoice : public AbstractChoiceWithValueDisplay {
public:
FileChooserChoice(std::string *value, const std::string &title, BrowseFileType fileType, LayoutParams *layoutParams = nullptr);
FileChooserChoice(RequesterToken token, std::string *value, const std::string &title, BrowseFileType fileType, LayoutParams *layoutParams = nullptr);
std::string ValueText() const override;

Event OnChange;

private:
std::string *value_;
BrowseFileType fileType_;
RequesterToken token_;
};

class FolderChooserChoice : public AbstractChoiceWithValueDisplay {
public:
FolderChooserChoice(std::string *value, const std::string &title, LayoutParams *layoutParams = nullptr);
FolderChooserChoice(RequesterToken token, std::string *value, const std::string &title, LayoutParams *layoutParams = nullptr);
std::string ValueText() const override;

Event OnChange;

private:
std::string *value_;
BrowseFileType fileType_;
RequesterToken token_;
};


} // namespace UI
16 changes: 16 additions & 0 deletions Common/UI/Screen.cpp
View file
@@ -1,4 +1,5 @@
#include "Common/System/Display.h"
#include "Common/System/Request.h"
#include "Common/Input/InputState.h"
#include "Common/UI/Root.h"
#include "Common/UI/Screen.h"
Expand All @@ -21,6 +22,21 @@ void Screen::focusChanged(ScreenFocusChange focusChange) {
DEBUG_LOG(SYSTEM, "Screen %s got %s", this->tag(), eventName);
}

int Screen::GetRequesterToken() {
if (token_ < 0) {
token_ = g_requestManager.GenerateRequesterToken();
}
return token_;
}

Screen::~Screen() {
screenManager_ = nullptr;
if (token_ >= 0) {
// To avoid expired callbacks getting called.
g_requestManager.ForgetRequestsWithToken(token_);
}
}

ScreenManager::~ScreenManager() {
shutdown();
}
Expand Down

0 comments on commit 7918e49

Please sign in to comment.