ITEM: Correct handling of Items in sw
...that use internally an sw::BroadcastingModify*. This caused
an error/crash with the testfile ooo58307-1.sxw, thanks to
Caolan to find it. For BG info please see comments in
the changed sw/source/core/attr/swatrset.cxx
Change-Id: Ia91fff19ffb39873c7e2bd5ff8806b95fc5ac7ba
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/157383
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
diff --git a/include/svl/poolitem.hxx b/include/svl/poolitem.hxx
index ce4d94e..4714e2f 100644
--- a/include/svl/poolitem.hxx
+++ b/include/svl/poolitem.hxx
@@ -121,6 +121,15 @@ friend class SfxItemSet;
bool m_bDeleteOnIdle : 1;
bool m_bStaticDefault : 1;
bool m_bPoolDefault : 1;
protected:
// Defines if the Item can be shared/RefCounted else it will be cloned.
// Default is true - as it should be for all Items. It is needed by some
// SW items, so protected to let them set it in constructor. If this could
// be fixed at that Items we may remove this again.
bool m_bShareable : 1;
private:
#ifdef DBG_UTIL
// this flag will make debugging item stuff much simpler
bool m_bDeleted : 1;
@@ -151,6 +160,7 @@ public:
bool isDeleteOnIdle() const { return m_bDeleteOnIdle; }
bool isStaticDefault() const { return m_bStaticDefault; }
bool isPoolDefault() const { return m_bPoolDefault; }
bool isShareable() const { return m_bShareable; }
private:
inline sal_uInt32 ReleaseRef(sal_uInt32 n = 1) const
diff --git a/svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx
index 4f71a8a..48f0679 100644
--- a/svl/source/items/itemset.cxx
+++ b/svl/source/items/itemset.cxx
@@ -159,7 +159,7 @@ SfxPoolItem const* SfxItemSet::implCreateItemEntry(SfxPoolItem const* pSource, s
// these need to be cloned
return pSource->Clone();
if (bItemIsSetMember && IsPooledItem(pSource))//!IsPoolDefaultItem(pSource) && GetPool()->IsItemPoolable(*pSource))
if (pSource->isShareable() && bItemIsSetMember && IsPooledItem(pSource))//!IsPoolDefaultItem(pSource) && GetPool()->IsItemPoolable(*pSource))
{
// shortcut: if we know that the Item is already a member
// of another SfxItemSet we can just copy the pointer and increase RefCount
diff --git a/svl/source/items/poolitem.cxx b/svl/source/items/poolitem.cxx
index a4f02d1..928ac3d 100644
--- a/svl/source/items/poolitem.cxx
+++ b/svl/source/items/poolitem.cxx
@@ -475,6 +475,7 @@ SfxPoolItem::SfxPoolItem(sal_uInt16 const nWhich)
, m_bDeleteOnIdle(false)
, m_bStaticDefault(false)
, m_bPoolDefault(false)
, m_bShareable(true)
#ifdef DBG_UTIL
, m_bDeleted(false)
#endif
diff --git a/sw/source/core/attr/cellatr.cxx b/sw/source/core/attr/cellatr.cxx
index 9023cca..09b070e 100644
--- a/sw/source/core/attr/cellatr.cxx
+++ b/sw/source/core/attr/cellatr.cxx
@@ -58,6 +58,9 @@ SwTableBoxFormula::SwTableBoxFormula( const OUString& rFormula )
SwTableFormula( rFormula ),
m_pDefinedIn( nullptr )
{
// ITEM: mark this Item to be non-shareable/non-RefCountable. For more
// info see comment @SwAttrSet::SetModifyAtAttr
m_bShareable = false;
}
bool SwTableBoxFormula::operator==( const SfxPoolItem& rAttr ) const
diff --git a/sw/source/core/attr/swatrset.cxx b/sw/source/core/attr/swatrset.cxx
index 6d778c1..d909d859 100644
--- a/sw/source/core/attr/swatrset.cxx
+++ b/sw/source/core/attr/swatrset.cxx
@@ -95,6 +95,10 @@ SwAttrPool::~SwAttrPool()
/// Notification callback
void SwAttrSet::changeCallback(const SfxPoolItem* pOld, const SfxPoolItem* pNew) const
{
// will have no effect, return
if (nullptr == m_pOldSet && nullptr == m_pNewSet)
return;
// at least one SfxPoolItem has to be provided, else this is an error
assert(nullptr != pOld || nullptr != pNew);
sal_uInt16 nWhich(0);
@@ -233,6 +237,10 @@ SwAttrSet SwAttrSet::CloneAsValue( bool bItems ) const
bool SwAttrSet::Put_BC( const SfxPoolItem& rAttr,
SwAttrSet* pOld, SwAttrSet* pNew )
{
// direct call when neither pOld nor pNew is set, no need for callback
if (nullptr == pOld && nullptr == pNew)
return nullptr != SfxItemSet::Put( rAttr );
m_pNewSet = pNew;
m_pOldSet = pOld;
setCallback(m_aCallbackHolder);
@@ -245,6 +253,10 @@ bool SwAttrSet::Put_BC( const SfxPoolItem& rAttr,
bool SwAttrSet::Put_BC( const SfxItemSet& rSet,
SwAttrSet* pOld, SwAttrSet* pNew )
{
// direct call when neither pOld nor pNew is set, no need for callback
if (nullptr == pOld && nullptr == pNew)
return SfxItemSet::Put( rSet );
m_pNewSet = pNew;
m_pOldSet = pOld;
setCallback(m_aCallbackHolder);
@@ -257,6 +269,10 @@ bool SwAttrSet::Put_BC( const SfxItemSet& rSet,
sal_uInt16 SwAttrSet::ClearItem_BC( sal_uInt16 nWhich,
SwAttrSet* pOld, SwAttrSet* pNew )
{
// direct call when neither pOld nor pNew is set, no need for callback
if (nullptr == pOld && nullptr == pNew)
return SfxItemSet::ClearItem( nWhich );
m_pNewSet = pNew;
m_pOldSet = pOld;
setCallback(m_aCallbackHolder);
@@ -270,10 +286,19 @@ sal_uInt16 SwAttrSet::ClearItem_BC( sal_uInt16 nWhich1, sal_uInt16 nWhich2,
SwAttrSet* pOld, SwAttrSet* pNew )
{
OSL_ENSURE( nWhich1 <= nWhich2, "no valid range" );
sal_uInt16 nRet = 0;
// direct call when neither pOld nor pNew is set, no need for callback
if (nullptr == pOld && nullptr == pNew)
{
for( ; nWhich1 <= nWhich2; ++nWhich1 )
nRet = nRet + SfxItemSet::ClearItem( nWhich1 );
return nRet;
}
m_pNewSet = pNew;
m_pOldSet = pOld;
setCallback(m_aCallbackHolder);
sal_uInt16 nRet = 0;
for( ; nWhich1 <= nWhich2; ++nWhich1 )
nRet = nRet + SfxItemSet::ClearItem( nWhich1 );
clearCallback();
@@ -284,6 +309,13 @@ sal_uInt16 SwAttrSet::ClearItem_BC( sal_uInt16 nWhich1, sal_uInt16 nWhich2,
int SwAttrSet::Intersect_BC( const SfxItemSet& rSet,
SwAttrSet* pOld, SwAttrSet* pNew )
{
// direct call when neither pOld nor pNew is set, no need for callback
if (nullptr == pOld && nullptr == pNew)
{
SfxItemSet::Intersect( rSet );
return 0; // as below when neither pOld nor pNew is set
}
m_pNewSet = pNew;
m_pOldSet = pOld;
setCallback(m_aCallbackHolder);
@@ -305,6 +337,33 @@ bool SwAttrSet::SetModifyAtAttr( const sw::BroadcastingModify* pModify )
{
bool bSet = false;
// ITEM: At ths place the paradigm of Item/Set/Pool gets broken:
// The three Items in Writer
// - SwFormatPageDesc
// - SwFormatDrop
// - SwTableBoxFormula
// contain a unique ptr to SwFormat (or: sw::BroadcastingModify
// if you wish), so the Item *cannot* be shared or re-used.
// But that is the intended nature of Items:
// - they are read-only (note the bad const_cast's below)
// - they are ref-counted to be re-usable in as many Sets as
// possible
// Thus if we need to change that ptr @ Item we *need* to make
// sure that Item is *unique*. This is now done by using the
// 'm_bShareable' at the Item (see where that gets set).
// This was done in the past using the 'poolable' flag, but that
// flag was/is for a completely different purpose - uniqueness is
// a side-effect. It already leaded to massively cloning that Item.
// That something is not 'clean' here can also be seen by using
// const_cast to *change* values at Items *in* the Set - these
// are returned by const reference for a purpose.
// If info/data at an Item has to be changed, the official/clean
// way is to create a new one (e.g. Clone), set the values (if
// not given to the constructor) and then set that Item at the Set.
// NOTE: I do not know if and how it would be possible, but holding
// a SwFormat*/sw::BroadcastingModify* at those Items should
// be fixed/removed ASAP
const SwFormatPageDesc* pPageDescItem = GetItemIfSet( RES_PAGEDESC, false );
if( pPageDescItem &&
pPageDescItem->GetDefinedIn() != pModify )
diff --git a/sw/source/core/layout/atrfrm.cxx b/sw/source/core/layout/atrfrm.cxx
index 5e3c26e..b3dd57c 100644
--- a/sw/source/core/layout/atrfrm.cxx
+++ b/sw/source/core/layout/atrfrm.cxx
@@ -634,6 +634,9 @@ SwFormatPageDesc::SwFormatPageDesc( const SwFormatPageDesc &rCpy )
m_oNumOffset( rCpy.m_oNumOffset ),
m_pDefinedIn( nullptr )
{
// ITEM: mark this Item to be non-shareable/non-RefCountable. For more
// info see comment @SwAttrSet::SetModifyAtAttr
m_bShareable = false;
}
SwFormatPageDesc::SwFormatPageDesc( const SwPageDesc *pDesc )
@@ -641,6 +644,9 @@ SwFormatPageDesc::SwFormatPageDesc( const SwPageDesc *pDesc )
SwClient( const_cast<SwPageDesc*>(pDesc) ),
m_pDefinedIn( nullptr )
{
// ITEM: mark this Item to be non-shareable/non-RefCountable. For more
// info see comment @SwAttrSet::SetModifyAtAttr
m_bShareable = false;
}
SwFormatPageDesc &SwFormatPageDesc::operator=(const SwFormatPageDesc &rCpy)
diff --git a/sw/source/core/para/paratr.cxx b/sw/source/core/para/paratr.cxx
index f724ac2..7d57bcd 100644
--- a/sw/source/core/para/paratr.cxx
+++ b/sw/source/core/para/paratr.cxx
@@ -44,6 +44,9 @@ SwFormatDrop::SwFormatDrop()
m_nChars( 0 ),
m_bWholeWord( false )
{
// ITEM: mark this Item to be non-shareable/non-RefCountable. For more
// info see comment @SwAttrSet::SetModifyAtAttr
m_bShareable = false;
}
SwFormatDrop::SwFormatDrop( const SwFormatDrop &rCpy )
@@ -55,6 +58,9 @@ SwFormatDrop::SwFormatDrop( const SwFormatDrop &rCpy )
m_nChars( rCpy.GetChars() ),
m_bWholeWord( rCpy.GetWholeWord() )
{
// ITEM: mark this Item to be non-shareable/non-RefCountable. For more
// info see comment @SwAttrSet::SetModifyAtAttr
m_bShareable = false;
}
SwFormatDrop::~SwFormatDrop()
