Phoenix icon indicating copy to clipboard operation
Phoenix copied to clipboard

Fatal exception when dataViewListCtrl.RowToItem without item

Open antonio-fr opened this issue 2 years ago • 14 comments

Operating system: wxPython version & source: pip wxPython 4.1.1 Python version & source: 3.9.11 Description of the problem: Windows fatal exception when dataViewListCtrl.RowToItem when no item was added to the DVLC.

MydataViewListCtrl1.RowToItem(0) throws Windows fatal exception: access violation when there's no item in the dataViewList.

Trace extract :

Python\Python39\lib\site-packages\wx\core.py, line 3407 in <lambda>
Python39\lib\site-packages\wx\core.py, line 2237 in MainLoop

antonio-fr avatar Apr 23 '22 16:04 antonio-fr

This may comes from the underlying wx C++ code. Let me know your investigation, I'll open a token on their side if needed.

antonio-fr avatar Apr 23 '22 16:04 antonio-fr

There is no row 0.

>>> dvlc.GetStore().GetCount()
0

I get two assertions triggered, which is correct behaviour.

grafik

DietmarSchwertberger avatar Apr 23 '22 17:04 DietmarSchwertberger

wxWidgets uses GetItem internally in several other methods. I doubt that it would make sense to make it more forgiving.

wxDataViewItem wxDataViewIndexListModel::GetItem( unsigned int row ) const
{
    wxASSERT( row < m_hash.GetCount() );
    return wxDataViewItem( m_hash[row] );
}

DietmarSchwertberger avatar Apr 23 '22 18:04 DietmarSchwertberger

That doesn't explain the access violation. I'm quite sure there's a bug somewhere, I have to investigate and share a minimal PoC.

antonio-fr avatar Apr 24 '22 10:04 antonio-fr

And maybe this is an issue when one calls the RowToItem method but the dataViewListCtrl was not fully created.

antonio-fr avatar Apr 24 '22 10:04 antonio-fr

There is an assertion. If you suppress assertions, this results in an address exception immediately afterwards. There is no bug.

DietmarSchwertberger avatar Apr 24 '22 12:04 DietmarSchwertberger

Here's a minimal PoC https://pastebin.com/fjf3ySx3

It crashes when as soon as it calls RowToItem before there's any element in the DataViewList. On Windows Python 3.9.11 and wxPython 4.1.1.

antonio-fr avatar Apr 24 '22 16:04 antonio-fr

And here's a small variant of the issue. When the index in RowToItem is less than -1, it crashes, whatever there's item in the list or not.

def add_line(self, event):
    self.list_ctrl.AppendItem(["hello"])
    self.list_ctrl.RowToItem(-3)

antonio-fr avatar Apr 24 '22 16:04 antonio-fr

And do you mean that when we does like that, when this is encountered, internally in WX there's an assertion, taht throws an underlying exception and the result is a fatal exception which immediately terminates the whole Python program running ? I would just expect there would have been somes checks in the Python code to prevent a such brutal close. Of course in the software it is easy not to be in this situation, which I only face in testing. Still if this surges in "real life applications", the applications would abruptly quits, like an emergency exit.

antonio-fr avatar Apr 24 '22 16:04 antonio-fr

To me, it should at least throw a Python exception, so we can catch it and see the stack trace eventually.

antonio-fr avatar Apr 24 '22 16:04 antonio-fr

Add app.SetAssertMode( wx.APP_ASSERT_DIALOG )

The default is wx.APP_ASSERT_EXCEPTION. In https://github.com/wxWidgets/Phoenix/blob/master/src/app_ex.cpp lines 144ff this will set a Python exception. Unfortunately, the address exception avoids the function returning.

DietmarSchwertberger avatar Apr 24 '22 17:04 DietmarSchwertberger

I will open an issue on wxWidgets to get their feedback on argument checking.

DietmarSchwertberger avatar Apr 24 '22 17:04 DietmarSchwertberger

Let's wait for feedback from https://github.com/wxWidgets/wxWidgets/issues/22358

DietmarSchwertberger avatar Apr 24 '22 19:04 DietmarSchwertberger

The wxWidgets change is implemented now. Once wxPython will build on wxWidgets 4.1.7, APP_ASSERT_EXCEPTION should work. I would suggest to close the ticket.

DietmarSchwertberger avatar Apr 26 '22 15:04 DietmarSchwertberger