Skip to content

Commit

Permalink
Fix a particular type of race condition in file dialog requests
Browse files Browse the repository at this point in the history 
It seems to be possible for a user to back out of a screen before
receiving the "dialog completed" callback on Android, in which case
things pointed to by the callback might be gone.

In this case, it's better to simply not call the callback, rather than
crashing.

This is accomplished by assigning "Tokens" to screens that cause
requests, and in ~Screen, invalidate any pending requests belonging to
that token.
  • Loading branch information
@hrydgard
hrydgard committed Jan 18, 2024
1 parent b4122ef commit 1304d04
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -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
Loading

0 comments on commit 1304d04

Please sign in to comment.