tdf#119856 vcl: Qt5/KDE5 RunInMainThread

The problem with the current approach of transferring calls to the main
thread with Q_EMIT signals is that if the code that should run in the
main thread needs SolarMutex, then the non-main-thread must use
SolarMutexReleaser - but then the main thread will run not only the call
that is needed right now, but will potentially process all pending
events, and the other thread hasn't prepared for that.

We need the inter-thread feature of Qt::BlockingQueuedConnection and the
non-queued feature of Qt::DirectConnection, but this combination doesn't
appear to exist.

So the SolarMutexReleaser needs to go - but then the main thread does
need SolarMutex for some things, and hence we need to trick it into
believing it has SolarMutex with the m_bNoYieldLock hack.

Then it becomes apparent that the main thread may be blocked on either
Qt events, which is fine, or on the SalYieldMutex's m_aMutex, which will
never be released now.

So the main thread must never block on m_aMutex; the alternative is to
use the same approach as the osx code (and, in a somewhat different
form, the svp code), and add some condition variables on which the main
thread can block if it fails to acquire the m_aMutex immediately.

It's even possible to do this in a somewhat generic way with lambdas.

This does appear to work, but it makes the Q_EMIT approach entirely
untenable, because now the main thread will be blocked on the condition
variable and the non-main-thread will be blocked until the Qt event is
processed.

Change-Id: I6480a6b909d5ec8814b2ff10dbefb0f3686a83c7
Reviewed-on: https://gerrit.libreoffice.org/68232
Tested-by: Jenkins
Reviewed-by: Katarina Behrens <Katarina.Behrens@cib.de>

4 files changed, 181 insertions, 32 deletions

diff --git a/vcl/inc/qt5/Qt5Instance.hxx b/vcl/inc/qt5/Qt5Instance.hxx
index 91682bd87950..38ead9ed9036 100644
--- a/vcl/inc/qt5/Qt5Instance.hxx
+++ b/vcl/inc/qt5/Qt5Instance.hxx
@@ -27,6 +27,8 @@
 
 #include <QtCore/QObject>
 
+#include <functional>
+
 class QApplication;
 class SalYieldMutex;
 class SalFrame;
@@ -50,15 +52,18 @@ public:
 
 private Q_SLOTS:
     bool ImplYield(bool bWait, bool bHandleAllCurrentEvents);
+    void ImplRunInMain();
 
 Q_SIGNALS:
     bool ImplYieldSignal(bool bWait, bool bHandleAllCurrentEvents);
-    std::unique_ptr<SalMenu> createMenuSignal(bool, Menu*);
+    void ImplRunInMainSignal();
 
 public:
     explicit Qt5Instance(bool bUseCairo = false);
     virtual ~Qt5Instance() override;
 
+    void RunInMainThread(std::function<void()> func);
+
     virtual SalFrame* CreateFrame(SalFrame* pParent, SalFrameStyleFlags nStyle) override;
     virtual SalFrame* CreateChildFrame(SystemParentData* pParent,
                                        SalFrameStyleFlags nStyle) override;
diff --git a/vcl/qt5/Qt5Instance.cxx b/vcl/qt5/Qt5Instance.cxx
index 2a190e4a0437..4527aaae5ebc 100644
--- a/vcl/qt5/Qt5Instance.cxx
+++ b/vcl/qt5/Qt5Instance.cxx
@@ -43,13 +43,165 @@
 #include <QtWidgets/QWidget>
 
 #include <vclpluginapi.h>
+#include <tools/debug.hxx>
+#include <comphelper/flagguard.hxx>
 #include <sal/log.hxx>
 #include <osl/process.h>
 
 #include <headless/svpbmp.hxx>
 
+#include <mutex>
+#include <condition_variable>
+
+/// TODO: not much Qt5 specific here? could be generalised, esp. for OSX...
+/// this subclass allows for the transfer of a closure for running on the main
+/// thread, to handle all the thread affine stuff in Qt5; the SolarMutex is
+/// "loaned" to the main thread for the execution of the closure.
+/// @note it doesn't work to just use "emit" and signals/slots to move calls to
+/// the main thread, because the other thread has the SolarMutex; the other
+/// thread (typically) cannot release SolarMutex, because then the main thread
+/// will handle all sorts of events and whatnot; this design ensures that the
+/// main thread only runs the passed closure (unless the closure releases
+/// SolarMutex itself, which should probably be avoided).
+class Qt5YieldMutex : public SalYieldMutex
+{
+public:
+    /// flag only accessed on main thread:
+    /// main thread has "borrowed" SolarMutex from another thread
+    bool m_bNoYieldLock = false;
+    /// members for communication from non-main thread to main thread
+    std::mutex m_RunInMainMutex;
+    std::condition_variable m_InMainCondition;
+    bool m_isWakeUpMain = false;
+    std::function<void()> m_Closure; ///< code for main thread to run
+    /// members for communication from main thread to non-main thread
+    std::condition_variable m_ResultCondition;
+    bool m_isResultReady = false;
+
+    virtual bool IsCurrentThread() const override;
+    virtual void doAcquire(sal_uInt32 nLockCount) override;
+    virtual sal_uInt32 doRelease(bool const bUnlockAll) override;
+};
+
+bool Qt5YieldMutex::IsCurrentThread() const
+{
+    auto const* pSalInst(static_cast<Qt5Instance const*>(GetSalData()->m_pInstance));
+    assert(pSalInst);
+    if (pSalInst->IsMainThread() && m_bNoYieldLock)
+    {
+        return true; // main thread has borrowed SolarMutex
+    }
+    return SalYieldMutex::IsCurrentThread();
+}
+
+void Qt5YieldMutex::doAcquire(sal_uInt32 nLockCount)
+{
+    auto const* pSalInst(static_cast<Qt5Instance const*>(GetSalData()->m_pInstance));
+    assert(pSalInst);
+    if (!pSalInst->IsMainThread())
+    {
+        SalYieldMutex::doAcquire(nLockCount);
+        return;
+    }
+    if (m_bNoYieldLock)
+    {
+        return; // special case for main thread: borrowed from other thread
+    }
+    do // main thread acquire...
+    {
+        std::function<void()> func; // copy of closure on thread stack
+        {
+            std::unique_lock<std::mutex> g(m_RunInMainMutex);
+            if (m_aMutex.tryToAcquire())
+            {
+                // if there's a closure, the other thread holds m_aMutex
+                assert(!m_Closure);
+                m_isWakeUpMain = false;
+                --nLockCount; // have acquired once!
+                ++m_nCount;
+                break;
+            }
+            m_InMainCondition.wait(g, [this]() { return m_isWakeUpMain; });
+            m_isWakeUpMain = false;
+            std::swap(func, m_Closure);
+        }
+        if (func)
+        {
+            assert(!m_bNoYieldLock);
+            m_bNoYieldLock = true; // execute closure with borrowed SolarMutex
+            func();
+            m_bNoYieldLock = false;
+            std::unique_lock<std::mutex> g(m_RunInMainMutex);
+            assert(!m_isResultReady);
+            m_isResultReady = true;
+            m_ResultCondition.notify_all(); // unblock other thread
+        }
+    } while (true);
+    SalYieldMutex::doAcquire(nLockCount);
+}
+
+sal_uInt32 Qt5YieldMutex::doRelease(bool const bUnlockAll)
+{
+    auto const* pSalInst(static_cast<Qt5Instance const*>(GetSalData()->m_pInstance));
+    assert(pSalInst);
+    if (pSalInst->IsMainThread() && m_bNoYieldLock)
+    {
+        return 1; // dummy value
+    }
+
+    std::unique_lock<std::mutex> g(m_RunInMainMutex);
+    // read m_nCount before doRelease (it's guarded by m_aMutex)
+    bool const isReleased(bUnlockAll || m_nCount == 1);
+    sal_uInt32 nCount = SalYieldMutex::doRelease(bUnlockAll);
+    if (isReleased && !pSalInst->IsMainThread())
+    {
+        m_isWakeUpMain = true;
+        m_InMainCondition.notify_all(); // unblock main thread
+    }
+    return nCount;
+}
+
+// this could be abstracted to be independent of Qt5 by passing in the
+// event-trigger as another function parameter...
+// it could also be a template of the return type, then it could return the
+// result of func... but then how to handle the result in doAcquire?
+void Qt5Instance::RunInMainThread(std::function<void()> func)
+{
+    DBG_TESTSOLARMUTEX();
+    if (IsMainThread())
+    {
+        func();
+        return;
+    }
+
+    Qt5YieldMutex* const pMutex(static_cast<Qt5YieldMutex*>(GetYieldMutex()));
+    {
+        std::unique_lock<std::mutex> g(pMutex->m_RunInMainMutex);
+        assert(!pMutex->m_Closure);
+        pMutex->m_Closure = func;
+        // unblock main thread in case it is blocked on condition
+        pMutex->m_isWakeUpMain = true;
+        pMutex->m_InMainCondition.notify_all();
+    }
+    // wake up main thread in case it is blocked on event queue
+    // TriggerUserEventProcessing() appears to be insufficient in case the
+    // main thread does QEventLoop::WaitForMoreEvents
+    Q_EMIT ImplRunInMainSignal();
+    {
+        std::unique_lock<std::mutex> g(pMutex->m_RunInMainMutex);
+        pMutex->m_ResultCondition.wait(g, [pMutex]() { return pMutex->m_isResultReady; });
+        pMutex->m_isResultReady = false;
+    }
+}
+
+void Qt5Instance::ImplRunInMain()
+{
+    SolarMutexGuard g; // trigger the dispatch code in Qt5YieldMutex::doAcquire
+    (void)this; // suppress unhelpful [loplugin:staticmethods]; can't be static
+}
+
 Qt5Instance::Qt5Instance(bool bUseCairo)
-    : SalGenericInstance(std::make_unique<SalYieldMutex>())
+    : SalGenericInstance(std::make_unique<Qt5YieldMutex>())
     , m_postUserEventId(-1)
     , m_bUseCairo(bUseCairo)
 {
@@ -65,8 +217,8 @@ Qt5Instance::Qt5Instance(bool bUseCairo)
     // is processed before the thread emitting the signal continues
     connect(this, SIGNAL(ImplYieldSignal(bool, bool)), this, SLOT(ImplYield(bool, bool)),
             Qt::BlockingQueuedConnection);
-    connect(this, &Qt5Instance::createMenuSignal, this, &Qt5Instance::CreateMenu,
-            Qt::BlockingQueuedConnection);
+    connect(this, &Qt5Instance::ImplRunInMainSignal, this, &Qt5Instance::ImplRunInMain,
+            Qt::QueuedConnection); // no Blocking!
 }
 
 Qt5Instance::~Qt5Instance()
@@ -136,15 +288,14 @@ Qt5Instance::CreateVirtualDevice(SalGraphics* pGraphics, long& nDX, long& nDY, D
 
 std::unique_ptr<SalMenu> Qt5Instance::CreateMenu(bool bMenuBar, Menu* pVCLMenu)
 {
-    if (qApp->thread() != QThread::currentThread())
-    {
-        SolarMutexReleaser aReleaser;
-        return Q_EMIT createMenuSignal(bMenuBar, pVCLMenu);
-    }
-
-    Qt5Menu* pSalMenu = new Qt5Menu(bMenuBar);
-    pSalMenu->SetMenu(pVCLMenu);
-    return std::unique_ptr<SalMenu>(pSalMenu);
+    std::unique_ptr<SalMenu> pRet;
+    RunInMainThread([&pRet, bMenuBar, pVCLMenu]() {
+        Qt5Menu* pSalMenu = new Qt5Menu(bMenuBar);
+        pRet.reset(pSalMenu);
+        pSalMenu->SetMenu(pVCLMenu);
+    });
+    assert(pRet);
+    return pRet;
 }
 
 std::unique_ptr<SalMenuItem> Qt5Instance::CreateMenuItem(const SalItemParams& rItemData)
diff --git a/vcl/unx/kde5/KDE5SalInstance.cxx b/vcl/unx/kde5/KDE5SalInstance.cxx
index 6c5bc9341705..6350d10c2187 100644
--- a/vcl/unx/kde5/KDE5SalInstance.cxx
+++ b/vcl/unx/kde5/KDE5SalInstance.cxx
@@ -44,21 +44,16 @@ KDE5SalInstance::KDE5SalInstance()
     pSVData->maAppData.mxToolkitName = OUString("kde5");
 
     KDE5SalData::initNWF();
-
-    connect(this, &KDE5SalInstance::createFrameSignal, this, &KDE5SalInstance::CreateFrame,
-            Qt::BlockingQueuedConnection);
-    connect(this, &KDE5SalInstance::createFilePickerSignal, this,
-            &KDE5SalInstance::createFilePicker, Qt::BlockingQueuedConnection);
 }
 
 SalFrame* KDE5SalInstance::CreateFrame(SalFrame* pParent, SalFrameStyleFlags nState)
 {
-    if (!IsMainThread())
-    {
-        SolarMutexReleaser aReleaser;
-        return Q_EMIT createFrameSignal(pParent, nState);
-    }
-    return new KDE5SalFrame(static_cast<KDE5SalFrame*>(pParent), nState, true);
+    SalFrame* pRet(nullptr);
+    RunInMainThread(std::function([&pRet, pParent, nState]() {
+        pRet = new KDE5SalFrame(static_cast<KDE5SalFrame*>(pParent), nState, true);
+    }));
+    assert(pRet);
+    return pRet;
 }
 
 uno::Reference<ui::dialogs::XFilePicker2>
@@ -66,7 +61,11 @@ KDE5SalInstance::createFilePicker(const uno::Reference<uno::XComponentContext>&
 {
     if (!IsMainThread())
     {
-        return Q_EMIT createFilePickerSignal(xMSF);
+        uno::Reference<ui::dialogs::XFilePicker2> xRet;
+        RunInMainThread(
+            std::function([&xRet, this, xMSF]() { xRet = this->createFilePicker(xMSF); }));
+        assert(xRet);
+        return xRet;
     }
 
     // In order to insert custom controls, KDE5FilePicker currently relies on KFileWidget
diff --git a/vcl/unx/kde5/KDE5SalInstance.hxx b/vcl/unx/kde5/KDE5SalInstance.hxx
index 3cca255f9b2c..5980ea4699cc 100644
--- a/vcl/unx/kde5/KDE5SalInstance.hxx
+++ b/vcl/unx/kde5/KDE5SalInstance.hxx
@@ -42,13 +42,7 @@ public:
 
     virtual bool IsMainThread() const override;
 
-Q_SIGNALS:
-    SalFrame* createFrameSignal(SalFrame* pParent, SalFrameStyleFlags nStyle);
-
-    css::uno::Reference<css::ui::dialogs::XFilePicker2>
-    createFilePickerSignal(const css::uno::Reference<css::uno::XComponentContext>&);
-
-private Q_SLOTS:
+private:
     virtual SalFrame* CreateFrame(SalFrame* pParent, SalFrameStyleFlags nStyle) override;
 
     virtual css::uno::Reference<css::ui::dialogs::XFilePicker2>