Fix salhelper::Timer

Using it was prone to cause deadlocks on shutdown, when the main thread during
exit destroys the static salhelper::TimerManager instance, which blocks
destroying its m_notEmpty member because the salhelper::TimerManager::run thread
is blocking at

>         m_notEmpty.wait(pDelay);

Change-Id: If72700cb622e945f5f314a00ded57538961ab8d7
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/160788
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <stephan.bergmann@allotropia.de>

salhelper/CppunitTest_salhelper_testapi.mk

@@ -17,6 +17,7 @@ $(eval $(call gb_CppunitTest_CppunitTest,salhelper_testapi))
 
 $(eval $(call gb_CppunitTest_add_exception_objects,salhelper_testapi,\
     salhelper/qa/test_api \
+    salhelper/qa/timer \
 ))
 
 $(eval $(call gb_CppunitTest_use_external,salhelper_testapi,boost_headers))

salhelper/qa/timer.cxx

@@ -0,0 +1,67 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include <sal/config.h>
+
+#include <condition_variable>
+#include <mutex>
+
+#include <cppunit/TestAssert.h>
+#include <cppunit/TestFixture.h>
+#include <cppunit/extensions/HelperMacros.h>
+#include <rtl/ref.hxx>
+#include <salhelper/timer.hxx>
+
+namespace
+{
+class TestTimer : public salhelper::Timer
+{
+public:
+    TestTimer()
+        : Timer(salhelper::TTimeValue(0, 1))
+    {
+    }
+
+    void SAL_CALL onShot() override
+    {
+        {
+            std::scoped_lock l(mutex);
+            done = true;
+        }
+        cond.notify_all();
+    }
+
+    std::mutex mutex;
+    std::condition_variable cond;
+    bool done = false;
+};
+
+class TimerTest : public CppUnit::TestFixture
+{
+public:
+    void test()
+    {
+        rtl::Reference<TestTimer> t(new TestTimer);
+        t->start();
+        {
+            std::unique_lock l(t->mutex);
+            t->cond.wait(l, [t] { return t->done; });
+        }
+        CPPUNIT_ASSERT(t->isExpired());
+    }
+
+    CPPUNIT_TEST_SUITE(TimerTest);
+    CPPUNIT_TEST(test);
+    CPPUNIT_TEST_SUITE_END();
+};
+
+CPPUNIT_TEST_SUITE_REGISTRATION(TimerTest);
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */

salhelper/source/timer.cxx

@@ -19,17 +19,19 @@
 #include <salhelper/timer.hxx>
 
 #include <osl/thread.hxx>
-#include <osl/conditn.hxx>
 
+#include <condition_variable>
 #include <mutex>
 
 using namespace salhelper;
 
-class salhelper::TimerManager : public osl::Thread
+class salhelper::TimerManager final : public osl::Thread
 {
 public:
     TimerManager();
 
+    ~TimerManager();
+
     /// register timer
     void registerTimer(salhelper::Timer* pTimer);
 
@@ -48,10 +50,11 @@ protected:
 
     /// sorted-queue data
     salhelper::Timer*       m_pHead;
+    bool m_terminate;
     /// List Protection
     std::mutex                  m_Lock;
     /// Signal the insertion of a timer
-    osl::Condition              m_notEmpty;
+    std::condition_variable     m_notEmpty;
 
     /// "Singleton Pattern"
     //static salhelper::TimerManager* m_pManager;
@@ -203,46 +206,60 @@ TTimeValue Timer::getRemainingTime() const
 **/
 
 TimerManager::TimerManager() :
-    m_pHead(nullptr)
+    m_pHead(nullptr), m_terminate(false)
 {
-    m_notEmpty.reset();
-
     // start thread
     create();
 }
 
+TimerManager::~TimerManager() {
+    {
+        std::scoped_lock g(m_Lock);
+        m_terminate = true;
+    }
+    m_notEmpty.notify_all();
+    join();
+}
+
 void TimerManager::registerTimer(Timer* pTimer)
 {
     if (!pTimer)
         return;
 
-    std::lock_guard Guard(m_Lock);
-
-    // try to find one with equal or lower remaining time.
-    Timer** ppIter = &m_pHead;
-
-    while (*ppIter)
+    bool notify = false;
     {
-        if (pTimer->expiresBefore(*ppIter))
+        std::lock_guard Guard(m_Lock);
+
+        // try to find one with equal or lower remaining time.
+        Timer** ppIter = &m_pHead;
+
+        while (*ppIter)
         {
-            // next element has higher remaining time,
-            // => insert new timer before
-            break;
+            if (pTimer->expiresBefore(*ppIter))
+            {
+                // next element has higher remaining time,
+                // => insert new timer before
+                break;
+            }
+            ppIter= &((*ppIter)->m_pNext);
+        }
+
+        // next element has higher remaining time,
+        // => insert new timer before
+        pTimer->m_pNext= *ppIter;
+        *ppIter = pTimer;
+
+
+        if (pTimer == m_pHead)
+        {
+            notify = true;
         }
-        ppIter= &((*ppIter)->m_pNext);
     }
 
-    // next element has higher remaining time,
-    // => insert new timer before
-    pTimer->m_pNext= *ppIter;
-    *ppIter = pTimer;
-
-
-    if (pTimer == m_pHead)
-    {
+    if (notify) {
         // it was inserted as new head
         // signal it to TimerManager Thread
-        m_notEmpty.set();
+        m_notEmpty.notify_all();
     }
 }
 
@@ -334,28 +351,28 @@ void TimerManager::run()
 
     while (schedule())
     {
-        TTimeValue delay;
-        TTimeValue* pDelay=nullptr;
-
         {
-            std::lock_guard a_Guard(m_Lock);
+            std::unique_lock a_Guard(m_Lock);
 
             if (m_pHead != nullptr)
             {
-                delay = m_pHead->getRemainingTime();
-                pDelay=&delay;
+                TTimeValue delay = m_pHead->getRemainingTime();
+                m_notEmpty.wait_for(
+                    a_Guard,
+                    std::chrono::nanoseconds(
+                        sal_Int64(delay.Seconds) * 1'000'000'000 + delay.Nanosec),
+                    [this] { return m_terminate; });
             }
             else
             {
-                pDelay=nullptr;
+                m_notEmpty.wait(a_Guard, [this] { return m_terminate || m_pHead != nullptr; });
             }
 
-
-            m_notEmpty.reset();
+            if (m_terminate) {
+                break;
+            }
         }
 
-        m_notEmpty.wait(pDelay);
-
         checkForTimeout();
     }