framework: AutoRecovery uses Timer without SolarMutex

This results in a SolarMutex assert when creating a new Base document.

... and also implts_startListening() is looking less thread safe
than advertised.

Change-Id: I4635064733c16416f903dab4caee59fb2de3cb00
(cherry picked from commit b2c467e47f438b2011aa304cca9bf403eaa1c8e2)
Reviewed-on: https://gerrit.libreoffice.org/33308
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Miklos Vajna <vmiklos@collabora.co.uk>

1 files changed, 28 insertions, 12 deletions

diff --git a/framework/source/services/autorecovery.cxx b/framework/source/services/autorecovery.cxx
index dec1ee8392ba..222f4eaa0780 100644
--- a/framework/source/services/autorecovery.cxx
+++ b/framework/source/services/autorecovery.cxx
@@ -381,6 +381,7 @@ private:
 
     /** @short  the timer, which is used to be informed about the next
                 saving time ...
+        @remark must lock SolarMutex to use
      */
     Timer m_aTimer;
 
@@ -1252,12 +1253,13 @@ void AutoRecovery::initListeners()
 
     // establish callback for our internal used timer.
     // Note: Its only active, if the timer will be started ...
+    SolarMutexGuard g;
     m_aTimer.SetTimeoutHdl(LINK(this, AutoRecovery, implts_timerExpired));
 }
 
 AutoRecovery::~AutoRecovery()
 {
-    disposing();
+    assert(!m_aTimer.IsActive());
 }
 
 void AutoRecovery::disposing()
@@ -1295,7 +1297,7 @@ void SAL_CALL AutoRecovery::dispatch(const css::util::URL&
     bool bAsync;
     DispatchParams aParams;
     /* SAFE */ {
-    osl::MutexGuard g(cppu::WeakComponentImplHelperBase::rBHelper.rMutex);
+    osl::ClearableMutexGuard g(cppu::WeakComponentImplHelperBase::rBHelper.rMutex);
 
     // still running operation ... ignoring AUTO_SAVE.
     // All other requests has higher prio!
@@ -1331,6 +1333,7 @@ void SAL_CALL AutoRecovery::dispatch(const css::util::URL&
             // don't enable AutoSave hardly !
             // reload configuration to know the current state.
             implts_readAutoSaveConfig();
+            g.clear();
             implts_updateTimer();
             // can it happen that might be the listener was stopped ? .-)
             // make sure it runs always ... even if AutoSave itself was disabled temporarly.
@@ -2145,21 +2148,28 @@ void AutoRecovery::implts_startListening()
     css::uno::Reference< css::util::XChangesNotifier > xCFG;
     css::uno::Reference< css::frame::XGlobalEventBroadcaster > xBroadcaster;
     bool bListenForDocEvents;
+    bool bListenForConfigChanges;
     /* SAFE */ {
     osl::MutexGuard g(cppu::WeakComponentImplHelperBase::rBHelper.rMutex);
     xCFG.set              (m_xRecoveryCFG, css::uno::UNO_QUERY);
     xBroadcaster        = m_xNewDocBroadcaster;
     bListenForDocEvents = m_bListenForDocEvents;
+    bListenForConfigChanges = m_bListenForConfigChanges;
     } /* SAFE */
 
     if (
         (  xCFG.is()                ) &&
-        (! m_bListenForConfigChanges)
+        (! bListenForConfigChanges)
        )
     {
-        m_xRecoveryCFGListener = new WeakChangesListener(this);
-        xCFG->addChangesListener(m_xRecoveryCFGListener);
+        css::uno::Reference<css::util::XChangesListener> const xListener(
+                new WeakChangesListener(this));
+        xCFG->addChangesListener(xListener);
+        /* SAFE */ {
+        osl::MutexGuard g2(cppu::WeakComponentImplHelperBase::rBHelper.rMutex);
+        m_xRecoveryCFGListener = xListener;
         m_bListenForConfigChanges = true;
+        } /* SAFE */
     }
 
     if (!xBroadcaster.is())
@@ -2176,10 +2186,12 @@ void AutoRecovery::implts_startListening()
         (! bListenForDocEvents)
        )
     {
-        m_xNewDocBroadcasterListener = new WeakDocumentEventListener(this);
-        xBroadcaster->addDocumentEventListener(m_xNewDocBroadcasterListener);
+        css::uno::Reference<css::document::XDocumentEventListener> const
+            xListener(new WeakDocumentEventListener(this));
+        xBroadcaster->addDocumentEventListener(xListener);
         /* SAFE */ {
         osl::MutexGuard g2(cppu::WeakComponentImplHelperBase::rBHelper.rMutex);
+        m_xNewDocBroadcasterListener = xListener;
         m_bListenForDocEvents = true;
         } /* SAFE */
     }
@@ -2249,6 +2261,8 @@ void AutoRecovery::implts_updateTimer()
 {
     implts_stopTimer();
 
+    sal_Int32 nMilliSeconds = 0;
+
     /* SAFE */ {
     osl::MutexGuard g(cppu::WeakComponentImplHelperBase::rBHelper.rMutex);
 
@@ -2258,7 +2272,6 @@ void AutoRecovery::implts_updateTimer()
        )
         return;
 
-    sal_Int32 nMilliSeconds = 0;
     if (m_eTimerType == AutoRecovery::E_NORMAL_AUTOSAVE_INTERVALL)
     {
         nMilliSeconds = (m_nAutoSaveTimeIntervall*60000); // [min] => 60.000 ms
@@ -2270,15 +2283,17 @@ void AutoRecovery::implts_updateTimer()
     else if (m_eTimerType == AutoRecovery::E_POLL_TILL_AUTOSAVE_IS_ALLOWED)
         nMilliSeconds = 300; // there is a minimum time frame, where the user can lose some key input data!
 
-    m_aTimer.SetTimeout(nMilliSeconds);
-    m_aTimer.Start();
 
     } /* SAFE */
+
+    SolarMutexGuard g;
+    m_aTimer.SetTimeout(nMilliSeconds);
+    m_aTimer.Start();
 }
 
 void AutoRecovery::implts_stopTimer()
 {
-    osl::MutexGuard g(cppu::WeakComponentImplHelperBase::rBHelper.rMutex);
+    SolarMutexGuard g;
 
     if (!m_aTimer.IsActive())
         return;
@@ -2326,13 +2341,14 @@ IMPL_LINK_NOARG(AutoRecovery, implts_timerExpired, Timer *, void)
         // If we poll for an user idle period, may be we must
         // do nothing here and start the timer again.
         /* SAFE */ {
-        osl::MutexGuard g(cppu::WeakComponentImplHelperBase::rBHelper.rMutex);
+        osl::ClearableMutexGuard g(cppu::WeakComponentImplHelperBase::rBHelper.rMutex);
 
         if (m_eTimerType == AutoRecovery::E_POLL_FOR_USER_IDLE)
         {
             bool bUserIdle = (Application::GetLastInputInterval()>MIN_TIME_FOR_USER_IDLE);
             if (!bUserIdle)
             {
+                g.clear();
                 implts_updateTimer();
                 return;
             }