Review Request: Fix for debugger crash and memory leak

[Vladimir Prus]

On Friday 16 April 2010 12:53:34 Thomas Schöps wrote:

> 
> > On 2010-04-16 08:37:10, vladimir_prus wrote:
> > > dynamic_cast, when applied to EllipsisItem, is supposed to return NULL, and therefore be ignored. I wonder if elipsis item is actually deleted, while more_ is set to true by mistake.
> 
> Okay, sorry, my knowledge of C++ style casts is weak. But where do you think the crash then comes from?
> And what do you think about the suggestion for TreeItem::deleteChildren()?
> 
> 
> - Thomas
> 
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3619/#review5072
> -----------------------------------------------------------
> 
> 
> On 2010-04-16 07:24:26, Thomas Schöps wrote:
> > 
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > http://reviewboard.kde.org/r/3619/
> > -----------------------------------------------------------
> > 
> > (Updated 2010-04-16 07:24:26)
> > 
> > 
> > Review request for KDevelop.
> > 
> > 
> > Summary
> > -------
> > 
> > As the debugger is rather unstable for me and I think it is important to improve this before the 4.0.0 release, I tried to debug it a bit. I had a look at this crash:
> > 
> > Thread 1 (Thread 0xb76d4700 (LWP 4615)):
> > [KCrash Handler]
> > #6  0x008be7bc in __dynamic_cast () from /usr/lib/libstdc++.so.6
> > #7  0x07a5982c in KDevelop::Variable::setInScope (this=0xa7db178, v=true) at /home/thomas/Data/kdevplatform/debugger/variable/variablecollection.cpp:103
> > #8  0x025ff505 in KDevelop::GdbVariable::handleUpdate (this=0xa7db178, var=...) at /home/thomas/Data/kdevelop/debuggers/gdb/gdbvariable.cpp:267
> > #9  0x025facae in GDBDebugger::VariableController::handleVarUpdate (this=0xa728b88, r=...) at /home/thomas/Data/kdevelop/debuggers/gdb/variablecontroller.cpp:94
> > #10 0x025cf83c in GDBDebugger::GDBCommand::invokeHandler (this=0xd1366e8, r=...) at /home/thomas/Data/kdevelop/debuggers/gdb/gdbcommand.cpp:76
> > #11 0x025c7e08 in GDBDebugger::GDB::processLine (this=0xccab8e8, line=...) at /home/thomas/Data/kdevelop/debuggers/gdb/gdb.cpp:278
> > #12 0x025c8a2e in GDBDebugger::GDB::readyReadStandardOutput (this=0xccab8e8) at /home/thomas/Data/kdevelop/debuggers/gdb/gdb.cpp:175
> > #13 0x025c4eb3 in GDBDebugger::GDB::qt_metacall (this=0xccab8e8, _c=QMetaObject::InvokeMetaMethod, _id=15, _a=0xbfeb648c) at /home/thomas/Data/kdevelop/build/debuggers/gdb/moc_gdb.cpp:105
> > #14 0x0506c263 in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib/libQtCore.so.4
> > ...
> > 
> > The problem seems to be that KDevelop::Variable::setInScope assumes that all its children can be casted to Variable*, but there can be an ellipsis item as the last one which is of type EllipsisItem and not Variable.
> > I tested it: setInScope is called with more_ being true (meaning that there is an ellipsis item). So this code will fail, because childCount() includes the ellipsis item and child() returns it:
> > 
> > for (int i=0; i < childCount(); ++i) {
> >     if (Variable *var = dynamic_cast<Variable*>(child(i))) {
> >         var->setInScope(v);
> >     }
> > }
> > 
> > Iterating over childItems excludes the ellipsis item.
> > 
> > While looking for the cause, I also had a look at TreeItem::deleteChildren(). The "copy" vector is currently unused there, and because clear() calls childItems.clear(), the child items won't be deleted in the following loop. Apparently the author wanted to use the copy for deleting. Also, IVariableController::stateChanged calls this method only on the first level of Locals while removing them, so I think it should recursively delete all children instead of only the first level. I've added the recursive deletion in the patch.

I assume you can reproduce the crash? Do you think you can, before dynamic_cast,
add Q_ASSERT that the pointer being casted is not NULL?

I am underwater now, so don't have comment re other bits -- ping me next week
if I don't respond or somebody else don't comment.

- Volodya