From 586641120cdf9264041c445fda1975e989d1c503 Mon Sep 17 00:00:00 2001 From: hiker Date: Wed, 5 Mar 2014 08:21:19 +1100 Subject: [PATCH] Fixed crash in online user search (triggered when pressing ESC while the search is being executed). --- src/online/current_user.cpp | 2 +- src/online/current_user.hpp | 2 +- src/online/request.hpp | 13 +++++-- src/online/request_manager.cpp | 2 +- src/states_screens/online_user_search.cpp | 42 +++++++++++++++++++++-- src/states_screens/online_user_search.hpp | 2 +- 6 files changed, 54 insertions(+), 9 deletions(-) diff --git a/src/online/current_user.cpp b/src/online/current_user.cpp index 6927b658f..d4058b5e5 100644 --- a/src/online/current_user.cpp +++ b/src/online/current_user.cpp @@ -366,7 +366,7 @@ namespace Online * search term. * \param search_string the string to search for. */ - const XMLRequest* + XMLRequest* CurrentUser::requestUserSearch(const core::stringw &search_string) const { assert(m_state == US_SIGNED_IN); diff --git a/src/online/current_user.hpp b/src/online/current_user.hpp index f5b00e03c..022fc7645 100644 --- a/src/online/current_user.hpp +++ b/src/online/current_user.hpp @@ -196,7 +196,7 @@ namespace Online const irr::core::stringw &new_password, const irr::core::stringw &new_password_ver) const; - const XMLRequest * requestUserSearch(const irr::core::stringw & search_string) const; + XMLRequest * requestUserSearch(const irr::core::stringw & search_string) const; void onSTKQuit() const; void onAchieving(uint32_t achievement_id) const; diff --git a/src/online/request.hpp b/src/online/request.hpp index a3572e644..4ddec7387 100644 --- a/src/online/request.hpp +++ b/src/online/request.hpp @@ -66,15 +66,17 @@ namespace Online LEAK_CHECK() /** Type of the request. Has 0 as default value. */ - const int m_type; + const int m_type; + /** True if the memory for this Request should be managed by * http connector (i.e. this object is freed once the request * is handled). Otherwise the memory is not freed, so it must * be freed by the calling function. */ - const bool m_manage_memory; + bool m_manage_memory; + /** The priority of this request. The higher the value the more important this request is. */ - const int m_priority; + const int m_priority; /** The different state of the requst: * - S_PREPARING:\n The request is created and can be configured, it @@ -140,6 +142,11 @@ namespace Online * by network_http (i.e. freed once the request is handled). */ bool manageMemory() const { return m_manage_memory; } // -------------------------------------------------------------------- + /** Sets the memory management flag of this request. This function + * must only be called by the main thread, since it is only tested by + * the main thread. */ + void setManageMemory(bool m) { m_manage_memory = m; } + // -------------------------------------------------------------------- /** Returns the priority of this request. */ int getPriority() const { return m_priority; } // -------------------------------------------------------------------- diff --git a/src/online/request_manager.cpp b/src/online/request_manager.cpp index b717c4d46..14be63b88 100644 --- a/src/online/request_manager.cpp +++ b/src/online/request_manager.cpp @@ -263,7 +263,7 @@ namespace Online else request->setDone(); } - } + } // handleResultQueue // ------------------------------------------------------------------------ /** Should be called every frame and takes care of processing the result diff --git a/src/states_screens/online_user_search.cpp b/src/states_screens/online_user_search.cpp index 67c64ed21..1b9095192 100644 --- a/src/states_screens/online_user_search.cpp +++ b/src/states_screens/online_user_search.cpp @@ -88,8 +88,46 @@ void OnlineUserSearch::init() */ void OnlineUserSearch::tearDown() { - delete m_search_request; - m_search_request = NULL; + // The search request can be in one of three states: + // 1. It does not exist, nothing more to do. + // 2. It has been executed by the request manager, had its callback done + // and waits for this widget to collect the results. In this case, the + // requests state is 'isDone', and the memory of this object need to be + // freed here. + // 3. It is being executed by the request manager thread. In this case the + // request can not be freed (since the request manager might still + // write to it). In this case we set the flag that the request manager + // should manage the memory for the request, i.e. the request will be + // deleted once it is in the request manager's ready queue. + // Note that there is no race condition here: setting a request to be + // 'done', and checking if its memory need to be freed is done by the + // main thread (i.e. the same thread that executes this function ). So it + // is not possible that the thread stage changes to be 'isDone' while + // this function executes, or that the request is checked if it should + // be freed. + + if (m_search_request) + { + // Check if the request is ready (but its result have not been + // received here ... otherwise it would have been deleted). + if (m_search_request->isDone()) + { + delete m_search_request; + } + else + { + // This request is currently being handled by the request manager. + // We can set the memory management flag, since the separate + // request manager thread does not read or write this flag. + // The actual deletion of the request is done by the main + // thread, so there is no risk that the request is deleted + // between the next two calls!! + m_search_request->setManageMemory(true); + + // Set cancel flag to speed up cancellation of this request + m_search_request->cancel(); + } + } // if m_search_request } // tearDown diff --git a/src/states_screens/online_user_search.hpp b/src/states_screens/online_user_search.hpp index 3d8c7720f..0bee15cdb 100644 --- a/src/states_screens/online_user_search.hpp +++ b/src/states_screens/online_user_search.hpp @@ -60,7 +60,7 @@ private: Online::Profile::IDList m_users; /** The online request to search for users. */ - const Online::XMLRequest * m_search_request; + Online::XMLRequest *m_search_request; void parseResult(const XMLNode * input); void showList();