Relax SolarMutexReleaser precondition to not require SolarMutex lock

As discussed on https://gerrit.libreoffice.org/c/core/+/164843/2#message-8873d3d119de7206b33bc824f5809b8b1d3d97da,
it is impossible at times to know in advance, if a specific code, that
must not be guarded by SolarMutex (e.g., calling to other threads, which
might need to grab the mutex), will itself be guarded by SolarMutex.
Before this change, a required pre-requisite for SolarMutexReleaser use
was existing lock of SolarMutex; otherwise, an attempt to release it
would call abort(). Thus, in some places we had to grab the mutex prior
to releasing it, and that itself introduced more potential for deadlock.

Now the SolarMutexReleaser is safe to use without the lock, in which
case, it will do nothing.

Change-Id: I8759c2f6ed448598b3be4d6c5109804b5e7523ed
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165262
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>

5 files changed, 26 insertions, 31 deletions

diff --git a/embeddedobj/source/msole/olecomponent.cxx b/embeddedobj/source/msole/olecomponent.cxx
index 745e0e83a7e3..a19c9aa72b20 100644
--- a/embeddedobj/source/msole/olecomponent.cxx
+++ b/embeddedobj/source/msole/olecomponent.cxx
@@ -207,15 +207,6 @@ private:
     }
 };
 
-namespace
-{
-struct SafeSolarMutexReleaser
-{
-    SolarMutexGuard guard; // To make sure we actually hold it prior to release
-    SolarMutexReleaser releaser;
-};
-}
-
 static DWORD GetAspectFromFlavor( const datatransfer::DataFlavor& aFlavor )
 {
     if ( aFlavor.MimeType.indexOf( ";Aspect=THUMBNAIL" ) != -1 )
@@ -570,7 +561,7 @@ void OleComponent::RetrieveObjectDataFlavors_Impl()
             HRESULT hr;
             sal::systools::COMReference< IEnumFORMATETC > pFormatEnum;
             {
-                SafeSolarMutexReleaser releaser;
+                SolarMutexReleaser releaser;
                 hr = pDataObject->EnumFormatEtc(DATADIR_GET, &pFormatEnum);
             }
             if ( SUCCEEDED( hr ) && pFormatEnum )
@@ -860,7 +851,7 @@ void OleComponent::InitEmbeddedCopyOfLink( rtl::Reference<OleComponent> const &
     // the object must be already disconnected from the temporary URL
     auto pStorage(m_pNativeImpl->CreateNewStorage(getTempURL()));
 
-    SafeSolarMutexReleaser releaser;
+    SolarMutexReleaser releaser;
 
     auto pDataObject(pOleLinkComponentObj.QueryInterface<IDataObject>(sal::systools::COM_QUERY));
     if ( pDataObject && SUCCEEDED( OleQueryCreateFromData( pDataObject ) ) )
@@ -1007,7 +998,7 @@ void OleComponent::CloseObject()
     auto pOleObject(m_pNativeImpl->get<IOleObject>());
     if (pOleObject && OleIsRunning(pOleObject))
     {
-        SafeSolarMutexReleaser releaser;
+        SolarMutexReleaser releaser;
         HRESULT hr = pOleObject->Close(OLECLOSE_NOSAVE); // must be saved before
         SAL_WARN_IF(FAILED(hr), "embeddedobj.ole", "IOleObject::Close failed");
     }
@@ -1025,7 +1016,7 @@ uno::Sequence< embed::VerbDescriptor > OleComponent::GetVerbList()
         sal::systools::COMReference< IEnumOLEVERB > pEnum;
         HRESULT hr;
         {
-            SafeSolarMutexReleaser releaser;
+            SolarMutexReleaser releaser;
             hr = pOleObject->EnumVerbs(&pEnum);
         }
         if (SUCCEEDED(hr))
@@ -1068,7 +1059,7 @@ void OleComponent::ExecuteVerb( sal_Int32 nVerbID )
     if (!pOleObject)
         throw embed::WrongStateException(); // TODO
 
-    SafeSolarMutexReleaser releaser;
+    SolarMutexReleaser releaser;
 
     // TODO: probably extents should be set here and stored in aRect
     // TODO: probably the parent window also should be set
@@ -1085,7 +1076,7 @@ void OleComponent::SetHostName( const OUString& aEmbDocName )
     if (!pOleObject)
         throw embed::WrongStateException(); // TODO: the object is in wrong state
 
-    SafeSolarMutexReleaser releaser;
+    SolarMutexReleaser releaser;
     pOleObject->SetHostNames(L"app name", o3tl::toW(aEmbDocName.getStr()));
 }
 
@@ -1101,7 +1092,7 @@ void OleComponent::SetExtent( const awt::Size& aVisAreaSize, sal_Int64 nAspect )
     SIZEL aSize = { aVisAreaSize.Width, aVisAreaSize.Height };
     HRESULT hr;
     {
-        SafeSolarMutexReleaser releaser;
+        SolarMutexReleaser releaser;
         hr = pOleObject->SetExtent(nMSAspect, &aSize);
     }
 
@@ -1137,7 +1128,7 @@ awt::Size OleComponent::GetExtent( sal_Int64 nAspect )
 
             HRESULT hr;
             {
-                SafeSolarMutexReleaser releaser;
+                SolarMutexReleaser releaser;
                 hr = pDataObject->GetData(&aFormat, &aMedium);
             }
 
@@ -1224,7 +1215,7 @@ awt::Size OleComponent::GetCachedExtent( sal_Int64 nAspect )
 
     HRESULT hr;
     {
-        SafeSolarMutexReleaser releaser;
+        SolarMutexReleaser releaser;
         hr = pViewObject2->GetExtent(nMSAspect, -1, nullptr, &aSize);
     }
 
@@ -1255,7 +1246,7 @@ awt::Size OleComponent::GetRecommendedExtent( sal_Int64 nAspect )
     SIZEL aSize;
     HRESULT hr;
     {
-        SafeSolarMutexReleaser releaser;
+        SolarMutexReleaser releaser;
         hr = pOleObject->GetExtent(nMSAspect, &aSize);
     }
     if ( FAILED( hr ) )
@@ -1276,7 +1267,7 @@ sal_Int64 OleComponent::GetMiscStatus( sal_Int64 nAspect )
 
     DWORD nResult = 0;
     {
-        SafeSolarMutexReleaser releaser;
+        SolarMutexReleaser releaser;
         pOleObject->GetMiscStatus(static_cast<DWORD>(nAspect), &nResult);
     }
     return static_cast<sal_Int64>(nResult); // first 32 bits are for MS flags
@@ -1292,7 +1283,7 @@ uno::Sequence< sal_Int8 > OleComponent::GetCLSID()
     GUID aCLSID;
     HRESULT hr;
     {
-        SafeSolarMutexReleaser releaser;
+        SolarMutexReleaser releaser;
         hr = pOleObject->GetUserClassID(&aCLSID);
     }
     if ( FAILED( hr ) )
@@ -1315,7 +1306,7 @@ bool OleComponent::IsDirty()
     if ( !pPersistStorage )
         throw io::IOException(); // TODO
 
-    SafeSolarMutexReleaser releaser;
+    SolarMutexReleaser releaser;
     HRESULT hr = pPersistStorage->IsDirty();
     return ( hr != S_FALSE );
 }
@@ -1331,7 +1322,7 @@ void OleComponent::StoreOwnTmpIfNecessary()
     if ( !pPersistStorage )
         throw io::IOException(); // TODO
 
-    SafeSolarMutexReleaser releaser;
+    SolarMutexReleaser releaser;
 
     if ( m_bWorkaroundActive || pPersistStorage->IsDirty() != S_FALSE )
     {
@@ -1593,7 +1584,7 @@ uno::Any SAL_CALL OleComponent::getTransferData( const datatransfer::DataFlavor&
 
                 HRESULT hr;
                 {
-                    SafeSolarMutexReleaser releaser;
+                    SolarMutexReleaser releaser;
                     hr = pDataObject->GetData(&aFormat, &aMedium);
                 }
                 if ( SUCCEEDED( hr ) )
diff --git a/fpicker/source/win32/VistaFilePickerImpl.cxx b/fpicker/source/win32/VistaFilePickerImpl.cxx
index 8a2531ab0119..a3ee8c9ad3e2 100644
--- a/fpicker/source/win32/VistaFilePickerImpl.cxx
+++ b/fpicker/source/win32/VistaFilePickerImpl.cxx
@@ -946,7 +946,6 @@ void VistaFilePickerImpl::impl_sta_ShowDialogModal(Request& rRequest)
     {
         // tdf#146007: Make sure we don't hold solar mutex: COM may need to forward
         // the execution to the main thread, and holding solar mutex could deadlock
-        SolarMutexGuard g; // First acquire, to avoid releaser failure
         SolarMutexReleaser r;
         // show dialog and wait for user decision
         hResult = iDialog->Show(m_hParentWindow ? m_hParentWindow
diff --git a/include/vcl/svapp.hxx b/include/vcl/svapp.hxx
index dcd1374fa3f5..d5f1739be1ae 100644
--- a/include/vcl/svapp.hxx
+++ b/include/vcl/svapp.hxx
@@ -1414,8 +1414,16 @@ class SolarMutexReleaser
 {
     const sal_uInt32 mnReleased;
 public:
-    SolarMutexReleaser(): mnReleased(Application::ReleaseSolarMutex()) {}
-    ~SolarMutexReleaser() { Application::AcquireSolarMutex( mnReleased ); }
+    SolarMutexReleaser()
+        : mnReleased(
+            Application::GetSolarMutex().IsCurrentThread() ? Application::ReleaseSolarMutex() : 0)
+    {
+    }
+    ~SolarMutexReleaser()
+    {
+        if (mnReleased)
+            Application::AcquireSolarMutex(mnReleased);
+    }
 };
 
 VCL_DLLPUBLIC Application* GetpApp();
diff --git a/vcl/source/app/scheduler.cxx b/vcl/source/app/scheduler.cxx
index 72f04a40a08f..4f2543641c3c 100644
--- a/vcl/source/app/scheduler.cxx
+++ b/vcl/source/app/scheduler.cxx
@@ -284,9 +284,7 @@ Scheduler::IdlesLockGuard::IdlesLockGuard()
         // condition to proceed. Only main thread returning to Application::Execute guarantees that
         // the flag really took effect.
         pSVData->m_inExecuteCondtion.reset();
-        std::optional<SolarMutexReleaser> releaser;
-        if (pSVData->mpDefInst->GetYieldMutex()->IsCurrentThread())
-            releaser.emplace();
+        SolarMutexReleaser releaser;
         pSVData->m_inExecuteCondtion.wait();
     }
 }
diff --git a/vcl/source/helper/threadex.cxx b/vcl/source/helper/threadex.cxx
index 1590b91f1167..cbd342bd28df 100644
--- a/vcl/source/helper/threadex.cxx
+++ b/vcl/source/helper/threadex.cxx
@@ -52,7 +52,6 @@ void SolarThreadExecutor::execute()
         m_aStart.reset();
         m_aFinish.reset();
         ImplSVEvent* nEvent = Application::PostUserEvent(LINK(this, SolarThreadExecutor, worker));
-        SolarMutexGuard aGuard;
         SolarMutexReleaser aReleaser;
         if (m_aStart.wait() == osl::Condition::result_timeout)
         {