Opened 6 years ago

Closed 17 months ago

#10085 closed defect (fixed)

Fix for crash in generic wxTreeCtrl

Reported by: lillo Owned by: pcor
Priority: normal Milestone:
Component: GUI-generic Version: stable-latest
Keywords: idle Cc:
Blocked By: Blocking:
Patch: yes

Description

Hi,

using SVN trunk, the following app generates an assert when using the generic tree control (as is the case under OSX for example) on src/generic/treectlg.cpp, line 207 ("must call CalculateSize() first"), after which the app crashes:

#include "wx/wx.h"
#include "wx/treectrl.h"


class TestApp : public wxApp
{
public:
    virtual bool OnInit() {
	wxFrame *f = new wxFrame(NULL, -1, "Test");	
	wxBoxSizer *sizer = new wxBoxSizer(wxVERTICAL);
	
	wxTreeCtrl *t = new wxTreeCtrl(f, -1, wxDefaultPosition, wxSize(200,200));
	wxTreeItemId rootId = t->AddRoot(_T("root"));
	t->AppendItem(rootId, _T("node"));
	
	sizer->Add(t, 1, wxEXPAND | wxALL, 5);
	f->SetSizer(sizer);
	sizer->SetSizeHints(f);
	f->Center();
	f->Show();
	SetTopWindow(f);
	return true;
    }
};

IMPLEMENT_APP(TestApp)

This is because appending new items via AppendItem (or InsertItem or whatever calls wxGenericTreeCtrl::DoInsertItem()) creates a new wxGenericTreeItem to be appended, but does not call CalculateSize() on it.
Calling wxTreeCtrl::SetItemText() or any other setter function of wxGenericTreeCtrl that sets an item attribute automatically calls CalculateSize() on it, and infact a workaround to avoid the app shown above to crash is to place a

t->SetItemText(_T("node"));

right after the AppendItem() call.

This is clearly wrong and shouldn't be needed, thus I'm proposing a small patch fixing the issue inside wxGenericTreeCtrl::DoInsertItem (and AddRoot), where CalculateSize() is always called on the newly created item after it is appended.

Attachments (2)

wx_generic_treectrl.patch download (826 bytes) - added by lillo 6 years ago.
The patch
calculatesize.patch download (400 bytes) - added by chrisstankevitz 3 years ago.
Patch for the 2.8 branch

Download all attachments as: .zip

Change History (14)

Changed 6 years ago by lillo

The patch

comment:1 Changed 6 years ago by vadz

  • Cc roebling csomor added
  • Keywords idle added
  • Summary changed from Fix for crash in generic wxTreeCtrl to Difference in idle and paint event orders in wxGTK and wxMac

The patch seems to make sense to me but I was surprised that I wasn't able to reproduce the problem under wxGTK (which uses the same generic control version too). There is no assert and running under debugger the wxGenericTreeItem::CalculateSize() method is called before GetTextHeight() one because under wxGTK the idle events (the tree control does recalculation during the idle time) are sent before the paint ones.

So I believe that Mac must do it in the reverse order (i.e. send idle events after repainting) and I wonder if it wouldn't be better to modify wxMac to do it like this as well? OTOH I wouldn't be surprised if wxMSW sent the idle events later too so maybe it's wxGTK that should send them later as well? Stefan, Robert, what do you think?

P.S. On a separate topic, we really need to do something to make asserts shown during repainting non fatal as currently they invariably crash the program...

comment:2 Changed 6 years ago by roebling

So I believe that Mac must do it in the reverse order (i.e.
send idle events after repainting) and I wonder if it wouldn't
be better to modify wxMac to do it like this as well? OTOH
I wouldn't be surprised if wxMSW sent the idle events later
too so maybe it's wxGTK that should send them later as well?
Stefan, Robert, what do you think?

I can see arguments for both, i.e. "idle" can be meant to
be after everything is finished, including painting or it
can be used to do everything before finally painting things
on screen, which is the wxGTK way to do it.

comment:3 Changed 6 years ago by pcor

I can reproduce this with wxGTK. And, wxGTK does _not_ send idle events before paint events.

comment:4 follow-up: Changed 6 years ago by vadz

  • Status changed from new to confirmed

Huh. I don't understand what's going on because I don't see any asserts when running the attached code with the trunk version of wx. And, as I wrote, putting a breakpoint on the 2 above mentioned methods I definitely see that the first is called from idle processing code before the second one.

I am using (remote) Cygwin X server, could this somehow explain it?

comment:5 Changed 6 years ago by csomor

I'd assume that idle events are sent after paint events on MSW from my earlier days..., on OSX I only send them if no events are in the queue, so on OSX it definitely occurs after the paint event

comment:6 Changed 6 years ago by vadz

Ok, if all the platforms already behave consistently here there is no problem.

I still don't know why doesn't wxGTK behave as it's supposed to here though. In any case, Paul, as you can reproduce the bug, could you please commit the patch after verifying that it does fix the problem? I don't want to do it blindly.

TIA!

comment:7 in reply to: ↑ 4 Changed 6 years ago by pcor

  • Cc roebling csomor removed
  • Owner set to pcor
  • Status changed from confirmed to accepted

Replying to vadz:

Huh. I don't understand what's going on because I don't see any asserts when running the attached code with the trunk version of wx. And, as I wrote, putting a breakpoint on the 2 above mentioned methods I definitely see that the first is called from idle processing code before the second one.

I am using (remote) Cygwin X server, could this somehow explain it?

Yes, I think that's probably it. I said idle events are not sent before paint events in wxGTK, but what I should have said was idle events are not sent unless there is nothing else to do. Expose events from a remote server could be delayed enough for the event queue to go empty.

comment:8 Changed 6 years ago by pcor

  • Resolution set to fixed
  • Status changed from accepted to closed
  • Summary changed from Difference in idle and paint event orders in wxGTK and wxMac to Fix for crash in generic wxTreeCtrl

I decided it would be better to just call CalculateSize from the paint event, fixed in r56474.

comment:9 Changed 3 years ago by riban

  • Resolution fixed deleted
  • Status changed from closed to reopened

This issue was fixed two years ago and targeted at milestone 2.9.0 and so did not hit any of the 2.8 releases since. This issue affects 2.8.11 (current stable release) but could be fixed by application of this patch.

Is there a chance that this patch can be applied to the stable release?

comment:10 Changed 3 years ago by vadz

  • Milestone 2.9.0 deleted
  • Resolution set to port to stable
  • Status changed from reopened to portneeded

Changed 3 years ago by chrisstankevitz

Patch for the 2.8 branch

comment:11 Changed 17 months ago by pcor

The example no longer asserts, probably due to r66912, but I'll go ahead an apply this, it looks pretty safe.

comment:12 Changed 17 months ago by PC

  • Resolution changed from port to stable to fixed
  • Status changed from portneeded to closed

(In [73015]) backport r56474, "fix for assert failure when first paint event occurs before first idle event"
closes #10085

Note: See TracTickets for help on using tickets.