Stricter API for LIB_TABLE

Prevent nickname map or row parent getting out of sync

Fixes https://gitlab.com/kicad/code/kicad/-/issues/15315


(cherry picked from commit 4c4bbdc8f3)

common/lib_table_base.cpp

@@ -127,6 +127,13 @@ LIB_TABLE::~LIB_TABLE()
 }
 
 
+void LIB_TABLE::Clear()
+{
+    m_rows.clear();
+    m_rowsMap.clear();
+}
+
+
 bool LIB_TABLE::IsEmpty( bool aIncludeFallback )
 {
     if( !aIncludeFallback || !m_fallBack )
@@ -191,31 +198,26 @@ LIB_TABLE_ROW* LIB_TABLE::findRow( const wxString& aNickName, bool aCheckIfEnabl
 
     do
     {
-        cur->ensureIndex();
+        std::shared_lock<std::shared_mutex> lock( cur->m_mutex, std::try_to_lock );
 
-        std::shared_lock<std::shared_mutex> lock( cur->m_nickIndexMutex );
+        if( !cur->m_rowsMap.count( aNickName ) )
+            continue;
 
-        for( const std::pair<const wxString, int>& entry : cur->m_nickIndex )
+        row = &*cur->m_rowsMap.at( aNickName );
-        {
-            if( entry.first == aNickName )
-            {
-                row = &cur->m_rows[entry.second];
 
-                if( !aCheckIfEnabled || row->GetIsEnabled() )
+        if( !aCheckIfEnabled || row->GetIsEnabled() )
-                    return row;
+            return row;
-            }
-        }
 
         // Repeat, this time looking for names that were "fixed" by legacy versions because
         // the old eeschema file format didn't support spaces in tokens.
-        for( const std::pair<const wxString, int>& entry : cur->m_nickIndex )
+        for( const std::pair<const wxString, LIB_TABLE_ROWS_ITER>& entry : cur->m_rowsMap )
         {
             wxString legacyLibName = entry.first;
             legacyLibName.Replace( " ", "_" );
 
             if( legacyLibName == aNickName )
             {
-                row = &cur->m_rows[entry.second];
+                row = &*entry.second;
 
                 if( !aCheckIfEnabled || row->GetIsEnabled() )
                     return row;
@@ -235,9 +237,7 @@ const LIB_TABLE_ROW* LIB_TABLE::FindRowByURI( const wxString& aURI )
 
     do
     {
-        cur->ensureIndex();
+        for( unsigned i = 0; i < cur->m_rows.size(); i++ )
-
-        for( unsigned i = 0;  i < cur->m_rows.size();  i++ )
         {
             wxString tmp = cur->m_rows[i].GetFullURI( true );
 
@@ -276,10 +276,10 @@ std::vector<wxString> LIB_TABLE::GetLogicalLibs()
 
     do
     {
-        for( LIB_TABLE_ROWS_CITER it = cur->m_rows.begin();  it!=cur->m_rows.end();  ++it )
+        for( const LIB_TABLE_ROW& row : cur->m_rows )
         {
-            if( it->GetIsEnabled() )
+            if( row.GetIsEnabled() )
-                unique.insert( it->GetNickName() );
+                unique.insert( row.GetNickName() );
         }
 
     } while( ( cur = cur->m_fallBack ) != nullptr );
@@ -303,28 +303,120 @@ std::vector<wxString> LIB_TABLE::GetLogicalLibs()
 
 bool LIB_TABLE::InsertRow( LIB_TABLE_ROW* aRow, bool doReplace )
 {
-    ensureIndex();
+    std::lock_guard<std::shared_mutex> lock( m_mutex );
 
-    std::lock_guard<std::shared_mutex> lock( m_nickIndexMutex );
+    auto it = m_rowsMap.find( aRow->GetNickName() );
 
-    INDEX_CITER it = m_nickIndex.find( aRow->GetNickName() );
+    if( it != m_rowsMap.end() )
+    {
+        if( !doReplace )
+            return false;
 
-    aRow->SetParent( this );
+        m_rows.replace( it->second, aRow );
-
+    }
-    if( it == m_nickIndex.end() )
+    else
     {
         m_rows.push_back( aRow );
-        m_nickIndex.insert( INDEX_VALUE( aRow->GetNickName(), m_rows.size() - 1 ) );
-        return true;
     }
 
-    if( doReplace )
+    aRow->SetParent( this );
+    reindex();
+    return true;
+}
+
+
+bool LIB_TABLE::RemoveRow( const LIB_TABLE_ROW* aRow )
+{
+    std::lock_guard<std::shared_mutex> lock( m_mutex );
+
+    bool found = false;
+    auto it = m_rowsMap.find( aRow->GetNickName() );
+
+    if( it != m_rowsMap.end() )
     {
-        m_rows.replace( it->second, aRow );
+        if( &*it->second == aRow )
-        return true;
+        {
+            found = true;
+            m_rows.erase( it->second );
+        }
     }
 
-    return false;
+    if( !found )
+    {
+        // Bookkeeping got messed up...
+        for( size_t i = m_rows.size() - 1; i >= 0; --i )
+        {
+            if( &m_rows[i] == aRow )
+            {
+                m_rows.erase( m_rows.begin() + i );
+                found = true;
+                break;
+            }
+        }
+    }
+
+    if( found )
+        reindex();
+
+    return found;
+}
+
+
+bool LIB_TABLE::ReplaceRow( size_t aIndex, LIB_TABLE_ROW* aRow )
+{
+    std::lock_guard<std::shared_mutex> lock( m_mutex );
+
+    if( aIndex >= m_rows.size() )
+        return false;
+
+    m_rowsMap.erase( m_rows[aIndex].GetNickName() );
+
+    m_rows.replace( aIndex, aRow );
+    reindex();
+    return true;
+}
+
+
+bool LIB_TABLE::ChangeRowOrder( size_t aIndex, int aOffset )
+{
+    std::lock_guard<std::shared_mutex> lock( m_mutex );
+
+    if( aIndex >= m_rows.size() )
+        return false;
+
+    int newPos = static_cast<int>( aIndex ) + aOffset;
+
+    if( newPos < 0 || newPos > static_cast<int>( m_rows.size() ) - 1 )
+        return false;
+
+    auto element = m_rows.release( m_rows.begin() + aIndex );
+
+    m_rows.insert( m_rows.begin() + newPos, element.release() );
+    reindex();
+
+    return true;
+}
+
+
+void LIB_TABLE::TransferRows( LIB_TABLE_ROWS& aRowsList )
+{
+    std::lock_guard<std::shared_mutex> lock( m_mutex );
+
+    m_rows.transfer( m_rows.end(), aRowsList.begin(), aRowsList.end(), aRowsList );
+
+    reindex();
+}
+
+
+void LIB_TABLE::reindex()
+{
+    m_rowsMap.clear();
+
+    for( LIB_TABLE_ROWS_ITER it = m_rows.begin(); it != m_rows.end(); ++it )
+    {
+        it->SetParent( this );
+        m_rowsMap[it->GetNickName()] = it;
+    }
 }
 
 

eeschema/dialogs/panel_sym_lib_table.cpp

@@ -180,7 +180,7 @@ protected:
                     tbl->AppendRows( tmp_tbl.GetCount() - tbl->GetNumberRows() );
 
                 for( unsigned i = 0;  i < tmp_tbl.GetCount();  ++i )
-                    tbl->m_rows.replace( i, tmp_tbl.At( i ).clone() );
+                    tbl->ReplaceRow( i, tmp_tbl.At( i ).clone() );
             }
 
             m_grid->AutoSizeColumns( false );
@@ -447,9 +447,6 @@ bool PANEL_SYM_LIB_TABLE::verifyTables()
         {
             SYMBOL_LIB_TABLE_ROW& row = dynamic_cast<SYMBOL_LIB_TABLE_ROW&>( table->At( r ) );
 
-            if( !row.GetParent() )
-                row.SetParent( table );
-
             if( !row.GetIsEnabled() )
                 continue;
 
@@ -667,11 +664,7 @@ void PANEL_SYM_LIB_TABLE::moveUpHandler( wxCommandEvent& event )
     // @todo: add multiple selection moves.
     if( curRow >= 1 )
     {
-        boost::ptr_vector< LIB_TABLE_ROW >::auto_type move_me =
+        tbl->ChangeRowOrder( curRow--, -1 );
-            tbl->m_rows.release( tbl->m_rows.begin() + curRow );
-
-        --curRow;
-        tbl->m_rows.insert( tbl->m_rows.begin() + curRow, move_me.release() );
 
         if( tbl->GetView() )
         {
@@ -697,11 +690,7 @@ void PANEL_SYM_LIB_TABLE::moveDownHandler( wxCommandEvent& event )
     // @todo: add multiple selection moves.
     if( unsigned( curRow + 1 ) < tbl->m_rows.size() )
     {
-        boost::ptr_vector< LIB_TABLE_ROW >::auto_type move_me =
+        tbl->ChangeRowOrder( curRow++, 1 );
-            tbl->m_rows.release( tbl->m_rows.begin() + curRow );
-
-        ++curRow;
-        tbl->m_rows.insert( tbl->m_rows.begin() + curRow, move_me.release() );
 
         if( tbl->GetView() )
         {
@@ -889,9 +878,7 @@ bool PANEL_SYM_LIB_TABLE::TransferDataFromWindow()
         m_parent->m_GlobalTableChanged = true;
 
         m_globalTable->Clear();
-        m_globalTable->m_rows.transfer( m_globalTable->m_rows.end(), global_model()->m_rows.begin(),
+        m_globalTable->TransferRows( global_model()->m_rows );
-                                      global_model()->m_rows.end(), global_model()->m_rows );
-        m_globalTable->reindex();
     }
 
     if( project_model() && *project_model() != *m_projectTable )
@@ -899,9 +886,7 @@ bool PANEL_SYM_LIB_TABLE::TransferDataFromWindow()
         m_parent->m_ProjectTableChanged = true;
 
         m_projectTable->Clear();
-        m_projectTable->m_rows.transfer( m_projectTable->m_rows.end(), project_model()->m_rows.begin(),
+        m_projectTable->TransferRows( project_model()->m_rows );
-                                       project_model()->m_rows.end(), project_model()->m_rows );
-        m_projectTable->reindex();
     }
 
     return true;

eeschema/symbol_lib_table.cpp

@@ -281,10 +281,8 @@ void SYMBOL_LIB_TABLE::Format( OUTPUTFORMATTER* aOutput, int aIndentLevel ) cons
     aOutput->Print( aIndentLevel, "(sym_lib_table\n" );
     aOutput->Print( aIndentLevel + 1, "(version %d)\n", m_version );
 
-    for( LIB_TABLE_ROWS_CITER it = m_rows.begin();  it != m_rows.end();  ++it )
+    for( const LIB_TABLE_ROW& row : m_rows )
-    {
+        row.Format( aOutput, aIndentLevel + 1 );
-        it->Format( aOutput, aIndentLevel+1 );
-    }
 
     aOutput->Print( aIndentLevel, ")\n" );
 }
@@ -359,7 +357,9 @@ void SYMBOL_LIB_TABLE::LoadSymbolLib( std::vector<LIB_SYMBOL*>& aSymbolList,
                                       const wxString& aNickname, bool aPowerSymbolsOnly )
 {
     SYMBOL_LIB_TABLE_ROW* row = FindRow( aNickname, true );
-    wxCHECK( row && row->plugin, /* void */  );
+
+    if( !row || !row->plugin )
+        return;
 
     std::lock_guard<std::mutex> lock( row->GetMutex() );
 

include/lib_table_base.h

@@ -328,13 +328,7 @@ public:
     virtual ~LIB_TABLE();
 
     /// Delete all rows.
-    void Clear()
+    void Clear();
-    {
-        std::lock_guard<std::shared_mutex> lock( m_nickIndexMutex );
-
-        m_rows.clear();
-        m_nickIndex.clear();
-    }
 
     /**
      * Compares this table against another.
@@ -444,23 +438,30 @@ public:
     bool InsertRow( LIB_TABLE_ROW* aRow, bool doReplace = false );
 
     /**
-     * Removes a row from the table.
+     * Removes a row from the table and frees the pointer
      * @param aRow is the row to remove
      * @return true if the row was found (and removed)
      */
-    bool RemoveRow( const LIB_TABLE_ROW* aRow )
+    bool RemoveRow( const LIB_TABLE_ROW* aRow );
-    {
+
-        for( auto iter = m_rows.begin(); iter != m_rows.end(); ++iter )
+    /**
-        {
+     * Replaces the Nth row with the given new row
-            if( *iter == *aRow )
+     * @return true if successful
-            {
+     */
-                m_rows.erase( iter, iter + 1 );
+    bool ReplaceRow( size_t aIndex, LIB_TABLE_ROW* aRow );
-                reindex();
+
-                return true;
+    /**
-            }
+     * Moves a row within the table
-        }
+     * @param aIndex is the current index of the row to move
-        return false;
+     * @param aOffset is the number of positions to move it by in the table
-    }
+     * @return true if the move resulted in a change
+     */
+    bool ChangeRowOrder( size_t aIndex, int aOffset );
+
+    /**
+     * Takes ownership of another list of rows; the original list will be freed
+     */
+    void TransferRows( LIB_TABLE_ROWS& aRowsList );
 
     /**
      * @return a #LIB_TABLE_ROW pointer if \a aURI is found in this table or in any chained
@@ -521,18 +522,6 @@ public:
         return m_version;
     }
 
-    /**
-     * While this is an encapsulation leak, calling it before threaded loads *may* prevent
-     * some Sentry crashes we're seeing (KICAD-4S).
-     */
-    void EnsureIndex()
-    {
-        ensureIndex();
-
-        if( m_fallBack )
-            m_fallBack->EnsureIndex();
-    }
-
 protected:
     /**
      * Return a #LIB_TABLE_ROW if \a aNickname is found in this table or in any chained
@@ -552,60 +541,24 @@ protected:
      */
     bool migrate();
 
-    /**
+    void reindex();
-     * Rebuilds the m_nickIndex
-     *
-     * @param aForce is to avoid rebuilding the index multiple times because multiple threads hit ensureIndex
-     * at the same time
-     */
-    void reindex()
-    {
-        std::lock_guard<std::shared_mutex> lock( m_nickIndexMutex );
-
-        m_nickIndex.clear();
-
-        for( LIB_TABLE_ROWS_ITER it = m_rows.begin(); it != m_rows.end(); ++it )
-            m_nickIndex.insert( INDEX_VALUE( it->GetNickName(), it - m_rows.begin() ) );
-    }
-
-    void ensureIndex()
-    {
-        // The dialog lib table editor may not maintain the nickIndex.
-        // Lazy indexing may be required.  To handle lazy indexing, we must enforce
-        // that "nickIndex" is either empty or accurate, but never inaccurate.
-        {
-            std::shared_lock<std::shared_mutex> lock( m_nickIndexMutex );
-
-            if( m_nickIndex.size() )
-                return;
-        }
-
-        reindex();
-    }
-
-private:
-    friend class PANEL_FP_LIB_TABLE;
-    friend class LIB_TABLE_GRID;
 
 protected:
-    LIB_TABLE_ROWS m_rows;
-
-    /// this is a non-owning index into the LIB_TABLE_ROWS table
-    typedef std::map<wxString,int>      INDEX;              // "int" is std::vector array index
-    typedef INDEX::iterator             INDEX_ITER;
-    typedef INDEX::const_iterator       INDEX_CITER;
-    typedef INDEX::value_type           INDEX_VALUE;
-
-    /// this particular key is the nickName within each row.
-    INDEX m_nickIndex;
-
     LIB_TABLE* m_fallBack;
 
     /// Versioning to handle importing old tables
     mutable int m_version;
 
-    /// Mutex to protect access to the nickIndex variable
+    /// Owning set of rows.
-    mutable std::shared_mutex m_nickIndexMutex;
+    // TODO: This should really be private; but the lib table grids re-use it
+    //       (without using m_rowsMap).
+    LIB_TABLE_ROWS m_rows;
+
+    /// this is a non-owning index into the LIB_TABLE_ROWS table
+    std::map<wxString, LIB_TABLE_ROWS_ITER> m_rowsMap;
+
+    /// Mutex to protect access to the rows vector
+    mutable std::shared_mutex m_mutex;
 };
 
 #endif  // _LIB_TABLE_BASE_H_

pcbnew/dialogs/panel_fp_lib_table.cpp

@@ -987,9 +987,7 @@ bool PANEL_FP_LIB_TABLE::TransferDataFromWindow()
             m_parent->m_GlobalTableChanged = true;
 
             m_global->Clear();
-            m_global->m_rows.transfer( m_global->m_rows.end(), global_model()->m_rows.begin(),
+            m_global->TransferRows( global_model()->m_rows );
-                                     global_model()->m_rows.end(), global_model()->m_rows );
-            m_global->reindex();
         }
 
         if( project_model() && *project_model() != *m_project )
@@ -997,9 +995,7 @@ bool PANEL_FP_LIB_TABLE::TransferDataFromWindow()
             m_parent->m_ProjectTableChanged = true;
 
             m_project->Clear();
-            m_project->m_rows.transfer( m_project->m_rows.end(), project_model()->m_rows.begin(),
+            m_project->TransferRows( project_model()->m_rows );
-                                      project_model()->m_rows.end(), project_model()->m_rows );
-            m_project->reindex();
         }
 
         return true;

pcbnew/footprint_info_impl.cpp

@@ -268,12 +268,6 @@ void FOOTPRINT_LIST_IMPL::loadFootprints()
                 return 1;
             };
 
-    // While the private ensureIndex() is supposedly thread-safe, and having a public call is
-    // a bit of an encapsulation leak, we have at least one non-reproducible Sentry issue
-    // (KICAD-4S) that *might* be prevented by doing the EnsureIndex() before spooling up the
-    // multi-threaded part.
-    m_lib_table->EnsureIndex();
-
     for( size_t ii = 0; ii < num_elements; ++ii )
         returns[ii] = tp.submit( fp_thread );