summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMichael Stahl <Michael.Stahl@cib.de>2019-05-27 18:55:31 +0200
committerMichael Stahl <Michael.Stahl@cib.de>2019-05-28 15:26:29 +0200
commit774a0185fb470c55ff5ab9e4a4a7c31dff14cf0b (patch)
tree4f743da2ad2d07351a35061de0cd97e15bde3624
parentf2d963236d7f44e935ba30e5ac2f26564f03e10d (diff)
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 3d37463eec0c891243a8971a34903b2da01c9e24 and/or commit 31ae7509003b1e650463ee1468c0b315ba13efe6 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 1424d51a44ed2fa8b37b2d132af8a608a170ec8e) Change-Id: I7bef1b340a453d6dd44d51a1dc69ee5fd0b697db
-rw-r--r--sw/source/core/bastyp/swcache.cxx17
-rw-r--r--sw/source/core/inc/swcache.hxx20
-rw-r--r--sw/source/core/inc/txtfrm.hxx4
-rw-r--r--sw/source/core/text/porrst.cxx2
-rw-r--r--sw/source/core/text/txtcache.cxx20
-rw-r--r--sw/source/core/text/txtcache.hxx2
-rw-r--r--sw/source/core/text/txtfrm.cxx5
7 files changed, 52 insertions, 18 deletions
diff --git a/sw/source/core/bastyp/swcache.cxx b/sw/source/core/bastyp/swcache.cxx
index e90f89b87612..69f5bc91d586 100644
--- a/sw/source/core/bastyp/swcache.cxx
+++ b/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;
diff --git a/sw/source/core/inc/swcache.hxx b/sw/source/core/inc/swcache.hxx
index 2cc6138cb2b1..af6c5a45a1d2 100644
--- a/sw/source/core/inc/swcache.hxx
+++ b/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;
}
diff --git a/sw/source/core/inc/txtfrm.hxx b/sw/source/core/inc/txtfrm.hxx
index 98a18f7ed8ad..5656353d361a 100644
--- a/sw/source/core/inc/txtfrm.hxx
+++ b/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
diff --git a/sw/source/core/text/porrst.cxx b/sw/source/core/text/porrst.cxx
index 24778b6ec9d6..24b9bb98b532 100644
--- a/sw/source/core/text/porrst.cxx
+++ b/sw/source/core/text/porrst.cxx
@@ -331,7 +331,7 @@ bool SwTextFrame::FormatEmpty()
ClearPara();
ResetBlinkPor();
}
- SetCacheIdx( USHRT_MAX );
+ RemoveFromCache();
if( !IsEmpty() )
{
SetEmpty( true );
diff --git a/sw/source/core/text/txtcache.cxx b/sw/source/core/text/txtcache.cxx
index 20b992d9e275..eba78696b6fe 100644
--- a/sw/source/core/text/txtcache.cxx
+++ b/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
{
diff --git a/sw/source/core/text/txtcache.hxx b/sw/source/core/text/txtcache.hxx
index 491040c46934..f6573dc0cb9c 100644
--- a/sw/source/core/text/txtcache.hxx
+++ b/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)
diff --git a/sw/source/core/text/txtfrm.cxx b/sw/source/core/text/txtfrm.cxx
index c759be22aa02..a2d8001c80bc 100644
--- a/sw/source/core/text/txtfrm.cxx
+++ b/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 {