Still segfaults when destroying dropdown widgets

[GoogleCodeExporter]

You were unable to properly apply the patch of issue 10, and you didn't
bother reading the follow-up comment. So the bug reported against guichan
0.6 is still present in the released 0.7 versions and in the svn trunk. I'm
opening a new bug-report in hope it finally gets fixed. I'm also attaching
the part of the patch you messed. See
http://code.google.com/p/guichan/issues/detail?id=10 for explanations.

Original issue reported on code.google.com by guillaum...@gmail.com on 21 Jul 2007 at 5:36

Attachments:

• dropdown.patch

[GoogleCodeExporter]

Geez, cut the attitude. I didn't read the follow-up because I was never notified
about your last comment. I'm quite new to Google's issue tracker and I thought I
would be automatically notified, guess I was not.

Secondly, your original patch was broken and gave me a segfault (you called
removeDeathListener on the ScrollArea which BasicContainer takes care of... But 
I
guess that might just be an old Guichan bug propagated to your patch...) so I 
messed
about with the patch for a while until I didn't get a segfault. 

I don't have a segfault without the last line. Of course, that might just be my
configuration, but I looked it up and I'm not sure what good the last line will 
do.
Exactly where is the internal focus handler used after the destructor? I can't 
really
see where a segfault might occur. When the destructor of DropDown is called the
destructor of BasicContainer is also called, and that destructor never uses the 
focus
handler, it even sets all the widgets' focus handler inside the BasicContainer 
to NULL.

I'm curious, are you using vanilla Guichan?

Original comment by olof.nae...@gmail.com on 7 Aug 2007 at 6:35

[GoogleCodeExporter]

I am using vanilla guichan, as Debian packages have yet to be updated to 
guichan 0.7.

DropDown::~DropDown is a destructor, so it first executes its body, then it 
destroys
the data members of the DropDown object, and finally it calls the destructor of 
its
ancestor, namely BasicContainer::~BasicContainer. As a consequence, at the time
BasicContainer::~BasicContainer is executed, DropDown::mInternalFocusHandler is 
not
valid anymore as it has already been freed.

Now, here is what happens. During its execution, the body of
BasicContainer::~BasicContainer calls BasicContainer::clear, which calls
BasicContainer::_setFocusHandler. In turn, since mInternalFocusHandler is not 
null,
it calls Widget::_setFocusHandler. This function now notices that there is an
existing focus handler, so it calls FocusHandler::remove on it, before 
replacing it
by null. Unfortunately, the existing focus handler happens to be
DropDown::mInternalFocusHandler which has already been destroyed. That's where 
it
crashes.

Do not ask me why the focus handler happens to be the internal focus handler. 
Perhaps
 it is caused by another bug elsewhere in guichan, and my patch simply hides it by
ensuring that Widget::_setFocusHandler is not called. But what is sure is that 
it
fixes the segfault.

Anyway, even if there is no segfault on your computer, you could still run 
valgrind
on your library and it should tell you something similar to:

Invalid read of size 8
   at 0x61FC5CE: gcn::FocusHandler::remove(gcn::Widget*)
   by 0x6204CE0: gcn::Widget::_setFocusHandler(gcn::FocusHandler*)
   by 0x61FA4B2: gcn::BasicContainer::_setFocusHandler(gcn::FocusHandler*)
   by 0x61FA6CE: gcn::BasicContainer::clear()
   by 0x61FA950: gcn::BasicContainer::~BasicContainer()
   by 0x620A560: gcn::DropDown::~DropDown()
 Address 0xBD6F208 is 0 bytes inside a block of size 16 free'd
   at 0x4A1F37C: operator delete(void*)
   by 0x620A533: gcn::DropDown::~DropDown()

Original comment by guillaum...@gmail.com on 7 Aug 2007 at 7:22

[GoogleCodeExporter]

You are completely right. The focus handler should be the internal focus 
handler as
the widget checks last added focus handler. The patch has been applied. Thanks!

Original comment by olof.nae...@gmail.com on 7 Aug 2007 at 10:05

• Changed state: Fixed

[GoogleCodeExporter]

Original comment by b.lindeijer on 15 May 2008 at 2:59