Resolves: tdf#119259 reused in-use SdStyleSheet removed by undo

since...

commit a4cd841541a729d7b8126d27d91fa28e30b01403
Date:   Thu Feb 13 16:10:47 2014 +0530

    n#757432: Styles (rename &) copy to different decks.

    While copying slides to different slide decks,
    styles were not being copied if there is already one
    with the same name. This patch renames and copies those
    to keep the formatting intact.

if a SdStyleSheet is not freshly created, but reused, it ends up in
the rCreatedSheets list, and so when the copy is undone
(via SdMoveStyleSheetsUndoAction), the SdStyleSheet is unconditionally
remove from the StylePool and so its ref count hits 0 and is removed,
even though there are still objects depending on it.

the problem becames more common since...

bash-4.4$ git show 57db6e24b5ad43d447c30e44a112c74c7e75b46b
commit 57db6e24b5ad43d447c30e44a112c74c7e75b46b
Date:   Tue Aug 15 02:51:07 2017 +0530

    Removing old SfxItemSet::getHash usage

improves the SfxItemSet comparison.

Assuming that its correct to leave SdStyleSheetPool::CopySheets
with those reused SdStyleSheets in the list, change things so
that a flag is passed around to indicate if the SdStyleSheet
should be removed or not by undo

Change-Id: I82b5bd93183fd1ff9e67957ccfb5babef81fd36d
Reviewed-on: https://gerrit.libreoffice.org/59434
Tested-by: Jenkins
Tested-by: Xisco Faulí <xiscofauli@libreoffice.org>
Reviewed-by: Miklos Vajna <vmiklos@collabora.co.uk>

9 files changed, 75 insertions, 49 deletions

diff --git a/sd/inc/stlpool.hxx b/sd/inc/stlpool.hxx
index cfe639038f9a..f4cf930d26e3 100644
--- a/sd/inc/stlpool.hxx
+++ b/sd/inc/stlpool.hxx
@@ -70,12 +70,12 @@ public:
     SD_DLLPUBLIC void                CreateLayoutStyleSheets(const OUString& rLayoutName, bool bCheck = false );
     static void         CreateLayoutSheetNames(const OUString& rLayoutName, std::vector<OUString> &aNameList);
     void                CreateLayoutSheetList(const OUString& rLayoutName, SdStyleSheetVector& rLayoutSheets);
-    void                CopyLayoutSheets(const OUString& rLayoutName, SdStyleSheetPool& rSourcePool, SdStyleSheetVector& rCreatedSheets );
+    void                CopyLayoutSheets(const OUString& rLayoutName, SdStyleSheetPool& rSourcePool, StyleSheetCopyResultVector& rCreatedSheets);
     void                CopyGraphicSheets(SdStyleSheetPool& rSourcePool);
     void                CopyCellSheets(SdStyleSheetPool& rSourcePool);
     void                CopyTableStyles(SdStyleSheetPool const & rSourcePool);
-    void                CopyCellSheets(SdStyleSheetPool& rSourcePool, SdStyleSheetVector& rCreatedSheets);
-    void                RenameAndCopyGraphicSheets(SdStyleSheetPool& rSourcePool, SdStyleSheetVector& rCreatedSheets, OUString const &rRenameSuffix);
+    void                CopyCellSheets(SdStyleSheetPool& rSourcePool, StyleSheetCopyResultVector& rCreatedSheets);
+    void                RenameAndCopyGraphicSheets(SdStyleSheetPool& rSourcePool, StyleSheetCopyResultVector& rCreatedSheets, OUString const &rRenameSuffix);
 
     void                CreatePseudosIfNecessary();
     void                UpdateStdNames();
@@ -117,8 +117,8 @@ public:
 
 private:
     void CopySheets(SdStyleSheetPool& rSourcePool, SfxStyleFamily eFamily );
-    void CopySheets(SdStyleSheetPool& rSourcePool, SfxStyleFamily eFamily, SdStyleSheetVector& rCreatedSheets );
-    void CopySheets(SdStyleSheetPool& rSourcePool, SfxStyleFamily eFamily, SdStyleSheetVector& rCreatedSheets, const OUString &rRenameSuffix );
+    void CopySheets(SdStyleSheetPool& rSourcePool, SfxStyleFamily eFamily, StyleSheetCopyResultVector& rCreatedSheets );
+    void CopySheets(SdStyleSheetPool& rSourcePool, SfxStyleFamily eFamily, StyleSheetCopyResultVector& rCreatedSheets, const OUString &rRenameSuffix );
 
     virtual SfxStyleSheetBase* Create(const OUString& rName, SfxStyleFamily eFamily, SfxStyleSearchBits nMask) override;
 
diff --git a/sd/inc/stlsheet.hxx b/sd/inc/stlsheet.hxx
index ae21105070a8..e8e0842e00c7 100644
--- a/sd/inc/stlsheet.hxx
+++ b/sd/inc/stlsheet.hxx
@@ -155,6 +155,19 @@ private:
 
 typedef std::vector< rtl::Reference< SdStyleSheet > > SdStyleSheetVector;
 
+struct StyleSheetCopyResult
+{
+    rtl::Reference<SdStyleSheet> m_xStyleSheet;
+    bool m_bCreatedByCopy;
+    StyleSheetCopyResult(SdStyleSheet* pStyleSheet, bool bCreatedByCopy)
+        : m_xStyleSheet(pStyleSheet)
+        , m_bCreatedByCopy(bCreatedByCopy)
+    {
+    }
+};
+
+typedef std::vector<StyleSheetCopyResult> StyleSheetCopyResultVector;
+
 #endif // INCLUDED_SD_INC_STLSHEET_HXX
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sd/source/core/drawdoc.cxx b/sd/source/core/drawdoc.cxx
index 0c152e067594..00b21c0deba8 100644
--- a/sd/source/core/drawdoc.cxx
+++ b/sd/source/core/drawdoc.cxx
@@ -637,7 +637,7 @@ SdDrawDocument* SdDrawDocument::AllocSdDrawDocument() const
             // Move with all of the master page's layouts
             OUString aOldLayoutName(const_cast<SdDrawDocument*>(this)->GetMasterSdPage(i, PageKind::Standard)->GetLayoutName());
             aOldLayoutName = aOldLayoutName.copy( 0, aOldLayoutName.indexOf( SD_LT_SEPARATOR ) );
-            SdStyleSheetVector aCreatedSheets;
+            StyleSheetCopyResultVector aCreatedSheets;
             pNewStylePool->CopyLayoutSheets(aOldLayoutName, *pOldStylePool, aCreatedSheets );
         }
 
diff --git a/sd/source/core/drawdoc3.cxx b/sd/source/core/drawdoc3.cxx
index e598dc35e981..631796dde2ae 100644
--- a/sd/source/core/drawdoc3.cxx
+++ b/sd/source/core/drawdoc3.cxx
@@ -336,26 +336,26 @@ namespace
 {
 
 void
-lcl_removeUnusedStyles(SfxStyleSheetBasePool* const pStyleSheetPool, SdStyleSheetVector& rStyles)
+lcl_removeUnusedStyles(SfxStyleSheetBasePool* const pStyleSheetPool, StyleSheetCopyResultVector& rStyles)
 {
-    SdStyleSheetVector aUsedStyles;
+    StyleSheetCopyResultVector aUsedStyles;
     aUsedStyles.reserve(rStyles.size());
-    for (SdStyleSheetVector::const_iterator aIt(rStyles.begin()), aLast(rStyles.end()); aIt != aLast; ++aIt)
+    for (const auto& a : rStyles)
     {
-        if ((*aIt)->IsUsed())
-            aUsedStyles.push_back(*aIt);
+        if (a.m_xStyleSheet->IsUsed())
+            aUsedStyles.push_back(a);
         else
-            pStyleSheetPool->Remove((*aIt).get());
+            pStyleSheetPool->Remove(a.m_xStyleSheet.get());
     }
     rStyles = aUsedStyles;
 }
 
-SfxStyleSheet *lcl_findStyle(SdStyleSheetVector& rStyles, const OUString& aStyleName)
+SfxStyleSheet *lcl_findStyle(StyleSheetCopyResultVector& rStyles, const OUString& aStyleName)
 {
-    for(SdStyleSheetVector::const_iterator aIt(rStyles.begin()), aLast(rStyles.end()); aIt != aLast; ++aIt)
+    for (const auto& a : rStyles)
     {
-        if((*aIt)->GetName().startsWith(aStyleName))
-            return (*aIt).get();
+        if (a.m_xStyleSheet->GetName().startsWith(aStyleName))
+            return a.m_xStyleSheet.get();
     }
     return nullptr;
 }
@@ -504,7 +504,7 @@ bool SdDrawDocument::InsertBookmarkAsPage(
     std::vector<OUString>::const_iterator pIter;
     for ( pIter = aLayoutsToTransfer.begin(); pIter != aLayoutsToTransfer.end(); ++pIter )
     {
-        SdStyleSheetVector aCreatedStyles;
+        StyleSheetCopyResultVector aCreatedStyles;
         OUString layoutName = *pIter;
 
         rStyleSheetPool.CopyLayoutSheets(layoutName, rBookmarkStyleSheetPool,aCreatedStyles);
@@ -523,12 +523,12 @@ bool SdDrawDocument::InsertBookmarkAsPage(
     // that are not used in any of the inserted pages. The unused styles
     // are then removed at the end of the function, where we also create
     // undo records for the inserted styles.
-    SdStyleSheetVector aNewGraphicStyles;
+    StyleSheetCopyResultVector aNewGraphicStyles;
     OUString aRenameStr;
     if(!bReplace && !bNoDialogs)
         aRenameStr = "_";
     rStyleSheetPool.RenameAndCopyGraphicSheets(rBookmarkStyleSheetPool, aNewGraphicStyles, aRenameStr);
-    SdStyleSheetVector aNewCellStyles;
+    StyleSheetCopyResultVector aNewCellStyles;
     rStyleSheetPool.CopyCellSheets(rBookmarkStyleSheetPool, aNewCellStyles);
 
     // TODO handle undo of table styles too
@@ -1304,8 +1304,12 @@ void SdDrawDocument::RemoveUnnecessaryMasterPages(SdPage* pMasterPage, bool bOnl
 
                     if( bUndo )
                     {
+                        StyleSheetCopyResultVector aUndoRemove;
+                        aUndoRemove.reserve(aRemove.size());
+                        for (const auto& a : aRemove)
+                            aUndoRemove.emplace_back(a.get(), true);
                         // This list belongs to UndoAction
-                        SdMoveStyleSheetsUndoAction* pMovStyles = new SdMoveStyleSheetsUndoAction( this, aRemove, false );
+                        SdMoveStyleSheetsUndoAction* pMovStyles = new SdMoveStyleSheetsUndoAction(this, aUndoRemove, false);
 
                         if (pUndoMgr)
                             pUndoMgr->AddUndoAction(pMovStyles);
@@ -1544,7 +1548,7 @@ void SdDrawDocument::SetMasterPage(sal_uInt16 nSdPageNum,
             pSourceStyleSheetPool->SetSearchMask(SfxStyleFamily::Page);
             static_cast<SdStyleSheetPool*>( mxStyleSheetPool.get())->SetSearchMask(SfxStyleFamily::Page);
 
-            SdStyleSheetVector aCreatedStyles;          // List of created stylesheets
+            StyleSheetCopyResultVector aCreatedStyles;          // List of created stylesheets
             SfxStyleSheetBase* pHisSheet = pSourceStyleSheetPool->First();
 
             while (pHisSheet)
@@ -1589,7 +1593,7 @@ void SdDrawDocument::SetMasterPage(sal_uInt16 nSdPageNum,
                         pMySheet->GetItemSet().ClearItem();  // Delete all
                         pMySheet->GetItemSet().Put(pHisSheet->GetItemSet());
 
-                        aCreatedStyles.emplace_back( static_cast< SdStyleSheet* >( pMySheet ) );
+                        aCreatedStyles.emplace_back(static_cast<SdStyleSheet*>(pMySheet), true);
                     }
 
                     StyleReplaceData aReplData;
@@ -1801,7 +1805,11 @@ void SdDrawDocument::SetMasterPage(sal_uInt16 nSdPageNum,
 
         if( bUndo )
         {
-            SdMoveStyleSheetsUndoAction* pMovStyles = new SdMoveStyleSheetsUndoAction(this, aCreatedStyles, true);
+            StyleSheetCopyResultVector aUndoInsert;
+            aUndoInsert.reserve(aCreatedStyles.size());
+            for (const auto& a : aCreatedStyles)
+                aUndoInsert.emplace_back(a.get(), true);
+            SdMoveStyleSheetsUndoAction* pMovStyles = new SdMoveStyleSheetsUndoAction(this, aUndoInsert, true);
             pUndoMgr->AddUndoAction(pMovStyles);
         }
 
diff --git a/sd/source/core/stlpool.cxx b/sd/source/core/stlpool.cxx
index 506cdb08a2f4..884c9ffa1e68 100644
--- a/sd/source/core/stlpool.cxx
+++ b/sd/source/core/stlpool.cxx
@@ -90,14 +90,14 @@ OUString lcl_findRenamedStyleName(std::vector< std::pair< OUString, OUString > >
     return OUString();
 }
 
-SfxStyleSheet *lcl_findStyle(SdStyleSheetVector& rStyles, const OUString& aStyleName)
+SfxStyleSheet *lcl_findStyle(StyleSheetCopyResultVector& rStyles, const OUString& aStyleName)
 {
     if( aStyleName.isEmpty() )
         return nullptr;
-    for(SdStyleSheetVector::const_iterator aIt(rStyles.begin()), aLast(rStyles.end()); aIt != aLast; ++aIt)
+    for (const auto& a : rStyles)
     {
-        if((*aIt)->GetName() == aStyleName)
-            return (*aIt).get();
+        if (a.m_xStyleSheet->GetName() == aStyleName)
+            return a.m_xStyleSheet.get();
     }
     return nullptr;
 }
@@ -586,23 +586,23 @@ void SdStyleSheetPool::CopyTableStyles(SdStyleSheetPool const & rSourcePool)
     }
 }
 
-void SdStyleSheetPool::CopyCellSheets(SdStyleSheetPool& rSourcePool, SdStyleSheetVector& rCreatedSheets)
+void SdStyleSheetPool::CopyCellSheets(SdStyleSheetPool& rSourcePool, StyleSheetCopyResultVector& rCreatedSheets)
 {
     CopySheets( rSourcePool, SfxStyleFamily::Frame, rCreatedSheets );
 }
 
-void SdStyleSheetPool::RenameAndCopyGraphicSheets(SdStyleSheetPool& rSourcePool, SdStyleSheetVector& rCreatedSheets, OUString const &rRenameSuffix)
+void SdStyleSheetPool::RenameAndCopyGraphicSheets(SdStyleSheetPool& rSourcePool, StyleSheetCopyResultVector& rCreatedSheets, OUString const &rRenameSuffix)
 {
     CopySheets( rSourcePool, SfxStyleFamily::Para, rCreatedSheets, rRenameSuffix );
 }
 
 void SdStyleSheetPool::CopySheets(SdStyleSheetPool& rSourcePool, SfxStyleFamily eFamily )
 {
-    SdStyleSheetVector aTmpSheets;
+    StyleSheetCopyResultVector aTmpSheets;
     CopySheets(rSourcePool, eFamily, aTmpSheets);
 }
 
-void SdStyleSheetPool::CopySheets(SdStyleSheetPool& rSourcePool, SfxStyleFamily eFamily, SdStyleSheetVector& rCreatedSheets)
+void SdStyleSheetPool::CopySheets(SdStyleSheetPool& rSourcePool, SfxStyleFamily eFamily, StyleSheetCopyResultVector& rCreatedSheets)
 {
     CopySheets(rSourcePool, eFamily, rCreatedSheets, "");
 }
@@ -624,7 +624,7 @@ struct HasFamilyPredicate : svl::StyleSheetPredicate
 
 }
 
-void SdStyleSheetPool::CopySheets(SdStyleSheetPool& rSourcePool, SfxStyleFamily eFamily, SdStyleSheetVector& rCreatedSheets, const OUString& rRenameSuffix)
+void SdStyleSheetPool::CopySheets(SdStyleSheetPool& rSourcePool, SfxStyleFamily eFamily, StyleSheetCopyResultVector& rCreatedSheets, const OUString& rRenameSuffix)
 {
     std::vector< std::pair< rtl::Reference< SfxStyleSheetBase >, OUString > > aNewStyles;
     std::vector< std::pair< OUString, OUString > > aRenamedList;
@@ -689,13 +689,13 @@ void SdStyleSheetPool::CopySheets(SdStyleSheetPool& rSourcePool, SfxStyleFamily
             }
             xNewSheet->GetItemSet().Put( xSheet->GetItemSet() );
 
-            rCreatedSheets.emplace_back( static_cast< SdStyleSheet* >( xNewSheet.get() ) );
+            rCreatedSheets.emplace_back(static_cast<SdStyleSheet*>(xNewSheet.get()), true);
             aRenamedList.emplace_back( xSheet->GetName(), aName );
         }
         else if (bAddToList)
         {
             // Add to list - used for renaming
-            rCreatedSheets.emplace_back( static_cast< SdStyleSheet* >( pExistingSheet ) );
+            rCreatedSheets.emplace_back(static_cast<SdStyleSheet*>(pExistingSheet), false);
             aRenamedList.emplace_back( xSheet->GetName(), aName );
         }
     }
@@ -731,7 +731,7 @@ void SdStyleSheetPool::CopySheets(SdStyleSheetPool& rSourcePool, SfxStyleFamily
 |*
 \************************************************************************/
 
-void SdStyleSheetPool::CopyLayoutSheets(const OUString& rLayoutName, SdStyleSheetPool& rSourcePool, SdStyleSheetVector& rCreatedSheets)
+void SdStyleSheetPool::CopyLayoutSheets(const OUString& rLayoutName, SdStyleSheetPool& rSourcePool, StyleSheetCopyResultVector& rCreatedSheets)
 {
     SfxStyleSheetBase* pSheet = nullptr;
 
@@ -752,7 +752,7 @@ void SdStyleSheetPool::CopyLayoutSheets(const OUString& rLayoutName, SdStyleShee
                 OUString file;
                 rNewSheet.SetHelpId( file, pSourceSheet->GetHelpId( file ) );
                 rNewSheet.GetItemSet().Put(pSourceSheet->GetItemSet());
-                rCreatedSheets.emplace_back( static_cast< SdStyleSheet* >( &rNewSheet ) );
+                rCreatedSheets.emplace_back(static_cast<SdStyleSheet*>(&rNewSheet), true);
             }
         }
     }
diff --git a/sd/source/ui/app/sdxfer.cxx b/sd/source/ui/app/sdxfer.cxx
index 5db900b412bf..5bb95259fcbe 100644
--- a/sd/source/ui/app/sdxfer.cxx
+++ b/sd/source/ui/app/sdxfer.cxx
@@ -303,7 +303,7 @@ void SdTransferable::CreateData()
         sal_Int32 nPos = aOldLayoutName.indexOf( SD_LT_SEPARATOR );
         if( nPos != -1 )
             aOldLayoutName = aOldLayoutName.copy( 0, nPos );
-        SdStyleSheetVector aCreatedSheets;
+        StyleSheetCopyResultVector aCreatedSheets;
         pNewStylePool->CopyLayoutSheets( aOldLayoutName, *pOldStylePool, aCreatedSheets );
     }
 
diff --git a/sd/source/ui/func/unmovss.cxx b/sd/source/ui/func/unmovss.cxx
index f035a873e2b9..8805b324a1cc 100644
--- a/sd/source/ui/func/unmovss.cxx
+++ b/sd/source/ui/func/unmovss.cxx
@@ -23,7 +23,7 @@
 #include <stlsheet.hxx>
 #include <stlpool.hxx>
 
-SdMoveStyleSheetsUndoAction::SdMoveStyleSheetsUndoAction( SdDrawDocument* pTheDoc, SdStyleSheetVector& rTheStyles, bool bInserted)
+SdMoveStyleSheetsUndoAction::SdMoveStyleSheetsUndoAction( SdDrawDocument* pTheDoc, StyleSheetCopyResultVector& rTheStyles, bool bInserted)
 : SdUndoAction(pTheDoc)
 , mbMySheets( !bInserted )
 {
@@ -32,9 +32,9 @@ SdMoveStyleSheetsUndoAction::SdMoveStyleSheetsUndoAction( SdDrawDocument* pTheDo
     maListOfChildLists.resize( maStyles.size() );
     // create list with lists of style sheet children
     std::size_t i = 0;
-    for(SdStyleSheetVector::iterator iter = maStyles.begin(); iter != maStyles.end(); ++iter )
+    for (auto& a : maStyles)
     {
-        maListOfChildLists[i++] = SdStyleSheetPool::CreateChildList( (*iter).get() );
+        maListOfChildLists[i++] = SdStyleSheetPool::CreateChildList(a.m_xStyleSheet.get());
     }
 }
 
@@ -47,28 +47,33 @@ void SdMoveStyleSheetsUndoAction::Undo()
         // the styles have to be inserted in the pool
 
         // first insert all styles to the pool
-        for(SdStyleSheetVector::iterator iter = maStyles.begin(); iter != maStyles.end(); ++iter )
+        for (auto& a : maStyles)
         {
-            pPool->Insert((*iter).get());
+            if (!a.m_bCreatedByCopy) // tdf#119259, existed before this action, so leave it alone
+                continue;
+            pPool->Insert(a.m_xStyleSheet.get());
         }
 
         // now assign the children again
         std::vector< SdStyleSheetVector >::iterator childlistiter( maListOfChildLists.begin() );
-        for(SdStyleSheetVector::iterator iter = maStyles.begin(); iter != maStyles.end(); ++iter, ++childlistiter )
+        for (auto& a : maStyles)
         {
-            OUString aParent((*iter)->GetName());
+            OUString aParent(a.m_xStyleSheet->GetName());
             for( SdStyleSheetVector::iterator childiter = (*childlistiter).begin(); childiter != (*childlistiter).end(); ++childiter )
             {
                 (*childiter)->SetParent(aParent);
             }
+            ++childlistiter;
         }
     }
     else
     {
         // remove the styles again from the pool
-        for(SdStyleSheetVector::iterator iter = maStyles.begin(); iter != maStyles.end(); ++iter )
+        for (auto& a : maStyles)
         {
-            pPool->Remove((*iter).get());
+            if (!a.m_bCreatedByCopy) // tdf#119259, existed before this action, so leave it alone
+                continue;
+            pPool->Remove(a.m_xStyleSheet.get());
         }
     }
     mbMySheets = !mbMySheets;
diff --git a/sd/source/ui/inc/unmovss.hxx b/sd/source/ui/inc/unmovss.hxx
index f1f80384c106..474274d0cf57 100644
--- a/sd/source/ui/inc/unmovss.hxx
+++ b/sd/source/ui/inc/unmovss.hxx
@@ -28,12 +28,12 @@ class SdDrawDocument;
 
 class SdMoveStyleSheetsUndoAction : public SdUndoAction
 {
-    SdStyleSheetVector                  maStyles;
+    StyleSheetCopyResultVector          maStyles;
     std::vector< SdStyleSheetVector >   maListOfChildLists;
     bool                                mbMySheets;
 
 public:
-    SdMoveStyleSheetsUndoAction(SdDrawDocument* pTheDoc, SdStyleSheetVector& rTheStyles, bool bInserted);
+    SdMoveStyleSheetsUndoAction(SdDrawDocument* pTheDoc, StyleSheetCopyResultVector& rTheStyles, bool bInserted);
 
     virtual ~SdMoveStyleSheetsUndoAction() override;
     virtual void Undo() override;
diff --git a/sd/source/ui/sidebar/DocumentHelper.cxx b/sd/source/ui/sidebar/DocumentHelper.cxx
index 7198a19217d8..4d3f5d144403 100644
--- a/sd/source/ui/sidebar/DocumentHelper.cxx
+++ b/sd/source/ui/sidebar/DocumentHelper.cxx
@@ -258,7 +258,7 @@ void DocumentHelper::ProvideStyles (
         static_cast<SdStyleSheetPool*>(rSourceDocument.GetStyleSheetPool());
     SdStyleSheetPool* pTargetStyleSheetPool =
         static_cast<SdStyleSheetPool*>(rTargetDocument.GetStyleSheetPool());
-    SdStyleSheetVector aCreatedStyles;
+    StyleSheetCopyResultVector aCreatedStyles;
     pTargetStyleSheetPool->CopyLayoutSheets (
         sLayoutName,
         *pSourceStyleSheetPool,