sw/source/core/bastyp/swcache.cxx | 17 +++++++++++++---- sw/source/core/inc/swcache.hxx | 20 ++++++++++++++------ sw/source/core/inc/txtfrm.hxx | 4 +++- sw/source/core/text/frmform.cxx | 12 +++++------- sw/source/core/text/porrst.cxx | 2 +- sw/source/core/text/txtcache.cxx | 20 ++++++++++++++++++-- sw/source/core/text/txtcache.hxx | 2 ++ sw/source/core/text/txtfrm.cxx | 5 +---- 8 files changed, 57 insertions(+), 25 deletions(-)
New commits: commit a42291d0abd14c6c763962a5ae60b02a8552f641 Author: Michael Stahl <[email protected]> AuthorDate: Mon May 27 19:03:45 2019 +0200 Commit: Michael Stahl <[email protected]> CommitDate: Tue May 28 14:12:28 2019 +0200 Revert "crashtesting: frequent nulldef at end of SwTextFrame::ValidateFrame" This reverts commit c5338e3ad116dbde0aed801f459173231716efa3. The actual problem was fixed in previous commit for tdf#125475. ValidateFrame() must only be called in text formatting with a SwTextLineAccess keeping the corresponding SwTextLine alive. Change-Id: I5b092b094f620f2dc2524aa5a83c673ec6b1fbbe Reviewed-on: https://gerrit.libreoffice.org/73048 Tested-by: Jenkins Reviewed-by: Michael Stahl <[email protected]> diff --git a/sw/source/core/text/frmform.cxx b/sw/source/core/text/frmform.cxx index 3c018f0a702c..492a738941c3 100644 --- a/sw/source/core/text/frmform.cxx +++ b/sw/source/core/text/frmform.cxx @@ -107,13 +107,11 @@ void SwTextFrame::ValidateFrame() ValidateText( this ); // We at least have to save the MustFit flag! - OSL_ENSURE( HasPara(), "ResetPreps(), missing ParaPortion." ); - if (SwParaPortion *pPara = GetPara()) - { - const bool bMustFit = pPara->IsPrepMustFit(); - ResetPreps(); - pPara->SetPrepMustFit( bMustFit ); - } + assert(HasPara() && "ResetPreps(), missing ParaPortion, SwCache bug?"); + SwParaPortion *pPara = GetPara(); + const bool bMustFit = pPara->IsPrepMustFit(); + ResetPreps(); + pPara->SetPrepMustFit( bMustFit ); } // After a RemoveFootnote the BodyFrame and all Frames contained within it, need to be commit 1424d51a44ed2fa8b37b2d132af8a608a170ec8e Author: Michael Stahl <[email protected]> AuthorDate: Mon May 27 18:55:31 2019 +0200 Commit: Michael Stahl <[email protected]> CommitDate: Tue May 28 14:12:15 2019 +0200 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. Change-Id: I7bef1b340a453d6dd44d51a1dc69ee5fd0b697db Reviewed-on: https://gerrit.libreoffice.org/73047 Tested-by: Jenkins Reviewed-by: Michael Stahl <[email protected]> diff --git a/sw/source/core/bastyp/swcache.cxx b/sw/source/core/bastyp/swcache.cxx index 64eb233b6bf7..443e267b9006 100644 --- a/sw/source/core/bastyp/swcache.cxx +++ b/sw/source/core/bastyp/swcache.cxx @@ -296,6 +296,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; // deletes pObj CHECK; @@ -340,10 +341,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 ) @@ -374,7 +383,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(); @@ -485,12 +494,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 fe2d516168be..b8aa145e3a3e 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* nCacheId, const sal_uInt16 nIndex ); @@ -241,10 +249,10 @@ inline SwCacheAccess::SwCacheAccess( SwCache &rC, const void* nCacheId, 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 c9deeb0c827e..afe590517923 100644 --- a/sw/source/core/inc/txtfrm.hxx +++ b/sw/source/core/inc/txtfrm.hxx @@ -602,8 +602,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 3ae4b9623978..09611e728441 100644 --- a/sw/source/core/text/porrst.cxx +++ b/sw/source/core/text/porrst.cxx @@ -330,7 +330,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 fcc8b434cdd8..17eedcc17626 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, std::unique_ptr<SwParaPortion>(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 c5f52944460f..d04a0eb63323 100644 --- a/sw/source/core/text/txtcache.hxx +++ b/sw/source/core/text/txtcache.hxx @@ -29,6 +29,8 @@ class SwTextLine : public SwCacheObj { std::unique_ptr<SwParaPortion> pLine; + virtual void UpdateCachePos() override; + public: SwTextLine( SwTextFrame const *pFrame, std::unique_ptr<SwParaPortion> pNew = nullptr ); virtual ~SwTextLine() override; diff --git a/sw/source/core/text/txtfrm.cxx b/sw/source/core/text/txtfrm.cxx index 1c1443042649..931fbd75a4fa 100644 --- a/sw/source/core/text/txtfrm.cxx +++ b/sw/source/core/text/txtfrm.cxx @@ -881,10 +881,7 @@ void SwTextFrame::DestroyImpl() SwTextFrame::~SwTextFrame() { - if (GetCacheIdx() != USHRT_MAX) - { - s_pTextCache->Delete(this, GetCacheIdx()); - } + RemoveFromCache(); } namespace sw { _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
