fix ScPatternAttr lookup hashing (tdf#135215)

I made a mistake in 98d51bf8ce13bdac2d71f50f58d6d0ddb by using
SfxItemSet::Count(), which returns a number of existing items.
But EqualPatternSets() compares all items, even empty ones,
so I should have used TotalCount() (which is actually hardcoded
as a number in the code). Without this, the hash usually didn't
include all items, leading to matching hashes even though
the items clearly didn't match.

Change-Id: I1e25752d3ddca2b63c3c295a9a425965531cd4d3
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/130210
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Reviewed-by: Luboš Luňák <l.lunak@collabora.com>

2 files changed, 24 insertions, 3 deletions

diff --git a/sc/inc/patattr.hxx b/sc/inc/patattr.hxx
index c449190b2305..261cccc8cf00 100644
--- a/sc/inc/patattr.hxx
+++ b/sc/inc/patattr.hxx
@@ -145,6 +145,12 @@ public:
     void                    SetKey(sal_uInt64 nKey);
     sal_uInt64              GetKey() const;
 
+    // TODO: tdf#135215: This is a band-aid to detect changes and invalidate the hash,
+    // a proper way would be probably to override SfxItemSet::Changed(), but 6cb400f41df0dd10
+    // hardcoded SfxSetItem to contain SfxItemSet.
+    SfxItemSet& GetItemSet() { mxHashCode.reset(); return SfxSetItem::GetItemSet(); }
+    using SfxSetItem::GetItemSet;
+
 private:
     void                    CalcHashCode() const;
 };
diff --git a/sc/source/core/data/patattr.cxx b/sc/source/core/data/patattr.cxx
index 0414b957cada..aaab0183fa19 100644
--- a/sc/source/core/data/patattr.cxx
+++ b/sc/source/core/data/patattr.cxx
@@ -121,6 +121,8 @@ static bool StrCmp( const OUString* pStr1, const OUString* pStr2 )
     return *pStr1 == *pStr2;
 }
 
+constexpr size_t compareSize = ATTR_PATTERN_END - ATTR_PATTERN_START + 1;
+
 static bool EqualPatternSets( const SfxItemSet& rSet1, const SfxItemSet& rSet2 )
 {
     // #i62090# The SfxItemSet in the SfxSetItem base class always has the same ranges
@@ -130,10 +132,15 @@ static bool EqualPatternSets( const SfxItemSet& rSet1, const SfxItemSet& rSet2 )
     if ( rSet1.Count() != rSet2.Count() )
         return false;
 
+    // Actually test_tdf133629 from UITest_calc_tests9 somehow manages to have
+    // a different range (and I don't understand enough why), so better be safe and compare fully.
+    if( rSet1.TotalCount() != compareSize || rSet2.TotalCount() != compareSize )
+        return rSet1 == rSet2;
+
     SfxPoolItem const ** pItems1 = rSet1.GetItems_Impl();   // inline method of SfxItemSet
     SfxPoolItem const ** pItems2 = rSet2.GetItems_Impl();
 
-    return ( 0 == memcmp( pItems1, pItems2, (ATTR_PATTERN_END - ATTR_PATTERN_START + 1) * sizeof(pItems1[0]) ) );
+    return ( 0 == memcmp( pItems1, pItems2, compareSize * sizeof(pItems1[0]) ) );
 }
 
 bool ScPatternAttr::operator==( const SfxPoolItem& rCmp ) const
@@ -155,7 +162,7 @@ bool ScPatternAttr::operator==( const SfxPoolItem& rCmp ) const
 
 size_t ScPatternAttr::LookupHashCode() const
 {
-    if (!mxHashCode)
+    if (SAL_UNLIKELY(!mxHashCode))
         CalcHashCode();
     return *mxHashCode;
 }
@@ -1197,6 +1204,7 @@ void ScPatternAttr::SetStyleSheet( ScStyleSheet* pNewStyle, bool bClearDirectFor
         GetItemSet().SetParent(nullptr);
         pStyle = nullptr;
     }
+    mxHashCode.reset();
 }
 
 void ScPatternAttr::UpdateStyleSheet(const ScDocument& rDoc)
@@ -1222,6 +1230,7 @@ void ScPatternAttr::UpdateStyleSheet(const ScDocument& rDoc)
     }
     else
         pStyle = nullptr;
+    mxHashCode.reset();
 }
 
 void ScPatternAttr::StyleToName()
@@ -1233,6 +1242,7 @@ void ScPatternAttr::StyleToName()
         pName = pStyle->GetName();
         pStyle = nullptr;
         GetItemSet().SetParent( nullptr );
+        mxHashCode.reset();
     }
 }
 
@@ -1373,8 +1383,13 @@ sal_uInt64 ScPatternAttr::GetKey() const
 void ScPatternAttr::CalcHashCode() const
 {
     auto const & rSet = GetItemSet();
+    if( rSet.TotalCount() != compareSize ) // see EqualPatternSets()
+    {
+        mxHashCode = 0; // invalid
+        return;
+    }
     mxHashCode = 1; // Set up seed so that an empty pattern does not have an (invalid) hash of 0.
-    boost::hash_range(*mxHashCode, rSet.GetItems_Impl(), rSet.GetItems_Impl() + rSet.Count());
+    boost::hash_range(*mxHashCode, rSet.GetItems_Impl(), rSet.GetItems_Impl() + compareSize);
 }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */