vcl/source/app/svdata.cxx | 76 ++++++++++++++++++++++++++++++++++++----------
1 file changed, 60 insertions(+), 16 deletions(-)
New commits:
commit c635271eb17e3b9d50223356dbc09b771fcd643f
Author: Armin Le Grand (Collabora) <armin.le.gr...@me.com>
AuthorDate: Tue Nov 19 20:34:05 2024 +0100
Commit: Armin Le Grand <armin.le.gr...@me.com>
CommitDate: Wed Nov 20 16:53:11 2024 +0100
tdf#163428 CairoSDPR: Fix regression in freeing unified buffers
I had added BufferedData_ModifiedBitmapEx for CairoSDPR (but
not only) to allow Bitmaps modualted by BColorModifierStack
to be buffered in the standard way and at the original
Bitmap. This nicely can still hold the system-dependent form
of the Bitmap to be used at the Bitmap of that buffered
modified Bitmap.
This uses something I was not aware of: One Buffer implementation
may - as in this case - use another buffer. That may lead to the
second buffer being deleted/freed when the first one gets freed
- by timer or directly - what is wanted. This *needs* the methods
inside SystemDependentDataBuffer to do the actual potential
destruction of the Object outside the Mutex-Locked parts of
the code, else this might lead to a deadlock.
It is good to have found this, there may be other such
combinations in the future: That the implementation of a buffer
using that standard mechanism uses another class that may also be
buffered.
I have now made sure that buffered objects get deleted *outside*
the Mutex-Locked areas. This is better anyways, the destruction
does not need that lock - it is just needed so secure the
manipulation of the local unordered_map that organizes the
buffered elements globally.
Change-Id: Ie2f346a3f1a24105b89310ace3ff7a41472143c4
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/176795
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <armin.le.gr...@me.com>
Reviewed-by: Moritz Duge <moritz.d...@allotropia.de>
diff --git a/vcl/source/app/svdata.cxx b/vcl/source/app/svdata.cxx
index fa80f3dbf127..98cd3c6da16f 100644
--- a/vcl/source/app/svdata.cxx
+++ b/vcl/source/app/svdata.cxx
@@ -140,13 +140,26 @@ namespace
void endUsage(basegfx::SystemDependentData_SharedPtr& rData) override
{
- std::unique_lock aGuard(m_aMutex);
- EntryMap::iterator aFound(maEntries.find(rData));
+ // tdf#163428 prepare space for entry to hold to avoid early
deletion
+ basegfx::SystemDependentData_SharedPtr aToBeDeletedEntry;
- if(aFound != maEntries.end())
{
- maEntries.erase(aFound);
+ // lock m_aMutex in own scope
+ std::unique_lock aGuard(m_aMutex);
+ EntryMap::iterator aFound(maEntries.find(rData));
+
+ if(aFound != maEntries.end())
+ {
+ // tdf#163428 ensure to hold/not delete entry while
m_aMutex is locked
+ aToBeDeletedEntry = aFound->first;
+
+ // remove from list
+ maEntries.erase(aFound);
+ }
}
+
+ // tdf#163428 aToBeDeletedEntry will be destucted, thus the
+ // entry referenmced by SharedPtr may be deleted now
}
void touchUsage(basegfx::SystemDependentData_SharedPtr& rData) override
@@ -176,24 +189,55 @@ namespace
IMPL_LINK_NOARG(SystemDependentDataBuffer, implTimeoutHdl, Timer *, void)
{
- std::unique_lock aGuard(m_aMutex);
- EntryMap::iterator aIter(maEntries.begin());
+ // tdf#163428 prepare a temporary list for
SystemDependentData_SharedPtr
+ // entries that need to be removed from the unordered_map, but do not
need
+ // locking m_aMutex
+ ::std::vector<basegfx::SystemDependentData_SharedPtr>
aToBeDeletedEntries;
- while(aIter != maEntries.end())
{
- if(aIter->second)
- {
- aIter->second--;
- ++aIter;
- }
- else
+ // lock m_aMutex in own scope
+ std::unique_lock aGuard(m_aMutex);
+ EntryMap::iterator aIter(maEntries.begin());
+
+ while(aIter != maEntries.end())
{
- aIter = maEntries.erase(aIter);
+ if(aIter->second)
+ {
+ aIter->second--;
+ ++aIter;
+ }
+ else
+ {
+ // tdf#163428 Do not potentially directly delete
SystemDependentData_SharedPtr
+ // entries in the unordered_map (maEntries). That
can/would happen when the shared_ptr
+ // has only one reference left. This can cause problems,
e.g. we have
+ // BufferedData_ModifiedBitmapEx which can contain a
Bitmap with added
+ // DataBuffers itself, thus deleting this here *directly*
would trigger e.g.
+ // endUsage above. That would then block when we would
still hold m_aMutex.
+ // This can potentially be the case with other derivations
of
+ // basegfx::SystemDependentData, too.
+ // To avoid this, protect the part that needs protection
by the m_aMutex, that
+ // is the modification of the unordered_map itself.
Cleaning up the list entries can be
+ // done after this, without holding the lock.
+ // Thus, remember the SystemDependentData_SharedPtr in a
temporary list by adding
+ // a reference/shared_ptr to it to guarantee it gets not
deleted when it gets removed
+ // from the unordered_map.
+ // Anyways, only locking while manipulating the list is
better, destruction of
+ // objects may be expensive and hold m_aMutex longer than
necessary.
+ aToBeDeletedEntries.push_back(aIter->first);
+
+ // remove from list. This decrements the shared_ptr, but
delete is avoided
+ aIter = maEntries.erase(aIter);
+ }
}
+
+ if (maEntries.empty())
+ maTimer->Stop();
}
- if (maEntries.empty())
- maTimer->Stop();
+ // tdf#163428 here aToBeDeletedEntries will be destroyed, the entries
will be
+ // decremented and potentially deleted. These are of type
SystemDependentData_SharedPtr,
+ // so we do not need to do anything explicitely here
}
}
