tdf#125475 sw: update SwTextFrames when SwTextLine cache shrinks

SwCache::DeleteObj() may decide to shrink the cache, and then the
SwTextFrame::mnCacheIndex goes stale, because only
SwCacheObj::m_nCachePos is updated.

In this bugdoc, this can happen *inside* SwTextFrame::Format(), where
first it succeeds to find an existing SwTextLine, then some footnotes
anchored in this paragraph are moved around and formatted, creating new
SwTextLines, and SwCache::DeleteObj is called.  Later, any access of the
original frame's SwTextLine fails to find it and eventually some null
pointer crash happens.

Newly added SwTextLine::UpdateCachePos() requires that SwTextFrame lives
longer than its SwTextLine; there was another problem with that, because
SwTextFrame::FormatEmpty() was throwing away the mnCacheIndex, which
could then cause a second SwTextLine to be created for the same
SwTextFrame, and the first one is not deleted until it falls to the
bottom of the LRU list.

Apprently for this particular document the problem didn't happen before
commit 3d37463e and/or
commit 31ae7509 but that is mostly luck.

Reviewed-on: https://gerrit.libreoffice.org/73047
Tested-by: Jenkins
Reviewed-by: Michael Stahl <Michael.Stahl@cib.de>
(cherry picked from commit 1424d51a)

Change-Id: I7bef1b340a453d6dd44d51a1dc69ee5fd0b697db

sw/source/core/bastyp/swcache.cxx

@@ -299,6 +299,7 @@ void SwCache::DeleteObj( SwCacheObj *pObj )
         pObj->GetNext()->SetPrev( pObj->GetPrev() );
 
     m_aFreePositions.push_back( pObj->GetCachePos() );
+    assert(m_aCacheObjects[pObj->GetCachePos()].get() == pObj);
     m_aCacheObjects[pObj->GetCachePos()] = nullptr;
     delete pObj;
 
@@ -343,10 +344,18 @@ void SwCache::Delete( const void *pOwner )
         DeleteObj( pObj );
 }
 
-bool SwCache::Insert( SwCacheObj *pNew )
+bool SwCache::Insert(SwCacheObj *const pNew, bool const isDuplicateOwnerAllowed)
 {
     CHECK;
     OSL_ENSURE( !pNew->GetPrev() && !pNew->GetNext(), "New but not new." );
+    if (!isDuplicateOwnerAllowed)
+    {
+        for (auto const & rpObj : m_aCacheObjects)
+        {   // check owner doesn't have a cache object yet; required for SwTextLine
+            assert(!rpObj || rpObj->GetOwner() != pNew->GetOwner());
+            (void) rpObj;
+        }
+    }
 
     sal_uInt16 nPos;
     if ( m_aCacheObjects.size() < m_nCurMax )
@@ -377,7 +386,7 @@ bool SwCache::Insert( SwCacheObj *pNew )
         {
             SAL_WARN("sw.core", "SwCache overflow.");
             IncreaseMax(100); // embiggen & try again
-            return Insert(pNew);
+            return Insert(pNew, isDuplicateOwnerAllowed);
         }
 
         nPos = pObj->GetCachePos();
@@ -489,12 +498,12 @@ SwCacheAccess::~SwCacheAccess()
         m_pObj->Unlock();
 }
 
-void SwCacheAccess::Get_()
+void SwCacheAccess::Get_(bool const isDuplicateOwnerAllowed)
 {
     OSL_ENSURE( !m_pObj, "SwCacheAcces Obj already available." );
 
     m_pObj = NewObj();
-    if ( !m_rCache.Insert( m_pObj ) )
+    if (!m_rCache.Insert(m_pObj, isDuplicateOwnerAllowed))
     {
         delete m_pObj;
         m_pObj = nullptr;

sw/source/core/inc/swcache.hxx

@@ -101,7 +101,7 @@ public:
                      const bool bToTop = true );
     void ToTop( SwCacheObj *pObj );
 
-    bool Insert( SwCacheObj *pNew );
+    bool Insert(SwCacheObj *pNew, bool isDuplicateOwnerAllowed);
     void Delete(const void * pOwner, sal_uInt16 nIndex);
     void Delete( const void *pOwner );
 
@@ -146,7 +146,15 @@ class SwCacheObj
     void SetNext( SwCacheObj *pNew )  { m_pNext = pNew; }
     void SetPrev( SwCacheObj *pNew )  { m_pPrev = pNew; }
 
-    void   SetCachePos( const sal_uInt16 nNew ) { m_nCachePos = nNew; }
+    void SetCachePos(const sal_uInt16 nNew)
+    {
+        if (m_nCachePos != nNew)
+        {
+            m_nCachePos = nNew;
+            UpdateCachePos();
+        }
+    }
+    virtual void UpdateCachePos() { }
 
 protected:
     const void *m_pOwner;
@@ -187,7 +195,7 @@ class SwCacheAccess
 {
     SwCache &m_rCache;
 
-    void Get_();
+    void Get_(bool isDuplicateOwnerAllowed);
 
 protected:
     SwCacheObj *m_pObj;
@@ -195,7 +203,7 @@ protected:
 
     virtual SwCacheObj *NewObj() = 0;
 
-    inline SwCacheObj *Get();
+    inline SwCacheObj *Get(bool isDuplicateOwnerAllowed);
 
     inline SwCacheAccess( SwCache &rCache, const void *pOwner, bool bSeek );
     inline SwCacheAccess( SwCache &rCache, const void *pOwner, const sal_uInt16 nIndex );
@@ -241,10 +249,10 @@ inline SwCacheAccess::SwCacheAccess( SwCache &rC, const void *pOwn,
         m_pObj->Lock();
 }
 
-inline SwCacheObj *SwCacheAccess::Get()
+inline SwCacheObj *SwCacheAccess::Get(bool const isDuplicateOwnerAllowed = true)
 {
     if ( !m_pObj )
-        Get_();
+        Get_(isDuplicateOwnerAllowed);
     return m_pObj;
 }
 

sw/source/core/inc/txtfrm.hxx

@@ -567,8 +567,10 @@ public:
     sal_uInt16 GetCacheIdx() const { return mnCacheIndex; }
     void   SetCacheIdx( const sal_uInt16 nNew ) { mnCacheIndex = nNew; }
 
-    /// Removes the Line information from the Cache
+    /// Removes the Line information from the Cache but retains the entry itself
     void ClearPara();
+    /// Removes this frame completely from the Cache
+    void RemoveFromCache();
 
     /// Am I a FootnoteFrame, with a number at the start of the paragraph?
     bool IsFootnoteNumFrame() const

sw/source/core/text/porrst.cxx

@@ -331,7 +331,7 @@ bool SwTextFrame::FormatEmpty()
         ClearPara();
         ResetBlinkPor();
     }
-    SetCacheIdx( USHRT_MAX );
+    RemoveFromCache();
     if( !IsEmpty() )
     {
         SetEmpty( true );

sw/source/core/text/txtcache.cxx

@@ -34,6 +34,13 @@ SwTextLine::~SwTextLine()
 {
 }
 
+void SwTextLine::UpdateCachePos()
+{
+    // note: SwTextFrame lives longer than its SwTextLine, see ~SwTextFrame
+    assert(m_pOwner);
+    const_cast<SwTextFrame *>(static_cast<SwTextFrame const *>(m_pOwner))->SetCacheIdx(GetCachePos());
+}
+
 SwCacheObj *SwTextLineAccess::NewObj()
 {
     return new SwTextLine( static_cast<SwTextFrame const *>(m_pOwner) );
@@ -46,7 +53,7 @@ SwParaPortion *SwTextLineAccess::GetPara()
         pRet = static_cast<SwTextLine*>(m_pObj);
     else
     {
-        pRet = static_cast<SwTextLine*>(Get());
+        pRet = static_cast<SwTextLine*>(Get(false));
         const_cast<SwTextFrame *>(static_cast<SwTextFrame const *>(m_pOwner))->SetCacheIdx( pRet->GetCachePos() );
     }
     if ( !pRet->GetPara() )
@@ -109,6 +116,15 @@ void SwTextFrame::ClearPara()
     }
 }
 
+void SwTextFrame::RemoveFromCache()
+{
+    if (GetCacheIdx() != USHRT_MAX)
+    {
+        s_pTextCache->Delete(this, GetCacheIdx());
+        SetCacheIdx(USHRT_MAX);
+    }
+}
+
 void SwTextFrame::SetPara( SwParaPortion *pNew, bool bDelete )
 {
     if ( GetCacheIdx() != USHRT_MAX )
@@ -129,7 +145,7 @@ void SwTextFrame::SetPara( SwParaPortion *pNew, bool bDelete )
     else if ( pNew )
     {   // Insert a new one
         SwTextLine *pTextLine = new SwTextLine( this, pNew );
-        if ( SwTextFrame::GetTextCache()->Insert( pTextLine ) )
+        if (SwTextFrame::GetTextCache()->Insert(pTextLine, false))
             mnCacheIndex = pTextLine->GetCachePos();
         else
         {

sw/source/core/text/txtcache.hxx

@@ -31,6 +31,8 @@ class SwTextLine : public SwCacheObj
 {
     std::unique_ptr<SwParaPortion> pLine;
 
+    virtual void UpdateCachePos() override;
+
 public:
     DECL_FIXEDMEMPOOL_NEWDEL(SwTextLine)
 

sw/source/core/text/txtfrm.cxx

@@ -693,10 +693,7 @@ void SwTextFrame::DestroyImpl()
 
 SwTextFrame::~SwTextFrame()
 {
-    if (GetCacheIdx() != USHRT_MAX)
-    {
-        s_pTextCache->Delete(this, GetCacheIdx());
-    }
+    RemoveFromCache();
 }
 
 namespace sw {