Revert "Move SwFntCache link from SwModify down to SwFormat"

This reverts commit 8dd78873a9de028c0d9f1f1aee537e85f74d2300, as it caused
heap-use-after-free during CppunitTest_sw_uiwriter:

> ==864890==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000e6a89c at pc 0x7f92775c8dd9 bp 0x7ffeb01b18d0 sp 0x7ffeb01b18c8
> READ of size 2 at 0x604000e6a89c thread T0
>  #0 in SfxPoolItem::Which() const at include/svl/poolitem.hxx:151:53
>  #1 in SwAttrHandler::FontChg(SfxPoolItem const&, SwFont&, bool) at sw/source/core/text/atrstck.cxx:571:20
>  #2 in SwAttrHandler::ActivateTop(SwFont&, unsigned short) at sw/source/core/text/atrstck.cxx:515:9
>  #3 in SwAttrHandler::PopAndChg(SwTextAttr const&, SwFont&) at sw/source/core/text/atrstck.cxx:450:17
>  #4 in SwAttrIter::Rst(SwTextAttr const*) at sw/source/core/text/itratr.cxx:113:24
>  #5 in SwAttrIter::SeekFwd(int, int) at sw/source/core/text/itratr.cxx:275:52
>  #6 in SwAttrIter::Seek(o3tl::strong_int<int, Tag_TextFrameIndex>) at sw/source/core/text/itratr.cxx:418:13
>  #7 in SwAttrIter::SeekAndChgAttrIter(o3tl::strong_int<int, Tag_TextFrameIndex>, OutputDevice*) at sw/source/core/text/itratr.cxx:158:11
>  #8 in SwTextIter::SeekAndChg(SwTextSizeInfo&) at sw/source/core/text/itrtxt.hxx:312:12
>  #9 in SwTextFormatter::CalcAscent(SwTextFormatInfo&, SwLinePortion*) at sw/source/core/text/itrform2.cxx:815:24
>  #10 in SwTextFormatter::NewPortion(SwTextFormatInfo&) at sw/source/core/text/itrform2.cxx:1537:9
>  #11 in SwTextFormatter::BuildPortions(SwTextFormatInfo&) at sw/source/core/text/itrform2.cxx:707:16
[...]
> 0x604000e6a89c is located 12 bytes inside of 48-byte region [0x604000e6a890,0x604000e6a8c0)
> freed by thread T0 here:
>  #0 in operator delete(void*, unsigned long) at ~/llvm/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:172:3
>  #1 in SvxFontItem::~SvxFontItem() at include/editeng/fontitem.hxx:29:25
>  #2 in SfxItemPool::SetPoolDefaultItem(SfxPoolItem const&) at svl/source/items/itempool.cxx:543:13
>  #3 in SwDoc::SetDefault(SfxItemSet const&) at sw/source/core/doc/docfmt.cxx:550:23
>  #4 in SwDoc::SetDefault(SfxPoolItem const&) at sw/source/core/doc/docfmt.cxx:531:5
>  #5 in SwXTextDefaults::setPropertyValue(rtl::OUString const&, com::sun::star::uno::Any const&) at sw/source/core/unocore/SwXTextDefaults.cxx:118:17
>  #6 in writerfilter::dmapper::DomainMapper::DomainMapper(com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> const&, com::sun::star::uno::Reference<com::sun::star::io::XInputStream> const&, com::sun::star::uno::Reference<com::sun::star::lang::XComponent> const&, bool, writerfilter::dmapper::SourceDocumentType, utl::MediaDescriptor const&) at writerfilter/source/dmapper/DomainMapper.cxx:161:24
>  #7 in writerfilter::dmapper::DomainMapperFactory::createMapper(com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> const&, com::sun::star::uno::Reference<com::sun::star::io::XInputStream> const&, com::sun::star::uno::Reference<com::sun::star::lang::XComponent> const&, bool, writerfilter::dmapper::SourceDocumentType, utl::MediaDescriptor const&) at writerfilter/source/dmapper/domainmapperfactory.cxx:33:34
>  #8 in (anonymous namespace)::WriterFilter::filter(com::sun::star::uno::Sequence<com::sun::star::beans::PropertyValue> const&) at writerfilter/source/filter/WriterFilter.cxx:185:13
>  #9 in SwDOCXReader::Read(SwDoc&, rtl::OUString const&, SwPaM&, rtl::OUString const&) at sw/source/filter/docx/swdocxreader.cxx:86:18
>  #10 in SwReader::Read(Reader const&) at sw/source/filter/basflt/shellio.cxx:191:22
>  #11 in SwView::InsertMedium(unsigned short, std::unique_ptr<SfxMedium, std::default_delete<SfxMedium> >, short) at sw/source/uibase/uiview/view2.cxx:2309:40
[...]
> previously allocated by thread T0 here:
>  #0 in operator new(unsigned long) at ~/llvm/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:99:3
>  #1 in SvxFontItem::Clone(SfxItemPool*) const at editeng/source/items/textitem.cxx:297:12
>  #2 in SfxItemPool::SetPoolDefaultItem(SfxPoolItem const&) at svl/source/items/itempool.cxx:538:42
>  #3 in SwDoc::SetDefault(SfxItemSet const&) at sw/source/core/doc/docfmt.cxx:550:23
>  #4 in SwDoc::SetDefault(SfxPoolItem const&) at sw/source/core/doc/docfmt.cxx:531:5
>  #5 in SwXTextDefaults::setPropertyValue(rtl::OUString const&, com::sun::star::uno::Any const&) at sw/source/core/unocore/SwXTextDefaults.cxx:118:17
>  #6 in SvXMLImportPropertyMapper::FillPropertySet_(std::__debug::vector<XMLPropertyState, std::allocator<XMLPropertyState> > const&, com::sun::star::uno::Reference<com::sun::star::beans::XPropertySet> const&, com::sun::star::uno::Reference<com::sun::star::beans::XPropertySetInfo> const&, rtl::Reference<XMLPropertySetMapper> const&, SvXMLImport&, ContextID_Index_Pair*) at xmloff/source/style/xmlimppr.cxx:509:27
>  #7 in SvXMLImportPropertyMapper::FillPropertySet(std::__debug::vector<XMLPropertyState, std::allocator<XMLPropertyState> > const&, com::sun::star::uno::Reference<com::sun::star::beans::XPropertySet> const&, ContextID_Index_Pair*) const at xmloff/source/style/xmlimppr.cxx:466:20
>  #8 in XMLTextStyleContext::FillPropertySet(com::sun::star::uno::Reference<com::sun::star::beans::XPropertySet> const&) at xmloff/source/text/txtstyli.cxx:456:20
>  #9 in XMLTextStyleContext::SetDefaults() at xmloff/source/text/txtstyli.cxx:234:17
>  #10 in SvXMLStylesContext::CopyStylesToDoc(bool, bool) at xmloff/source/style/xmlstyle.cxx:752:37
>  #11 in SwXMLImport::InsertStyles(bool) at sw/source/filter/xml/xmlfmt.cxx:999:22
[...]

Change-Id: I4df8db29054da3eb543e5524fec6cb79e8568b66
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/109363
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

6 files changed, 49 insertions, 62 deletions

diff --git a/sw/inc/calbck.hxx b/sw/inc/calbck.hxx
index b5b6ff9a3e30..31df9791291b 100644
--- a/sw/inc/calbck.hxx
+++ b/sw/inc/calbck.hxx
@@ -178,6 +178,7 @@ class SW_DLLPUBLIC SwModify: public SwClient
     sw::WriterListener* m_pWriterListeners;                // the start of the linked list of clients
     bool m_bModifyLocked : 1;         // don't broadcast changes now
     bool m_bInCache   : 1;
+    bool m_bInSwFntCache : 1;
 
     SwModify(SwModify const &) = delete;
     SwModify &operator =(const SwModify&) = delete;
@@ -185,7 +186,7 @@ protected:
     virtual void SwClientNotify(const SwModify&, const SfxHint& rHint) override;
 public:
     SwModify()
-        : SwClient(), m_pWriterListeners(nullptr), m_bModifyLocked(false), m_bInCache(false)
+        : SwClient(), m_pWriterListeners(nullptr), m_bModifyLocked(false), m_bInCache(false), m_bInSwFntCache(false)
     {}
 
     // broadcasting mechanism
@@ -203,9 +204,11 @@ public:
     void LockModify()                   { m_bModifyLocked = true;  }
     void UnlockModify()                 { m_bModifyLocked = false; }
     void SetInCache( bool bNew )        { m_bInCache = bNew;       }
+    void SetInSwFntCache( bool bNew )   { m_bInSwFntCache = bNew;  }
     void SetInDocDTOR();
     bool IsModifyLocked() const     { return m_bModifyLocked;  }
     bool IsInCache()      const     { return m_bInCache;       }
+    bool IsInSwFntCache() const     { return m_bInSwFntCache;  }
 
     void CheckCaching( const sal_uInt16 nWhich );
     bool HasOnlyOneListener() const { return m_pWriterListeners && m_pWriterListeners->IsLast(); }
diff --git a/sw/inc/format.hxx b/sw/inc/format.hxx
index 96e03b342eec..e596a26bb882 100644
--- a/sw/inc/format.hxx
+++ b/sw/inc/format.hxx
@@ -22,7 +22,6 @@
 #include "swdllapi.h"
 #include "swatrset.hxx"
 #include "calbck.hxx"
-#include "hintids.hxx"
 #include <memory>
 
 class IDocumentSettingAccess;
@@ -60,25 +59,7 @@ class SW_DLLPUBLIC SwFormat : public sw::BroadcastingModify
     bool   m_bAutoUpdateFormat : 1;/**< TRUE: Set attributes of a whole paragraph
                                        at format (UI-side!). */
     bool m_bHidden : 1;
-    bool m_bInSwFntCache : 1;
     std::shared_ptr<SfxGrabBagItem> m_pGrabBagItem; ///< Style InteropGrabBag.
-    void InvalidateInSwFntCache(sal_uInt16 nWhich)
-    {
-        if(isCHRATR(nWhich))
-        {
-            m_bInSwFntCache = false;
-        }
-        else
-        {
-            switch(nWhich)
-            {
-                case RES_OBJECTDYING:
-                case RES_FMT_CHG:
-                case RES_ATTRSET_CHG:
-                    m_bInSwFntCache = false;
-            }
-        }
-    };
 
 protected:
     SwFormat( SwAttrPool& rPool, const char* pFormatNm,
@@ -94,9 +75,7 @@ public:
     SwFormat &operator=(const SwFormat&);
 
     /// for Querying of Writer-functions.
-    sal_uInt16 Which() const { return m_nWhichId; };
-    bool IsInSwFntCache() const { return m_bInSwFntCache; };
-    void SetInSwFntCache() { m_bInSwFntCache = true; };
+    sal_uInt16 Which() const { return m_nWhichId; }
 
     /// Copy attributes even among documents.
     void CopyAttrs( const SwFormat& );
diff --git a/sw/source/core/attr/calbck.cxx b/sw/source/core/attr/calbck.cxx
index ef6008f3ce77..1c86c75fc992 100644
--- a/sw/source/core/attr/calbck.cxx
+++ b/sw/source/core/attr/calbck.cxx
@@ -17,17 +17,15 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
-#include <algorithm>
-
-#include <format.hxx>
 #include <frame.hxx>
+#include <format.hxx>
 #include <hintids.hxx>
 #include <hints.hxx>
-#include <osl/diagnose.h>
-#include <sal/log.hxx>
 #include <swcache.hxx>
+#include <swfntcch.hxx>
 #include <tools/debug.hxx>
-
+#include <sal/log.hxx>
+#include <algorithm>
 #ifdef DBG_UTIL
 #include <sal/backtrace.hxx>
 #endif
@@ -158,6 +156,9 @@ SwModify::~SwModify()
     if ( IsInCache() )
         SwFrame::GetCache().Delete( this );
 
+    if ( IsInSwFntCache() )
+        pSwFontCache->Delete( this );
+
     // notify all clients that they shall remove themselves
     SwPtrMsgPoolItem aDyObject( RES_OBJECTDYING, this );
     SwModify::SwClientNotify(*this, sw::LegacyModifyHint(&aDyObject, &aDyObject));
@@ -273,13 +274,21 @@ SwClient* SwModify::Remove( SwClient* pDepend )
     return pDepend;
 }
 
-void SwModify::CheckCaching(const sal_uInt16 nWhich)
+void SwModify::CheckCaching( const sal_uInt16 nWhich )
 {
-    switch(nWhich)
+    if( isCHRATR( nWhich ) )
     {
+        SetInSwFntCache( false );
+    }
+    else
+    {
+        switch( nWhich )
+        {
         case RES_OBJECTDYING:
         case RES_FMT_CHG:
         case RES_ATTRSET_CHG:
+            SetInSwFntCache( false );
+            [[fallthrough]];
         case RES_UL_SPACE:
         case RES_LR_SPACE:
         case RES_BOX:
@@ -287,11 +296,13 @@ void SwModify::CheckCaching(const sal_uInt16 nWhich)
         case RES_FRM_SIZE:
         case RES_KEEP:
         case RES_BREAK:
-            if(IsInCache())
+            if( IsInCache() )
             {
-                SwFrame::GetCache().Delete(this);
-                SetInCache(false);
+                SwFrame::GetCache().Delete( this );
+                SetInCache( false );
             }
+            break;
+        }
     }
 }
 
@@ -341,7 +352,7 @@ void SwModify::SwClientNotify(const SwModify&, const SfxHint& rHint)
     if(auto pLegacyHint = dynamic_cast<const sw::LegacyModifyHint*>(&rHint))
     {
         DBG_TESTSOLARMUTEX();
-        if(IsInCache())
+        if(IsInCache() || IsInSwFntCache())
             CheckCaching(pLegacyHint->GetWhich());
         if(IsModifyLocked())
             return;
diff --git a/sw/source/core/attr/format.cxx b/sw/source/core/attr/format.cxx
index 8dd42c7fc935..6e0f55ef8da0 100644
--- a/sw/source/core/attr/format.cxx
+++ b/sw/source/core/attr/format.cxx
@@ -17,23 +17,22 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include <doc.hxx>
 #include <DocumentSettingManager.hxx> //For SwFmt::getIDocumentSettingAccess()
 #include <IDocumentTimerAccess.hxx>
-#include <doc.hxx>
 #include <fmtcolfunc.hxx>
-#include <format.hxx>
 #include <frame.hxx>
-#include <frmatr.hxx>
+#include <format.hxx>
 #include <hintids.hxx>
 #include <hints.hxx>
+#include <swcache.hxx>
+#include <frmatr.hxx>
 #include <osl/diagnose.h>
-#include <sal/log.hxx>
 #include <svl/grabbagitem.hxx>
 #include <svx/sdr/attribute/sdrallfillattributeshelper.hxx>
 #include <svx/unobrushitemhelper.hxx>
 #include <svx/xdef.hxx>
-#include <swcache.hxx>
-#include <swfntcch.hxx>
+#include <sal/log.hxx>
 
 using namespace com::sun::star;
 
@@ -50,7 +49,7 @@ SwFormat::SwFormat( SwAttrPool& rPool, const char* pFormatNm,
 {
     m_bAutoUpdateFormat = false; // LAYER_IMPL
     m_bAutoFormat = true;
-    m_bFormatInDTOR = m_bHidden = m_bInSwFntCache = false;
+    m_bFormatInDTOR = m_bHidden = false;
 
     if( pDrvdFrame )
     {
@@ -71,7 +70,7 @@ SwFormat::SwFormat( SwAttrPool& rPool, const OUString& rFormatNm,
 {
     m_bAutoUpdateFormat = false; // LAYER_IMPL
     m_bAutoFormat = true;
-    m_bFormatInDTOR = m_bHidden = m_bInSwFntCache = false;
+    m_bFormatInDTOR = m_bHidden = false;
 
     if( pDrvdFrame )
     {
@@ -91,7 +90,6 @@ SwFormat::SwFormat( const SwFormat& rFormat ) :
     m_bFormatInDTOR = false; // LAYER_IMPL
     m_bAutoFormat = rFormat.m_bAutoFormat;
     m_bHidden = rFormat.m_bHidden;
-    m_bInSwFntCache = false;
     m_bAutoUpdateFormat = rFormat.m_bAutoUpdateFormat;
 
     if( auto pDerived = rFormat.DerivedFrom() )
@@ -118,7 +116,7 @@ SwFormat &SwFormat::operator=(const SwFormat& rFormat)
         SwFrame::GetCache().Delete( this );
         SetInCache( false );
     }
-    m_bInSwFntCache = false;
+    SetInSwFntCache( false );
 
     // copy only array with attributes delta
     SwAttrSet aOld( *m_aSet.GetPool(), m_aSet.GetRanges() ),
@@ -185,7 +183,7 @@ void SwFormat::CopyAttrs( const SwFormat& rFormat )
         SwFrame::GetCache().Delete( this );
         SetInCache( false );
     }
-    m_bInSwFntCache = false;
+    SetInSwFntCache( false );
 
     // special treatments for some attributes
     SwAttrSet* pChgSet = const_cast<SwAttrSet*>(&rFormat.m_aSet);
@@ -232,8 +230,6 @@ SwFormat::~SwFormat()
     for(SwClient* pClient = aIter.First(); pClient; pClient = aIter.Next())
         pClient->CheckRegistrationFormat(*this);
     assert(!HasWriterListeners());
-    if(m_bInSwFntCache)
-        pSwFontCache->Delete( this );
 }
 
 void SwFormat::SwClientNotify(const SwModify&, const SfxHint& rHint)
@@ -361,7 +357,7 @@ bool SwFormat::SetDerivedFrom(SwFormat *pDerFrom)
         SwFrame::GetCache().Delete( this );
         SetInCache( false );
     }
-    m_bInSwFntCache = false;
+    SetInSwFntCache( false );
 
     pDerFrom->Add( this );
     m_aSet.SetParent( &pDerFrom->m_aSet );
@@ -462,9 +458,11 @@ SfxItemState SwFormat::GetBackgroundState(std::unique_ptr<SvxBrushItem>& rItem)
 
 bool SwFormat::SetFormatAttr( const SfxPoolItem& rAttr )
 {
-    const sal_uInt16 nWhich = rAttr.Which();
-    CheckCaching( nWhich );
-    InvalidateInSwFntCache( nWhich );
+    if ( IsInCache() || IsInSwFntCache() )
+    {
+        const sal_uInt16 nWhich = rAttr.Which();
+        CheckCaching( nWhich );
+    }
 
     bool bRet = false;
 
@@ -549,7 +547,7 @@ bool SwFormat::SetFormatAttr( const SfxItemSet& rSet )
         SwFrame::GetCache().Delete( this );
         SetInCache( false );
     }
-    m_bInSwFntCache = false;
+    SetInSwFntCache( false );
 
     bool bRet = false;
 
@@ -646,16 +644,11 @@ bool SwFormat::ResetFormatAttr( sal_uInt16 nWhich1, sal_uInt16 nWhich2 )
     if( !nWhich2 || nWhich2 < nWhich1 )
         nWhich2 = nWhich1; // then set to 1st ID, only this item
 
-    if ( IsInCache() )
+    if ( IsInCache() || IsInSwFntCache() )
     {
         for( sal_uInt16 n = nWhich1; n < nWhich2; ++n )
             CheckCaching( n );
     }
-    if( m_bInSwFntCache )
-    {
-        for( sal_uInt16 n = nWhich1; n < nWhich2; ++n )
-            InvalidateInSwFntCache( n );
-    }
 
     // if Modify is locked then no modifications will be sent
     if( IsModifyLocked() )
@@ -682,7 +675,7 @@ sal_uInt16 SwFormat::ResetAllFormatAttr()
         SwFrame::GetCache().Delete( this );
         SetInCache( false );
     }
-    m_bInSwFntCache = false;
+    SetInSwFntCache( false );
 
     // if Modify is locked then no modifications will be sent
     if( IsModifyLocked() )
@@ -706,7 +699,7 @@ void SwFormat::DelDiffs( const SfxItemSet& rSet )
         SwFrame::GetCache().Delete( this );
         SetInCache( false );
     }
-    m_bInSwFntCache = false;
+    SetInSwFntCache( false );
 
     // if Modify is locked then no modifications will be sent
     if( IsModifyLocked() )
diff --git a/sw/source/core/txtnode/ndtxt.cxx b/sw/source/core/txtnode/ndtxt.cxx
index 64a7f2a5da85..99331779c4c5 100644
--- a/sw/source/core/txtnode/ndtxt.cxx
+++ b/sw/source/core/txtnode/ndtxt.cxx
@@ -2832,6 +2832,7 @@ void SwTextNode::NumRuleChgd()
         SwFrame::GetCache().Delete( this );
         SetInCache( false );
     }
+    SetInSwFntCache( false );
 
     // Sending "noop" modify in order to cause invalidations of registered
     // <SwTextFrame> instances to get the list style change respectively the change
diff --git a/sw/source/core/txtnode/swfntcch.cxx b/sw/source/core/txtnode/swfntcch.cxx
index 100a0e7a7965..49783fd25942 100644
--- a/sw/source/core/txtnode/swfntcch.cxx
+++ b/sw/source/core/txtnode/swfntcch.cxx
@@ -60,7 +60,7 @@ SwFontObj *SwFontAccess::Get( )
 
 SwCacheObj *SwFontAccess::NewObj( )
 {
-    const_cast<SwTextFormatColl*>(static_cast<const SwTextFormatColl*>(m_pOwner))->SetInSwFntCache();
+    const_cast<SwTextFormatColl*>(static_cast<const SwTextFormatColl*>(m_pOwner))->SetInSwFntCache( true );
     return new SwFontObj( m_pOwner, m_pShell );
 }