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.
Created attachment 23557 [details] Valgrind output with comments
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);
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(' '));
I forgot to tell: these results are apply to rev35864
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.
Created attachment 24053 [details] Patch for robotized work of QtLauncher
Created attachment 24054 [details] The list of links that I loaded for testing with valgrind
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.
(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 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 :)
Created attachment 24227 [details] proposed patch
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 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!
*** Bug 22037 has been marked as a duplicate of this bug. ***
Created attachment 25012 [details] proposed patch
Could you be so kind and take a look to the updated patch? I think it is clear now.
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.
Created attachment 25881 [details] proposed patch (with coding style correctings)
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 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 on attachment 25881 [details] proposed patch (with coding style correctings) Landed in r39170
I have an idea about the Font leak: WebCore::SimpleFontData::platformDestroy should do something (now it is empty). What is your opinion?
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.