diff options
author | Bjørn Lindeijer <bjorn@lindeijer.nl> | 2008-12-05 21:38:31 +0100 |
---|---|---|
committer | Ira Rice <irarice@gmail.com> | 2008-12-05 15:55:14 -0700 |
commit | 7d5bfc11b5e467b502a3b56e853200b887859808 (patch) | |
tree | 4efe49ce0b3e830911700be9d249ce678b3f99cd | |
parent | 4eec3f17deafad4ed2fc28fae76fe1f4994e3076 (diff) | |
download | mana-client-7d5bfc11b5e467b502a3b56e853200b887859808.tar.gz mana-client-7d5bfc11b5e467b502a3b56e853200b887859808.tar.bz2 mana-client-7d5bfc11b5e467b502a3b56e853200b887859808.tar.xz mana-client-7d5bfc11b5e467b502a3b56e853200b887859808.zip |
Fix race condition with a std::string access
The downloading thread was writing to a std::string while the main
thread was trying to draw it, for example. Now access to the label
caption is guarded with a mutex.
Should fix crashes while downloading updates.
-rw-r--r-- | src/Makefile.am | 1 | ||||
-rw-r--r-- | src/gui/updatewindow.cpp | 31 | ||||
-rw-r--r-- | src/gui/updatewindow.h | 13 | ||||
-rw-r--r-- | src/utils/mutex.h | 94 |
4 files changed, 124 insertions, 15 deletions
diff --git a/src/Makefile.am b/src/Makefile.am index 76773d49..75434d8e 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -229,6 +229,7 @@ aethyra_SOURCES = gui/widgets/dropdown.cpp \ utils/strprintf.h \ utils/tostring.h \ utils/trim.h \ + utils/mutex.h \ utils/xml.cpp \ utils/xml.h \ animatedsprite.cpp \ diff --git a/src/gui/updatewindow.cpp b/src/gui/updatewindow.cpp index 64f02f54..9d9f5616 100644 --- a/src/gui/updatewindow.cpp +++ b/src/gui/updatewindow.cpp @@ -46,7 +46,7 @@ /** * Calculates the Alder-32 checksum for the given file. */ -unsigned long fadler32(FILE *file) +static unsigned long fadler32(FILE *file) { // Obtain file size fseek(file, 0, SEEK_END); @@ -145,15 +145,9 @@ UpdaterWindow::UpdaterWindow(const std::string &updateHost, UpdaterWindow::~UpdaterWindow() { if (mThread) - { - SDL_WaitThread(mThread, NULL); - mThread = NULL; - } + SDL_WaitThread(mThread, NULL); - if (mMemoryBuffer) - { - free(mMemoryBuffer); - } + free(mMemoryBuffer); // Remove possibly leftover temporary download ::remove((mUpdatesDir + "/download.temp").c_str()); @@ -168,8 +162,9 @@ void UpdaterWindow::setProgress(float p) void UpdaterWindow::setLabel(const std::string &str) { - mLabel->setCaption(str); - mLabel->adjustSize(); + // Do delayed label text update, since Guichan isn't thread-safe + MutexLocker lock(mLabelMutex); + mNewLabelCaption = str; } void UpdaterWindow::enable() @@ -426,6 +421,17 @@ void UpdaterWindow::logic() // Update Scroll logic mScrollArea->logic(); + // Synchronize label caption when necessary + { + MutexLocker lock(mLabelMutex); + + if (mLabel->getCaption() != mNewLabelCaption) + { + mLabel->setCaption(mNewLabelCaption); + mLabel->adjustSize(); + } + } + switch (mDownloadStatus) { case UPDATE_ERROR: @@ -447,7 +453,8 @@ void UpdaterWindow::logic() mBrowserBox->addRow("##1 It is strongly recommended that"); mBrowserBox->addRow("##1 you try again later"); mBrowserBox->addRow(mCurlError); - mScrollArea->setVerticalScrollAmount(mScrollArea->getVerticalMaxScroll()); + mScrollArea->setVerticalScrollAmount( + mScrollArea->getVerticalMaxScroll()); mDownloadStatus = UPDATE_COMPLETE; break; case UPDATE_NEWS: diff --git a/src/gui/updatewindow.h b/src/gui/updatewindow.h index 4c302e85..9cd3405e 100644 --- a/src/gui/updatewindow.h +++ b/src/gui/updatewindow.h @@ -31,12 +31,13 @@ #include "../guichanfwd.h" +#include "../utils/mutex.h" + class BrowserBox; class Button; class ProgressBar; class ScrollArea; -struct SDL_mutex; struct SDL_Thread; /** @@ -89,7 +90,7 @@ class UpdaterWindow : public Window, public gcn::ActionListener int updateState; - protected: +private: void download(); /** @@ -134,6 +135,12 @@ class UpdaterWindow : public Window, public gcn::ActionListener /** The file currently downloading. */ std::string mCurrentFile; + /** The new label caption to be set in the logic method. */ + std::string mNewLabelCaption; + + /** The mutex used to guard access to mNewLabelCaption. */ + Mutex mLabelMutex; + /** The Adler32 checksum of the file currently downloading. */ unsigned long mCurrentChecksum; @@ -165,7 +172,7 @@ class UpdaterWindow : public Window, public gcn::ActionListener Button *mCancelButton; /**< Button to stop the update process. */ Button *mPlayButton; /**< Button to start playing. */ ProgressBar *mProgressBar; /**< Update progress bar. */ - BrowserBox* mBrowserBox; /**< Box to display news. */ + BrowserBox *mBrowserBox; /**< Box to display news. */ ScrollArea *mScrollArea; /**< Used to scroll news box. */ }; diff --git a/src/utils/mutex.h b/src/utils/mutex.h new file mode 100644 index 00000000..6c35987c --- /dev/null +++ b/src/utils/mutex.h @@ -0,0 +1,94 @@ +/* + * The Mana World + * Copyright 2008 The Mana World Development Team + * + * This file is part of The Mana World. + * + * The Mana World is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * any later version. + * + * The Mana World is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with The Mana World; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#ifndef TMW_MUTEX_H +#define TMW_MUTEX_H + +#include <SDL_thread.h> + +#include "../log.h" + +/** + * A mutex provides mutual exclusion of access to certain data that is + * accessed by multiple threads. + */ +class Mutex +{ +public: + Mutex(); + ~Mutex(); + + void lock(); + void unlock(); + +private: + SDL_mutex *mMutex; +}; + +/** + * A convenience class for locking a mutex. + */ +class MutexLocker +{ +public: + MutexLocker(Mutex mutex); + ~MutexLocker(); + +private: + Mutex mMutex; +}; + + +inline Mutex::Mutex() +{ + mMutex = SDL_CreateMutex(); +} + +inline Mutex::~Mutex() +{ + SDL_DestroyMutex(mMutex); +} + +inline void Mutex::lock() +{ + if (SDL_mutexP(mMutex) == -1) + logger->log("Mutex locking failed: %s", SDL_GetError()); +} + +inline void Mutex::unlock() +{ + if (SDL_mutexV(mMutex) == -1) + logger->log("Mutex unlocking failed: %s", SDL_GetError()); +} + + +inline MutexLocker::MutexLocker(Mutex mutex): + mMutex(mutex) +{ + mMutex.lock(); +} + +inline MutexLocker::~MutexLocker() +{ + mMutex.unlock(); +} + +#endif // TMW_MUTEX_H |