wxWidgets / wxWidgets

Cross-Platform C++ GUI Library
https://www.wxwidgets.org/
5.91k stars 1.73k forks source link

Crash in wxDataViewModel::ItemsAdded or wxDataViewModel::ItemsDeleted in app built for Linux gtk wxWidgets 3.2.4 #24148

Open svprok opened 6 months ago

svprok commented 6 months ago

This app build for Linix GTK wxWidgets 3.2.4. App with the same code base built for Windows (wxWidgets 3.1.4) works just fine. App uses two trees based on wxDataViewCtrl, connected to the same model derived form wxDataViewModel. In first tree one of tree branch is changed so wxDataViewModel::ItemsDeleted is called and crashed with

signal SIGSEGV: address not mapped to object (fault address: 0x20)

stack of crash:

wxVector<void*>::size() const (/home/sergey/Projects/mwmapp_cmake/build/subprojects/Source/wxWidgets_external/include/wx/vector.h:421)
wxBaseArray<void*, wxSortedArray_SortFunction<void*>>::GetCount() const (/home/sergey/Projects/mwmapp_cmake/build/subprojects/Source/wxWidgets_external/include/wx/dynarray.h:126)
wxDataViewCtrlInternal::GetIndexOf(wxDataViewItem const&, wxDataViewItem const&) (/home/sergey/Projects/mwmapp_cmake/build/subprojects/Source/wxWidgets_external/src/gtk/dataview.cpp:4418)
wxGtkDataViewModelNotifier::ItemDeleted(wxDataViewItem const&, wxDataViewItem const&) (/home/sergey/Projects/mwmapp_cmake/build/subprojects/Source/wxWidgets_external/src/gtk/dataview.cpp:1831)
wxDataViewModelNotifier::ItemsDeleted(wxDataViewItem const&, wxDataViewItemArray const&) (/home/sergey/Projects/mwmapp_cmake/build/subprojects/Source/wxWidgets_external/src/common/datavcmn.cpp:120)
wxDataViewModel::ItemsDeleted(wxDataViewItem const&, wxDataViewItemArray const&) (/home/sergey/Projects/mwmapp_cmake/build/subprojects/Source/wxWidgets_external/src/common/datavcmn.cpp:220)

in src/gtk/dataview.cpp:

bool wxGtkDataViewModelNotifier::ItemDeleted( const wxDataViewItem &parent, const wxDataViewItem &item )
{
    GtkWxTreeModel *wxgtk_model = m_internal->GetGtkModel();
#if 0
    // using _get_path for a deleted item cannot be
    // a good idea
    GtkTreeIter iter;
    iter.stamp = wxgtk_model->stamp;
    iter.user_data = (gpointer) item.GetID();
    wxGtkTreePath path(wxgtk_tree_model_get_path(
        GTK_TREE_MODEL(wxgtk_model), &iter ));
#else
    // so get the path from the parent
    GtkTreeIter parentIter;
    parentIter.stamp = wxgtk_model->stamp;
    parentIter.user_data = (gpointer) parent.GetID();
    wxGtkTreePath path(wxgtk_tree_model_get_path(
        GTK_TREE_MODEL(wxgtk_model), &parentIter ));

    // and add the final index ourselves
    int index = m_internal->GetIndexOf( parent, item );
    gtk_tree_path_append_index( path, index );
#endif

    m_internal->ItemDeleted( parent, item );

    gtk_tree_model_row_deleted(
        GTK_TREE_MODEL(wxgtk_model), path );

    // Did we remove the last child, causing 'parent' to become a leaf?
    if ( !m_wx_model->IsContainer(parent) )
    {
        gtk_tree_path_up(path);
        gtk_tree_model_row_has_child_toggled
        (
            GTK_TREE_MODEL(wxgtk_model),
            path,
            &parentIter
        );
    }

    return true;
}

// item can be deleted already in the model
int wxDataViewCtrlInternal::GetIndexOf( const wxDataViewItem &parent, const wxDataViewItem &item )
{
    if (m_wx_model->IsVirtualListModel())
    {
        return wxPtrToUInt(item.GetID()) - 1;
    }
    else
    {
        wxGtkTreeModelNode *parent_node = FindNode( parent );
        wxGtkTreeModelChildren &children = parent_node->GetChildren();
        size_t j;
        for (j = 0; j < children.GetCount(); j++)
        {
            if (children[j] == item.GetID())
               return j;
        }
    }
    return -1;
}

After experimenting with crash reproduction I found out that it would not crashed if changed tree branch is opened in second tree as well.

I was able to reproduce this crash during debugging of the following code fragment :

        wxGtkTreeModelNode *parent_node = FindNode( parent );
        wxGtkTreeModelChildren &children = parent_node->GetChildren();
        size_t j;
        for (j = 0; j < children.GetCount(); j++)

FindNode returnes NULL here, so GetChildren() returns wrong address, so it makes sence for children.GetCount() to crash with: signal SIGSEGV: address not mapped to object (fault address: 0x20)

It looks like code not supporting multiple views connected to the same model. Is it just a bug in src/gtk/dataview.cpp or somthing else was wrong here?

And other case with wxDataViewModel::ItemsAdded.

If tree branch changing required calling wxDataViewModel::ItemsAdded instead of wxDataViewModel::ItemsDeleted then crash inside gtk would occure, but, in strange way, it lools like the problem is the same :

signal SIGSEGV: address not mapped to object (fault address: 0xfffffffffffffffc)

___lldb_unnamed_symbol18772 (@___lldb_unnamed_symbol18772:40)
___lldb_unnamed_symbol17518 (@___lldb_unnamed_symbol17518:25)
g_closure_invoke (@g_closure_invoke:111)
___lldb_unnamed_symbol1078 (@___lldb_unnamed_symbol1078:379)
g_signal_emit_valist (@g_signal_emit_valist:977)
g_signal_emit (@g_signal_emit:29)
wxGtkDataViewModelNotifier::ItemAdded(wxDataViewItem const&, wxDataViewItem const&) (/home/sergey/Projects/mwmapp_cmake/build/subprojects/Source/wxWidgets_external/src/gtk/dataview.cpp:1805)
wxDataViewModelNotifier::ItemsAdded(wxDataViewItem const&, wxDataViewItemArray const&) (/home/sergey/Projects/mwmapp_cmake/build/subprojects/Source/wxWidgets_external/src/common/datavcmn.cpp:110)
wxDataViewModel::ItemsAdded(wxDataViewItem const&, wxDataViewItemArray const&) (/home/sergey/Projects/mwmapp_cmake/build/subprojects/Source/wxWidgets_external/src/common/datavcmn.cpp:205)

Crash is inside gtk_tree_model_row_inserted:

bool wxGtkDataViewModelNotifier::ItemAdded( const wxDataViewItem &parent, const wxDataViewItem &item )
{
    m_internal->ItemAdded( parent, item );
    GtkWxTreeModel *wxgtk_model = m_internal->GetGtkModel();

    GtkTreeIter iter;
    iter.stamp = wxgtk_model->stamp;
    iter.user_data = item.GetID();

    wxGtkTreePath path(wxgtk_tree_model_get_path(
        GTK_TREE_MODEL(wxgtk_model), &iter ));
    gtk_tree_model_row_inserted(
        GTK_TREE_MODEL(wxgtk_model), path, &iter);

    return true;
}

Thank you. Sergey.

vadz commented 6 months ago

It would be great if you could please provide a simple way of reproducing the problem. Ideal is to have a minimal, i.e. as small as possible, patch to one of the wxWidgets samples, e.g. dataview one in this case. But a standalone example is also acceptable, provided it's really short.

Without a way of easily reproducing the problem it's time-consuming to debug it and/or test that it is fixed, so having such a reproducer helps a lot.

Thanks in advance!

P.S. I've also reformatted your submission so that the code snippets in it appear correctly.

svprok commented 6 months ago

Ok. I'll try to modify dataview to reproduce a problem tomorrow.

But, Is it not clear from the following 2 lines snippet from dataview.cpp that it is assumed that FindNode(parent) always have to find and return not NULL parent_note? I just don't understand how it can be assumed here if multiple tree views with there own nodes should be supported. All such nodes would come from the same model, but history of nodes creation can be different so some nodes can be present in one tree view and not created yet in enother view. So when user modified some tree branch in first view and ItemsDeleted(wxDataViewItem(parentItem, itemsArray); have to be called for branch's parentItem it would try to update all connected views. But only the first view must have that parentItem present. all others can be in different state and can be without that parentItem present, so code should be able just return form ItemsDeleted if FindNode retuns NULL.

wxGtkTreeModelNode *parent_node = FindNode( parent ); wxGtkTreeModelChildren &children = parent_node->GetChildren();

Thank you. Sergey.

vadz commented 6 months ago

To be honest, it's mostly not clear because I didn't have time to look at this at all yet. But if/when I have time for it, it would be great to have a way to see it -- if only to be able to test that it's fixed.

And having a simple example would also help with understanding what you're actually doing because even this is not immediately obvious.

svprok commented 6 months ago

Added here a patch for dataview sample (against the master branch) to reproduce the issue. Steps to reproduce: 1.Run dataview sample and remain on the MyMusicTreeModel tab. You should see splitter window with two tree views. 2.Don't open root branch on second view. Just play with adding and deleting leaf items on the first one. Crash should occure with Signal SIGSEGV: address not mapped to object (fault address: 0x20) after deleting item

signal SIGSEGV: address not mapped to object (fault address: 0xfffffffffffffffc) after adding item

P.S. To reproduce the issue I had to add more music categories here in sample dataview model (for list originating from the root is always created by wxWidgets wxDataViewCtrl code for both first and second tree views) Thank you. Sergey.

dataview_sample_reproducing_crash_issue24148.patch

svprok commented 6 months ago

One step more to play with the sample.

3.If you open branches of second view tree before adding and deleting items in the first tree view - there will be no crashes.

svprok commented 6 months ago

Could someone reproduce the issue with a patch for dataview sample provided here? Or just confirm that there is no problem with using more than one wxDataViewCtrl with the same model? I just wonder if it is a bug of wxWidgets with gtk or it is just some strange problem with my particular wxWidgets build, or using wrong libraries?

As for now I use the following way around to avoid the issue: 1) Do not use more than one tree view with the same model. 2) Find all expanded tree view nodes and avoid calling wxDataViewModel::ItemsAdded, wxDataViewModel::ItemsDeleted for wxDataViewItem parent which is not among founded expanded nodes.

Looks like it is bigger problem then using more than one tree view with the same model, because just trying to call wxDataViewModel::ItemsAdded with not currently existing in wxDastaViewCtrl-tree node would crash my application anyway even when using one wxDastaViewCtrl with wxDataViewModel.