Re: [lmi] a slight access violation

[Greg Chicares]

On 2014-10-25 21:28Z, Vadim Zeitlin wrote:
> On Sat, 25 Oct 2014 14:27:54 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> Vadim: I built locally with 'lmi_wx_test_3.patch' from your
> GC> 2014-10-14T23:33Z email (and no further patch, yet). The
> GC> GUI test ran as expected. However, when it was done, I
> GC> single-clicked on the "Log" menu, then the application
> GC> vanished and my JIT debugger popped up.
> 
>  Thanks for reporting this. It turns out there is nothing really slight
> about this access violation as it points to one and a half real bugs.
>
>  The first bug is that Skeleton keeps using "frame_" pointer even when it
> is no longer valid. The original assumption beyond this was that the event
> handlers using this pointer couldn't be called after the frame was closed
> and destroyed, but this is not true any more in the test where the log
> frame also has a menu which can be opened after the main frame destruction.

If class SkeletonTest didn't exist, would this be a defect in class Skeleton?
I don't see how it would be. But this is a matter of opinion, of course.

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

That's why I preferred this approach:

>  I can work around this purely in the test code with the following patch:

Committed 20141027T1435Z, revision 6002.

> [...] But I think that it would be preferable to update Skeleton
> itself to be safer, even if it doesn't need it on its own (but only when
> used as part of the tests). My preferred solution would probably be to
> check if the window associated with the menu item is really the main frame,
> i.e. do something like this:
[...snip patch...]
> Unfortunately currently this doesn't work under MSW because the menu is
> always NULL when the "Window" menu is opened. This is, of course, another
> bug, and should be fixed... If you agree that it's worth fixing this in
> Skeleton itself, I'd like to do it and then apply the patch above, but this
> would require you to update your wxWidgets version yet again.

Although I committed the first patch instead, this wx issue should of course
be fixed; and I don't mind updating wx again, because that has become easy.

>  For completeness, it would also be possible to fix this at Skeleton level
> without changes to wxWidgets itself by resetting "frame_" pointer to NULL
> when the frame is destroyed and checking for this in UponMenuOpen().

I've see the usage
  delete p;
  p = NULL;
elsewhere over the years, so often that I've become allergic to it--so much
that I hesitate to consider it even for a reason that might be reasonable;
but here it's no longer necessary.

> GC> Here's what the JIT debugger says. Doesn't the fifth line
> GC>   ExceptionCode = 80000003
> GC> signify INT 3?
> 
>  This is indeed correct and this "INT 3" corresponds to debug breakpoint
> triggered by wxWidgets assertion at src/common/list.cpp:162
> 
>     wxASSERT_MSG( !list.m_destroy,
>                   wxT("copying list which owns it's elements is a bad idea") 
> );
> 
> The assertion is bogus because the list being copied doesn't actually own
> its elements, it's just totally corrupted because it's being accessed via a
> dangling pointer ("frame_" from above). But it nevertheless does point to
> what I consider to be half a bug: why does the code in skeleton.cpp copy
> the list of children in the first place? There is no danger of this list
> being modified while we're iterating over it, so we could just use a
> reference here:
[...patch elided...]
> This would be slightly more efficient and while the difference is probably
> negligible, I don't consider this to be a premature optimization but rather
> an avoidance of a premature pessimization.

Yes, making a copy of the list was an oversight on my part.

Committed 20141027T1211Z, revision 6000.

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

| wxWindowList& wxWindow::GetChildren()
...
| const wxWindowList& wxWindow::GetChildren() const
| This is an overloaded member function, provided for convenience. It
| differs from the above function only in what argument(s) it accepts.

It doesn't actually accept any argument.

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

> the simple fix in Skeleton::UponMenuOpen() from working in view of
> committing that fix instead (or maybe in addition?).

...and the rest is already done as discussed above.