Wednesday, November 21, 2007

Memory leak: Ui files and direct approach

The KDE codebase often uses a forward declaration in the .h-file to speedup compilation. The code often looks like this:
// header file
namespace Ui { class MyWidget; }
class MyDialog : public KDialog {
// ...
private:
Ui::MyWidget *ui;
};
The impl looks like this:
// source file
#include "mydialog.h"
#include "ui_mywidget.h"
MyDialog::MyDialog() : KDialog()
{
QWidget *w = new QWidget(this);
setMainWidget(w);
ui = new Ui::MyWidget(); // allocation
ui->setupUi(w);
// ui->...
}
See the memory leak? You have to call »delete ui;« in the destructor if you use the »direct approach«. Searching in lxr.kde.org shows lots of results, and in some places this delete is missing indeed. Happy fixing :)


Update: The really correct fix is to guard the pointer with an auto_ptr or scoped_ptr. For further details read the comments below. Or use another approach to include your ui-file.


If you do not want to delete it manually, you can for instance use a workaround like this:
// header file
namespace Ui { class MyWidget; }
class MyWidget;
class MyDialog : public KDialog {
// ...
private:
Ui::MyWidget *ui;
};
Source file:
// source file
#include "mydialog.h"
#include "ui_mywidget.h"

class MyWidget : public QWidget, public Ui::MyWidget {
public:
MyWidget( QWidget * parent = 0 ) : QWidget( parent )
{ setupUi(this); }
};

MyDialog::MyDialog() : KDialog()
{
ui = new MyWidget(this);
setMainWidget(ui);
QWidget *w = new QWidget(this);
setMainWidget(w);
ui = new Ui::MyDialog(); // allocation
ui->setupUi(w);
// ui->...
}

17 comments:

Unknown said...

This is of course due to my unsufficiently deep understanding of the Qt object system, but I can't see why "ui" isn't deleted if it's in the Ui namespace... Could you please detail this ?

Thanks

Ben Schleimer said...

Um, If you read the qt docs for QWidget, you will see:


explicit QWidget::QWidget ( QWidget * parent = 0, const char * name = 0, WFlags f = 0 )
Constructs a widget which is a child of parent, with the name name and widget flags set to f.

If parent is 0, the new widget becomes a top-level window. If parent is another widget, this widget becomes a child window inside parent. The new widget is deleted when its parent is deleted.


Qt automatically deletes all QObjects when the parent is deleted.

So you should revert your changes.

Cheers
Ben

dhaumann said...

Of course all QObject derived classes are deleted automatically. The class Ui::MyWidget is *not* QObject-derived and we do a »new Ui::MyWidget()«.

Ui::MyWidget has quite some member variables: pointers to the widgets. All pointers take 4 byte. Yes, those widgets *behind* those pointers are deleted, but those N*4 bytes for the N pointers in the Ui::MyWidget class are not deleted.

Either I'm really getting something wrong here or you misunderstood what I wanted to say =)

Diederik van der Boor said...

If you want to be sure it's a leak, run:

valgrind --tool=memcheck --leak-check=yes /path/to/your/app

Paolo Capriotti said...

I think the best approach here is to use std::auto_ptr (or even better, tr1::scoped_ptr).

dhaumann said...

I tested it with a ui_mywidget.h file with 6 pointers, the result I got is:

==10045== 24 bytes in 1 blocks are definitely lost in loss record 39 of 135
==10045== at 0x402297F: operator new(unsigned) (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==10045== by 0x804B214: MyWidget::MyWidget() (in temp/cmake/foo)
==10045== by 0x804A77D: main (in temp/cmake/foo)

That looks exactly like those 6*4 bytes.

@Paolo: auto_ptr / scoped_ptr sounds like a really good idea :)

Thomas said...

What I always do is to define it like this;

private:
Ui::Foo widget;

which is faster (mallocs are slow!!) and avoids this whole problem.

Laurent said...
This comment has been removed by the author.
Laurent said...

Why to you need at all a Ui::MyWidget data *member* in the class?!

I always proceed like the Qt-4.3 manual says:

MyDialog::MyDialog() : KDialog()
{
QWidget *w = new QWidget(this);
setMainWidget(w);
Ui::MyWidget ui; // create a temporary object on the stack
ui.setupUi(w);
}

and that's it! The header file MyWidget.h does not even need to forward-declare UI::MyWidget. Only the MyWidget.cpp file has to include the full definition of it, in "ui_mywidget.h".

That way, which is the way documented by Trolltech, no memory leak, as the UI object is only created and used in the constructor itself.

dhaumann said...

@Laurent: Usually you need an accessor to the elements, e.g. if you want to read data from a lineedit in another function.

Note that I've blogged about it because the KDE codebase *does have those memory leaks*. I just want to make people aware of it. I don't say version X or Y is the way to go. But a way without memory leaks is certainly best :)

Laurent said...

I usually use qFindChild to retrieve pointers to the child widgets created that way. I can admit that it is not a strait-forward solution.

Unknown said...

I don't know anything about Qt and honestly I don't really care much :-), but I'd like to stress the relevance of Paolo Capriotti's remark.

Using an auto_ptr or scoped_ptr is more than good (readable! code is about communicating) style: it's the only correct solution in the presense of exceptions.

The dtor of an object is only run if that object has been fully created. If the construction of a member variable fails, members allocated on the free store may leak. Consider the following example:

#include iostream
#include boost/scoped_ptr.hpp

struct A
{ ~A() { std::cout << "dtor A" << std::endl; } };
struct B
{
B() { throw 1; }
~B() { std::cout << "dtor B" << std::endl; }
};

struct partial_constructed
{
partial_constructed()
: a_( new A )
, b_( new B )
{ }

~partial_constructed()
{ delete a_; delete b_; }

A *a_;
B *b_;
};

struct partial_constructed2
{
partial_constructed2()
: a_( new A )
, b_( new B )
{ }

~partial_constructed2()
{ /* nothing up my sleeve */ }

boost::scoped_ptr< A > a_;
boost::scoped_ptr< B > b_;
};

int main()
{
std::cout << "first test" << std::endl;
try
{ partial_constructed aap; }
catch( ... )
{ /* expected */ }
std::cout << "second test" << std::endl;
try
{ partial_constructed2 aap; }
catch( ... )
{ /* expected */ }
}

The first example leaks an A object. In this case it's 4 bytes which doesn't seem like a big deal, but things get bad if you hold file descriptors, mutexes and the like.

dhaumann said...

Indeed. I've added an update to the post.

Bardur Arantsson said...

Never use auto_ptr unless you know exactly what you're doing. It has very surprising semantics (to "most" people). Even in a situation such as this where it does work it passively encourages people to use it in other places where it (sublty) doesn't work.

Just use shared_ptr and be happy. The advantage of shared_ptr is that it calls the proper destructor even when the pointee is forward-declared. :)

Paolo Capriotti said...

@bardur

Widgets are typically noncopyable, so auto_ptr is perfectly safe in this context.
As I said, scoped_ptr is generally better, because then you have noncopyable behaviour explicitly enforced.

Bardur Arantsson said...

Unless they changed it "recently" (in the last two years) scoped_ptr doesn't work properly with forward declared classes with virtual destructors. (The base class destructor is called instead.)

Laurent said...

@Niels Aan de Brugh: My solution is safe also, because it does not allocate anything but on the stack.