include/svl/poolitem.hxx | 10 ++++++
svl/source/items/itemset.cxx | 2 -
svl/source/items/poolitem.cxx | 1
sw/source/core/attr/cellatr.cxx | 3 +
sw/source/core/attr/swatrset.cxx | 61 ++++++++++++++++++++++++++++++++++++++-
sw/source/core/layout/atrfrm.cxx | 6 +++
sw/source/core/para/paratr.cxx | 6 +++
7 files changed, 87 insertions(+), 2 deletions(-)
New commits:
commit b45ab5256c9768914bcad854ce32b06caa0539b8
Author: Armin Le Grand (allotropia) <armin.le.grand.ext...@allotropia.de>
AuthorDate: Thu Sep 28 17:44:36 2023 +0200
Commit: Armin Le Grand <armin.le.gr...@me.com>
CommitDate: Fri Sep 29 11:18:52 2023 +0200
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.gr...@me.com>
diff --git a/include/svl/poolitem.hxx b/include/svl/poolitem.hxx
index ce4d94e782f9..4714e2f44be6 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 4f71a8aafb5e..48f0679e481d 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 a4f02d19b4e3..928ac3de3430 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 9023cca2f793..09b070e42c3a 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 6d778c1cb6b2..d909d8597e54 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 5e3c26e708c7..b3dd57ca6c7f 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 f724ac21c0fe..7d57bcd3c7a4 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()
