Merge pull request #4645 from Beep6581/fix-thumbbrowser-deadlock

Fix thumbbrowser deadlock (fixes #4576)

rtgui/filebrowserentry.cc

@@ -211,19 +211,8 @@ void FileBrowserEntry::procParamsChanged (Thumbnail* thm, int whoChangedIt)
 
 void FileBrowserEntry::updateImage (rtengine::IImage8* img, double scale, rtengine::procparams::CropParams cropParams)
 {
-
+    redrawRequests++;
-    {
+    feih->pending++;
-        GThreadLock lock;
-
-        if ( feih == nullptr ||
-                feih->destroyed ) {
-            img->free();
-            return;
-        }
-
-        redrawRequests++;
-        feih->pending++;
-    }
 
     struct tiupdate {
         FileBrowserEntryIdleHelper* feih;

rtgui/filebrowserentry.h

@@ -19,23 +19,27 @@
 #ifndef _FILEBROWSERENTRY_
 #define _FILEBROWSERENTRY_
 
+#include <atomic>
+
 #include <gtkmm.h>
-#include "thumbbrowserentrybase.h"
+
-#include "thumbnail.h"
-#include "filethumbnailbuttonset.h"
-#include "thumbnaillistener.h"
-#include "thumbimageupdater.h"
-#include "imageareatoollistener.h"
-#include "editenums.h"
 #include "../rtengine/rtengine.h"
+
 #include "crophandler.h"
+#include "editenums.h"
+#include "filethumbnailbuttonset.h"
+#include "imageareatoollistener.h"
+#include "thumbbrowserentrybase.h"
+#include "thumbimageupdater.h"
+#include "thumbnail.h"
+#include "thumbnaillistener.h"
 
 
 class FileBrowserEntry;
 struct FileBrowserEntryIdleHelper {
     FileBrowserEntry* fbentry;
     bool destroyed;
-    int pending;
+    std::atomic<int> pending;
 };
 
 class FileThumbnailButtonSet;

rtgui/thumbbrowserentrybase.h

@@ -19,12 +19,15 @@
 #ifndef _THUMBNAILBROWSERENTRYBASE_
 #define _THUMBNAILBROWSERENTRYBASE_
 
+#include <atomic>
+
 #include <gtkmm.h>
-#include "lwbuttonset.h"
+
-#include "thumbnail.h"
-#include "threadutils.h"
-#include "guiutils.h"
 #include "cursormanager.h"
+#include "guiutils.h"
+#include "lwbuttonset.h"
+#include "threadutils.h"
+#include "thumbnail.h"
 
 class ThumbBrowserBase;
 class ThumbBrowserEntryBase
@@ -71,7 +74,7 @@ protected:
 
     int ofsX, ofsY; // offset due to the scrolling of the parent
 
-    int redrawRequests;
+    std::atomic<int> redrawRequests;
 
     ThumbBrowserBase* parent;
     ThumbBrowserEntryBase* original;

rtgui/thumbimageupdater.cc

@@ -17,9 +17,13 @@
  *  along with RawTherapee.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <atomic>
 #include <set>
-#include "thumbimageupdater.h"
+
 #include <gtkmm.h>
+
+#include "thumbimageupdater.h"
+
 #include "guiutils.h"
 #include "threadutils.h"
 
@@ -83,7 +87,7 @@ public:
 
     JobList jobs_;
 
-    unsigned int active_;
+    std::atomic<unsigned int> active_;
 
     bool inactive_waiting_;
 
@@ -157,13 +161,11 @@ public:
             j.listener_->updateImage(img, scale, thm->getProcParams().crop);
         }
 
-        {
+        if ( --active_ == 0 ) {
             Glib::Threads::Mutex::Lock lock(mutex_);
-
+            if (inactive_waiting_) {
-            if ( --active_ == 0 &&
-                    inactive_waiting_ ) {
                 inactive_waiting_ = false;
-                inactive_.signal();
+                inactive_.broadcast();
             }
         }
     }
@@ -181,8 +183,7 @@ ThumbImageUpdater::ThumbImageUpdater():
 {
 }
 
-void
+void ThumbImageUpdater::add(ThumbBrowserEntryBase* tbe, bool* priority, bool upgrade, ThumbImageUpdateListener* l)
-ThumbImageUpdater::add(ThumbBrowserEntryBase* tbe, bool* priority, bool upgrade, ThumbImageUpdateListener* l)
 {
     // nobody listening?
     if ( l == nullptr ) {
@@ -216,49 +217,51 @@ ThumbImageUpdater::add(ThumbBrowserEntryBase* tbe, bool* priority, bool upgrade,
 }
 
 
-void
+void ThumbImageUpdater::removeJobs(ThumbImageUpdateListener* listener)
-ThumbImageUpdater::removeJobs(ThumbImageUpdateListener* listener)
 {
     DEBUG("removeJobs(%p)", listener);
 
-    Glib::Threads::Mutex::Lock lock(impl_->mutex_);
+    {
+        Glib::Threads::Mutex::Lock lock(impl_->mutex_);
 
-    for( Impl::JobList::iterator i(impl_->jobs_.begin()); i != impl_->jobs_.end(); ) {
+        for( Impl::JobList::iterator i(impl_->jobs_.begin()); i != impl_->jobs_.end(); ) {
-        if (i->listener_ == listener) {
+            if (i->listener_ == listener) {
-            DEBUG("erasing specific job");
+                DEBUG("erasing specific job");
-            Impl::JobList::iterator e(i++);
+                Impl::JobList::iterator e(i++);
-            impl_->jobs_.erase(e);
+                impl_->jobs_.erase(e);
-        } else {
+            } else {
-            ++i;
+                ++i;
+            }
         }
     }
 
     while ( impl_->active_ != 0 ) {
-        // XXX this is nasty... it would be nicer if we weren't called with
-        // this lock held
-        GThreadUnLock unlock;
         DEBUG("waiting for running jobs1");
-        impl_->inactive_waiting_ = true;
+        {
-        impl_->inactive_.wait(impl_->mutex_);
+            Glib::Threads::Mutex::Lock lock(impl_->mutex_);
+            impl_->inactive_waiting_ = true;
+            impl_->inactive_.wait(impl_->mutex_);
+        }
     }
 }
 
-void
+void ThumbImageUpdater::removeAllJobs()
-ThumbImageUpdater::removeAllJobs()
 {
     DEBUG("stop");
 
-    Glib::Threads::Mutex::Lock lock(impl_->mutex_);
+    {
+        Glib::Threads::Mutex::Lock lock(impl_->mutex_);
 
-    impl_->jobs_.clear();
+        impl_->jobs_.clear();
+    }
 
     while ( impl_->active_ != 0 ) {
-        // XXX this is nasty... it would be nicer if we weren't called with
-        // this lock held
-        GThreadUnLock unlock;
         DEBUG("waiting for running jobs2");
-        impl_->inactive_waiting_ = true;
+        {
-        impl_->inactive_.wait(impl_->mutex_);
+            Glib::Threads::Mutex::Lock lock(impl_->mutex_);
+            impl_->inactive_waiting_ = true;
+            impl_->inactive_.wait(impl_->mutex_);
+        }
     }
 }