From f5afdd91a5190250ed0916b606652c07a70b2c73 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Mon, 10 May 2021 23:42:40 +0200 Subject: [PATCH 1/4] Preventing event recursion on different levels. --- src/lay/lay/layApplication.cc | 5 +++++ src/lay/lay/layMainWindow.h | 8 ++++++++ src/tl/tl/tlLog.cc | 15 ++++++++++++--- src/tl/tl/tlLog.h | 1 + 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/lay/lay/layApplication.cc b/src/lay/lay/layApplication.cc index 76a9000cd..31998ed02 100644 --- a/src/lay/lay/layApplication.cc +++ b/src/lay/lay/layApplication.cc @@ -1486,6 +1486,11 @@ GuiApplication::process_events_impl (QEventLoop::ProcessEventsFlags flags, bool { if (mp_mw) { + // prevent recursive process_events + if (mp_mw->is_busy ()) { + return; + } + if (silent && tl::DeferredMethodScheduler::instance ()) { tl::DeferredMethodScheduler::instance ()->enable (false); } diff --git a/src/lay/lay/layMainWindow.h b/src/lay/lay/layMainWindow.h index b5e37b283..20397b9f0 100644 --- a/src/lay/lay/layMainWindow.h +++ b/src/lay/lay/layMainWindow.h @@ -408,6 +408,14 @@ public: m_busy = bm; } + /** + * @brief Returns true if the application is busy + */ + bool is_busy () const + { + return m_busy; + } + /** * @brief Enable synchronous mode or disable it * diff --git a/src/tl/tl/tlLog.cc b/src/tl/tl/tlLog.cc index bfb5aafc9..1cebcb011 100644 --- a/src/tl/tl/tlLog.cc +++ b/src/tl/tl/tlLog.cc @@ -67,7 +67,7 @@ verbosity () // Channel implementation Channel::Channel () - : m_no_endl (false), m_active (false) + : m_no_endl (false), m_active (false), m_in_yield (false) { // .. nothing else .. } @@ -98,10 +98,19 @@ Channel::release_proxy () end (); m_active = false; m_no_endl = false; + bool in_yield = m_in_yield; + m_in_yield = true; m_lock.unlock (); - // after we have released the lock we give the receivers an opportunity to process events - yield (); + // after we have released the lock we give the receivers an opportunity to process events. Note that we allow only one thread to yield + // at the same time and no recursive yields. + if (! in_yield) { + yield (); + // this should be atomic, but execution reordering may place it before yield(). Hence the lock. + m_lock.lock (); + m_in_yield = false; + m_lock.unlock (); + } } ChannelEndl endl; diff --git a/src/tl/tl/tlLog.h b/src/tl/tl/tlLog.h index b085a14b7..775222c29 100644 --- a/src/tl/tl/tlLog.h +++ b/src/tl/tl/tlLog.h @@ -154,6 +154,7 @@ private: bool m_no_endl; bool m_active; + bool m_in_yield; }; /** From e7ae7338b4cf0d4a107850467f780f10e5f1d571 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Wed, 12 May 2021 22:46:54 +0200 Subject: [PATCH 2/4] Fixed a potential invalid read access problem. --- src/laybasic/laybasic/layRedrawThreadCanvas.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/laybasic/laybasic/layRedrawThreadCanvas.cc b/src/laybasic/laybasic/layRedrawThreadCanvas.cc index bb75bdb36..d1b422098 100644 --- a/src/laybasic/laybasic/layRedrawThreadCanvas.cc +++ b/src/laybasic/laybasic/layRedrawThreadCanvas.cc @@ -396,6 +396,13 @@ BitmapRedrawThreadCanvas::initialize_plane (lay::CanvasPlane *plane, unsigned in void BitmapRedrawThreadCanvas::to_image (const std::vector &view_ops, const lay::DitherPattern &dp, const lay::LineStyles &ls, QColor background, QColor foreground, QColor active, const lay::Drawings *drawings, QImage &img, unsigned int width, unsigned int height) { + if (width > m_width) { + width = m_width; + } + if (height > m_height) { + height = m_height; + } + // convert the plane data to image data bitmaps_to_image (view_ops, mp_plane_buffers, dp, ls, &img, width, height, true, &mutex ()); From 453ff7c7ad091ecb742fb3928dd98cc7c3169c5a Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Wed, 12 May 2021 22:47:18 +0200 Subject: [PATCH 3/4] Removed process_events from log output as this is the main reason of instability. --- src/lay/lay/layApplication.cc | 15 +++++++-------- src/lay/lay/layLogViewerDialog.cc | 10 ++++++++-- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/lay/lay/layApplication.cc b/src/lay/lay/layApplication.cc index 31998ed02..659f7124e 100644 --- a/src/lay/lay/layApplication.cc +++ b/src/lay/lay/layApplication.cc @@ -1491,16 +1491,15 @@ GuiApplication::process_events_impl (QEventLoop::ProcessEventsFlags flags, bool return; } - if (silent && tl::DeferredMethodScheduler::instance ()) { - tl::DeferredMethodScheduler::instance ()->enable (false); + if (silent) { + tl::DeferredMethodScheduler::enable (false); } -#if QT_VERSION < 0x050000 - QApplication::syncX (); -#endif - mp_mw->enter_busy_mode (true); try { +#if QT_VERSION < 0x050000 + QApplication::syncX (); +#endif QApplication::processEvents (flags); // Qt seems not to send posted UserEvents in some cases (e.g. in the unit test application with GLib? // Glib not doing this without a main window visible?). Hence we do this explicitly here. @@ -1510,8 +1509,8 @@ GuiApplication::process_events_impl (QEventLoop::ProcessEventsFlags flags, bool } mp_mw->enter_busy_mode (false); - if (silent && tl::DeferredMethodScheduler::instance ()) { - tl::DeferredMethodScheduler::instance ()->enable (true); + if (silent) { + tl::DeferredMethodScheduler::enable (true); } } diff --git a/src/lay/lay/layLogViewerDialog.cc b/src/lay/lay/layLogViewerDialog.cc index 27b8ce6db..fbc175204 100644 --- a/src/lay/lay/layLogViewerDialog.cc +++ b/src/lay/lay/layLogViewerDialog.cc @@ -254,6 +254,12 @@ LogFile::add (LogFileEntry::mode_type mode, const std::string &msg, bool continu void LogFile::yield () { +#if 0 + // This looked like a good idea, but in fact it introduces a hell lot of instability + // as it potentially leads to a recursion of events inside innocent functions. Remember + // that log output may be generated from every function called in response of an event + // and not every such function may process further events + bool can_yield = false; { @@ -268,10 +274,10 @@ LogFile::yield () // use this opportunity to process events // NOTE: as process events may trigger further log output, it's necessary to do process events outside any other // method (e.g. add) which is subject to locking. Hence we avoid deadlocks. - // We accept the risk of recursion inside process_events as we have a timeout before accepting new yield calls. if (can_yield) { - lay::ApplicationBase::instance ()->process_events (QEventLoop::AllEvents); + lay::ApplicationBase::instance ()->process_events (QEventLoop::ExcludeUserInputEvents | QEventLoop::ExcludeSocketNotifiers, true /*silent*/); } +#endif } int From f7631b2b2dfea8bd42569d4751eb29d5c50229f3 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Wed, 12 May 2021 23:42:14 +0200 Subject: [PATCH 4/4] Fixing a problem with log output in log window --- src/lay/lay/layLogViewerDialog.cc | 13 ++++++------- src/lay/lay/layLogViewerDialog.h | 1 - 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/lay/lay/layLogViewerDialog.cc b/src/lay/lay/layLogViewerDialog.cc index fbc175204..c5e3abe33 100644 --- a/src/lay/lay/layLogViewerDialog.cc +++ b/src/lay/lay/layLogViewerDialog.cc @@ -41,7 +41,7 @@ namespace lay // LogReceiver implementation LogReceiver::LogReceiver (LogFile *file, int verbosity, void (LogFile::*method)(const std::string &, bool)) - : mp_file (file), m_method (method), m_continued (false), m_verbosity (verbosity) + : mp_file (file), m_method (method), m_verbosity (verbosity) { // .. nothing yet .. } @@ -64,7 +64,9 @@ LogReceiver::puts (const char *s) } if (*s == '\n') { - endl (); + QMutexLocker locker (&m_lock); + (mp_file->*m_method) (m_text, true); + m_text.clear (); ++s; } @@ -78,9 +80,8 @@ LogReceiver::endl () { if (tl::verbosity () >= m_verbosity) { QMutexLocker locker (&m_lock); - (mp_file->*m_method) (m_text, m_continued); + (mp_file->*m_method) (m_text, false); m_text.clear (); - m_continued = true; } } @@ -99,9 +100,7 @@ LogReceiver::end () void LogReceiver::begin () { - QMutexLocker locker (&m_lock); - m_continued = false; - m_text.clear (); + // .. nothing yet .. } // ----------------------------------------------------------------- diff --git a/src/lay/lay/layLogViewerDialog.h b/src/lay/lay/layLogViewerDialog.h index 4f6725fe8..a449c7c08 100644 --- a/src/lay/lay/layLogViewerDialog.h +++ b/src/lay/lay/layLogViewerDialog.h @@ -96,7 +96,6 @@ private: LogFile *mp_file; void (LogFile::*m_method)(const std::string &, bool); std::string m_text; - bool m_continued; int m_verbosity; QMutex m_lock; };