From e2bd6ec8f73e0943a2dd9b9328642f578504497f Mon Sep 17 00:00:00 2001
From: Ilya Sinitsyn <the_siv@mail.ru>
Date: Sat, 7 Sep 2019 01:29:47 +0700
Subject: [PATCH] Fix clearing selection in a grid with reordered columns

Since the changes of 04f7f1fd32a9f061bb5ad41d709a9e9ea7fe76c8 (frozen
rows/columns implementation), RefreshBlock() could call GetColPos() with
an invalid index. This didn't matter most of the time as the function
simply returned the same index as long as the columns were using their
natural order, but resulted in a crash due to an out of bound access to
m_colAt array as soon as they were reordered.

Fix this by avoiding using invalid indices in RefreshBlock() and, more
generally, improving its precondition check and making the assumptions
about the input parameters more clear. Also add a defensive check to
GetColPos() itself.

Finally, add a unit test exercising this code.

Closes https://github.com/wxWidgets/wxWidgets/pull/1536
---
 include/wx/generic/grid.h   |  4 ++++
 src/generic/grid.cpp        | 27 +++++++++++++++++++++++++++
 tests/controls/gridtest.cpp | 31 +++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+)

diff --git a/include/wx/generic/grid.h b/include/wx/generic/grid.h
index 01a02393b3..c81e3fc9dd 100644
--- a/include/wx/generic/grid.h
+++ b/include/wx/generic/grid.h
@@ -1131,6 +1131,8 @@ public:
                          const wxArrayString& lines,
                          long *width, long *height ) const;
 
+    // If bottomRight is invalid, i.e. == wxGridNoCellCoords, it defaults to
+    // topLeft. If topLeft itself is invalid, the function simply returns.
     void RefreshBlock(const wxGridCellCoords& topLeft,
                       const wxGridCellCoords& bottomRight);
 
@@ -1522,6 +1524,8 @@ public:
     // reverse mapping currently
     int GetColPos(int idx) const
     {
+        wxASSERT_MSG( idx >= 0 && idx < m_numCols, "invalid column index" );
+
         if ( m_colAt.IsEmpty() )
             return idx;
 
diff --git a/src/generic/grid.cpp b/src/generic/grid.cpp
index b7f7f387d6..3b2b8cfbe7 100644
--- a/src/generic/grid.cpp
+++ b/src/generic/grid.cpp
@@ -5277,6 +5277,33 @@ void wxGrid::RefreshBlock(const wxGridCellCoords& topLeft,
 void wxGrid::RefreshBlock(int topRow, int leftCol,
                           int bottomRow, int rightCol)
 {
+    // Note that it is valid to call this function with wxGridNoCellCoords as
+    // either or even both arguments, but we can't have a mix of valid and
+    // invalid columns/rows for each corner coordinates.
+    const bool noTopLeft = topRow == -1 || leftCol == -1;
+    const bool noBottomRight = bottomRow == -1 || rightCol == -1;
+
+    if ( noTopLeft )
+    {
+        // So check that either both or none of the components are valid.
+        wxASSERT( topRow == -1 && leftCol == -1 );
+
+        // And specifying bottom right corner when the top left one is not
+        // specified doesn't make sense neither.
+        wxASSERT( noBottomRight );
+
+        return;
+    }
+
+    if ( noBottomRight )
+    {
+        wxASSERT( bottomRow == -1 && rightCol == -1 );
+
+        bottomRow = topRow;
+        rightCol = leftCol;
+    }
+
+
     int row = topRow;
     int col = leftCol;
 
diff --git a/tests/controls/gridtest.cpp b/tests/controls/gridtest.cpp
index 62f9a3d729..76912fc499 100644
--- a/tests/controls/gridtest.cpp
+++ b/tests/controls/gridtest.cpp
@@ -49,6 +49,7 @@ private:
     CPPUNIT_TEST_SUITE( GridTestCase );
         WXUISIM_TEST( CellEdit );
         NONGTK_TEST( CellClick );
+        NONGTK_TEST( ReorderedColumnsCellClick );
         NONGTK_TEST( CellSelect );
         NONGTK_TEST( LabelClick );
         NONGTK_TEST( SortClick );
@@ -83,6 +84,7 @@ private:
 
     void CellEdit();
     void CellClick();
+    void ReorderedColumnsCellClick();
     void CellSelect();
     void LabelClick();
     void SortClick();
@@ -232,6 +234,35 @@ void GridTestCase::CellClick()
 #endif
 }
 
+void GridTestCase::ReorderedColumnsCellClick()
+{
+#if wxUSE_UIACTIONSIMULATOR
+    EventCounter click(m_grid, wxEVT_GRID_CELL_LEFT_CLICK);
+
+    wxUIActionSimulator sim;
+
+    wxArrayInt neworder;
+    neworder.push_back(1);
+    neworder.push_back(0);
+
+    m_grid->SetColumnsOrder(neworder);
+
+    wxRect rect = m_grid->CellToRect(0, 1);
+    wxPoint point = m_grid->CalcScrolledPosition(rect.GetPosition());
+    point = m_grid->ClientToScreen(point + wxPoint(m_grid->GetRowLabelSize(),
+        m_grid->GetColLabelSize())
+        + wxPoint(2, 2));
+
+    sim.MouseMove(point);
+    wxYield();
+
+    sim.MouseClick();
+    wxYield();
+
+    CPPUNIT_ASSERT_EQUAL(1, click.GetCount());
+#endif
+}
+
 void GridTestCase::CellSelect()
 {
 #if wxUSE_UIACTIONSIMULATOR