Fix crashes on x-thread signal emission.

Fix crashes on x-thread signal emission if the emitting object
is destroyed between the storage of the message on the queue
and the emission of the object in the UI thread.

diff --git a/src/lib/content.cc b/src/lib/content.cc
index fcc658717900eb8e4468883815332e5d3f40c72e..b00ea3e5774ec02d89e2f77778892861af818bb9 100644 (file)
--- a/src/lib/content.cc
+++ b/src/lib/content.cc
@@ -153,9 +153,7 @@ Content::examine (shared_ptr<Job> job)
 void
 Content::signal_changed (int p)
 {
-       if (ui_signaller) {
-               ui_signaller->emit (boost::bind (boost::ref (Changed), shared_from_this (), p, _change_signals_frequent));
-       }
+       emit (boost::bind (boost::ref (Changed), shared_from_this (), p, _change_signals_frequent));
 }
 
 void

diff --git a/src/lib/content.h b/src/lib/content.h
index c6cede5fa6cafde4d04f18dc1943a3551c858a2c..2b966110bfd398d65f58f545a8052db5ccff8f6b 100644 (file)
--- a/src/lib/content.h
+++ b/src/lib/content.h
@@ -25,6 +25,7 @@
 #define DCPOMATIC_CONTENT_H
 
 #include "types.h"
+#include "signaller.h"
 #include "dcpomatic_time.h"
 #include <libxml++/libxml++.h>
 #include <libcxml/cxml.h>
@@ -53,7 +54,7 @@ public:
 /** @class Content
  *  @brief A piece of content represented by one or more files on disk.
  */
-class Content : public boost::enable_shared_from_this<Content>, public boost::noncopyable
+class Content : public boost::enable_shared_from_this<Content>, public Signaller, public boost::noncopyable
 {
 public:
        Content (boost::shared_ptr<const Film>);

diff --git a/src/lib/film.cc b/src/lib/film.cc
index 80755a4cb8c20972a415cb66495adba1f4725162..35773c797e25c591c20b65c819a1415a1d8bdc99 100644 (file)
--- a/src/lib/film.cc
+++ b/src/lib/film.cc
@@ -32,7 +32,6 @@
 #include "exceptions.h"
 #include "examine_content_job.h"
 #include "config.h"
-#include "ui_signaller.h"
 #include "playlist.h"
 #include "player.h"
 #include "dcp_content_type.h"
@@ -790,9 +789,7 @@ Film::signal_changed (Property p)
                break;
        }
 
-       if (ui_signaller) {
-               ui_signaller->emit (boost::bind (boost::ref (Changed), p));
-       }
+       emit (boost::bind (boost::ref (Changed), p));
 }
 
 void
@@ -995,9 +992,7 @@ Film::playlist_content_changed (boost::weak_ptr<Content> c, int p)
                signal_changed (NAME);
        }
 
-       if (ui_signaller) {
-               ui_signaller->emit (boost::bind (boost::ref (ContentChanged), c, p));
-       }
+       emit (boost::bind (boost::ref (ContentChanged), c, p));
 }
 
 void

diff --git a/src/lib/film.h b/src/lib/film.h
index 3cd370a0d109a753c14314592346518208f3fae2..f61062be0cc3bfde33d368e3c12e0941a90b79d9 100644 (file)
--- a/src/lib/film.h
+++ b/src/lib/film.h
@@ -29,6 +29,7 @@
 #include "types.h"
 #include "isdcf_metadata.h"
 #include "frame_rate_change.h"
+#include "signaller.h"
 #include "ratio.h"
 #include <dcp/key.h>
 #include <dcp/encrypted_kdm.h>
@@ -55,7 +56,7 @@ struct isdcf_name_test;
  *
  *  The content of a Film is held in a Playlist (created and managed by the Film).
  */
-class Film : public boost::enable_shared_from_this<Film>, public boost::noncopyable
+class Film : public boost::enable_shared_from_this<Film>, public Signaller, public boost::noncopyable
 {
 public:
        Film (boost::filesystem::path, bool log = true);

diff --git a/src/lib/job.cc b/src/lib/job.cc
index eadafbf73a1f8d6b20c3ffca9527d90a68f2fb18..f281466325f8c34a7e78d8e0ad8229910335946b 100644 (file)
--- a/src/lib/job.cc
+++ b/src/lib/job.cc
@@ -203,8 +203,8 @@ Job::set_state (State s)
                }
        }
 
-       if (finished && ui_signaller) {
-               ui_signaller->emit (boost::bind (boost::ref (Finished)));
+       if (finished) {
+               emit (boost::bind (boost::ref (Finished)));
        }       
 }
 
@@ -239,9 +239,7 @@ Job::set_progress (float p, bool force)
                _pause_changed.wait (lm2);
        }
 
-       if (ui_signaller) {
-               ui_signaller->emit (boost::bind (boost::ref (Progress)));
-       }
+       emit (boost::bind (boost::ref (Progress)));
 }
 
 /** @return fractional progress of the current sub-job, if known */
@@ -301,9 +299,7 @@ Job::set_progress_unknown ()
        _progress.reset ();
        lm.unlock ();
 
-       if (ui_signaller) {
-               ui_signaller->emit (boost::bind (boost::ref (Progress)));
-       }
+       emit (boost::bind (boost::ref (Progress)));
 }
 
 /** @return Human-readable status of this job */

diff --git a/src/lib/job.h b/src/lib/job.h
index 7c6707880d752cc62d3c7a0c602c7f7272ebaff2..8fe87747c2186a465b48799ab8b4734bc9a58fe6 100644 (file)
--- a/src/lib/job.h
+++ b/src/lib/job.h
@@ -24,6 +24,7 @@
 #ifndef DCPOMATIC_JOB_H
 #define DCPOMATIC_JOB_H
 
+#include "signaller.h"
 #include <boost/thread/mutex.hpp>
 #include <boost/enable_shared_from_this.hpp>
 #include <boost/signals2.hpp>
@@ -35,7 +36,7 @@ class Film;
 /** @class Job
  *  @brief A parent class to represent long-running tasks which are run in their own thread.
  */
-class Job : public boost::enable_shared_from_this<Job>, public boost::noncopyable
+class Job : public boost::enable_shared_from_this<Job>, public Signaller, public boost::noncopyable
 {
 public:
        Job (boost::shared_ptr<const Film>);

diff --git a/src/lib/job_manager.cc b/src/lib/job_manager.cc
index 2b727b0aaa43985942674f07d4f881051369236b..63db662d0f185aed9502c342d5d7dd794a75f68b 100644 (file)
--- a/src/lib/job_manager.cc
+++ b/src/lib/job_manager.cc
@@ -64,9 +64,7 @@ JobManager::add (shared_ptr<Job> j)
                _jobs.push_back (j);
        }
 
-       if (ui_signaller) {
-               ui_signaller->emit (boost::bind (boost::ref (JobAdded), weak_ptr<Job> (j)));
-       }
+       emit (boost::bind (boost::ref (JobAdded), weak_ptr<Job> (j)));
        
        return j;
 }
@@ -138,9 +136,7 @@ JobManager::scheduler ()
 
                if (active_jobs != _last_active_jobs) {
                        _last_active_jobs = active_jobs;
-                       if (ui_signaller) {
-                               ui_signaller->emit (boost::bind (boost::ref (ActiveJobsChanged), active_jobs));
-                       }
+                       emit (boost::bind (boost::ref (ActiveJobsChanged), active_jobs));
                }
 
                dcpomatic_sleep (1);

diff --git a/src/lib/job_manager.h b/src/lib/job_manager.h
index 9d8620cbb58165ef320d40497235a92736e325e6..b946c1a98e0836893eb758b2d19554d4643871e2 100644 (file)
--- a/src/lib/job_manager.h
+++ b/src/lib/job_manager.h
@@ -21,6 +21,7 @@
  *  @brief A simple scheduler for jobs.
  */
 
+#include "signaller.h"
 #include <boost/thread/mutex.hpp>
 #include <boost/thread.hpp>
 #include <boost/signals2.hpp>
@@ -32,7 +33,7 @@ extern void wait_for_jobs ();
 /** @class JobManager
  *  @brief A simple scheduler for jobs.
  */
-class JobManager : public boost::noncopyable
+class JobManager : public Signaller, public boost::noncopyable
 {
 public:
 

diff --git a/src/lib/server_finder.cc b/src/lib/server_finder.cc
index 979046dabdd701829e176c86ddcc675eb860d9a9..72a9a4ef5d9f166d3945959849cb73fa3f0fc20b 100644 (file)
--- a/src/lib/server_finder.cc
+++ b/src/lib/server_finder.cc
@@ -173,7 +173,7 @@ ServerFinder::handle_accept (boost::system::error_code ec, shared_ptr<Socket> so
                        boost::mutex::scoped_lock lm (_mutex);
                        _servers.push_back (sd);
                }
-               ui_signaller->emit (boost::bind (boost::ref (ServerFound), sd));
+               emit (boost::bind (boost::ref (ServerFound), sd));
        }
 
        start_accept ();

diff --git a/src/lib/server_finder.h b/src/lib/server_finder.h
index 3fab6864a332ef43b3b62a59bf2ccab75d49aae6..dc62f998deb0d79f5e9926753fbd5c81c95f7559 100644 (file)
--- a/src/lib/server_finder.h
+++ b/src/lib/server_finder.h
@@ -18,9 +18,10 @@
 */
 
 #include "server.h"
+#include "signaller.h"
 #include <boost/signals2.hpp>
 
-class ServerFinder : public ExceptionStore
+class ServerFinder : public Signaller, public ExceptionStore
 {
 public:
        boost::signals2::connection connect (boost::function<void (ServerDescription)>);

diff --git a/src/lib/signaller.h b/src/lib/signaller.h
new file mode 100644 (file)
index 0000000..408cfcf
--- /dev/null
+++ b/src/lib/signaller.h
@@ -0,0 +1,131 @@
+/*
+    Copyright (C) 2015 Carl Hetherington <cth@carlh.net>
+
+    This program 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
+    (at your option) any later version.
+
+    This program 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 this program; if not, write to the Free Software
+    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+
+*/
+
+#ifndef DCPOMATIC_SIGNALLER_H
+#define DCPOMATIC_SIGNALLER_H
+
+#include "ui_signaller.h"
+#include <boost/thread/mutex.hpp>
+#include <boost/signals2.hpp>
+
+class WrapperBase
+{
+public:
+       WrapperBase ()
+               : _valid (true)
+               , _finished (false)
+       {}
+
+       virtual ~WrapperBase () {}
+
+       /* Can be called from any thread */
+       void invalidate ()
+       {
+               boost::mutex::scoped_lock lm (_mutex);
+               _valid = false;
+       }
+
+       bool finished () const {
+               boost::mutex::scoped_lock lm (_mutex);
+               return _finished;
+       }
+
+protected:
+       /* Protect _valid and _finished */
+       mutable boost::mutex _mutex;
+       bool _valid;
+       bool _finished;
+};
+
+/** Helper class to manage lifetime of signals, specifically to address
+ *  the problem where an object containing a signal is deleted before
+ *  its signal is emitted.
+ */
+template <class T>
+class Wrapper : public WrapperBase
+{
+public:
+       Wrapper (T signal)
+               : _signal (signal)
+       {
+
+       }
+
+       /* Called by the UI thread only */
+       void signal ()
+       {
+               boost::mutex::scoped_lock lm (_mutex);
+               if (_valid) {
+                       _signal ();
+               }
+               _finished = true;
+       }
+
+private:
+       T _signal;
+};
+
+/** Parent for any class which needs to raise cross-thread signals (from non-UI
+ *  to UI).  Subclasses should call, e.g. emit (boost::bind (boost::ref (MySignal), foo, bar));
+ */
+class Signaller
+{
+public:
+       /* Can be called from any thread */
+       virtual ~Signaller () {
+               boost::mutex::scoped_lock lm (_mutex);
+               for (std::list<WrapperBase*>::iterator i = _wrappers.begin(); i != _wrappers.end(); ++i) {
+                       (*i)->invalidate ();
+               }
+       }
+
+       /* Can be called from any thread */
+       template <class T>
+       void emit (T signal)
+       {
+               Wrapper<T>* w = new Wrapper<T> (signal);
+               if (ui_signaller) {
+                       ui_signaller->emit (boost::bind (&Wrapper<T>::signal, w));
+               }
+               
+               boost::mutex::scoped_lock lm (_mutex);
+
+               /* Clean up finished Wrappers */
+               std::list<WrapperBase*>::iterator i = _wrappers.begin ();
+               while (i != _wrappers.end ()) {
+                       std::list<WrapperBase*>::iterator tmp = i;
+                       ++tmp;
+                       if ((*i)->finished ()) {
+                               delete *i;
+                               _wrappers.erase (i);
+                       }
+                       i = tmp;
+               }
+
+               /* Add the new one */
+               _wrappers.push_back (w);
+       }
+
+private:
+       /* Protect _wrappers */
+       boost::mutex _mutex;
+       std::list<WrapperBase*> _wrappers;
+};
+
+#endif

diff --git a/src/lib/ui_signaller.h b/src/lib/ui_signaller.h
index ee4d230d407e566114ae57b491700196c19d21c5..9d4495cd18921822b2fcfa02836a3c916f2cfedd 100644 (file)
--- a/src/lib/ui_signaller.h
+++ b/src/lib/ui_signaller.h
@@ -24,6 +24,8 @@
 #include <boost/asio.hpp>
 #include <boost/thread.hpp>
 
+class Signaller;
+
 /** A class to allow signals to be emitted from non-UI threads and handled
  *  by a UI thread.
  */
@@ -37,23 +39,6 @@ public:
                _ui_thread = boost::this_thread::get_id ();
        }
 
-       /** Emit a signal from any thread whose handlers will be called in the UI
-        *  thread.  Use something like:
-        *
-        *  ui_signaller->emit (boost::bind (boost::ref (SomeSignal), parameter));
-        */
-       template <typename T>
-       void emit (T f) {
-               if (boost::this_thread::get_id() == _ui_thread) {
-                       /* already in the UI thread */
-                       f ();
-               } else {
-                       /* non-UI thread; post to the service and wake up the UI */
-                       _service.post (f);
-                       wake_ui ();
-               }
-       }
-
        /* Do something next time the UI is idle */
        template <typename T>
        void when_idle (T f) {
@@ -73,6 +58,25 @@ public:
        }
 
 private:
+       /** Emit a signal from any thread whose handlers will be called in the UI
+        *  thread.  Use something like:
+        *
+        *  ui_signaller->emit (boost::bind (boost::ref (SomeSignal), parameter));
+        */
+       template <typename T>
+       void emit (T f) {
+               if (boost::this_thread::get_id() == _ui_thread) {
+                       /* already in the UI thread */
+                       f ();
+               } else {
+                       /* non-UI thread; post to the service and wake up the UI */
+                       _service.post (f);
+                       wake_ui ();
+               }
+       }
+
+       friend class Signaller;
+       
        /** A io_service which is used as the conduit for messages */
        boost::asio::io_service _service;
        /** Object required to keep io_service from stopping when it has nothing to do */

diff --git a/src/lib/update.cc b/src/lib/update.cc
index a05df8ef304dae26ee0d4b509113b3d8502198c0..26944ecc32d2bc3503d2472f91d7feff49849533 100644 (file)
--- a/src/lib/update.cc
+++ b/src/lib/update.cc
@@ -168,7 +168,7 @@ UpdateChecker::set_state (State s)
                _emits++;
        }
 
-       ui_signaller->emit (boost::bind (boost::ref (StateChanged)));
+       emit (boost::bind (boost::ref (StateChanged)));
 }
 
 UpdateChecker *

diff --git a/src/lib/update.h b/src/lib/update.h
index 5bb9e95016de1ed2bace2ec556fd54f9dccfef07..461217a372ea61ae3802f0018a0026da76ff2339 100644 (file)
--- a/src/lib/update.h
+++ b/src/lib/update.h
@@ -21,6 +21,7 @@
  *  @brief UpdateChecker class.
  */
 
+#include "signaller.h"
 #include <curl/curl.h>
 #include <boost/signals2.hpp>
 #include <boost/thread/mutex.hpp>
@@ -30,7 +31,7 @@
 struct update_checker_test;
 
 /** Class to check for the existance of an update for DCP-o-matic on a remote server */
-class UpdateChecker : public boost::noncopyable
+class UpdateChecker : public Signaller, public boost::noncopyable
 {
 public:
        UpdateChecker ();