storing WeakReference in std::set is a bad idea

because if we allocate two things in the same location, we can subtlely
lose the second one.

Change-Id: I4dda5c738802da8544d280c020e059da63b5d71c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/145779
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

2 files changed, 29 insertions, 31 deletions

diff --git a/sw/inc/unochart.hxx b/sw/inc/unochart.hxx
index d56b3356da0a..956d5f721e94 100644
--- a/sw/inc/unochart.hxx
+++ b/sw/inc/unochart.hxx
@@ -20,7 +20,7 @@
 #define INCLUDED_SW_INC_UNOCHART_HXX
 
 #include <map>
-#include <set>
+#include <vector>
 
 #include <com/sun/star/chart2/data/XDataProvider.hpp>
 #include <com/sun/star/chart2/data/XDataSource.hpp>
@@ -105,20 +105,11 @@ class SwChartDataProvider final :
 {
 
     // used to keep weak-references to all data-sequences of a single table
-    // see set definition below...
-    struct lt_DataSequenceRef
-    {
-        bool operator()( const unotools::WeakReference< SwChartDataSequence > & xWRef1, const unotools::WeakReference< SwChartDataSequence >& xWRef2 ) const
-        {
-            rtl::Reference< SwChartDataSequence > xRef1( xWRef1 );
-            rtl::Reference< SwChartDataSequence > xRef2( xWRef2 );
-            return xRef1.get() < xRef2.get();
-        }
-    };
-    typedef std::set< unotools::WeakReference < SwChartDataSequence >, lt_DataSequenceRef > Set_DataSequenceRef_t;
+    // see definition below...
+    typedef std::vector< unotools::WeakReference < SwChartDataSequence > > Vec_DataSequenceRef_t;
 
     // map of data-sequence sets for each table
-    typedef std::map< const SwTable *, Set_DataSequenceRef_t > Map_Set_DataSequenceRef_t;
+    typedef std::map< const SwTable *, Vec_DataSequenceRef_t > Map_Set_DataSequenceRef_t;
 
     // map of all data-sequences provided directly or indirectly (e.g. via
     // data-source) by this object. Since there is only one object of this type
diff --git a/sw/source/core/unocore/unochart.cxx b/sw/source/core/unocore/unochart.cxx
index 14be7d0c711e..40fa3b0fe882 100644
--- a/sw/source/core/unocore/unochart.cxx
+++ b/sw/source/core/unocore/unochart.cxx
@@ -1405,12 +1405,23 @@ uno::Sequence< OUString > SAL_CALL SwChartDataProvider::getSupportedServiceNames
 
 void SwChartDataProvider::AddDataSequence( const SwTable &rTable, rtl::Reference< SwChartDataSequence > const &rxDataSequence )
 {
-    m_aDataSequences[ &rTable ].insert( rxDataSequence );
+    Vec_DataSequenceRef_t& rVec = m_aDataSequences[ &rTable ];
+    assert(std::find_if(rVec.begin(), rVec.end(),
+        [&rxDataSequence](const unotools::WeakReference < SwChartDataSequence >& i)
+        {
+            return i.get() == rxDataSequence;
+        }) == rVec.end() && "duplicate insert");
+    rVec.push_back( rxDataSequence );
 }
 
 void SwChartDataProvider::RemoveDataSequence( const SwTable &rTable, rtl::Reference< SwChartDataSequence > const &rxDataSequence )
 {
-    m_aDataSequences[ &rTable ].erase( rxDataSequence );
+    Vec_DataSequenceRef_t& rVec = m_aDataSequences[ &rTable ];
+    rVec.erase( std::remove_if(rVec.begin(), rVec.end(),
+        [&rxDataSequence](const unotools::WeakReference < SwChartDataSequence >& i)
+        {
+            return i.get() == rxDataSequence;
+        }), rVec.end());
 }
 
 void SwChartDataProvider::InvalidateTable( const SwTable *pTable, bool bImmediate )
@@ -1422,8 +1433,8 @@ void SwChartDataProvider::InvalidateTable( const SwTable *pTable, bool bImmediat
     if (!m_bDisposed)
        pTable->GetFrameFormat()->GetDoc()->getIDocumentChartDataProviderAccess().GetChartControllerHelper().StartOrContinueLocking();
 
-    const Set_DataSequenceRef_t &rSet = m_aDataSequences[ pTable ];
-    for (const unotools::WeakReference<SwChartDataSequence>& rItem : rSet)
+    const Vec_DataSequenceRef_t &rVec = m_aDataSequences[ pTable ];
+    for (const unotools::WeakReference<SwChartDataSequence>& rItem : rVec)
     {
         rtl::Reference< SwChartDataSequence > xRef(rItem);
         if (xRef.is())
@@ -1447,13 +1458,11 @@ void SwChartDataProvider::DeleteBox( const SwTable *pTable, const SwTableBox &rB
     if (!m_bDisposed)
         pTable->GetFrameFormat()->GetDoc()->getIDocumentChartDataProviderAccess().GetChartControllerHelper().StartOrContinueLocking();
 
-    Set_DataSequenceRef_t &rSet = m_aDataSequences[ pTable ];
+    Vec_DataSequenceRef_t &rVec = m_aDataSequences[ pTable ];
 
     // iterate over all data-sequences for that table...
-    Set_DataSequenceRef_t::iterator aIt( rSet.begin() );
-    Set_DataSequenceRef_t::iterator aEndIt( rSet.end() );
-    Set_DataSequenceRef_t::iterator aDelIt;     // iterator used for deletion when appropriate
-    while (aIt != aEndIt)
+    auto aIt( rVec.begin() );
+    while (aIt != rVec.end())
     {
         bool bNowEmpty = false;
         bool bSeqDisposed = false;
@@ -1472,18 +1481,16 @@ void SwChartDataProvider::DeleteBox( const SwTable *pTable, const SwTableBox &rB
                 bNowEmpty = true;
                 bSeqDisposed = true;
             }
-
-            if (bNowEmpty)
-                aDelIt = aIt;
         }
-        ++aIt;
 
         if (bNowEmpty)
         {
-            rSet.erase( aDelIt );
+            aIt = rVec.erase( aIt );
             if (pDataSeq && !bSeqDisposed)
                 pDataSeq->dispose();    // the current way to tell chart that sth. got removed
         }
+        else
+            ++aIt;
     }
 }
 
@@ -1500,9 +1507,9 @@ void SwChartDataProvider::DisposeAllDataSequences( const SwTable *pTable )
     //! This is necessary since calling 'dispose' will implicitly remove an element
     //! of the original container, and thus any iterator in the original container
     //! would become invalid.
-    const Set_DataSequenceRef_t aSet( m_aDataSequences[ pTable ] );
+    const Vec_DataSequenceRef_t aVec( m_aDataSequences[ pTable ] );
 
-    for (const unotools::WeakReference<SwChartDataSequence>& rItem : aSet)
+    for (const unotools::WeakReference<SwChartDataSequence>& rItem : aVec)
     {
         rtl::Reference< SwChartDataSequence > xRef(rItem);
         if (xRef.is())
@@ -1572,8 +1579,8 @@ void SwChartDataProvider::AddRowCols(
     }
 
     // iterate over all data-sequences for the table
-    const Set_DataSequenceRef_t &rSet = m_aDataSequences[ &rTable ];
-    for (const unotools::WeakReference<SwChartDataSequence>& rItem : rSet)
+    const Vec_DataSequenceRef_t &rVec = m_aDataSequences[ &rTable ];
+    for (const unotools::WeakReference<SwChartDataSequence>& rItem : rVec)
     {
         rtl::Reference< SwChartDataSequence > pDataSeq(rItem);
         if (pDataSeq.is())