Opened 11 years ago

Last modified 9 years ago

#12774 confirmed defect

infinite loop when wxTreeCtrl::CollapseAndReset() called from EVT_TREE_ITEM_COLLAPSING

Reported by: Loaden Owned by:
Priority: low Milestone:
Component: wxMSW Version: stable-latest
Keywords: wxTreeCtrl infinite loop Cc:
Blocked By: Blocking:
Patch: no

Description

Hi, wx-team! I found a infinite loop in wxTreeCtrl, like this:

OnTreeItemCollapsing -> CollapseAndReset -> IsTreeEventAllowed -> HandleTreeEvent -> OnTreeItemCollapsing  ...

When call tree->CollapseAndReset(item), will handle tree event again:

void wxTreeCtrl::DoExpand(const wxTreeItemId& item, int flag)
{

wxASSERT_MSG( flag == TVE_COLLAPSE
flag == (TVE_COLLAPSE | TVE_COLLAPSERESET) flag == TVE_EXPAND

flag == TVE_TOGGLE,
wxT("Unknown flag in wxTreeCtrl::DoExpand") );

A hidden root can be neither expanded nor collapsed.
wxCHECK_RET( IsHiddenRoot(item),

wxT("Can't expand/collapse hidden root node!") );

TreeView_Expand doesn't send TVN_ITEMEXPAND(ING) messages, so we must
emulate them. This behaviour has changed slightly with comctl32.dll
v 4.70 - now it does send them but only the first time. To maintain
compatible behaviour and also in order to not have surprises with the
future versions, don't rely on this and still do everything ourselves.
To avoid that the messages be sent twice when the item is expanded for
the first time we must clear TVIS_EXPANDEDONCE style manually.

wxTreeViewItem tvItem(item, TVIF_STATE, TVIS_EXPANDEDONCE);
tvItem.state = 0;
DoSetItem(&tvItem);

if ( IsExpanded(item) )
{

wxTreeEvent event(wxEVT_COMMAND_TREE_ITEM_COLLAPSING,

this, wxTreeItemId(item));

if ( IsTreeEventAllowed(event) )

return;

}

if ( TreeView_Expand(GetHwnd(), HITEM(item), flag) )
{

if ( IsExpanded(item) )

return;

wxTreeEvent event(wxEVT_COMMAND_TREE_ITEM_COLLAPSED, this, item);
(void)HandleTreeEvent(event);

}
else: change didn't took place, so do nothing at all

}

In here:

    if ( IsExpanded(item) )
    {
        wxTreeEvent event(wxEVT_COMMAND_TREE_ITEM_COLLAPSING,
                          this, wxTreeItemId(item));

        if ( !IsTreeEventAllowed(event) )
            return;
    }

It handle the event again and again.

See http://forums.codeblocks.org/index.php/topic,12026.msg93254/topicseen.html for more information.

Attachments (1)

fix_infinite_loop.patch download (582 bytes) - added by Loaden 11 years ago.

Download all attachments as: .zip

Change History (6)

Changed 11 years ago by Loaden

comment:1 Changed 11 years ago by vadz

  • Milestone 2.9.2 deleted
  • Patch unset
  • Status changed from new to infoneeded_new

Sorry, how exactly can this infinite loop be reproduced? If you select "Collapse and reset" from the menu in the treetest sample nothing untoward seems to happen. So what exactly is needed in order to see the bug?

As a side note, removing the generation of wxEVT_COMMAND_TREE_ITEM_COLLAPSED event is, of course, not the solution so this patch can't/shouldn't be applied.

comment:2 Changed 11 years ago by Loaden

  • Status changed from infoneeded_new to new

You can reprouce the infinite loop by this patch.

Index: treectrl/treetest.cpp
===================================================================
--- treectrl/treetest.cpp	(revision 66360)
+++ treectrl/treetest.cpp	(working copy)
@@ -1533,6 +1533,7 @@
 void MyTreeCtrl::OnItemCollapsing(wxTreeEvent& event)
 {
     wxLogMessage(wxT("OnItemCollapsing"));
+    CollapseAndReset(event.GetItem());
 
     // for testing, prevent the user from collapsing the first child folder
     wxTreeItemId itemId = event.GetItem();

comment:3 Changed 11 years ago by vadz

  • Status changed from new to confirmed
  • Summary changed from infinite loop in wxTreeCtrl to infinite loop when wxTreeCtrl::CollapseAndReset() called from EVT_TREE_ITEM_COLLAPSING

Ok, I can see the bug now, it wasn't clear that infinite loop only happened if the item was collapsed from "collapsing" event handler. Unfortunately I don't immediately see any good way to fix it. As I mentioned before, just removing the collapsing events generation would be wrong for the situation when this function is called from outside of "collapsing" event handler. The only solution I see is to maintain a flag indicating if we're already handling such event but it's rather ugly.

Any better ideas?

comment:4 Changed 11 years ago by dconnet

I'm not sure this can be considered a WX bug - if you make any api call from an event handler that generates that same event, it seems like it's the users code that is broken, not WX.

I haven't looked into the specifics of this, but I've run into this with standard windows messages in the past (before I moved to WX) - there it wasn't an infinite loop - rather it was infinite recursion until the stack gave out.

comment:5 Changed 11 years ago by vadz

  • Priority changed from normal to low

Well, this is slightly different as you call CollapseAndReset() and not just Collapse() but I agree that it's not a high priority bug especially as I assume that a simple check for reentrancy in the event handler would fix it. I actually meant to reduce its priority but forgot to do it.

It would still be better to at least assert and return instead of crashing due to infinite recursion, of course.

Note: See TracTickets for help on using tickets.