From: Carl Hetherington Date: Wed, 13 May 2015 20:57:40 +0000 (+0100) Subject: Fix crashes on x-thread signal emission. X-Git-Tag: v2.0.48~89^2~20 X-Git-Url: https://main.carlh.net/gitweb/?p=dcpomatic.git;a=commitdiff_plain;h=05c37bfdb86be26497d5baa448a0cbda20e33bed 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 fcc658717..b00ea3e57 100644 --- a/src/lib/content.cc +++ b/src/lib/content.cc @@ -153,9 +153,7 @@ Content::examine (shared_ptr 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 c6cede5fa..2b966110b 100644 --- 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 #include @@ -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, public boost::noncopyable +class Content : public boost::enable_shared_from_this, public Signaller, public boost::noncopyable { public: Content (boost::shared_ptr); diff --git a/src/lib/film.cc b/src/lib/film.cc index 80755a4cb..35773c797 100644 --- 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 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 3cd370a0d..f61062be0 100644 --- 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 #include @@ -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, public boost::noncopyable +class Film : public boost::enable_shared_from_this, 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 eadafbf73..f28146632 100644 --- 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 7c6707880..8fe87747c 100644 --- 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 #include #include @@ -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, public boost::noncopyable +class Job : public boost::enable_shared_from_this, public Signaller, public boost::noncopyable { public: Job (boost::shared_ptr); diff --git a/src/lib/job_manager.cc b/src/lib/job_manager.cc index 2b727b0aa..63db662d0 100644 --- a/src/lib/job_manager.cc +++ b/src/lib/job_manager.cc @@ -64,9 +64,7 @@ JobManager::add (shared_ptr j) _jobs.push_back (j); } - if (ui_signaller) { - ui_signaller->emit (boost::bind (boost::ref (JobAdded), weak_ptr (j))); - } + emit (boost::bind (boost::ref (JobAdded), weak_ptr (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 9d8620cbb..b946c1a98 100644 --- 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 #include #include @@ -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 979046dab..72a9a4ef5 100644 --- 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 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 3fab6864a..dc62f998d 100644 --- a/src/lib/server_finder.h +++ b/src/lib/server_finder.h @@ -18,9 +18,10 @@ */ #include "server.h" +#include "signaller.h" #include -class ServerFinder : public ExceptionStore +class ServerFinder : public Signaller, public ExceptionStore { public: boost::signals2::connection connect (boost::function); diff --git a/src/lib/signaller.h b/src/lib/signaller.h new file mode 100644 index 000000000..408cfcf5b --- /dev/null +++ b/src/lib/signaller.h @@ -0,0 +1,131 @@ +/* + Copyright (C) 2015 Carl Hetherington + + 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 +#include + +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 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::iterator i = _wrappers.begin(); i != _wrappers.end(); ++i) { + (*i)->invalidate (); + } + } + + /* Can be called from any thread */ + template + void emit (T signal) + { + Wrapper* w = new Wrapper (signal); + if (ui_signaller) { + ui_signaller->emit (boost::bind (&Wrapper::signal, w)); + } + + boost::mutex::scoped_lock lm (_mutex); + + /* Clean up finished Wrappers */ + std::list::iterator i = _wrappers.begin (); + while (i != _wrappers.end ()) { + std::list::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 _wrappers; +}; + +#endif diff --git a/src/lib/ui_signaller.h b/src/lib/ui_signaller.h index ee4d230d4..9d4495cd1 100644 --- a/src/lib/ui_signaller.h +++ b/src/lib/ui_signaller.h @@ -24,6 +24,8 @@ #include #include +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 - 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 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 + 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 a05df8ef3..26944ecc3 100644 --- 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 5bb9e9501..461217a37 100644 --- a/src/lib/update.h +++ b/src/lib/update.h @@ -21,6 +21,7 @@ * @brief UpdateChecker class. */ +#include "signaller.h" #include #include #include @@ -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 ();