pcbnew: Ensure Connectivity RTree gets updated

When removing items, we perform a two-pass removal, checking first
for the cached rectangle collision.  Then, if we do not find the item
being removed, we perform a second, more expensive pass over the full
tree.  This second pass is required as we cannot be certain that an
item's bbox has not been modified between insertion and deletion.  In
which case, keeping stale pointers in the tree will lead to segfaults.

Fixes: lp:1777246
* https://bugs.launchpad.net/kicad/+bug/1777246

include/geometry/rtree.h

@@ -116,7 +116,8 @@ public:
     /// \param a_min Min of bounding rect
     /// \param a_max Max of bounding rect
     /// \param a_dataId Positive Id of data.  Maybe zero, but negative numbers not allowed.
-    void Remove( const ELEMTYPE     a_min[NUMDIMS],
+    /// \return  1 if record not found, 0 if success.
+    bool Remove( const ELEMTYPE     a_min[NUMDIMS],
                  const ELEMTYPE     a_max[NUMDIMS],
                  const DATATYPE&    a_dataId );
 
@@ -666,7 +667,7 @@ void RTREE_QUAL::Insert( const ELEMTYPE     a_min[NUMDIMS],
 
 
 RTREE_TEMPLATE
-void RTREE_QUAL::Remove( const ELEMTYPE     a_min[NUMDIMS],
+bool RTREE_QUAL::Remove( const ELEMTYPE     a_min[NUMDIMS],
                          const ELEMTYPE     a_max[NUMDIMS],
                          const DATATYPE&    a_dataId )
 {
@@ -687,7 +688,7 @@ void RTREE_QUAL::Remove( const ELEMTYPE     a_min[NUMDIMS],
         rect.m_max[axis]    = a_max[axis];
     }
 
-    RemoveRect( &rect, a_dataId, &m_root );
+    return RemoveRect( &rect, a_dataId, &m_root );
 }
 
 

pcbnew/connectivity_algo.h

@@ -284,6 +284,7 @@ private:
     ///> valid flag, used to identify garbage items (we use lazy removal)
     bool m_valid;
 
+protected:
     ///> dirty flag, used to identify recently added item not yet scanned into the connectivity search
     bool m_dirty;
 
@@ -560,9 +561,12 @@ public:
         return m_cachedPoly->ContainsPoint( p, zone->GetMinThickness() );
     }
 
-    const BOX2I& BBox() const
+    const BOX2I& BBox()
     {
-        return m_cachedPoly->BBox();
+        if( m_dirty )
+            m_bbox = m_cachedPoly->BBox();
+
+        return m_bbox;
     }
 
     virtual int             AnchorCount() const override;

pcbnew/connectivity_rtree.h

@@ -69,11 +69,23 @@ public:
      */
     void Remove( T aItem )
     {
+
+        // First, attempt to remove the item using its given BBox
         const BOX2I&    bbox    = aItem->BBox();
         const int       mmin[2] = { bbox.GetX(), bbox.GetY() };
         const int       mmax[2] = { bbox.GetRight(), bbox.GetBottom() };
 
-        m_tree->Remove( mmin, mmax, aItem );
+        // If we are not successful ( 1 == not found ), then we expand
+        // the search to the full tree
+        if( m_tree->Remove( mmin, mmax, aItem ) )
+        {
+            // N.B. We must search the whole tree for the pointer to remove
+            // because the item may have been moved before we have the chance to
+            // delete it from the tree
+            const int       mmin2[2] = { INT_MIN, INT_MIN };
+            const int       mmax2[2] = { INT_MAX, INT_MAX };
+            m_tree->Remove( mmin2, mmax2, aItem );
+        }
     }
 
     /**