Re: [lmi] a slight access violation

[Vadim Zeitlin]

On Mon, 27 Oct 2014 15:01:33 +0000 Greg Chicares <address@hidden> wrote:

GC> >  The first bug is that Skeleton keeps using "frame_" pointer even when it
GC> > is no longer valid. The original assumption beyond this was that the event
GC> > handlers using this pointer couldn't be called after the frame was closed
GC> > and destroyed, but this is not true any more in the test where the log
GC> > frame also has a menu which can be opened after the main frame 
destruction.
GC> 
GC> If class SkeletonTest didn't exist, would this be a defect in class 
Skeleton?
GC> I don't see how it would be. But this is a matter of opinion, of course.

 Yes, indeed. If you consider Skeleton class a completely self-contained
component, it isn't. But I'm wary of this because, in my experience, in the
code written by competent people bugs most often arise exactly because of
an unforeseen interaction between the different components. So I prefer to
be as defensive as reasonably possible.

GC> Suppose that (due to some later change, e.g. in wx) wxLogWindow acquires a
GC> toolbar with its own wxID_PREFERENCES and wxID_ABOUT buttons, which is shown
GC> by class SkeletonTest after all tests have been run. Wouldn't those events'
GC> handlers in class Skeleton then be called, and segfault? Thus, if we treat
GC> this as a defect in class Skeleton, shouldn't those events' handlers first
GC> check whether Skeleton::frame_ is valid? And then shouldn't we be adding
GC> checks of Skeleton::frame_'s validity everywhere? Where would it stop?

 This is why I used the weasel word "reasonably" above. I can't provide any
absolute rule, but I think that reacting to all events in UponMenuOpen(),
independently of where they come from, is not very safe and that it would be
better to at least check that they do come from our own frame. For
wxID_PREFERENCES or wxID_ABOUT this is less clear, as there is unlikely to
be more than one of those in a single application.

 But then, personally, I wouldn't be doing this at wxApp level at all
anyhow, but rather in wxFrame containing the menus to be updated or the
commands to be processed, because I think that it is best to handle the
events as locally as possible.


GC> >  For completeness, it would also be possible to fix this at Skeleton level
GC> > without changes to wxWidgets itself by resetting "frame_" pointer to NULL
GC> > when the frame is destroyed and checking for this in UponMenuOpen().
GC> 
GC> I've see the usage
GC>   delete p;
GC>   p = NULL;
GC> elsewhere over the years, so often that I've become allergic to it--so much
GC> that I hesitate to consider it even for a reason that might be reasonable;
GC> but here it's no longer necessary.

 Thinking more about this, a simpler and safer fix would be to use
wxWeakRef<wxMDIParentFrame> instead of wxMDIParentFrame* for "frame_". This
is a weak pointer which becomes invalid (i.e. null) when the corresponding
window is destroyed. It's a bit of a blunt tool, but quite simple to use
and it is safer than raw pointers, at least.


GC> BTW...I wanted to check the wx documentation, to double check that
GC> GetChildren() returns a reference (since I had overlooked that).
GC> So I went here:
GC>   http://docs.wxwidgets.org/trunk/
GC> and pasted "GetChildren" without quotes into the search box, and it
GC> returned five results...but none of them was for class wxWindow;
GC> shouldn't that have been found?

 Doxygen search is very strange, sometimes it finds things in unexpected
places (i.e. where they don't seem to be mentioned at all) and often it
doesn't find things you'd expect it to find. Unfortunately I don't know
what to do about it.

 Personally I usually use Google to search wx documentation if I have to,
and searching for "GetChildren site:docs.wxwidgets.org" finds the wxWindow
method as the first match (albeit in 3.0 docs, not trunk ones).

GC> I found it anyway:
GC>   
http://docs.wxwidgets.org/trunk/classwx_window.html#ad500085ad0511879b5e018706c91a494
GC> and I think there's a slight issue:
GC> 
GC> | wxWindowList& wxWindow::GetChildren()
GC> ...
GC> | const wxWindowList& wxWindow::GetChildren() const
GC> | This is an overloaded member function, provided for convenience. It
GC> | differs from the above function only in what argument(s) it accepts.
GC> 
GC> It doesn't actually accept any argument.

 This is the unfortunate result of using the standard Doxygen "@overload"
macro here. I've fixed this use of it, interestingly this seems to be the
only place in the entire [documented] wxWidgets API where two void methods
are overloaded on const-ness, in all other case at least one of the group
of overloaded methods takes parameters (e.g. "void GetSize(int*,int*)" for
"wxSize GetSize()"), so I think using "@overload" for them remains
acceptable.

GC> >  But the important bug remains the first one. Please let me know if you'd
GC> > prefer to commit the fix to the test or fix the bug in wxWidgets 
preventing
GC>                                           ^^^^^^^^^^^^^^^^^^^^^^^^
GC> Yes to that...

 Thanks, I've fixed this now and will push it out soon. There is still one
discrepancy between wxEVT_MENU_{OPEN,CLOSE} events in wxMDIParentFrame
between wxGTK and wxMSW remaining however: in wxGTK, when a child window is
active (and so its menu bar is used), these events are sent to the child
frame, while in wxMSW they're sent to the parent one. They should probably
be sent to both of them, first the child and then the parent one, but for
now I won't touch this...

 Regards,
VZ