Re: [lmi] a slight access violation

[Vadim Zeitlin]

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.
Unfortunately, I hadn't seen this in my original testing because until
recently Skeleton::UponMenuOpen() was not called anyhow because of a bug in
wx that resulted in "Window|Next" and "Previous" items not being enabled
correctly, that you also pointed out. But fixing it now triggers this
crash.

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

------------------ >8 ------------------
diff --git a/main_wx_test.cpp b/main_wx_test.cpp
index 521b01a..46ec9c8 100644
--- a/main_wx_test.cpp
+++ b/main_wx_test.cpp
@@ -446,6 +446,10 @@ class SkeletonTest : public Skeleton
   private:
     void RunTheTests();
 
+    // This event handler only exists to prevent the base class from handling
+    // this event, see the comment near its use.
+    void ConsumeMenuOpen(wxMenuEvent&) {}
+
     std::string runtime_error_;
     bool is_running_tests_;
 };
@@ -610,6 +614,13 @@ void SkeletonTest::RunTheTests()
     log->GetFrame()->Maximize();
     log->GetFrame()->SetFocus();
     SetExitOnFrameDelete(false);
+
+    // Before closing the main window, ensure that the base class event handler
+    // relying on it being alive is not called any more, otherwise 
dereferencing
+    // the pointer to the main frame inside it would simply crash on any 
attempt
+    // to open the log frame menu.
+    Bind(wxEVT_MENU_OPEN, &SkeletonTest::ConsumeMenuOpen, this);
+
     mainWin->Close();
 }
 
------------------ >8 ------------------

i.e. by preventing the code that crashes now from getting the event in the
first place. 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:

------------------ >8 ------------------
diff --git a/skeleton.cpp b/skeleton.cpp
index 76090d7..65f6498 100644
--- a/skeleton.cpp
+++ b/skeleton.cpp
@@ -773,6 +773,13 @@ void Skeleton::UponMenuOpen(wxMenuEvent& event)
 {
     event.Skip();
 
+    wxMenu* const menu = event.GetMenu();
+    if(!menu || menu->GetWindow() != frame_)
+        {
+        // This is an event from some other frame, ignore it.
+        return;
+        }
+
     int child_frame_count = 0;
     wxWindowList wl = frame_->GetChildren();
     for(wxWindowList::const_iterator i = wl.begin(); i != wl.end(); ++i)
------------------ >8 ------------------

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.

 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().
However I think that the patch above would be preferable (if it worked!)
because it would also avoid executing this code unnecessarily if a menu of
some other frame is opened while the main frame still exists.


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:

------------------ >8 ------------------
diff --git a/skeleton.cpp b/skeleton.cpp
index 76090d7..473a7fb 100644
--- a/skeleton.cpp
+++ b/skeleton.cpp
@@ -774,7 +774,7 @@ void Skeleton::UponMenuOpen(wxMenuEvent& event)
     event.Skip();
 
     int child_frame_count = 0;
-    wxWindowList wl = frame_->GetChildren();
+    const wxWindowList& wl = frame_->GetChildren();
     for(wxWindowList::const_iterator i = wl.begin(); i != wl.end(); ++i)
         {
         child_frame_count += !!dynamic_cast<wxMDIChildFrame*>(*i);
@@ -1290,7 +1290,7 @@ bool Skeleton::ProcessCommandLine(int argc, char* argv[])
 
 void Skeleton::UpdateViews()
 {
-    wxWindowList wl = frame_->GetChildren();
+    const wxWindowList& wl = frame_->GetChildren();
     boost::shared_ptr<progress_meter> meter
         (create_progress_meter
             (wl.size()
------------------ >8 ------------------

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.

 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
the simple fix in Skeleton::UponMenuOpen() from working in view of
committing that fix instead (or maybe in addition?).

 And thanks again for finding this bug!
VZ