Make SchedulerMutex non-recursive

While thinking about the "Unlock scheduler in deinit for
ProcessEventsToIdle" patch, I came to the conclusion, that this
mutex should actually be non-recursive. I had a look at the code
and did run "make check" for my symbol build on Linux, but for
the rest I'm reying on the LO CI.

Maybe this can be converted to a std::mutex later. I've updated
the vcl/README.scheduler and added a TODO.

Change-Id: Ib9cb086af74b51e48f99ebfa1201d14db12b140e
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/107254
Tested-by: Jenkins
Reviewed-by: Jan-Marek Glogowski <glogow@fbihome.de>

4 files changed, 41 insertions, 35 deletions

diff --git a/include/vcl/scheduler.hxx b/include/vcl/scheduler.hxx
index 9318b2109641..7219146cbf39 100644
--- a/include/vcl/scheduler.hxx
+++ b/include/vcl/scheduler.hxx
@@ -36,8 +36,8 @@ class VCL_DLLPUBLIC Scheduler final
 
     static void ImplStartTimer ( sal_uInt64 nMS, bool bForce, sal_uInt64 nTime );
 
-    static void Lock( sal_uInt32 nLockCount = 1 );
-    static sal_uInt32 Unlock( bool bUnlockAll = false );
+    static void Lock();
+    static void Unlock();
 
 public:
     static constexpr sal_uInt64 ImmediateTimeoutMs = 0;
diff --git a/vcl/README.scheduler b/vcl/README.scheduler
index 718509033e79..466485f16645 100644
--- a/vcl/README.scheduler
+++ b/vcl/README.scheduler
@@ -69,6 +69,10 @@ Scheduler, while the SchedulerLock would be long gone. OTOH this makes
 Task handling less error-prone, than doing "special" manual cleanup.
 There is also a "= TODOs and ideas =" to get rid if static Tasks.
 
+Actually the SchedulerMutex should never be locked when calling into
+non-scheduler code, so it was converted to simulate a non-recursive
+mutex later. As an alternative it could use a std::mutex.
+
 
 = Lifecycle / thread-safety of Scheduler-based objects =
 
@@ -403,3 +407,9 @@ will also affect the Gtk+ and KDE backend for the user event handling.
 
 This way it can be easier used to profile Tasks, eventually to improve LO's
 interactivity.
+
+== Convert SchedulerMutex to std::mutex ==
+
+Since the SchedulerMutex is non-recursive, it could use a std::mutex instead
+of simulating one still using the osl::Mutex. Not sure, if a deadlock instead
+of a crash, when called recursively, is the better "user experience"...
diff --git a/vcl/inc/schedulerimpl.hxx b/vcl/inc/schedulerimpl.hxx
index ffb08084252c..26a9c47de11f 100644
--- a/vcl/inc/schedulerimpl.hxx
+++ b/vcl/inc/schedulerimpl.hxx
@@ -40,15 +40,18 @@ struct ImplSchedulerData final
 
 class SchedulerMutex final
 {
-    sal_uInt32          mnLockDepth;
-    osl::Mutex          maMutex;
+    // this simulates a non-recursive mutex
+    bool m_bIsLocked;
+    osl::Mutex m_aMutex;
 
 public:
-    SchedulerMutex() : mnLockDepth( 0 ) {}
+    SchedulerMutex()
+        : m_bIsLocked(false)
+    {
+    }
 
-    void acquire( sal_uInt32 nLockCount = 1 );
-    sal_uInt32 release( bool bUnlockAll = false );
-    sal_uInt32 lockDepth() const { return mnLockDepth; }
+    void acquire();
+    void release();
 };
 
 class SchedulerGuard final
diff --git a/vcl/source/app/scheduler.cxx b/vcl/source/app/scheduler.cxx
index 9ae60f3f344d..1759178e2e2f 100644
--- a/vcl/source/app/scheduler.cxx
+++ b/vcl/source/app/scheduler.cxx
@@ -124,15 +124,13 @@ void Scheduler::ImplDeInitScheduler()
               "DeInit the scheduler - pending tasks: " << nTasks );
 
     // clean up all the sfx::SfxItemDisruptor_Impl Idles
-    sal_uInt32 nLockCount = Unlock(true);
-    assert(1 == nLockCount);
+    Unlock();
     ProcessEventsToIdle();
-    Lock(nLockCount);
+    Lock();
 #endif
     rSchedCtx.mbActive = false;
 
     assert( nullptr == rSchedCtx.mpSchedulerStack );
-    assert( 1 == rSchedCtx.maMutex.lockDepth() );
 
     if (rSchedCtx.mpSalTimer) rSchedCtx.mpSalTimer->Stop();
     delete rSchedCtx.mpSalTimer;
@@ -209,41 +207,36 @@ next_priority:
     rSchedCtx.mnTimerPeriod        = InfiniteTimeoutMs;
 }
 
-void SchedulerMutex::acquire( sal_uInt32 nLockCount )
+void SchedulerMutex::acquire()
 {
-    assert(nLockCount > 0);
-    for (sal_uInt32 i = 0; i != nLockCount; ++i) {
-        if (!maMutex.acquire())
-            abort();
-    }
-    mnLockDepth += nLockCount;
+    if (!m_aMutex.acquire())
+        std::abort();
+    if (m_bIsLocked)
+        std::abort();
+    m_bIsLocked = true;
 }
 
-sal_uInt32 SchedulerMutex::release( bool bUnlockAll )
+void SchedulerMutex::release()
 {
-    assert(mnLockDepth > 0);
-    const sal_uInt32 nLockCount =
-        (bUnlockAll || 0 == mnLockDepth) ? mnLockDepth : 1;
-    mnLockDepth -= nLockCount;
-    for (sal_uInt32 i = 0; i != nLockCount; ++i) {
-        if (!maMutex.release())
-            abort();
-    }
-    return nLockCount;
+    if (!m_bIsLocked)
+        std::abort();
+    m_bIsLocked = false;
+    if (!m_aMutex.release())
+        std::abort();
 }
 
-void Scheduler::Lock( sal_uInt32 nLockCount )
+void Scheduler::Lock()
 {
     ImplSVData* pSVData = ImplGetSVData();
     assert( pSVData != nullptr );
-    pSVData->maSchedCtx.maMutex.acquire( nLockCount );
+    pSVData->maSchedCtx.maMutex.acquire();
 }
 
-sal_uInt32 Scheduler::Unlock( bool bUnlockAll )
+void Scheduler::Unlock()
 {
     ImplSVData* pSVData = ImplGetSVData();
     assert( pSVData != nullptr );
-    return pSVData->maSchedCtx.maMutex.release( bUnlockAll );
+    pSVData->maSchedCtx.maMutex.release();
 }
 
 /**
@@ -476,7 +469,7 @@ bool Scheduler::ProcessTaskScheduling()
         rSchedCtx.mpSchedulerStackTop = pMostUrgent;
 
         // invoke the task
-        sal_uInt32 nLockCount = Unlock( true );
+        Unlock();
         /*
         * Current policy is that scheduler tasks aren't allowed to throw an exception.
         * Because otherwise the exception is caught somewhere totally unrelated.
@@ -503,7 +496,7 @@ bool Scheduler::ProcessTaskScheduling()
             SAL_WARN("vcl.schedule", "Uncaught exception during Task::Invoke()!");
             std::abort();
         }
-        Lock( nLockCount );
+        Lock();
         pMostUrgent->mbInScheduler = false;
 
         SAL_INFO( "vcl.schedule", tools::Time::GetSystemTicks() << " "