Implement VCL Scheduler locking

Replces the SolarMutex scheduler locking by using a distinct mutex
included in the scheduler context.

It should also get rid of the spurious assert( !bAniIdle ) failures,
as the test now holds the Scheduler lock for the whole time.

This includes reverting the additional scheduler de-init from commit
2e29a518b04250b5f9cc9d0d77da3df076834d60, as I couldn't reproduce
the bug running the unit test in valgrind.

Change-Id: If33f815fe86c8f82175e96adceb1084b925319dd
Reviewed-on: https://gerrit.libreoffice.org/40497
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Jan-Marek Glogowski <glogow@fbihome.de>

4 files changed, 126 insertions, 24 deletions

diff --git a/vcl/inc/schedulerimpl.hxx b/vcl/inc/schedulerimpl.hxx
index 2c29de6a6b30..df10fc930bda 100644
--- a/vcl/inc/schedulerimpl.hxx
+++ b/vcl/inc/schedulerimpl.hxx
@@ -21,6 +21,8 @@
 #define INCLUDED_VCL_INC_SCHEDULERIMPL_HXX
 
 #include <salwtype.hxx>
+#include <osl/mutex.hxx>
+#include <vcl/scheduler.hxx>
 
 class Task;
 
@@ -35,6 +37,39 @@ struct ImplSchedulerData final
     const char *GetDebugName() const;
 };
 
+class SchedulerMutex final
+{
+    sal_uInt32          mnLockDepth;
+    osl::Mutex          maMutex;
+
+public:
+    SchedulerMutex() : mnLockDepth( 0 ) {}
+
+    bool acquire( sal_uInt32 nLockCount = 1 );
+    sal_uInt32 release( bool bUnlockAll = false );
+    sal_uInt32 lockDepth() const { return mnLockDepth; }
+};
+
+class SchedulerGuard final
+{
+    bool mbLocked;
+
+public:
+    SchedulerGuard()
+        : mbLocked( false )
+    {
+        mbLocked = Scheduler::Lock();
+        assert( mbLocked );
+    }
+
+    ~SchedulerGuard()
+    {
+        if ( !mbLocked )
+            return;
+        Scheduler::Unlock();
+    }
+};
+
 #endif // INCLUDED_VCL_INC_SCHEDULERIMPL_HXX
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/vcl/inc/svdata.hxx b/vcl/inc/svdata.hxx
index 3501d2c25d45..a633798d54ad 100644
--- a/vcl/inc/svdata.hxx
+++ b/vcl/inc/svdata.hxx
@@ -38,6 +38,7 @@
 #include <unordered_map>
 #include <boost/functional/hash.hpp>
 #include "ControlCacheKey.hxx"
+#include "schedulerimpl.hxx"
 
 struct ImplPostEventData;
 struct ImplTimerData;
@@ -46,7 +47,6 @@ struct ImplConfigData;
 class ImplDirectFontSubstitution;
 struct ImplHotKey;
 struct ImplEventHook;
-struct ImplSchedulerData;
 class Point;
 class ImplAccelManager;
 class PhysicalFontCollection;
@@ -324,6 +324,8 @@ struct ImplSchedulerContext
     SalTimer*               mpSalTimer = nullptr;           ///< interface to sal event loop / system timer
     sal_uInt64              mnTimerStart = 0;               ///< start time of the timer
     sal_uInt64              mnTimerPeriod = SAL_MAX_UINT64; ///< current timer period
+    SchedulerMutex          maMutex;                        ///< lock counting mutex for scheduler locking
+    bool                    mbActive = true;                ///< is the scheduler active?
 };
 
 struct ImplSVData
diff --git a/vcl/source/app/scheduler.cxx b/vcl/source/app/scheduler.cxx
index 972beb741c1d..41d7e7124d85 100644
--- a/vcl/source/app/scheduler.cxx
+++ b/vcl/source/app/scheduler.cxx
@@ -84,20 +84,29 @@ inline std::basic_ostream<charT, traits> & operator <<(
 
 void Scheduler::ImplDeInitScheduler()
 {
-    ImplSVData*           pSVData = ImplGetSVData();
+    ImplSVData* pSVData = ImplGetSVData();
     assert( pSVData != nullptr );
     ImplSchedulerContext &rSchedCtx = pSVData->maSchedCtx;
 
+    DBG_TESTSOLARMUTEX();
+
+    SchedulerGuard aSchedulerGuard;
+    rSchedCtx.mbActive = false;
+
+    assert( nullptr == rSchedCtx.mpSchedulerStack );
+    assert( 1 == rSchedCtx.maMutex.lockDepth() );
+
     if (rSchedCtx.mpSalTimer) rSchedCtx.mpSalTimer->Stop();
     DELETEZ( rSchedCtx.mpSalTimer );
 
     ImplSchedulerData* pSchedulerData = rSchedCtx.mpFirstSchedulerData;
     while ( pSchedulerData )
     {
-        if ( pSchedulerData->mpTask )
+        Task *pTask = pSchedulerData->mpTask;
+        if ( pTask )
         {
-            pSchedulerData->mpTask->mbActive = false;
-            pSchedulerData->mpTask->mpSchedulerData = nullptr;
+            pTask->mbActive = false;
+            pTask->mpSchedulerData = nullptr;
         }
         ImplSchedulerData* pDeleteSchedulerData = pSchedulerData;
         pSchedulerData = pSchedulerData->mpNext;
@@ -109,6 +118,55 @@ void Scheduler::ImplDeInitScheduler()
     rSchedCtx.mnTimerPeriod        = InfiniteTimeoutMs;
 }
 
+bool SchedulerMutex::acquire( sal_uInt32 nLockCount )
+{
+    do {
+        if ( !maMutex.acquire() )
+            return false;
+        ++mnLockDepth;
+    }
+    while ( --nLockCount );
+    return true;
+}
+
+sal_uInt32 SchedulerMutex::release( bool bUnlockAll )
+{
+    sal_uInt32 nLockCount = 0;
+    if ( mnLockDepth )
+    {
+        if ( bUnlockAll )
+        {
+            nLockCount = mnLockDepth;
+            do {
+                --mnLockDepth;
+                maMutex.release();
+            }
+            while ( mnLockDepth );
+        }
+        else
+        {
+            nLockCount = 1;
+            --mnLockDepth;
+            maMutex.release();
+        }
+    }
+    return nLockCount;
+}
+
+bool Scheduler::Lock( sal_uInt32 nLockCount )
+{
+    ImplSVData* pSVData = ImplGetSVData();
+    assert( pSVData != nullptr );
+    return pSVData->maSchedCtx.maMutex.acquire( nLockCount );
+}
+
+sal_uInt32 Scheduler::Unlock( bool bUnlockAll )
+{
+    ImplSVData* pSVData = ImplGetSVData();
+    assert( pSVData != nullptr );
+    return pSVData->maSchedCtx.maMutex.release( bUnlockAll );
+}
+
 /**
  * Start a new timer if we need to for nMS duration.
  *
@@ -119,16 +177,12 @@ void Scheduler::ImplDeInitScheduler()
 void Scheduler::ImplStartTimer(sal_uInt64 nMS, bool bForce, sal_uInt64 nTime)
 {
     ImplSVData* pSVData = ImplGetSVData();
-    if (pSVData->mbDeInit)
-    {
-        // do not start new timers during shutdown - if that happens after
-        // ImplSalStopTimer() on WNT the timer queue is restarted and never ends
+    ImplSchedulerContext &rSchedCtx = pSVData->maSchedCtx;
+    if ( !rSchedCtx.mbActive )
         return;
-    }
 
     DBG_TESTSOLARMUTEX();
 
-    ImplSchedulerContext &rSchedCtx = pSVData->maSchedCtx;
     if (!rSchedCtx.mpSalTimer)
     {
         rSchedCtx.mnTimerStart = 0;
@@ -227,10 +281,14 @@ bool Scheduler::ProcessTaskScheduling()
 {
     ImplSVData *pSVData = ImplGetSVData();
     ImplSchedulerContext &rSchedCtx = pSVData->maSchedCtx;
-    sal_uInt64 nTime = tools::Time::GetSystemTicks();
-    if ( pSVData->mbDeInit || InfiniteTimeoutMs == rSchedCtx.mnTimerPeriod )
+
+    DBG_TESTSOLARMUTEX();
+
+    SchedulerGuard aSchedulerGuard;
+    if ( !rSchedCtx.mbActive || InfiniteTimeoutMs == rSchedCtx.mnTimerPeriod )
         return false;
 
+    sal_uInt64 nTime = tools::Time::GetSystemTicks();
     if ( nTime < rSchedCtx.mnTimerStart + rSchedCtx.mnTimerPeriod )
     {
         SAL_WARN( "vcl.schedule", "we're too early - restart the timer!" );
@@ -248,8 +306,6 @@ bool Scheduler::ProcessTaskScheduling()
     sal_uInt64         nMostUrgentPeriod = InfiniteTimeoutMs;
     sal_uInt64         nReadyPeriod = InfiniteTimeoutMs;
 
-    DBG_TESTSOLARMUTEX();
-
     pSchedulerData = rSchedCtx.mpFirstSchedulerData;
     while ( pSchedulerData )
     {
@@ -329,7 +385,9 @@ next_entry:
         // defer pushing the scheduler stack to next run, as most tasks will
         // not run a nested Scheduler loop and don't need a stack push!
         pMostUrgent->mbInScheduler = true;
+        sal_uInt32 nLockCount = Unlock( true );
         pTask->Invoke();
+        Lock( nLockCount );
         pMostUrgent->mbInScheduler = false;
 
         SAL_INFO( "vcl.schedule", tools::Time::GetSystemTicks() << " "
@@ -382,13 +440,11 @@ void Task::SetDeletionFlags()
 void Task::Start()
 {
     ImplSVData *const pSVData = ImplGetSVData();
-    if (pSVData->mbDeInit)
-    {
-        return;
-    }
     ImplSchedulerContext &rSchedCtx = pSVData->maSchedCtx;
 
-    DBG_TESTSOLARMUTEX();
+    SchedulerGuard aSchedulerGuard;
+    if ( !rSchedCtx.mbActive )
+        return;
 
     // Mark timer active
     mbActive = true;
@@ -453,6 +509,7 @@ Task::Task( const Task& rTask )
 
 Task::~Task() COVERITY_NOEXCEPT_FALSE
 {
+    SchedulerGuard aSchedulerGuard;
     if ( mpSchedulerData )
         mpSchedulerData->mpTask = nullptr;
 }
diff --git a/vcl/source/app/svapp.cxx b/vcl/source/app/svapp.cxx
index a9b7df7675c7..5d32d0532eeb 100644
--- a/vcl/source/app/svapp.cxx
+++ b/vcl/source/app/svapp.cxx
@@ -496,6 +496,13 @@ void Scheduler::ProcessEventsToSignal(bool& bSignal)
 void Scheduler::ProcessEventsToIdle()
 {
     int nSanity = 1;
+#if OSL_DEBUG_LEVEL > 0
+    const ImplSVData* pSVData = ImplGetSVData();
+    bool bIsMainThread = pSVData->mpDefInst->IsMainThread();
+    bool mbLocked = false;
+    if ( bIsMainThread )
+        mbLocked = Scheduler::Lock();
+#endif
     while( Application::Reschedule( true ) )
     {
         if (0 == ++nSanity % 1000)
@@ -507,10 +514,9 @@ void Scheduler::ProcessEventsToIdle()
     // If we yield from a non-main thread we just can guarantee that all idle
     // events were processed at some point, but our check can't prevent further
     // processing in the main thread, which may add new events, so skip it.
-    const ImplSVData* pSVData = ImplGetSVData();
-    if ( !pSVData->mpDefInst->IsMainThread() )
+    if ( !bIsMainThread )
         return;
-    const ImplSchedulerData* pSchedulerData = ImplGetSVData()->maSchedCtx.mpFirstSchedulerData;
+    const ImplSchedulerData* pSchedulerData = pSVData->maSchedCtx.mpFirstSchedulerData;
     bool bAnyIdle = false;
     while ( pSchedulerData )
     {
@@ -520,12 +526,14 @@ void Scheduler::ProcessEventsToIdle()
             if ( pIdle && pIdle->IsActive() )
             {
                 bAnyIdle = true;
-                SAL_WARN( "vcl.schedule",  "Unprocessed Idle: " << pIdle->GetDebugName() );
+                SAL_WARN( "vcl.schedule", "Unprocessed Idle: " << pIdle->GetDebugName() );
             }
         }
         pSchedulerData = pSchedulerData->mpNext;
     }
     assert( !bAnyIdle );
+    if ( mbLocked )
+        Scheduler::Unlock();
 #endif
 }