[ wxwindows-Bugs-1660913 ] Crash in window dtor
SourceForge.net
noreply at sourceforge.net
Wed May 9 12:41:20 PDT 2007
Bugs item #1660913, was opened at 2007-02-15 23:37
Message generated for change (Comment added) made by stsp
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=109863&aid=1660913&group_id=9863
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: wxMSW specific
Group: Fatal
>Status: Open
Resolution: None
Priority: 7
Private: No
Submitted By: Stas Sergeev (stsp)
Assigned to: Nobody/Anonymous (nobody)
Summary: Crash in window dtor
Initial Comment:
Hi.
Some not very old change in wx made one of my
prog to crash under wxMSW somethimes (but not
under wxGTK).
After a lot of so-called debugging (gdb doesn't
work for me under mingw, so debugging the MSW-
specific problems is just a pain), I can outline
the basic crash scenario.
- The crash happens in the destructor of the
wxDialog-derived dialog class of mine (but the
destructor is not overriden).
- The dialog has the buttons, one of which (not
any one) must have a focus for the crash to happen.
- ~wxWindowMSW() calls DestroyChildren(). When
the button that has the focus, gets destroyed,
the call to ::DestroyWindow() yields the system
message that sends the focus to another button -
the one from the same dialog, which is not yet
deleted.
- That button, to which the focus is being
transferred, asserts in wxButton::SetTmpDefault().
- Assert is not being handled the usual way and
raises an unhandled exception. The reason is
unknown - perhaps it have something to do with
the destructor (this all happens in a destructor).
Obviously this requires more investigations, but
as I said, debugging an MSW-specific problem is
very difficult for me, so it would be nice to have
some hints.
So far I see the following ways to proceed:
investigate why SetTmpDefault() asserts and
why the assert causes an unhandled exception;
or to implement some protection so that the
messages are not being sent to the childs of the
window that is in a process of being destroyed.
Which way would be a better fix? I mean, if it is
known that the SetTmpDefault() should never be
called inside a destructor of a parent, then you
can save a lot of my efforts by letting me know
this beforehand. :)
Or maybe someone can recall a CVS commit that
could cause something like this?
Any input is appreciated.
----------------------------------------------------------------------
>Comment By: Stas Sergeev (stsp)
Date: 2007-05-09 23:41
Message:
Logged In: YES
user_id=501371
Originator: YES
> The problem with that is obviously that I obviously want to use MSVC
> debugger instead of gdb under Windows.
Oh, ok. Fair enough.
> Creates a temporary unnamed dialog on stack and deletes it
> immediately?
For the test-case - yes.
> And the point being ... ?
To demonstrate the bug.
> Just for the record, doing this does
> crash but simply because non-modal dialogs must be allocated on the
heap
> and not on the stack.
This is not a non-modal dialog.
It displays nothing. You can create it
on a heap and later delete it, and it
will likely to crash too.
If you claim this piece of code is _supposed_
to crash, then:
1. Please try it with my patch
2. Provide some details as to why should it crash
> Please don't tell me that your problem is explained
> by this...
Sorry, haven't seen an explanation yet. :)
If you really can explain why this code _should_
crash, and why does it not crash with my patch,
then I'll be in a big troubles about the origins
of this bug entry, indeed. :)
----------------------------------------------------------------------
Comment By: Vadim Zeitlin (vadz)
Date: 2007-05-09 23:28
Message:
Logged In: YES
user_id=71618
Originator: NO
The problem with that is obviously that I obviously want to use MSVC
debugger instead of gdb under Windows.
I added ShowModal() because the code without it makes no sense to me, what
does it do? Creates a temporary unnamed dialog on stack and deletes it
immediately? And the point being ... ? Just for the record, doing this does
crash but simply because non-modal dialogs must be allocated on the heap
and not on the stack. Please don't tell me that your problem is explained
by this...
----------------------------------------------------------------------
Comment By: Stas Sergeev (stsp)
Date: 2007-05-09 23:20
Message:
Logged In: YES
user_id=501371
Originator: YES
> Ok, I merged this into the minimal sample but it doesn't crash for me.
> What should I do to make it crash?
Why can't you just compile my test-case?
It is really only just about typing
configure && make
What is the problem with that?
As for the test-case - you took the liberty
to change a few pieces, but the bug is
very elusive. From the first glance I see
you added ShowModal(), which is not in my
test-case. Please try to remove it.
----------------------------------------------------------------------
Comment By: Vadim Zeitlin (vadz)
Date: 2007-05-09 22:40
Message:
Logged In: YES
user_id=71618
Originator: NO
Ok, I merged this into the minimal sample but it doesn't crash for me.
What should I do to make it crash?
Attached is the patch to the minimal sample I used.
File Added: minimal.diff
----------------------------------------------------------------------
Comment By: Stas Sergeev (stsp)
Date: 2007-05-09 14:20
Message:
Logged In: YES
user_id=501371
Originator: YES
> I'm going to try to look at this but, frankly, I could really do with a
> patch to minimal sample
This is almost a minimal sample - the small
wxGlade-drawn dialog with most things ripped
out by hands. I don't know how to do the patch -
it crash on that particular dialog, whilst the
wx samples do not crash. The patch is certainly
possible, but creating it will take a lot of time
since I have no idea what is needed to be changed.
> instead of a tarball containing half a dozen of
> source files + another half megabyte of unrelated stuff...
That's a bit unfair - I already spent 2 days
stripping out my project to only 2 source
files, less than 4K in total size. So I have
no idea where do you see the "dozen of source
files", if there are only 2 (actually, really
only 1 that makes sense).
This is almost a minimal test-case I could do.
As for the unrelated stuff - it is an autoconf
stuff. It lets you to just type
./configure && make
to quickly build a test-case.
I thought I did everything to make it easy for
the bug to be demonstrated. Please take another
look and maybe you'll reconsider.
----------------------------------------------------------------------
Comment By: Vadim Zeitlin (vadz)
Date: 2007-05-09 04:59
Message:
Logged In: YES
user_id=71618
Originator: NO
I'm going to try to look at this but, frankly, I could really do with a
patch to minimal sample instead of a tarball containing half a dozen of
source files + another half megabyte of unrelated stuff...
----------------------------------------------------------------------
Comment By: Stas Sergeev (stsp)
Date: 2007-05-03 21:23
Message:
Logged In: YES
user_id=501371
Originator: YES
Well, this is not exactly very easy - that
bug is very elusive...
OK, after 2 days of trying, here is the test-case
that reproduce it with the sufficient reliability
(on my PC at least).
And with my patch it never happens at all.
File Added: tst-1.tar.gz
----------------------------------------------------------------------
Comment By: Vadim Zeitlin (vadz)
Date: 2007-05-01 04:22
Message:
Logged In: YES
user_id=71618
Originator: NO
I'd like to fix this bug but unfortunately I can't reproduce it at all.
Could you please provide a patch allowing to do this?
----------------------------------------------------------------------
Comment By: Stas Sergeev (stsp)
Date: 2007-03-16 08:57
Message:
Logged In: YES
user_id=501371
Originator: YES
> Sorry, the patch is indeed inappropriate.
No need to be sorry - I know myself this patch is not OK.
> OTOH I'd prefer a more
> general and more elegant fix too...
What kind of, and what can be done on my side?
> We didn't configure it like this and if you know how to configure it
> differently, please let us know.
I don't know how, but I definitely know this is
possible. I once asked the admin of the other
project (where I was a developer) to reconfigure
the tracker that way, and he did so in a minute.
After that, we started to receive every change from
every item. That is, if you posted to some thread
or started to monitor it, then you would receive
all the messages from it _twice_. That was annoying
too, but much better than not receiving anything
at all. Unfortunately I can't tell you how exactly
he did such a configuration...
----------------------------------------------------------------------
Comment By: Vadim Zeitlin (vadz)
Date: 2007-03-16 03:10
Message:
Logged In: YES
user_id=71618
Originator: NO
Sorry, the patch is indeed inappropriate. We don't want to have to
manually call Destruct() from all the derived classes. Besides, I still
don't understand why did the behaviour change recently and I don't like
changing the code blindly just to make bugs go away.
As for your initial question, I agree that neither of
Set/Unset[Tmp]Default() methods should be called when the window is being
destroyed (it's useless after all). So maybe we should ignore events sent
to the buttons when their parent is being destroyed. OTOH I'd prefer a more
general and more elegant fix too...
P.S. And:
> But I know you guys configured the tracker to generate the notification
message only for the initial submission, so I doubt anyone is listening...
We didn't configure it like this and if you know how to configure it
differently, please let us know. I don't like this neither but I don't have
enough time to manually click "monitor" on all the items neither.
----------------------------------------------------------------------
Comment By: Stas Sergeev (stsp)
Date: 2007-02-23 13:27
Message:
Logged In: YES
user_id=501371
Originator: YES
The problem is that, I beleive, my patch is not
the best possible solution and needs to be discussed.
Otherwise I would have posted it into a patch section.
But maybe the bug tracker is not the best place to
discuss the patches after all... :)
----------------------------------------------------------------------
Comment By: p_michalczyk (p_michalczyk)
Date: 2007-02-23 01:44
Message:
Logged In: YES
user_id=1409236
Originator: NO
You should post patch in "Patches" section. There it will be more visible
to does that apply patches.
----------------------------------------------------------------------
Comment By: Stas Sergeev (stsp)
Date: 2007-02-19 21:38
Message:
Logged In: YES
user_id=501371
Originator: YES
Tested and sanitized the diff.
File Added: aa.diff
----------------------------------------------------------------------
Comment By: Stas Sergeev (stsp)
Date: 2007-02-18 00:55
Message:
Logged In: YES
user_id=501371
Originator: YES
I forgot to tell what the patch does.
It makes the cleanup code to be called from
the derived classes (like wxDialog) instead
of from the base class wxWindowMSW. In that
case calling the overloaded virtual functions
works as expected.
There could be another fix - to disable the
message processing for the widgets that are
about to be deleted. That would probably be
better.
But I know you guys configured the tracker to
generate the notification message only for
the initial submission, so I doubt anyone is
listening...
----------------------------------------------------------------------
Comment By: Stas Sergeev (stsp)
Date: 2007-02-16 22:48
Message:
Logged In: YES
user_id=501371
Originator: YES
> investigate why SetTmpDefault() asserts and
> why the assert causes an unhandled exception;
OK, puzzle solved.
The problem is that the DestroyChildren() is called
from wxWindowMSW::~wxWindowMSW(), which is called
*after* wxDialog::~wxDialog(). What does this mean?
Well, that calling any virtual function overloaded
in wxDialog, will crash.
As I mentioned in the previous posting,
wxButton::SetTmpDefault() asserts. It calls
wxGetTopLevelParent(this), which calls
GetParent()->IsTopLevel(). GetParent() returns the
object in question - the one of wxDialog class, but
already underwent the wxDialog::~wxDialog(). IsTopLevel()
is its overloaded virtual function that should not be
called. It seems gcc generates the safe code here, so
there is no immediate crash, but rather just IsTopLevel()
returns false, whereas true is expected. Then the assert,
which yields an unhandled exception (not clear why the
exception - doesn't matter though).
Attached is a quick fix. It was not thoroughly tested yet,
it has an indentation problems and the white-space noise.
Sorry about that. I don't think it fixes the problem
entirely - I am pretty sure there are other instances of
that bug in other places. And I am pretty sure you won't
apply that patch. But the patch works and the problem is
severe. As always, I am ready to discuss and modify the
patch to some extent. :)
File Added: a.diff
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=109863&aid=1660913&group_id=9863
More information about the wx-dev
mailing list