embeddedobj/source/inc/oleembobj.hxx | 3 +- embeddedobj/source/msole/olepersist.cxx | 30 ++++++++++++++++++------- include/vcl/scheduler.hxx | 7 +++++ sw/source/core/doc/DocumentLayoutManager.cxx | 4 +++ vcl/inc/svdata.hxx | 5 ++++ vcl/source/app/scheduler.cxx | 32 +++++++++++++++++++++++++-- vcl/source/app/svapp.cxx | 4 +++ 7 files changed, 74 insertions(+), 11 deletions(-)
New commits: commit 3e0a2239e977a2d6f5252b2412378e02dde3a8b8 Author: Mike Kaganski <mike.kagan...@collabora.com> AuthorDate: Wed Mar 13 10:21:32 2024 +0500 Commit: Mike Kaganski <mike.kagan...@collabora.com> CommitDate: Wed Mar 13 13:47:53 2024 +0100 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.kagan...@collabora.com> Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com> 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 d1e9d36b0c86..72f04a40a08f 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 29f19d31c7e4..5400dcf5780f 100644 --- a/vcl/source/app/svapp.cxx +++ b/vcl/source/app/svapp.cxx @@ -365,7 +365,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;