Remove DeleteItemOnIdlex

There are some CrashReports in 7.6 which have
DeleteItemOnIdle on the stack, but there is nothing
reproducable. So I took a look...

I first thought it's a MCGR regression, due to classes
on the stack. But the Item involved is just random, can
happen with any Item.

Then I thought it may have to do with ITEM refactorings,
but it happens with DeleteItemOnIdle involved, so also
not the case. I already saw DeleteItemOnIdle when doing
these and qualified as 'hack' in the way. already

It is only on Windows and DeleteItemOnIdle is involved.
This again (took a deeper look now) is an old hack to
keep an SfxPoolItem 'alive' for some 'time'. For that,
it triggers an async reschedule which then deletes the
Item when being called. If the Item will be used after
that is pure coincidence - seems to work in most cases.

It seems as if for Windows the timing slightly changed
for some scenarios, so a reschedule is too early. This
can happen with this hack anytime.

DeleteItemOnIdle is used in scenarios where SfxPoolItem*
is e.g. returned, but is *not* anchored, so e.g. not
member of an SfxItemSet. Or in short: Lifetime is not
safe.

DeleteItemOnIdle exists since 1st import, but was
changed to AsyncEvent ca. 4 months ago (see
57145ac), so that may
have caused it. It is possible that these errors happen
on Windows since then. Before something more complicated
was used to delete it late, but surely also not really
safe.

Due to ITEM refactor I have the knowledge/tooling to
solve this. It will not be a 1-5 lines fix, but it is
a hack and in the way for further ITEM refactor anyways.
What we have nowadays is a SfxPoolItemHolder -> it's
like an SfxItemSet for a single Item. It safely holds/
controls the lifetime of an SfxPoolItem. It is already
used in quite some places. It helps to solve many hacks,
also the ones putting Items directly to the Pool - due
to there never was an alternative for that. In principle
the ItemPool/ItemSet/Item paradigm was never complete
without SfxPoolItemHolder.

Thus I started to fix that (and remove that hack for
good, sooo many changes over the years, sigh), but as
said is not straightforward. Will have to change
retvals of involved stuff to SfxPoolItemHolder - it's
just two pointers and designed to be copied (one is a
Pool, needed to cleanup when destructing).
CopyConstruct/destroy just counts the RefCnt up/down,
so cheap.

1st version compiling, let's check on gerrit...
Corrected one error in QueryState for securitypage, also
added some security features/asserts.

Change-Id: Ida49fd35ca88ead84b11d93e18b978cb9e395090
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161083
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>

avmedia/source/framework/mediaplayer.cxx

@@ -25,6 +25,7 @@
 #include <helpids.h>
 
 #include <svl/stritem.hxx>
+#include <svl/itemset.hxx>
 #include <sfx2/sfxsids.hrc>
 #include <sfx2/bindings.hxx>
 #include <sfx2/dispatch.hxx>

basctl/source/basicide/IDEComboBox.cxx

@@ -34,6 +34,7 @@
 #include <vcl/svapp.hxx>
 #include <vcl/toolbox.hxx>
 #include <vcl/event.hxx>
+#include <svl/itemset.hxx>
 
 namespace basctl
 {

basctl/source/basicide/baside2b.cxx

@@ -70,6 +70,7 @@
 #include "textwindowpeer.hxx"
 #include "uiobject.hxx"
 #include <basegfx/utils/zoomtools.hxx>
+#include <svl/itemset.hxx>
 
 namespace basctl
 {

basctl/source/basicide/bastype2.cxx

@@ -32,6 +32,7 @@
 #include <comphelper/processfactory.hxx>
 #include <sfx2/dispatch.hxx>
 #include <sfx2/sfxsids.hrc>
+#include <svl/itemset.hxx>
 
 #include <initializer_list>
 #include <memory>

basctl/source/basicide/bastypes.cxx

@@ -42,6 +42,7 @@
 #include <svl/intitem.hxx>
 #include <svl/stritem.hxx>
 #include <svl/srchdefs.hxx>
+#include <svl/itemset.hxx>
 #include <utility>
 #include <vcl/commandevent.hxx>
 #include <vcl/event.hxx>

basctl/source/basicide/brkdlg.cxx

@@ -25,6 +25,7 @@
 
 #include <sfx2/dispatch.hxx>
 #include <sfx2/sfxsids.hrc>
+#include <svl/itemset.hxx>
 
 namespace basctl
 {

cui/source/dialogs/hldocntp.cxx

@@ -274,12 +274,12 @@ IMPL_STATIC_LINK(SvxHyperlinkNewDocTp, DispatchDocument, void*, p, void)
         SfxStringItem aFlags (SID_OPTIONS, aStrFlags);
 
         // open url
-        const SfxPoolItem* pReturn = xExecuteInfo->pDispatcher->ExecuteList(
-                SID_OPENDOC, SfxCallMode::SYNCHRON,
-                { &aName, &aFlags, &aFrame, &aReferer });
+        const SfxPoolItemHolder aResult(xExecuteInfo->pDispatcher->ExecuteList(
+            SID_OPENDOC, SfxCallMode::SYNCHRON,
+            { &aName, &aFlags, &aFrame, &aReferer }));
 
         // save new doc
-        const SfxViewFrameItem *pItem = dynamic_cast<const SfxViewFrameItem*>( pReturn  );  // SJ: pReturn is NULL if the Hyperlink
+        const SfxViewFrameItem *pItem = dynamic_cast<const SfxViewFrameItem*>(aResult.getItem());  // aResult is NULL if the Hyperlink
         if ( pItem )                                                            // creation is cancelled #106216#
         {
             pViewFrame = pItem->GetFrame();

cui/source/options/treeopt.cxx

@@ -1424,11 +1424,12 @@ std::optional<SfxItemSet> OfaTreeOptionsDialog::CreateItemSet( sal_uInt16 nId )
             SfxViewFrame* pViewFrame = SfxViewFrame::Current();
             if ( pViewFrame )
             {
-                const SfxUInt16Item* pItem = nullptr;
-                SfxDispatcher* pDispatch = pViewFrame->GetDispatcher();
+                SfxPoolItemHolder aResult;
+                const SfxItemState aState(pViewFrame->GetDispatcher()->QueryState(SID_ATTR_YEAR2000, aResult));
+                const SfxUInt16Item* pItem(static_cast<const SfxUInt16Item*>(aResult.getItem()));
 
                 // miscellaneous - Year2000
-                if( SfxItemState::DEFAULT <= pDispatch->QueryState( SID_ATTR_YEAR2000, pItem ) )
+                if(SfxItemState::DEFAULT <= aState && nullptr != pItem)
                     pRet->Put( SfxUInt16Item( SID_ATTR_YEAR2000, pItem->GetValue() ) );
                 else
                     pRet->Put( SfxUInt16Item( SID_ATTR_YEAR2000, officecfg::Office::Common::DateFormat::TwoDigitYear::get() ) );
@@ -1474,20 +1475,28 @@ std::optional<SfxItemSet> OfaTreeOptionsDialog::CreateItemSet( sal_uInt16 nId )
             SfxViewFrame* pViewFrame = SfxViewFrame::Current();
             if ( pViewFrame )
             {
-                const SvxLanguageItem* pLangItem = nullptr;
-                SfxDispatcher* pDispatch = pViewFrame->GetDispatcher();
-                if(SfxItemState::DEFAULT <= pDispatch->QueryState(SID_ATTR_LANGUAGE, pLangItem))
-                    pRet->Put(*pLangItem, SID_ATTR_LANGUAGE);
-                if(SfxItemState::DEFAULT <= pDispatch->QueryState(SID_ATTR_CHAR_CJK_LANGUAGE, pLangItem))
-                    pRet->Put(*pLangItem, SID_ATTR_CHAR_CJK_LANGUAGE);
-                if(SfxItemState::DEFAULT <= pDispatch->QueryState(SID_ATTR_CHAR_CTL_LANGUAGE, pLangItem))
-                    pRet->Put(*pLangItem, SID_ATTR_CHAR_CTL_LANGUAGE);
+                SfxPoolItemHolder aResult;
+                SfxDispatcher* pDispatch(pViewFrame->GetDispatcher());
+
+                SfxItemState aState(pDispatch->QueryState(SID_ATTR_LANGUAGE, aResult));
+                if(SfxItemState::DEFAULT <= aState)
+                    pRet->Put(*aResult.getItem(), SID_ATTR_LANGUAGE);
+
+                aState = pDispatch->QueryState(SID_ATTR_CHAR_CJK_LANGUAGE, aResult);
+                if(SfxItemState::DEFAULT <= aState)
+                    pRet->Put(*aResult.getItem(), SID_ATTR_CHAR_CJK_LANGUAGE);
+
+                aState = pDispatch->QueryState(SID_ATTR_CHAR_CTL_LANGUAGE, aResult);
+                if(SfxItemState::DEFAULT <= aState)
+                    pRet->Put(*aResult.getItem(), SID_ATTR_CHAR_CTL_LANGUAGE);
 
                 pRet->Put(aHyphen);
-                const SfxPoolItem* pItem = nullptr;
-                if(SfxItemState::DEFAULT <= pDispatch->QueryState(SID_AUTOSPELL_CHECK, pItem))
+
+                aState = pDispatch->QueryState(SID_AUTOSPELL_CHECK, aResult);
+                if(SfxItemState::DEFAULT <= aState)
                 {
-                    pRet->Put(std::unique_ptr<SfxPoolItem>(pItem->Clone()));
+                    pRet->Put(*aResult.getItem()); // allow share/refcounting
+                    // pRet->Put(std::unique_ptr<SfxPoolItem>(aResult.getItem()->Clone()));
                 }
                 else
                 {

desktop/source/lib/init.cxx

@@ -909,18 +909,15 @@ void ExecuteOrientationChange()
     if ( mxUndoManager.is() )
         mxUndoManager->enterUndoContext( "" );
 
+    SfxPoolItemHolder aResult;
+    pViewFrm->GetBindings().GetDispatcher()->QueryState(SID_ATTR_PAGE_SIZE, aResult);
+    std::unique_ptr<SvxSizeItem> pPageSizeItem(static_cast<const SvxSizeItem*>(aResult.getItem())->Clone());
 
-    const SvxSizeItem* pSizeItem;
-    pViewFrm->GetBindings().GetDispatcher()->QueryState(SID_ATTR_PAGE_SIZE, pSizeItem);
-    std::unique_ptr<SvxSizeItem> pPageSizeItem(pSizeItem->Clone());
+    pViewFrm->GetBindings().GetDispatcher()->QueryState(SID_ATTR_PAGE_LRSPACE, aResult);
+    std::unique_ptr<SvxLongLRSpaceItem> pPageLRMarginItem(static_cast<const SvxLongLRSpaceItem*>(aResult.getItem())->Clone());
 
-    const SvxLongLRSpaceItem* pLRSpaceItem;
-    pViewFrm->GetBindings().GetDispatcher()->QueryState(SID_ATTR_PAGE_LRSPACE, pLRSpaceItem);
-    std::unique_ptr<SvxLongLRSpaceItem> pPageLRMarginItem(pLRSpaceItem->Clone());
-
-    const SvxLongULSpaceItem* pULSpaceItem;
-    pViewFrm->GetBindings().GetDispatcher()->QueryState(SID_ATTR_PAGE_ULSPACE, pULSpaceItem);
-    std::unique_ptr<SvxLongULSpaceItem> pPageULMarginItem(pULSpaceItem->Clone());
+    pViewFrm->GetBindings().GetDispatcher()->QueryState(SID_ATTR_PAGE_ULSPACE, aResult);
+    std::unique_ptr<SvxLongULSpaceItem> pPageULMarginItem(static_cast<const SvxLongULSpaceItem*>(aResult.getItem())->Clone());
 
     {
         bool bIsLandscape = false;

include/sfx2/bindings.hxx

@@ -52,6 +52,7 @@ class Timer;
 class SfxWorkWindow;
 struct SfxFoundCache_Impl;
 class SfxFoundCacheArr_Impl;
+class SfxPoolItemHolder;
 
 enum class SfxCallMode : sal_uInt16
 {
@@ -99,7 +100,7 @@ friend class SfxBindings_Impl;
     sal_uInt16       nRegLevel;      // Lock-Level while Reconfig
 
 private:
-    SAL_DLLPRIVATE const SfxPoolItem*  Execute_Impl( sal_uInt16 nSlot, const SfxPoolItem **pArgs, sal_uInt16 nModi,
+    SAL_DLLPRIVATE SfxPoolItemHolder Execute_Impl( sal_uInt16 nSlot, const SfxPoolItem **pArgs, sal_uInt16 nModi,
                                     SfxCallMode nCall, const SfxPoolItem **pInternalArgs, bool bGlobalOnly=false);
     SAL_DLLPRIVATE void SetSubBindings_Impl( SfxBindings* );
     SAL_DLLPRIVATE void UpdateSlotServer_Impl(); // Update SlotServer
@@ -155,7 +156,7 @@ public:
 
     sal_uInt16       QuerySlotId( const css::util::URL& aURL );
 
-    const SfxPoolItem*  ExecuteSynchron( sal_uInt16 nSlot,
+    SfxPoolItemHolder ExecuteSynchron( sal_uInt16 nSlot,
                                  const SfxPoolItem **pArgs = nullptr);
     bool             Execute( sal_uInt16 nSlot,
                                  const SfxPoolItem **pArgs = nullptr,

include/sfx2/dispatch.hxx

@@ -41,6 +41,7 @@ class SfxModule;
 class Point;
 struct SfxDispatcher_Impl;
 class VCLXPopupMenu;
+class SfxPoolItemHolder;
 
 namespace com::sun::star::awt { class XPopupMenu; }
 namespace vcl { class Window; }
@@ -104,24 +105,24 @@ public:
 
                         ~SfxDispatcher();
 
-    const SfxPoolItem*  Execute( sal_uInt16 nSlot,
+    SfxPoolItemHolder   Execute( sal_uInt16 nSlot,
                                  SfxCallMode nCall = SfxCallMode::SLOT,
                                  const SfxPoolItem **pArgs = nullptr,
                                  sal_uInt16 nModi = 0,
                                  const SfxPoolItem **pInternalArgs = nullptr);
 
-    const SfxPoolItem*  Execute(sal_uInt16 nSlot,
+    SfxPoolItemHolder   Execute(sal_uInt16 nSlot,
                                 SfxCallMode nCall,
                                 SfxItemSet const * pArgs,
                                 SfxItemSet const * pInternalArgs,
                                 sal_uInt16 nModi);
 
-    const SfxPoolItem*  ExecuteList(sal_uInt16 nSlot,
+    SfxPoolItemHolder   ExecuteList(sal_uInt16 nSlot,
                                     SfxCallMode nCall,
                                     std::initializer_list<SfxPoolItem const*> args,
                                     std::initializer_list<SfxPoolItem const*> internalargs = std::initializer_list<SfxPoolItem const*>());
 
-    const SfxPoolItem*  Execute( sal_uInt16 nSlot,
+    SfxPoolItemHolder   Execute( sal_uInt16 nSlot,
                                  SfxCallMode nCall,
                                  const SfxItemSet &rArgs );
 
@@ -152,12 +153,7 @@ public:
     void                HideUI( bool bHide = true );
     ToolbarId           GetObjectBarId( sal_uInt16 nPos ) const;
 
-    SfxItemState        QueryState( sal_uInt16 nSID, const SfxPoolItem* &rpState );
-    template<class T>
-    SfxItemState        QueryState( TypedWhichId<T> nSID, const T* &rpState )
-    {
-        return QueryState(sal_uInt16(nSID), reinterpret_cast<SfxPoolItem const*&>(rpState));
-    }
+    SfxItemState        QueryState( sal_uInt16 nSID, SfxPoolItemHolder& rState );
     SfxItemState        QueryState( sal_uInt16 nSID, css::uno::Any& rAny );
 
     void                SetDisableFlags( SfxDisableFlags nFlags );

include/sfx2/request.hxx

@@ -95,7 +95,7 @@ public:
 
     void                ReleaseArgs();
     void                SetReturnValue(const SfxPoolItem &);
-    const SfxPoolItem*  GetReturnValue() const;
+    const SfxPoolItemHolder& GetReturnValue() const;
 
     static css::uno::Reference< css::frame::XDispatchRecorder > GetMacroRecorder(const SfxViewFrame& rFrame);
     static bool         HasMacroRecorder(const SfxViewFrame& rFrame);

include/sfx2/shell.hxx

@@ -50,6 +50,7 @@ class SfxRepeatTarget;
 class SfxBindings;
 class SfxModule;
 class SfxUndoManager;
+class SfxPoolItemHolder;
 
 /**
     Id for <SfxInterface>s, gives a quasi-static access to the interface
@@ -231,7 +232,7 @@ public:
 
         <SfxShell::ExecuteSlot(SfxRequest&)>
         */
-    const SfxPoolItem*          GetSlotState( sal_uInt16 nSlotId, const SfxInterface *pIF = nullptr, SfxItemSet *pStateSet = nullptr );
+    SfxPoolItemHolder GetSlotState( sal_uInt16 nSlotId, const SfxInterface *pIF = nullptr, SfxItemSet *pStateSet = nullptr );
 
     /**
         This method allows you to forward a <SfxRequest> to the specified
@@ -263,7 +264,7 @@ public:
 
         <SfxShell::GetSlotState(sal_uInt16,const SfxInterface*,SfxItemSet*)>
         */
-    const SfxPoolItem*          ExecuteSlot( SfxRequest &rReq, const SfxInterface *pIF = nullptr );
+    const SfxPoolItemHolder& ExecuteSlot( SfxRequest &rReq, const SfxInterface *pIF = nullptr );
 
     /**
         Asynchronous ExecuteSlot for the RELOAD

include/svl/itemset.hxx

@@ -46,15 +46,23 @@ class SAL_WARN_UNUSED SVL_DLLPUBLIC SfxPoolItemHolder
 {
     SfxItemPool*            m_pPool;
     const SfxPoolItem*      m_pItem;
+#ifdef DBG_UTIL
+    bool                    m_bDeleted;
+#endif
 public:
-    SfxPoolItemHolder(SfxItemPool&, const SfxPoolItem* = nullptr);
+    SfxPoolItemHolder();
+    SfxPoolItemHolder(SfxItemPool&, const SfxPoolItem*, bool bPassingOwnership = false);
     SfxPoolItemHolder(const SfxPoolItemHolder&);
     ~SfxPoolItemHolder();
 
+#ifdef DBG_UTIL
+    bool isDeleted() const { return m_bDeleted; }
+#endif
+
     const SfxPoolItemHolder& operator=(const SfxPoolItemHolder&);
     bool operator==(const SfxPoolItemHolder &) const;
-    SfxItemPool& getPool() const { return *m_pPool; }
-    const SfxPoolItem* getItem() const { return m_pItem; }
+    SfxItemPool& getPool() const { assert(!isDeleted() && "Destructed instance used (!)"); return *m_pPool; }
+    const SfxPoolItem* getItem() const { assert(!isDeleted() && "Destructed instance used (!)"); return m_pItem; }
     sal_uInt16 Which() const { if(nullptr != m_pItem) return m_pItem->Which(); return 0; }
 };
 

include/svl/poolitem.hxx

@@ -131,17 +131,16 @@ class SVL_DLLPUBLIC SfxPoolItem
 
     // bitfield for flags (instead of SfxItemKind)
     bool        m_bIsVoidItem : 1;          // bit 0
-    bool        m_bDeleteOnIdle : 1;        // bit 1
-    bool        m_bStaticDefault : 1;       // bit 2
-    bool        m_bPoolDefault : 1;         // bit 3
-    bool        m_bRegisteredAtPool : 1;    // bit 4
-    bool        m_bExceptionalSCItem : 1;     // bit 5
-    bool        m_bIsSetItem : 1;           // bit 6
+    bool        m_bStaticDefault : 1;       // bit 1
+    bool        m_bPoolDefault : 1;         // bit 2
+    bool        m_bRegisteredAtPool : 1;    // bit 3
+    bool        m_bExceptionalSCItem : 1;   // bit 4
+    bool        m_bIsSetItem : 1;           // bit 5
 
 protected:
 #ifdef DBG_UTIL
     // this flag will make debugging item stuff much simpler
-    bool        m_bDeleted : 1;             // bit 7
+    bool        m_bDeleted : 1;             // bit 6
 #endif
 
 private:
@@ -153,7 +152,6 @@ private:
 
 protected:
     void setIsVoidItem() { m_bIsVoidItem = true; }
-    void setDeleteOnIdle() { m_bDeleteOnIdle = true; }
     void setStaticDefault() { m_bStaticDefault = true; }
     void setPoolDefault() { m_bPoolDefault = true; }
     void setRegisteredAtPool(bool bNew) { m_bRegisteredAtPool = bNew; }
@@ -173,7 +171,6 @@ public:
 #endif
 
     bool isVoidItem() const { return m_bIsVoidItem; }
-    bool isDeleteOnIdle() const { return m_bDeleteOnIdle; }
     bool isStaticDefault() const { return m_bStaticDefault; }
     bool isPoolDefault() const { return m_bPoolDefault; }
     bool isRegisteredAtPool() const { return m_bRegisteredAtPool; }

sc/source/ui/cctrl/cbnumberformat.cxx

@@ -24,6 +24,7 @@
 #include <sfx2/viewfrm.hxx>
 #include <sfx2/viewsh.hxx>
 #include <svl/intitem.hxx>
+#include <svl/itemset.hxx>
 #include <sc.hrc>
 
 ScNumberFormat::ScNumberFormat(vcl::Window* pParent)

sc/source/ui/dbgui/PivotLayoutDialog.cxx

@@ -484,10 +484,10 @@ void ScPivotLayoutDialog::ApplyChanges()
 
     SfxDispatcher* pDispatcher = GetBindings().GetDispatcher();
     SfxCallMode const nCallMode = SfxCallMode::SLOT | SfxCallMode::RECORD;
-    const SfxPoolItem* pResult = pDispatcher->ExecuteList(SID_PIVOT_TABLE,
-            nCallMode, { &aPivotItem });
+    const SfxPoolItemHolder aResult(pDispatcher->ExecuteList(SID_PIVOT_TABLE,
+            nCallMode, { &aPivotItem }));
 
-    if (pResult != nullptr)
+    if (nullptr != aResult.getItem())
     {
         // existing pivot table might have moved to a new range or a new sheet
         if ( pOldDPObj != nullptr  )

sc/source/ui/navipi/scenwnd.cxx

@@ -22,6 +22,7 @@
 #include <sfx2/viewfrm.hxx>
 #include <svl/slstitm.hxx>
 #include <svl/stritem.hxx>
+#include <svl/itemset.hxx>
 #include <vcl/commandevent.hxx>
 #include <vcl/event.hxx>
 #include <vcl/svapp.hxx>

sc/source/ui/sidebar/AlignmentPropertyPanel.cxx

@@ -25,6 +25,7 @@
 #include <sfx2/bindings.hxx>
 #include <sfx2/dispatch.hxx>
 #include <svl/intitem.hxx>
+#include <svl/itemset.hxx>
 #include <svx/rotmodit.hxx>
 #include <svtools/unitconv.hxx>
 #include <com/sun/star/lang/IllegalArgumentException.hpp>

sc/source/ui/sidebar/CellBorderStyleControl.cxx

@@ -25,6 +25,7 @@
 #include <svx/svxids.hrc>
 #include <vcl/settings.hxx>
 #include <editeng/lineitem.hxx>
+#include <svl/itemset.hxx>
 #include <memory>
 
 namespace sc::sidebar {

sc/source/ui/sidebar/CellLineStyleControl.cxx

@@ -27,6 +27,7 @@
 #include <sfx2/bindings.hxx>
 #include <sfx2/dispatch.hxx>
 #include <svx/svxids.hrc>
+#include <svl/itemset.hxx>
 #include <scresid.hxx>
 #include <strings.hrc>
 

sc/source/ui/sidebar/NumberFormatPropertyPanel.cxx

@@ -23,6 +23,7 @@
 #include <sfx2/dispatch.hxx>
 #include <svl/intitem.hxx>
 #include <svl/stritem.hxx>
+#include <svl/itemset.hxx>
 #include <svx/numfmtsh.hxx>
 #include <o3tl/string_view.hxx>
 #include <com/sun/star/lang/IllegalArgumentException.hpp>