Bug 20932 - Notable leaks in QtWebkit - from Qt library
Summary: Notable leaks in QtWebkit - from Qt library
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: QtLauncherRobot (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-09-19 00:24 PDT by Balazs Kelemen
Modified: 2009-03-06 10:00 PST (History)
1 user (show)

See Also:


Attachments
Valgrind output with comments (29.94 KB, text/plain)
2008-09-19 00:34 PDT, Balazs Kelemen
no flags Details
Backtraces of the leaks issues from QTextLine::setLineWidth(double) - qtextlayout.cpp (15.55 KB, application/zip)
2008-09-19 00:51 PDT, Balazs Kelemen
no flags Details
Backtraces of the leaks issues from WebCore::Font::Font(WebCore::FontDescription const&, short, short) - FontQt.cpp (7.81 KB, application/zip)
2008-09-19 00:58 PDT, Balazs Kelemen
no flags Details
Patch for robotized work of QtLauncher (3.24 KB, patch)
2008-10-03 08:44 PDT, Balazs Kelemen
hausmann: review-
Details | Formatted Diff | Diff
The list of links that I loaded for testing with valgrind (25.63 KB, text/plain)
2008-10-03 08:46 PDT, Balazs Kelemen
no flags Details
proposed patch (4.88 KB, patch)
2008-10-09 01:42 PDT, Balazs Kelemen
hausmann: review-
Details | Formatted Diff | Diff
proposed patch (4.29 KB, patch)
2008-11-10 02:18 PST, Balazs Kelemen
eric: review-
Details | Formatted Diff | Diff
proposed patch (with coding style correctings) (4.27 KB, patch)
2008-12-09 05:00 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2008-09-19 00:24:29 PDT
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.
Comment 1 Balazs Kelemen 2008-09-19 00:34:16 PDT
Created attachment 23557 [details]
Valgrind output with comments
Comment 2 Balazs Kelemen 2008-09-19 00:51:44 PDT
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);
Comment 3 Balazs Kelemen 2008-09-19 00:58:37 PDT
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(' '));
Comment 4 Balazs Kelemen 2008-09-19 01:04:50 PDT
I forgot to tell: these results are apply to rev35864
Comment 5 Balazs Kelemen 2008-10-03 08:41:41 PDT
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.
Comment 6 Balazs Kelemen 2008-10-03 08:44:45 PDT
Created attachment 24053 [details]
Patch for robotized work of QtLauncher
Comment 7 Balazs Kelemen 2008-10-03 08:46:48 PDT
Created attachment 24054 [details]
The list of links that I loaded for testing with valgrind
Comment 8 Simon Hausmann 2008-10-07 01:30:47 PDT
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.
Comment 9 Balazs Kelemen 2008-10-07 01:48:02 PDT
(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.


Comment 10 Simon Hausmann 2008-10-07 01:51:29 PDT
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 :)
Comment 11 Balazs Kelemen 2008-10-09 01:42:34 PDT
Created attachment 24227 [details]
proposed patch
Comment 12 Balazs Kelemen 2008-10-09 01:51:19 PDT
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.
Comment 13 Simon Hausmann 2008-11-05 08:05:46 PST
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 @@
> +2008-10-09  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!
Comment 14 Balazs Kelemen 2008-11-10 02:17:31 PST
*** Bug 22037 has been marked as a duplicate of this bug. ***
Comment 15 Balazs Kelemen 2008-11-10 02:18:50 PST
Created attachment 25012 [details]
proposed patch
Comment 16 Balazs Kelemen 2008-12-05 08:09:47 PST
Could you be so kind and take a look to the updated patch?
I think it is clear now.
Comment 17 Eric Seidel (no email) 2008-12-08 12:44:21 PST
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.
Comment 18 Balazs Kelemen 2008-12-09 05:00:24 PST
Created attachment 25881 [details]
proposed patch (with coding style correctings)
Comment 19 Balazs Kelemen 2008-12-09 05:12:00 PST
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 :)
Comment 20 Simon Hausmann 2008-12-10 07:58:47 PST
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.
Comment 21 Simon Hausmann 2008-12-10 08:00:13 PST
Comment on attachment 25881 [details]
proposed patch (with coding style correctings)

Landed in r39170
Comment 22 Balazs Kelemen 2009-03-06 09:30:30 PST
I have an idea about the Font leak: WebCore::SimpleFontData::platformDestroy should do something (now it is empty). What is your opinion?
Comment 23 Balazs Kelemen 2009-03-06 10:00:18 PST
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.