[Libreoffice-commits] core.git: sc/inc sc/source
Libreoffice Gerrit user
logerrit at kemper.freedesktop.org
Mon Oct 22 19:50:49 UTC 2018
sc/inc/document.hxx | 2 --
sc/inc/lookupcache.hxx | 6 +++++-
sc/source/core/data/documen2.cxx | 26 +++++++-------------------
sc/source/core/tool/lookupcache.cxx | 5 +++--
4 files changed, 15 insertions(+), 24 deletions(-)
New commits:
commit e2e9e617c634d143cd193c223ea5dd73570ade3c
Author: Stephan Bergmann <sbergman at redhat.com>
AuthorDate: Mon Oct 22 20:12:36 2018 +0200
Commit: Stephan Bergmann <sbergman at redhat.com>
CommitDate: Mon Oct 22 21:50:26 2018 +0200
Remove ScLookupCache from ScLookupCacheMap it had been added to
...instead of removing an arbitrary ScLookupCache with a matching ScRange from
the first ScLookupCacheMap that happens to contain one.
79449d73900d7a9bf061244d76f5f8eecc441198 "make VLOOKUP in Calc thread-safe"
introduced per-thread ScLookupCacheMaps, so that multiple ScLookupCacheMaps can
contain ScLookupCaches with identical ScRanges. For example, UITest_calc_tests6
adds ScLookupCaches for ScRange 1!R2C18:R97C18 to different threads'
ScLookupCacheMaps. That causes confusion so that calling
ScDocument::RemoveLookupCacheHelper to remove an ScLookupCache from a
mismatching ScLookupCacheMap accesses a different
ScLookupCache* pCache = (*it).second.release();
that may already have been destroyed; see failing ASan/UBSan builds like
<https://ci.libreoffice.org//job/lo_ubsan/1067/>.
Change-Id: I70c33b236dc502b8a98e0e313d422424eec5dbca
Reviewed-on: https://gerrit.libreoffice.org/62194
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman at redhat.com>
diff --git a/sc/inc/document.hxx b/sc/inc/document.hxx
index bf8790c6537d..6fe442954901 100644
--- a/sc/inc/document.hxx
+++ b/sc/inc/document.hxx
@@ -2495,8 +2495,6 @@ private:
void EndListeningGroups( const std::vector<ScAddress>& rPosArray );
void SetNeedsListeningGroups( const std::vector<ScAddress>& rPosArray );
-
- bool RemoveLookupCacheHelper( ScLookupCacheMap* cacheMap, ScLookupCache& rCache );
};
typedef std::unique_ptr<ScDocument, o3tl::default_delete<ScDocument>> ScDocumentUniquePtr;
diff --git a/sc/inc/lookupcache.hxx b/sc/inc/lookupcache.hxx
index cca7a52ad38c..3a2be452a2ee 100644
--- a/sc/inc/lookupcache.hxx
+++ b/sc/inc/lookupcache.hxx
@@ -27,6 +27,7 @@
#include <unordered_map>
class ScDocument;
+struct ScLookupCacheMap;
struct ScQueryEntry;
/** Lookup cache for one range used with interpreter functions such as VLOOKUP
@@ -106,7 +107,7 @@ public:
};
/// MUST be new'd because Notify() deletes.
- ScLookupCache( ScDocument * pDoc, const ScRange & rRange );
+ ScLookupCache( ScDocument * pDoc, const ScRange & rRange, ScLookupCacheMap & cacheMap );
virtual ~ScLookupCache() override;
/// Remove from document structure and delete (!) cache on modify hint.
virtual void Notify( const SfxHint& rHint ) override;
@@ -129,6 +130,8 @@ public:
const ScRange& getRange() const { return maRange; }
+ ScLookupCacheMap & getCacheMap() const { return mCacheMap; }
+
struct Hash
{
size_t operator()( const ScRange & rRange ) const
@@ -184,6 +187,7 @@ private:
std::unordered_map< QueryKey, QueryCriteriaAndResult, QueryKey::Hash > maQueryMap;
ScRange const maRange;
ScDocument * mpDoc;
+ ScLookupCacheMap & mCacheMap;
ScLookupCache( const ScLookupCache & ) = delete;
ScLookupCache & operator=( const ScLookupCache & ) = delete;
diff --git a/sc/source/core/data/documen2.cxx b/sc/source/core/data/documen2.cxx
index f2c317026f8e..32b3f15b6a35 100644
--- a/sc/source/core/data/documen2.cxx
+++ b/sc/source/core/data/documen2.cxx
@@ -1151,7 +1151,7 @@ ScLookupCache & ScDocument::GetLookupCache( const ScRange & rRange, ScInterprete
if (findIt == rpCacheMap->aCacheMap.end())
{
auto insertIt = rpCacheMap->aCacheMap.emplace_hint(findIt,
- rRange, o3tl::make_unique<ScLookupCache>(this, rRange) );
+ rRange, o3tl::make_unique<ScLookupCache>(this, rRange, *rpCacheMap) );
pCache = insertIt->second.get();
// The StartListeningArea() call is not thread-safe, as all threads
// would access the same SvtBroadcaster.
@@ -1170,29 +1170,17 @@ void ScDocument::RemoveLookupCache( ScLookupCache & rCache )
// a result of user input or recalc). If it turns out this can be the case, locking is needed
// here and also in ScLookupCache::Notify().
assert(!IsThreadedGroupCalcInProgress());
- if( RemoveLookupCacheHelper( GetNonThreadedContext().mScLookupCache, rCache ))
- return;
- // The cache may be possibly in the caches stored for other threads.
- for( ScLookupCacheMap* cacheMap : mThreadStoredScLookupCaches )
- if( RemoveLookupCacheHelper( cacheMap, rCache ))
- return;
- OSL_FAIL( "ScDocument::RemoveLookupCache: range not found in hash map");
-}
-
-bool ScDocument::RemoveLookupCacheHelper( ScLookupCacheMap* cacheMap, ScLookupCache& rCache )
-{
- if( cacheMap == nullptr )
- return false;
- auto it(cacheMap->aCacheMap.find(rCache.getRange()));
- if (it != cacheMap->aCacheMap.end())
+ auto & cacheMap = rCache.getCacheMap();
+ auto it(cacheMap.aCacheMap.find(rCache.getRange()));
+ if (it != cacheMap.aCacheMap.end())
{
ScLookupCache* pCache = (*it).second.release();
- cacheMap->aCacheMap.erase(it);
+ cacheMap.aCacheMap.erase(it);
assert(!IsThreadedGroupCalcInProgress()); // EndListeningArea() is not thread-safe
EndListeningArea(pCache->getRange(), false, &rCache);
- return true;
+ return;
}
- return false;
+ OSL_FAIL( "ScDocument::RemoveLookupCache: range not found in hash map");
}
void ScDocument::ClearLookupCaches()
diff --git a/sc/source/core/tool/lookupcache.cxx b/sc/source/core/tool/lookupcache.cxx
index fa6fbc0a1693..9522b46678fe 100644
--- a/sc/source/core/tool/lookupcache.cxx
+++ b/sc/source/core/tool/lookupcache.cxx
@@ -68,9 +68,10 @@ ScLookupCache::QueryCriteria::~QueryCriteria()
deleteString();
}
-ScLookupCache::ScLookupCache( ScDocument * pDoc, const ScRange & rRange ) :
+ScLookupCache::ScLookupCache( ScDocument * pDoc, const ScRange & rRange, ScLookupCacheMap & cacheMap ) :
maRange( rRange),
- mpDoc( pDoc)
+ mpDoc( pDoc),
+ mCacheMap(cacheMap)
{
}
More information about the Libreoffice-commits
mailing list