Introduce a guard to delay processing of idles

In a following scenario, there could be a crash:

1. Platform: a Windows system with MS Word installed.
2. LibreOffice is run in a listener mode;
3. A Java program opens a Writer document in a visible mode, with an
embedded Word OLE object;
4. It adds some text; then resizes the OLE object; then removes the
OLE object.

Word OLE objects have OLEMISC_RECOMPOSEONRESIZE flag [1]; this means,
that every re-layout of the document with this object must ask the
OLE server to re-layout the object content. So, the request thread
changes the document text, which triggers idle re-layout or redraw;
the idles start executing immediately in the idle main thread, with
solar mutex locked; then the request thread starts the OLE object
removal operation. The ongoing relayout in main thread would at some
stage need to execute a call to the OLE object, which temporarily
releases the solar mutex (this makes impossible using solar mutex to
synchronize the order of operations in this scenario). Other mutexes
guarding OLE object (in OleEmbeddedObject, and in OleComponent) are
also released for the duration of the call. Thus, the removal that
happens in the request thread proceeds, and the node containing the
OLE object is destroyed, while the main thread (processing exactly
this node) is waiting for the OLE server response, then for mutexes,
to proceed. After that, the main thread would attempt to access the
destroyed node object.

This change introduces a scheduler guard (a RAII object), that sets
a flag to not process idle events during the lifetime of the guard.
In its constructor, it also makes sure, that current pending idle
events are finished. This would make sure that guarded code started
from other threads would not race with idles potentially accessing
the model that is currently in transient state.

[1] https://learn.microsoft.com/en-us/windows/win32/api/oleidl/ne-oleidl-olemisc

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

7 files changed, 74 insertions, 11 deletions

diff --git a/embeddedobj/source/inc/oleembobj.hxx b/embeddedobj/source/inc/oleembobj.hxx
index 274ecfaf8847..cf7c5ebe4ab4 100644
--- a/embeddedobj/source/inc/oleembobj.hxx
+++ b/embeddedobj/source/inc/oleembobj.hxx
@@ -249,7 +249,8 @@ protected:
                             const css::uno::Reference< css::embed::XStorage >& xStorage,
                             const OUString& sEntName,
                             const css::uno::Sequence< css::beans::PropertyValue >& lObjArgs,
-                            bool bSaveAs );
+                            bool bSaveAs,
+                            osl::ResettableMutexGuard& rGuard);
 #ifdef _WIN32
     /// @throws css::uno::Exception
     void StoreObjectToStream( css::uno::Reference< css::io::XOutputStream > const & xOutStream );
diff --git a/embeddedobj/source/msole/olepersist.cxx b/embeddedobj/source/msole/olepersist.cxx
index 86403f41bb3e..381fc7b0d68c 100644
--- a/embeddedobj/source/msole/olepersist.cxx
+++ b/embeddedobj/source/msole/olepersist.cxx
@@ -58,6 +58,17 @@
 using namespace ::com::sun::star;
 using namespace ::comphelper;
 
+namespace
+{
+#if defined(_WIN32)
+template <class Proc> auto ExecUnlocked(Proc proc, osl::ResettableMutexGuard& guard)
+{
+    ClearedMutexArea area(guard);
+    return proc();
+}
+#endif
+}
+
 
 bool KillFile_Impl( const OUString& aURL, const uno::Reference< uno::XComponentContext >& xContext )
 {
@@ -1059,8 +1070,11 @@ void OleEmbeddedObject::StoreToLocation_Impl(
                             const uno::Reference< embed::XStorage >& xStorage,
                             const OUString& sEntName,
                             const uno::Sequence< beans::PropertyValue >& lObjArgs,
-                            bool bSaveAs )
+                            bool bSaveAs, osl::ResettableMutexGuard& rGuard)
 {
+#ifndef _WIN32
+    (void)rGuard;
+#endif
     // TODO: use lObjArgs
     // TODO: exchange StoreVisualReplacement by SO file format version?
 
@@ -1110,7 +1124,7 @@ void OleEmbeddedObject::StoreToLocation_Impl(
 #ifdef _WIN32
         // if the object was NOT modified after storing it can be just copied
         // as if it was in loaded state
-      || ( m_pOleComponent && !m_pOleComponent->IsDirty() )
+        || (m_pOleComponent && !ExecUnlocked([this] { return m_pOleComponent->IsDirty(); }, rGuard))
 #endif
     )
     {
@@ -1482,13 +1496,13 @@ void SAL_CALL OleEmbeddedObject::storeToEntry( const uno::Reference< embed::XSto
     }
     // end wrapping related part ====================
 
-    ::osl::MutexGuard aGuard( m_aMutex );
+    ::osl::ResettableMutexGuard aGuard( m_aMutex );
     if ( m_bDisposed )
         throw lang::DisposedException(); // TODO
 
     VerbExecutionControllerGuard aVerbGuard( m_aVerbExecutionController );
 
-    StoreToLocation_Impl( xStorage, sEntName, lObjArgs, false );
+    StoreToLocation_Impl( xStorage, sEntName, lObjArgs, false, aGuard );
 
     // TODO: should the listener notification be done?
 }
@@ -1509,13 +1523,13 @@ void SAL_CALL OleEmbeddedObject::storeAsEntry( const uno::Reference< embed::XSto
     }
     // end wrapping related part ====================
 
-    ::osl::MutexGuard aGuard( m_aMutex );
+    ::osl::ResettableMutexGuard aGuard( m_aMutex );
     if ( m_bDisposed )
         throw lang::DisposedException(); // TODO
 
     VerbExecutionControllerGuard aVerbGuard( m_aVerbExecutionController );
 
-    StoreToLocation_Impl( xStorage, sEntName, lObjArgs, true );
+    StoreToLocation_Impl( xStorage, sEntName, lObjArgs, true, aGuard );
 
     // TODO: should the listener notification be done here or in saveCompleted?
 }
@@ -1691,7 +1705,7 @@ void SAL_CALL OleEmbeddedObject::storeOwn()
     // ask container to store the object, the container has to make decision
     // to do so or not
 
-    osl::ClearableMutexGuard aGuard(m_aMutex);
+    osl::ResettableMutexGuard aGuard(m_aMutex);
     if ( m_bDisposed )
         throw lang::DisposedException(); // TODO
 
@@ -1717,7 +1731,7 @@ void SAL_CALL OleEmbeddedObject::storeOwn()
     bool bStoreLoaded = true;
 
 #ifdef _WIN32
-    if ( m_nObjectState != embed::EmbedStates::LOADED && m_pOleComponent && m_pOleComponent->IsDirty() )
+    if ( m_nObjectState != embed::EmbedStates::LOADED && m_pOleComponent && ExecUnlocked([this] { return m_pOleComponent->IsDirty(); }, aGuard) )
     {
         bStoreLoaded = false;
 
diff --git a/include/vcl/scheduler.hxx b/include/vcl/scheduler.hxx
index 1b63404139bf..0181e52c33d2 100644
--- a/include/vcl/scheduler.hxx
+++ b/include/vcl/scheduler.hxx
@@ -81,6 +81,13 @@ public:
     static void       SetDeterministicMode(bool bDeterministic);
     /// Return the current state of deterministic mode.
     static bool       GetDeterministicMode();
+
+    // Makes sure that idles are not processed, until the guard is destroyed
+    struct VCL_DLLPUBLIC IdlesLockGuard final
+    {
+        IdlesLockGuard();
+        ~IdlesLockGuard();
+    };
 };
 
 #endif // INCLUDED_VCL_SCHEDULER_HXX
diff --git a/sw/source/core/doc/DocumentLayoutManager.cxx b/sw/source/core/doc/DocumentLayoutManager.cxx
index 6481f104c737..dc2b34977173 100644
--- a/sw/source/core/doc/DocumentLayoutManager.cxx
+++ b/sw/source/core/doc/DocumentLayoutManager.cxx
@@ -43,6 +43,7 @@
 #include <svx/svdobj.hxx>
 #include <svx/svdpage.hxx>
 #include <osl/diagnose.h>
+#include <vcl/scheduler.hxx>
 
 using namespace ::com::sun::star;
 
@@ -191,6 +192,9 @@ SwFrameFormat *DocumentLayoutManager::MakeLayoutFormat( RndStdIds eRequest, cons
 /// Deletes the denoted format and its content.
 void DocumentLayoutManager::DelLayoutFormat( SwFrameFormat *pFormat )
 {
+    // Do not paint, until the destruction is complete. Paint may access the layout and nodes
+    // while it's in inconsistent state, and crash.
+    Scheduler::IdlesLockGuard g;
     // A chain of frames needs to be merged, if necessary,
     // so that the Frame's contents are adjusted accordingly before we destroy the Frames.
     const SwFormatChain &rChain = pFormat->GetChain();
diff --git a/vcl/inc/svdata.hxx b/vcl/inc/svdata.hxx
index fd7ae855b5f2..3619de00d25b 100644
--- a/vcl/inc/svdata.hxx
+++ b/vcl/inc/svdata.hxx
@@ -23,6 +23,7 @@
 
 #include <o3tl/lru_map.hxx>
 #include <o3tl/hash_combine.hxx>
+#include <osl/conditn.hxx>
 #include <tools/fldunit.hxx>
 #include <unotools/options.hxx>
 #include <vcl/bitmapex.hxx>
@@ -387,6 +388,7 @@ struct ImplSchedulerContext
     std::mutex              maMutex;                        ///< the "scheduler mutex" (see
                                                             ///< vcl/README.scheduler)
     bool                    mbActive = true;                ///< is the scheduler active?
+    oslInterlockedCount     mnIdlesLockCount = 0;           ///< temporary ignore idles
 };
 
 struct ImplSVData
@@ -428,6 +430,9 @@ struct ImplSVData
     css::uno::Reference<css::datatransfer::clipboard::XClipboard> m_xSystemClipboard;
 #endif
 
+    osl::Condition m_inExecuteCondtion; // Set when code returns to Application::Execute,
+                                        // i.e. no nested message loops run
+
     Link<LinkParamNone*,void> maDeInitHook;
 
     // LOK & headless backend specific hooks
diff --git a/vcl/source/app/scheduler.cxx b/vcl/source/app/scheduler.cxx
index e6893055f633..8af341168dac 100644
--- a/vcl/source/app/scheduler.cxx
+++ b/vcl/source/app/scheduler.cxx
@@ -271,6 +271,32 @@ bool Scheduler::GetDeterministicMode()
     return g_bDeterministicMode;
 }
 
+Scheduler::IdlesLockGuard::IdlesLockGuard()
+{
+    ImplSVData* pSVData = ImplGetSVData();
+    ImplSchedulerContext& rSchedCtx = pSVData->maSchedCtx;
+    osl_atomic_increment(&rSchedCtx.mnIdlesLockCount);
+    if (!Application::IsMainThread())
+    {
+        // Make sure that main thread has reached the main message loop, so no idles are executing.
+        // It is important to ensure this, because e.g. ProcessEventsToIdle could be executed in a
+        // nested event loop, while an active processed idle in the main thread is waiting for some
+        // 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();
+        pSVData->m_inExecuteCondtion.wait();
+    }
+}
+
+Scheduler::IdlesLockGuard::~IdlesLockGuard()
+{
+    ImplSchedulerContext& rSchedCtx = ImplGetSVData()->maSchedCtx;
+    osl_atomic_decrement(&rSchedCtx.mnIdlesLockCount);
+}
+
 inline void Scheduler::UpdateSystemTimer( ImplSchedulerContext &rSchedCtx,
                                           const sal_uInt64 nMinPeriod,
                                           const bool bForce, const sal_uInt64 nTime )
@@ -457,8 +483,10 @@ void Scheduler::CallbackTaskScheduling()
     // Delay invoking tasks with idle priorities as long as there are user input or repaint events
     // in the OS event queue. This will often effectively compress such events and repaint only
     // once at the end, improving performance in cases such as repeated zooming with a complex document.
-    bool bDelayInvoking = bIsHighPriorityIdle &&
-        Application::AnyInput( VclInputFlags::MOUSE | VclInputFlags::KEYBOARD | VclInputFlags::PAINT );
+    bool bDelayInvoking
+        = bIsHighPriorityIdle
+          && (rSchedCtx.mnIdlesLockCount > 0
+              || Application::AnyInput(VclInputFlags::MOUSE | VclInputFlags::KEYBOARD | VclInputFlags::PAINT));
 
     /*
     * Current policy is that scheduler tasks aren't allowed to throw an exception.
diff --git a/vcl/source/app/svapp.cxx b/vcl/source/app/svapp.cxx
index 71b884423af3..8d14dc6593a4 100644
--- a/vcl/source/app/svapp.cxx
+++ b/vcl/source/app/svapp.cxx
@@ -364,7 +364,11 @@ void Application::Execute()
             std::abort();
         }
         while (!pSVData->maAppData.mbAppQuit)
+        {
             Application::Yield();
+            SolarMutexReleaser releaser; // Give a chance for the waiting threads to lock the mutex
+            pSVData->m_inExecuteCondtion.set();
+        }
     }
 
     pSVData->maAppData.mbInAppExecute = false;