Make sure --enable-online-update does not leave behind UpdateCheckThread

...which is known to cause test processes to crash during exit.  (Also see
5cd04405c6b2d1ee46294ce0696d89d2edc97d16 "Disable automatic onFirstVisibleTask
UpdateCheck during CppunitTests", so this commit now covers tests like
JunitTests and UITests that start a full soffice process.)

The relevant code is already rather complex and incomprehensible; it handles the
automatic update check triggered by the onFirstVisibleTask UpdateCheckJob,
manually triggered update checks, and downloading of new versions.  To avoid
regressions in all those various scenarios, I tried to add very targeted code
that should only wait for a still running UpdateCheckThread triggered by the
automatic onFirstVisibleTask UpdateCheckJob---which is the scenario that caused
issues during `make check`.  (For a manually triggered update check or download
it is probably both less likely that related threads would still run during
exit, as the user would likely wait for a response before closing LibreOffice,
and less severe if threads would still run during exit, as any resulting crashes
would likely be "harmless" and might not even be noticed by the user.)  However,
I note that this targeted fix does increase the complexity and inscrutability of
this code even further...

The new code does not actually try to join the UpdateCheckJob---see
UpdateCheckThread::join referencing
<https://bz.apache.org/ooo/show_bug.cgi?id=73893> "webdav ucp does not implement
XCommandProcessor::abort".  Rather, it waits for UpdateCheckJob::run to finish
(after unblocking at least any m_aCondition.wait calls).  This, however, can
take a while if the update check's HTTP request is in progress.

To be able to use a std::condition_variable (with sane semantics compared to
osl::ConditionVariable) in UpdateCheck::waitForUpdateCheckFinished, the
corresponding UpdateCheck::m_aMutex had to be changed from a (recursive)
osl::Mutex to a std::recursive_mutex one (sticking to a recursive one just to be
on the safe side, whether or not the code actually makes use of that).

Change-Id: I2726acd33b884b807d840cfee2786fcf45815ad7
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/103013
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

3 files changed, 150 insertions, 44 deletions

diff --git a/extensions/source/update/check/updatecheck.cxx b/extensions/source/update/check/updatecheck.cxx
index e7309e4a4725..348ef35faaca 100644
--- a/extensions/source/update/check/updatecheck.cxx
+++ b/extensions/source/update/check/updatecheck.cxx
@@ -215,12 +215,15 @@ class UpdateCheckThread : public WorkerThread
 
 public:
     UpdateCheckThread( osl::Condition& rCondition,
-        const uno::Reference<uno::XComponentContext>& xContext );
+        const uno::Reference<uno::XComponentContext>& xContext,
+        rtl::Reference<UpdateCheck> const & controller );
 
     virtual void SAL_CALL join() override;
     virtual void SAL_CALL terminate() override;
     virtual void cancel() override;
 
+    void cancelAsSoonAsPossible();
+
 protected:
     virtual ~UpdateCheckThread() override;
 
@@ -266,6 +269,8 @@ protected:
 private:
     const uno::Reference<uno::XComponentContext> m_xContext;
     uno::Reference<deployment::XUpdateInformationProvider> m_xProvider;
+    rtl::Reference<UpdateCheck> m_controller;
+    bool m_cancelAsSoonAsPossible;
 };
 
 
@@ -273,7 +278,7 @@ class ManualUpdateCheckThread : public UpdateCheckThread
 {
 public:
     ManualUpdateCheckThread( osl::Condition& rCondition, const uno::Reference<uno::XComponentContext>& xContext ) :
-        UpdateCheckThread(rCondition, xContext) {};
+        UpdateCheckThread(rCondition, xContext, {}) {};
 
     virtual void SAL_CALL run() override;
 };
@@ -334,9 +339,12 @@ private:
 
 
 UpdateCheckThread::UpdateCheckThread( osl::Condition& rCondition,
-                                      const uno::Reference<uno::XComponentContext>& xContext ) :
+                                      const uno::Reference<uno::XComponentContext>& xContext,
+                                      rtl::Reference<UpdateCheck> const & controller ) :
     m_aCondition(rCondition),
-    m_xContext(xContext)
+    m_xContext(xContext),
+    m_controller(controller),
+    m_cancelAsSoonAsPossible(false)
 {
     createSuspended();
 
@@ -382,6 +390,13 @@ UpdateCheckThread::cancel()
         xProvider->cancel();
 }
 
+void UpdateCheckThread::cancelAsSoonAsPossible() {
+    {
+        osl::MutexGuard g(m_aMutex);
+        m_cancelAsSoonAsPossible = true;
+    }
+    m_aCondition.set();
+}
 
 bool
 UpdateCheckThread::runCheck( bool & rbExtensionsChecked )
@@ -442,6 +457,12 @@ UpdateCheckThread::run()
 
     // Initial wait to avoid doing further time consuming tasks during start-up
     aResult = m_aCondition.wait(&tv);
+    {
+        osl::MutexGuard g(m_aMutex);
+        if (m_cancelAsSoonAsPossible) {
+            goto done;
+        }
+    }
 
     try {
         bool bExtensionsChecked = false;
@@ -494,6 +515,12 @@ UpdateCheckThread::run()
                     // This can not be > 32 Bit for now ..
                     tv.Seconds = static_cast< sal_Int32 > (next - systime.Seconds);
                     aResult = m_aCondition.wait(&tv);
+                    {
+                        osl::MutexGuard g(m_aMutex);
+                        if (m_cancelAsSoonAsPossible) {
+                            goto done;
+                        }
+                    }
                     continue;
                 }
             }
@@ -516,6 +543,12 @@ UpdateCheckThread::run()
 
                 tv.Seconds = nRetryInterval[n-1];
                 aResult = m_aCondition.wait(&tv);
+                {
+                    osl::MutexGuard g(m_aMutex);
+                    if (m_cancelAsSoonAsPossible) {
+                        goto done;
+                    }
+                }
             }
             else // reset retry counter
             {
@@ -529,6 +562,11 @@ UpdateCheckThread::run()
         // Silently catch all errors
         TOOLS_WARN_EXCEPTION("extensions.update", "Caught exception, thread terminated" );
     }
+
+done:
+    if (m_controller.is()) {
+        m_controller->notifyUpdateCheckFinished();
+    }
 }
 
 
@@ -705,6 +743,7 @@ UpdateCheck::UpdateCheck()
     , m_pThread(nullptr)
     , m_bHasExtensionUpdate(false)
     , m_bShowExtUpdDlg(false)
+    , m_updateCheckRunning(false)
 {
 }
 
@@ -714,7 +753,7 @@ void
 UpdateCheck::initialize(const uno::Sequence< beans::NamedValue >& rValues,
                         const uno::Reference<uno::XComponentContext>& xContext)
 {
-    osl::MutexGuard aGuard(m_aMutex);
+    std::scoped_lock aGuard(m_aMutex);
 
     if( NOT_INITIALIZED == m_eState )
     {
@@ -812,12 +851,12 @@ UpdateCheck::initialize(const uno::Sequence< beans::NamedValue >& rValues,
 void
 UpdateCheck::cancel()
 {
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
 
     WorkerThread *pThread = m_pThread;
     UpdateState eUIState = getUIState(m_aUpdateInfo);
 
-    aGuard.clear();
+    aGuard.unlock();
 
     if( nullptr != pThread )
         pThread->cancel();
@@ -829,10 +868,10 @@ UpdateCheck::cancel()
 void
 UpdateCheck::download()
 {
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
     UpdateInfo aInfo(m_aUpdateInfo);
     State eState = m_eState;
-    aGuard.clear();
+    aGuard.unlock();
 
     if (aInfo.Sources.empty())
     {
@@ -848,7 +887,7 @@ UpdateCheck::download()
             shutdownThread(true);
 
             {
-                osl::MutexGuard aGuard2(m_aMutex);
+                std::scoped_lock aGuard2(m_aMutex);
                 enableDownload(true);
             }
             setUIState(UPDATESTATE_DOWNLOADING);
@@ -864,7 +903,7 @@ UpdateCheck::download()
 void
 UpdateCheck::install()
 {
-    osl::MutexGuard aGuard(m_aMutex);
+    std::scoped_lock aGuard(m_aMutex);
 
     const uno::Reference< c3s::XSystemShellExecute > xShellExecute = c3s::SystemShellExecute::create( m_xContext );
 
@@ -908,13 +947,13 @@ UpdateCheck::install()
 void
 UpdateCheck::pause()
 {
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
 
     if( nullptr != m_pThread )
         m_pThread->suspend();
 
     rtl::Reference< UpdateCheckConfig > rModel = UpdateCheckConfig::get(m_xContext);
-    aGuard.clear();
+    aGuard.unlock();
 
     rModel->storeDownloadPaused(true);
     setUIState(UPDATESTATE_DOWNLOAD_PAUSED);
@@ -924,13 +963,13 @@ UpdateCheck::pause()
 void
 UpdateCheck::resume()
 {
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
 
     if( nullptr != m_pThread )
         m_pThread->resume();
 
     rtl::Reference< UpdateCheckConfig > rModel = UpdateCheckConfig::get(m_xContext);
-    aGuard.clear();
+    aGuard.unlock();
 
     rModel->storeDownloadPaused(false);
     setUIState(UPDATESTATE_DOWNLOADING);
@@ -940,26 +979,49 @@ UpdateCheck::resume()
 void
 UpdateCheck::closeAfterFailure()
 {
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
 
     if ( ( m_eState == DISABLED ) || ( m_eState == CHECK_SCHEDULED ) )
     {
         const UpdateState eUIState = getUIState( m_aUpdateInfo );
-        aGuard.clear();
+        aGuard.unlock();
         setUIState( eUIState, true );
     }
 }
 
+void UpdateCheck::notifyUpdateCheckFinished() {
+    std::scoped_lock l(m_aMutex);
+    m_updateCheckRunning = false;
+    m_updateCheckFinished.notify_all();
+}
+
+void UpdateCheck::waitForUpdateCheckFinished() {
+    UpdateCheckThread * thread;
+    {
+        std::scoped_lock l(m_aMutex);
+        thread = dynamic_cast<UpdateCheckThread *>(m_pThread);
+    }
+    if (thread != nullptr) {
+        thread->cancelAsSoonAsPossible();
+    }
+    for (;;) {
+        std::unique_lock lock(m_aMutex);
+        if (!m_updateCheckRunning) {
+            return;
+        }
+        m_updateCheckFinished.wait(lock);
+    }
+}
 
 void
 UpdateCheck::shutdownThread(bool join)
 {
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
 
     // copy thread object pointer to stack
     osl::Thread *pThread = m_pThread;
     m_pThread = nullptr;
-    aGuard.clear();
+    aGuard.unlock();
 
     if( nullptr != pThread )
     {
@@ -978,7 +1040,10 @@ void
 UpdateCheck::enableAutoCheck(bool enable)
 {
     if( enable )
-        m_pThread = new UpdateCheckThread(m_aCondition, m_xContext);
+    {
+        m_updateCheckRunning = true;
+        m_pThread = new UpdateCheckThread(m_aCondition, m_xContext, this);
+    }
 
     m_eState = enable ? CHECK_SCHEDULED : DISABLED;
 }
@@ -1013,7 +1078,7 @@ UpdateCheck::enableDownload(bool enable, bool paused)
 bool
 UpdateCheck::downloadTargetExists(const OUString& rFileName)
 {
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
 
     rtl::Reference< UpdateHandler > aUpdateHandler(getUpdateHandler());
     UpdateState eUIState = UPDATESTATE_DOWNLOADING;
@@ -1045,7 +1110,7 @@ UpdateCheck::downloadTargetExists(const OUString& rFileName)
         shutdownThread(false);
         enableDownload(false);
 
-        aGuard.clear();
+        aGuard.unlock();
         setUIState(eUIState);
     }
 
@@ -1055,7 +1120,7 @@ UpdateCheck::downloadTargetExists(const OUString& rFileName)
 
 bool UpdateCheck::checkDownloadDestination( const OUString& rFileName )
 {
-    osl::MutexGuard aGuard(m_aMutex);
+    std::scoped_lock aGuard(m_aMutex);
 
     rtl::Reference< UpdateHandler > aUpdateHandler( getUpdateHandler() );
 
@@ -1073,9 +1138,9 @@ bool UpdateCheck::checkDownloadDestination( const OUString& rFileName )
 void
 UpdateCheck::downloadStalled(const OUString& rErrorMessage)
 {
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
     rtl::Reference< UpdateHandler > aUpdateHandler(getUpdateHandler());
-    aGuard.clear();
+    aGuard.unlock();
 
     aUpdateHandler->setErrorMessage(rErrorMessage);
     setUIState(UPDATESTATE_ERROR_DOWNLOADING);
@@ -1085,9 +1150,9 @@ UpdateCheck::downloadStalled(const OUString& rErrorMessage)
 void
 UpdateCheck::downloadProgressAt(sal_Int8 nPercent)
 {
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
     rtl::Reference< UpdateHandler > aUpdateHandler(getUpdateHandler());
-    aGuard.clear();
+    aGuard.unlock();
 
     aUpdateHandler->setProgress(nPercent);
     setUIState(UPDATESTATE_DOWNLOADING);
@@ -1099,7 +1164,7 @@ UpdateCheck::downloadStarted(const OUString& rLocalFileName, sal_Int64 nFileSize
 {
     if ( nFileSize > 0 )
     {
-        osl::MutexGuard aGuard(m_aMutex);
+        std::scoped_lock aGuard(m_aMutex);
 
         rtl::Reference< UpdateCheckConfig > aModel(UpdateCheckConfig::get(m_xContext));
         aModel->storeLocalFileName(rLocalFileName, nFileSize);
@@ -1115,7 +1180,7 @@ UpdateCheck::downloadStarted(const OUString& rLocalFileName, sal_Int64 nFileSize
 void
 UpdateCheck::downloadFinished(const OUString& rLocalFileName)
 {
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
 
     // no more retries
     m_pThread->terminate();
@@ -1123,7 +1188,7 @@ UpdateCheck::downloadFinished(const OUString& rLocalFileName)
     m_aImageName = getImageFromFileName(rLocalFileName);
     UpdateInfo aUpdateInfo(m_aUpdateInfo);
 
-    aGuard.clear();
+    aGuard.unlock();
     setUIState(UPDATESTATE_DOWNLOAD_AVAIL);
 
     // Bring-up release note for position 2 ..
@@ -1139,7 +1204,7 @@ UpdateCheck::cancelDownload()
 {
     shutdownThread(true);
 
-    osl::MutexGuard aGuard(m_aMutex);
+    std::scoped_lock aGuard(m_aMutex);
     enableDownload(false);
 
     rtl::Reference< UpdateCheckConfig > rModel = UpdateCheckConfig::get(m_xContext);
@@ -1163,7 +1228,7 @@ UpdateCheck::cancelDownload()
 void
 UpdateCheck::showDialog(bool forceCheck)
 {
-    osl::ResettableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
 
     bool update_found = !m_aUpdateInfo.BuildId.isEmpty();
     bool bSetUIState = ! m_aUpdateHandler.is();
@@ -1200,9 +1265,9 @@ UpdateCheck::showDialog(bool forceCheck)
 
     if( bSetUIState )
     {
-        aGuard.clear();
+        aGuard.unlock();
         setUIState(eDialogState, true); // suppress bubble as Dialog will be visible soon
-        aGuard.reset();
+        aGuard.lock();
     }
 
     getUpdateHandler()->setVisible();
@@ -1224,7 +1289,7 @@ UpdateCheck::showDialog(bool forceCheck)
 void
 UpdateCheck::setUpdateInfo(const UpdateInfo& aInfo)
 {
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
 
     bool bSuppressBubble = aInfo.BuildId == m_aUpdateInfo.BuildId;
     m_aUpdateInfo = aInfo;
@@ -1287,7 +1352,7 @@ UpdateCheck::setUpdateInfo(const UpdateInfo& aInfo)
         rModel->clearUpdateFound();
     }
 
-    aGuard.clear();
+    aGuard.unlock();
     setUIState(eUIState, bSuppressBubble);
 }
 
@@ -1336,7 +1401,7 @@ void UpdateCheck::handleMenuBarUI( const rtl::Reference< UpdateHandler >& rUpdat
 
 void UpdateCheck::setUIState(UpdateState eState, bool suppressBubble)
 {
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
 
     if( ! m_xMenuBarUI.is() &&
         (DISABLED != m_eState) &&
@@ -1360,7 +1425,7 @@ void UpdateCheck::setUIState(UpdateState eState, bool suppressBubble)
     UpdateInfo aUpdateInfo(m_aUpdateInfo);
     OUString aImageName(m_aImageName);
 
-    aGuard.clear();
+    aGuard.unlock();
 
     handleMenuBarUI( aUpdateHandler, eState, suppressBubble );
 
@@ -1485,7 +1550,7 @@ void UpdateCheck::showExtensionDialog()
 rtl::Reference<UpdateHandler>
 UpdateCheck::getUpdateHandler()
 {
-    osl::MutexGuard aGuard(m_aMutex);
+    std::scoped_lock aGuard(m_aMutex);
 
     if( ! m_aUpdateHandler.is() )
         m_aUpdateHandler = new UpdateHandler(m_xContext, this);
@@ -1497,7 +1562,7 @@ UpdateCheck::getUpdateHandler()
 uno::Reference< task::XInteractionHandler >
 UpdateCheck::getInteractionHandler() const
 {
-    osl::MutexGuard aGuard(m_aMutex);
+    std::scoped_lock aGuard(m_aMutex);
 
     uno::Reference< task::XInteractionHandler > xHandler;
 
@@ -1511,7 +1576,7 @@ UpdateCheck::getInteractionHandler() const
 bool
 UpdateCheck::isDialogShowing() const
 {
-    osl::MutexGuard aGuard(m_aMutex);
+    std::scoped_lock aGuard(m_aMutex);
     return m_aUpdateHandler.is() && m_aUpdateHandler->isVisible();
 };
 
@@ -1519,7 +1584,7 @@ UpdateCheck::isDialogShowing() const
 void
 UpdateCheck::autoCheckStatusChanged(bool enabled)
 {
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    std::unique_lock aGuard(m_aMutex);
 
     if( (CHECK_SCHEDULED == m_eState) && !enabled )
         shutdownThread(false);
@@ -1528,7 +1593,7 @@ UpdateCheck::autoCheckStatusChanged(bool enabled)
     {
         enableAutoCheck(enabled);
         UpdateState eState = getUIState(m_aUpdateInfo);
-        aGuard.clear();
+        aGuard.unlock();
         setUIState(eState);
     }
 };
diff --git a/extensions/source/update/check/updatecheck.hxx b/extensions/source/update/check/updatecheck.hxx
index e50fa3859b0c..fbadbe82a03e 100644
--- a/extensions/source/update/check/updatecheck.hxx
+++ b/extensions/source/update/check/updatecheck.hxx
@@ -20,6 +20,11 @@
 #ifndef INCLUDED_EXTENSIONS_SOURCE_UPDATE_CHECK_UPDATECHECK_HXX
 #define INCLUDED_EXTENSIONS_SOURCE_UPDATE_CHECK_UPDATECHECK_HXX
 
+#include <sal/config.h>
+
+#include <condition_variable>
+#include <mutex>
+
 #include <com/sun/star/beans/NamedValue.hpp>
 #include <com/sun/star/beans/XPropertySet.hpp>
 #include <com/sun/star/task/XInteractionHandler.hpp>
@@ -118,6 +123,10 @@ public:
     void resume() override;
     void closeAfterFailure() override;
 
+    void notifyUpdateCheckFinished();
+
+    void waitForUpdateCheckFinished();
+
 private:
 
     // Schedules or cancels next automatic check for updates
@@ -153,9 +162,10 @@ private:
     State m_eState;
     UpdateState m_eUpdateState;
 
-    mutable osl::Mutex m_aMutex;
+    mutable std::recursive_mutex m_aMutex;
     WorkerThread *m_pThread;
     osl::Condition m_aCondition;
+    osl::Condition m_NotInWaitState;
 
     UpdateInfo m_aUpdateInfo;
     OUString m_aImageName;
@@ -166,6 +176,9 @@ private:
     css::uno::Reference<css::beans::XPropertySet> m_xMenuBarUI;
     css::uno::Reference<css::uno::XComponentContext> m_xContext;
 
+    bool m_updateCheckRunning = false;
+    std::condition_variable_any m_updateCheckFinished;
+
     friend class UpdateCheckInitData;
 };
 
diff --git a/extensions/source/update/check/updatecheckjob.cxx b/extensions/source/update/check/updatecheckjob.cxx
index d457269e9ca6..6da52c02fab6 100644
--- a/extensions/source/update/check/updatecheckjob.cxx
+++ b/extensions/source/update/check/updatecheckjob.cxx
@@ -25,6 +25,9 @@
 #include "updateprotocol.hxx"
 
 #include <memory>
+#include <mutex>
+#include <utility>
+
 #include <cppuhelper/implbase.hxx>
 #include <cppuhelper/implementationentry.hxx>
 #include <cppuhelper/supportsservice.hxx>
@@ -60,6 +63,9 @@ private:
     uno::Sequence<beans::NamedValue> m_xParameters;
     bool m_bShowDialog;
     bool m_bTerminating;
+
+    std::mutex m_mutex;
+    rtl::Reference<UpdateCheck> m_controller;
 };
 
 class UpdateCheckJob :
@@ -125,6 +131,16 @@ void SAL_CALL InitUpdateCheckJobThread::run()
 
     try {
         rtl::Reference< UpdateCheck > aController( UpdateCheck::get() );
+        // At least for the automatic ("onFirstVisibleTask", i.e., !m_bShowDialog) check, wait for
+        // m_controller during setTerminating, to prevent m_controller from still having threads
+        // running during exit (ideally, we would make sure that all threads are joined before exit,
+        // but the UpdateCheck logic is rather convoluted, so play it safe for now and only address
+        // the automatic update check that is known to cause issues during `make check`, not the
+        // manually triggered update check scenario):
+        if (!m_bShowDialog) {
+            std::scoped_lock l(m_mutex);
+            m_controller = aController;
+        }
         aController->initialize( m_xParameters, m_xContext );
 
         if ( m_bShowDialog )
@@ -132,12 +148,24 @@ void SAL_CALL InitUpdateCheckJobThread::run()
     } catch (const uno::Exception &) {
         // fdo#64962 - don't bring the app down on some unexpected exception.
         TOOLS_WARN_EXCEPTION("extensions.update", "Caught init update exception, thread terminated" );
+        {
+            std::scoped_lock l(m_mutex);
+            m_controller.clear();
+        }
     }
 }
 
 void InitUpdateCheckJobThread::setTerminating() {
     m_bTerminating = true;
     m_aCondition.set();
+    rtl::Reference<UpdateCheck> controller;
+    {
+        std::scoped_lock l(m_mutex);
+        std::swap(controller, m_controller);
+    }
+    if (controller.is()) {
+        controller->waitForUpdateCheckFinished();
+    }
 }
 
 UpdateCheckJob::~UpdateCheckJob()