From 6ac9c3bce62a8fc79e23477417188108f0ad9fa6 Mon Sep 17 00:00:00 2001 From: Bjørn Lindeijer Date: Fri, 5 Dec 2008 21:38:31 +0100 Subject: 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. --- src/Makefile.am | 1 + src/gui/updatewindow.cpp | 31 +++++++++------- src/gui/updatewindow.h | 13 +++++-- src/utils/mutex.h | 94 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 124 insertions(+), 15 deletions(-) create mode 100644 src/utils/mutex.h (limited to 'src') diff --git a/src/Makefile.am b/src/Makefile.am index 867953ab..3387690a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -219,6 +219,7 @@ tmw_SOURCES = gui/widgets/resizegrip.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 ea0f86cd..6083725e 100644 --- a/src/gui/updatewindow.cpp +++ b/src/gui/updatewindow.cpp @@ -47,7 +47,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); @@ -146,15 +146,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()); @@ -169,8 +163,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() @@ -413,6 +408,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: @@ -434,7 +440,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 d7e3c4c7..a7dfe2cb 100644 --- a/src/gui/updatewindow.h +++ b/src/gui/updatewindow.h @@ -30,12 +30,13 @@ #include "../guichanfwd.h" +#include "../utils/mutex.h" + class BrowserBox; class Button; class ProgressBar; class ScrollArea; -struct SDL_mutex; struct SDL_Thread; /** @@ -88,7 +89,7 @@ class UpdaterWindow : public Window, public gcn::ActionListener int updateState; - protected: +private: void download(); /** @@ -133,6 +134,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; @@ -164,7 +171,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 + +#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 -- cgit v1.2.3-70-g09d2