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 | 69 ++++++++++++++++++---------
vcl/source/app/svapp.cxx | 4 +
vcl/source/app/svmain.cxx | 3 +
8 files changed, 94 insertions(+), 31 deletions(-)
New commits:
commit d6cbd9febae2e146e4e833e6b468283a634f7246
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 18:02:50 2024 +0500
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 75c8e86acdb2..425729833754 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 06d0aeb9b9af..725f0cda4260 100644
--- a/vcl/inc/svdata.hxx
+++ b/vcl/inc/svdata.hxx
@@ -24,6 +24,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>
@@ -383,6 +384,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
@@ -424,6 +426,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 7afdfd0846e6..c0fc79d4ac5c 100644
--- a/vcl/source/app/scheduler.cxx
+++ b/vcl/source/app/scheduler.cxx
@@ -272,6 +272,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 )
@@ -458,8 +484,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 d2c6294917f0..1f7831ea060b 100644
--- a/vcl/source/app/svapp.cxx
+++ b/vcl/source/app/svapp.cxx
@@ -450,7 +450,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;
commit a1dcf14f70f676cce856381c00e0c5035cfedeb1
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Wed Mar 13 10:15:07 2024 +0500
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Wed Mar 13 18:02:50 2024 +0500
Process pending events before ImplDeleteOnDeInit
The statics derived from DeleteOnDeinitBase are destroyed in the call
to ImplDeleteOnDeInit; and then they may be accessed, when a few lines
later, Scheduler::ImplDeInitScheduler is called, which itself calls
ProcessEventsToIdle. This change makes sure that the events, that may
potentially want to access these objects, are executed at a safe time.
Change-Id: Idc04100453ed12def815b721e3efe76cb3ca1100
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/164752
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
diff --git a/vcl/source/app/svmain.cxx b/vcl/source/app/svmain.cxx
index 7c5505f364cb..a6dc00dcd9e9 100644
--- a/vcl/source/app/svmain.cxx
+++ b/vcl/source/app/svmain.cxx
@@ -440,6 +440,9 @@ void DeInitVCL()
pSVData->mpBlendFrameCache->m_aLastResult.Clear();
pSVData->mbDeInit = true;
+ // Some events may need to access objects destroyed in ImplDeleteOnDeInit,
so process them first
+ Scheduler::ProcessEventsToIdle();
+
vcl::DeleteOnDeinitBase::ImplDeleteOnDeInit();
#if OSL_DEBUG_LEVEL > 0
commit 7907b2e51edafa8bbc2eeb6925fbcfb69536598b
Author: Michael Meeks <michael.me...@collabora.com>
AuthorDate: Mon Nov 21 17:41:58 2022 +0000
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Wed Mar 13 18:02:44 2024 +0500
tdf#148434 - avoid strange OS/X deadlock around AnyInput.
Apparently calling AnyInput on Mac and filtering for just input, gives
you window creation / re-sizing events which then trigger idle paint
events which then deadlock if called with the scheduler lock.
Try having a little more inefficiency and a different race for
this case to handle the Mac world.
Change-Id: I9985eaf18f8d0ba4d44e83c03746510a6ba6d664
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/143046
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.gran...@collabora.co.uk>
diff --git a/vcl/source/app/scheduler.cxx b/vcl/source/app/scheduler.cxx
index f83a4bb04d32..7afdfd0846e6 100644
--- a/vcl/source/app/scheduler.cxx
+++ b/vcl/source/app/scheduler.cxx
@@ -426,24 +426,6 @@ void Scheduler::CallbackTaskScheduling()
break;
}
-// tdf#148435 Apparently calling AnyInput on Mac and filtering for just input,
gives
-// you window creation / re-sizing events which then trigger idle paint
-// events which then deadlock if called with the scheduler lock.
-// So since this is an optimisation, just don't do this on mac.
-#ifndef MACOSX
- // 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.
- if ( pMostUrgent && pMostUrgent->mePriority >= TaskPriority::HIGH_IDLE
- && Application::AnyInput( VclInputFlags::MOUSE |
VclInputFlags::KEYBOARD | VclInputFlags::PAINT ))
- {
- SAL_INFO( "vcl.schedule", tools::Time::GetSystemTicks()
- << " idle priority task " << pMostUrgent << " delayed, system
events pending" );
- pMostUrgent = nullptr;
- nMinPeriod = 0;
- }
-#endif
-
if (InfiniteTimeoutMs != nMinPeriod)
SAL_INFO("vcl.schedule",
"Calculated minimum timeout as " << nMinPeriod << " of " <<
nTasks << " tasks");
@@ -459,9 +441,6 @@ void Scheduler::CallbackTaskScheduling()
comphelper::ProfileZone aZone( pTask->GetDebugName() );
- // prepare Scheduler object for deletion after handling
- pTask->SetDeletionFlags();
-
assert(!pMostUrgent->mbInScheduler);
pMostUrgent->mbInScheduler = true;
@@ -471,8 +450,17 @@ void Scheduler::CallbackTaskScheduling()
rSchedCtx.mpSchedulerStack = pMostUrgent;
rSchedCtx.mpSchedulerStackTop = pMostUrgent;
+ bool bIsHighPriorityIdle = pMostUrgent->mePriority >=
TaskPriority::HIGH_IDLE;
+
// invoke the task
Unlock();
+
+ // 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 );
+
/*
* Current policy is that scheduler tasks aren't allowed to throw an
exception.
* Because otherwise the exception is caught somewhere totally unrelated.
@@ -482,7 +470,16 @@ void Scheduler::CallbackTaskScheduling()
*/
try
{
- pTask->Invoke();
+ if (bDelayInvoking)
+ SAL_INFO( "vcl.schedule", tools::Time::GetSystemTicks()
+ << " idle priority task " << pTask->GetDebugName()
+ << " delayed, system events pending" );
+ else
+ {
+ // prepare Scheduler object for deletion after handling
+ pTask->SetDeletionFlags();
+ pTask->Invoke();
+ }
}
catch (css::uno::Exception&)
{
