Bug 20932 – Notable leaks in QtWebkit - from Qt library

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Balazs Kelemen]

I tested WebKit's Qt port on Linux with Valgrind to search for memory leaks. I modified QtLauncher by using the QtWebKit API and I used the loadFinished signal to load URLs from a list file one by one. I found several notable memory leaks (one of them is bigger than 3MB).
The attachment contains the call stacks (outputs of Valgrind) and my comments which show the points where the execution crosses the border of the source code of WebKit. (In some cases the traces were almost the same therefore I kept only one of them in the log file but the zip files contain all different traces.) Unfortunately, I cannot decide whether these are Qt bugs or WebKit uses Qt in a bad way.

[Balazs Kelemen]

Created attachment 23557 [details]
Valgrind output with comments

[Balazs Kelemen]

Created attachment 23560 [details]
Backtraces of the leaks issues from QTextLine::setLineWidth(double) - qtextlayout.cpp

These leaks are issues from that line: 
line.setLineWidth(INT_MAX/256);

[Balazs Kelemen]

Created attachment 23561 [details]
Backtraces of the leaks issues from WebCore::Font::Font(WebCore::FontDescription const&, short, short) - FontQt.cpp

These leaks issues from the line:
m_spaceWidth = metrics.width(QLatin1Char(' '));

[Balazs Kelemen]

I forgot to tell: these results are apply to rev35864

[Balazs Kelemen]

I'm surprised of there is no one who deals with leaks.
I create the patch for show my changes in QtLauncher, and send the urllist that
I used. Maybe this is helpful for you. I hope if my work is not valueless.

[Balazs Kelemen]

Created attachment 24053 [details]
Patch for robotized work of QtLauncher

[Balazs Kelemen]

Created attachment 24054 [details]
The list of links that I loaded for testing with valgrind

[Simon Hausmann]

Hi Kelemen,

we're pretty swamped right now, that explains our slow response times :(

I'm not sure if the m_spaceWidth = metrics.width(...) lines are real leaks, valgrind may report some leaks for allocations that happen just once.

[Balazs Kelemen]

(In reply to comment #8)
> I'm not sure if the m_spaceWidth = metrics.width(...) lines are real leaks,
> valgrind may report some leaks for allocations that happen just once.

There are many leaks (according to valgrind) comes from that line as you can see
in the second zip, so I don't think if there is only one allocation by the result
of that line.

[Simon Hausmann]

Comment on attachment 24053 [details]
Patch for robotized work of QtLauncher

97     QWebView *webView() const {

Small coding style nitpick: Please use QWebView* webView()

389     WTF::Vector<QString*> m_urls;

QString is a value based class, you can simplify your code a lot and free it from leaks by using WTF::Vector<QString> instead. Right now the QStrings allocated in it are never freed.

In fact for the init() function I suggest the use of QString for the inputFile argument and I also suggest the use of QFile and QTextStream instead of the C file API. It will make your code simplier and more portable :)

r- for now, but in principle I agree with your patch and idea and I'd love to see the patch go in :)

[Balazs Kelemen]

Created attachment 24227 [details]
proposed patch

[Balazs Kelemen]

Thank you for the good feedback.
Here is the new version. I followed your advices and kept my eye
on the code style guidelines. Now the feature is activating by
"-r filename" parameters.

[Simon Hausmann]

Comment on attachment 24227 [details]
proposed patch

This patch looks a lot better! :-)

> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 37444)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,24 @@
> [phone]  System User  <set EMAIL_ADDRESS environment variable>

You're missing your name and email address here :)

> Index: WebKit/qt/QtLauncher/QtLauncher.pro
> ===================================================================
> --- WebKit/qt/QtLauncher/QtLauncher.pro	(revision 37432)
> +++ WebKit/qt/QtLauncher/QtLauncher.pro	(working copy)
> @@ -3,6 +3,9 @@ SOURCES += main.cpp
>  CONFIG -= app_bundle
>  CONFIG += uitools
>  DESTDIR = ../../../bin
> +INCPATH += $$PWD/../../../JavaScriptCore/wtf
> +LIBS += -L../../../JavaScriptCore/ 
> +LIBS += -lJavaScriptCore

Ohh, is this needed because of the use of WTF::Vector?

I think in that case it would be easier to use QVector.
The code becomes a bit simpler at the same time - you could replace m_urls.insert(i++, url) with m_urls.append(url).

Then it should not be necessary to link against the static JavaScriptCore library.

>  include(../../../WebKit.pri)
>  
> Index: WebKit/qt/QtLauncher/main.cpp
> ===================================================================
> --- WebKit/qt/QtLauncher/main.cpp	(revision 37432)
> +++ WebKit/qt/QtLauncher/main.cpp	(working copy)
> @@ -42,6 +42,11 @@
>  
>  #include <QtUiTools/QUiLoader>
>  
> +#include <Vector.h>
> +#include <QTextStream>
> +#include <QFile>
> +#include <cstdio>
> +
>  class WebPage : public QWebPage
>  {
>  public:
> @@ -83,10 +88,14 @@ public:
>          }
>      }
>  
> -    QWebPage *webPage() const {
> +    QWebPage* webPage() const {
>          return view->page();
>      }
>  
> +    QWebView* webView() const {
> +        return view;
> +    }
> +
>  protected slots:
>  
>      void changeLocation() {
> @@ -322,6 +331,72 @@ QObject *WebPage::createPlugin(const QSt
>      return loader.createWidget(classId, view());
>  }
>  
> +class URLLoader : public QObject
> +{
> +    Q_OBJECT
> +public:
> +    URLLoader(QWebView* view, const QString& inputFileName)
> +        : m_view(view),
> +         stdOut(stdout)
> +    {
> +        init(inputFileName);
> +    }
> +
> +public slots:
> +    void loadNext()
> +    {
> +        QString qstr;
> +        if (getUrl(qstr)) {
> +            QUrl url(qstr, QUrl::StrictMode);
> +            if (url.isValid()) {
> +                stdOut<<"Loading "<<qstr<<" ......"<<endl;

Please put a space between the arguments.

r- because of the small issues, but in general the patch looks good!

[Balazs Kelemen]

*** Bug 22037 has been marked as a duplicate of this bug. ***

[Balazs Kelemen]

Created attachment 25012 [details]
proposed patch

[Balazs Kelemen]

Could you be so kind and take a look to the updated patch?
I think it is clear now.

[Eric Seidel (no email)]

Comment on attachment 25012 [details]
proposed patch

I'm confused.  You move the location of * (to match WebKit style I assume?) yet you don't clean up all the other style violations in this file (and add some of your own with this patch -- like { } around single line ifs).  I'm not sure what style the QtLauncher code is supposed to be in, but I assume WebKit style.  I didn't actually look at the substance of the patch.

[Balazs Kelemen]

Created attachment 25881 [details]
proposed patch (with coding style correctings)

[Balazs Kelemen]

I corrected my coding style violations. 
I didn't want to correct all of them in the file. The * was simply
above my code, and I thought I should take it to the right place :)

[Simon Hausmann]

Comment on attachment 25881 [details]
proposed patch (with coding style correctings)

Looks good. A small style bit is left in main(), but I'll fix it as I and it.

[Simon Hausmann]

Comment on attachment 25881 [details]
proposed patch (with coding style correctings)

Landed in r39170

[Balazs Kelemen]

I have an idea about the Font leak: WebCore::SimpleFontData::platformDestroy should do something (now it is empty). What is your opinion?

[Balazs Kelemen]

After a deeper look I realized that WebKit itself has the responsibility, not 
the Qt specific parts. Sorry for my thoughtlessness. The reason of the leaks is that WebCore::FontFallbackList::fontDataAt allocs SimpleFontData-s and gives back the pointer but these have never been freed. We are working on the problem.