Review Request: Fix for debugger crash and memory leak

[Milian Wolff]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3619/#review5074
-----------------------------------------------------------


some comments from me, even though I'm not acquainted with the debugger code:

First up: it would help greatly, if you could come up with a unit test.
If that is there, one can actually prove that the solution is correct.
Furthermore, we could run valgrind on it easily, to see how the model gets screwed up.
If you can't come up with a unit test, but know how to reproduce this issue,
it would probably help already. You could also try to reproduce while
running in valgrind. It would be slow, but it should show you the problem
directly. Don't forget --track-origins=yes.


trunk/extragear/sdk/kdevplatform/debugger/util/treeitem.cpp
<http://reviewboard.kde.org/r/3619/#comment4553>

    this should be handled in the dtor of the TreeItem, but looking at it, it does not update the model which might lead to invalid items.



trunk/extragear/sdk/kdevplatform/debugger/variable/variablecollection.cpp
<http://reviewboard.kde.org/r/3619/#comment4554>

    the EllipsisItem inherits from TreeItem, so this should not be required.
    
    Also: when you get a crash in a dynamic_cast, the pointer is dangling - the problem lies elsewhere


- Milian


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.
> 
> 
> Diffs
> -----
> 
>   trunk/extragear/sdk/kdevplatform/debugger/util/treeitem.cpp 1115078 
>   trunk/extragear/sdk/kdevplatform/debugger/variable/variablecollection.cpp 1115078 
> 
> Diff: http://reviewboard.kde.org/r/3619/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Thomas
> 
>