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;
