Bug 72513 – [Qt] Test fonts are not used with Qt5

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Balazs Kelemen]

The fontconfig code in DRT and WTR is guarded with Q_WS_X11 which is not defined with Qt5. Blame that it has missed our attention... Likely we would have to rebaseline the world.

[Balazs Kelemen]

Created attachment 115557 [details]
Patch

Patch for feedback. Not dealing with rebaselining results which is definitely necessary.

[Csaba Osztrogonác]

Comment on attachment 115557 [details]
Patch

cq-, because we should check its impact to the the test results before landing.

[Simon Hausmann]

Comment on attachment 115557 [details]
Patch

Looks good, but I'm pretty sure this patch is going to break with Qt 4.8, because Qt 4 doesn't support the configure tests. (r-)

However I think the solution is easy: For Qt 4 we make Qt DRT linux/x11 only and just _assume_ (thus require) the presence of font-config, i.e. set the define and pkgconfig if haveQt(4) in the .pro file (or the test passes with qt5).

[Balazs Kelemen]

Created attachment 116502 [details]
Patch

[Simon Hausmann]

Comment on attachment 116502 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116502&action=review

> Tools/DumpRenderTree/qt/main.cpp:-256
> -#ifdef Q_WS_X11
> -    FcConfigSetCurrent(0);
> -#endif

Why are you removing this function call?

> Tools/WebKitTestRunner/Target.pri:38
> -!embedded: PKGCONFIG += fontconfig
> +contains(config_test_fontconfig, yes) {
> +    PKGCONFIG += fontconfig
> +}

Is this actually needed? Isn't it only the injected bundle that's using fontconfig? Perhaps we can remove the font-config linkage for WTR (the app) altogether?

[Balazs Kelemen]

(In reply to comment #5)
> (From update of attachment 116502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116502&action=review
> 
> > Tools/DumpRenderTree/qt/main.cpp:-256
> > -#ifdef Q_WS_X11
> > -    FcConfigSetCurrent(0);
> > -#endif

That is just at the end of main. I don't think that has any purpose.
BTW, the WTR version don't has this.

> 
> Why are you removing this function call?
> 
> > Tools/WebKitTestRunner/Target.pri:38
> > -!embedded: PKGCONFIG += fontconfig
> > +contains(config_test_fontconfig, yes) {
> > +    PKGCONFIG += fontconfig
> > +}
> 
> Is this actually needed? Isn't it only the injected bundle that's using fontconfig? Perhaps we can remove the font-config linkage for WTR (the app) altogether?

You are right it's not needed for WTR, I'm going to remove it.

[Balazs Kelemen]

Created attachment 116518 [details]
Patch

[Simon Hausmann]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 116502 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=116502&action=review
> > 
> > > Tools/DumpRenderTree/qt/main.cpp:-256
> > > -#ifdef Q_WS_X11
> > > -    FcConfigSetCurrent(0);
> > > -#endif
> 
> That is just at the end of main. I don't think that has any purpose.
> BTW, the WTR version don't has this.

It was added to avoid a crash on exit. If that crash isn't reproducible anymore, then I'm fine with removing it. Have you experienced any (new?) crashes when running all the layout tests with that change?

[Simon Hausmann]

Comment on attachment 116518 [details]
Patch

r=me

(My concern about the FcSetCurrent(0) doesn't apply as you pointed out that the line comes after return app.exec() :-)

[Csaba Osztrogonác]

I tested this patch, it works fine on the qt-4.8 and qt-5.0 bots. 
But I got this strange crasher for all tests on my Ubuntu:

(gdb) bt
#0  0x031e0cc6 in ?? () from /lib/i386-linux-gnu/libc.so.6
#1  0x02d9e548 in QString::fromUtf8(char const*, int) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtCore.so.5
#2  0x04d5d468 in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/plugins/platforms/libxcb.so
#3  0x04d5d31a in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/plugins/platforms/libxcb.so
#4  0x0296509c in QGuiApplication::font() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtGui.so.5
#5  0x029f0efa in QFont::QFont() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtGui.so.5
#6  0x021a574f in QWidgetPrivate::QWidgetPrivate(int) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtWidgets.so.5
#7  0x021b508c in QWidget::QWidget(QWidget*, QFlags<Qt::WindowType>) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtWidgets.so.5
#8  0x08054576 in main ()

Have you got any idea what is it? (Unfortunately I have 32 bit Ubuntu and debug build is impossible on 32 bit.)

[Simon Hausmann]

(In reply to comment #10)
> I tested this patch, it works fine on the qt-4.8 and qt-5.0 bots. 
> But I got this strange crasher for all tests on my Ubuntu:
> 
> (gdb) bt
> #0  0x031e0cc6 in ?? () from /lib/i386-linux-gnu/libc.so.6
> #1  0x02d9e548 in QString::fromUtf8(char const*, int) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtCore.so.5
> #2  0x04d5d468 in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/plugins/platforms/libxcb.so
> #3  0x04d5d31a in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/plugins/platforms/libxcb.so
> #4  0x0296509c in QGuiApplication::font() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtGui.so.5
> #5  0x029f0efa in QFont::QFont() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtGui.so.5
> #6  0x021a574f in QWidgetPrivate::QWidgetPrivate(int) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtWidgets.so.5
> #7  0x021b508c in QWidget::QWidget(QWidget*, QFlags<Qt::WindowType>) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtWidgets.so.5
> #8  0x08054576 in main ()
> 
> Have you got any idea what is it? (Unfortunately I have 32 bit Ubuntu and debug build is impossible on 32 bit.)

Darn, that almost sounds like a widget being created before QApplication.

[Csaba Osztrogonác]

Simon suggested me to comment out the line "QApplication::setFont(QWidget().font());", but I still get similar crash:

(gdb) bt
#0  0x031e0cc6 in ?? () from /lib/i386-linux-gnu/libc.so.6
#1  0x02d9e548 in QString::fromUtf8(char const*, int) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtCore.so.5
#2  0x04d5d468 in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/plugins/platforms/libxcb.so
#3  0x04d5d31a in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/plugins/platforms/libxcb.so
#4  0x0296509c in QGuiApplication::font() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtGui.so.5
#5  0x029f0efa in QFont::QFont() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtGui.so.5
#6  0x002ecd41 in QWebSettings::QWebSettings() () from /storage/WebKit/WebKitBuild/Release/lib/libQtWebKit.so.4
#7  0x002ef0a6 in QWebSettings::globalSettings() () from /storage/WebKit/WebKitBuild/Release/lib/libQtWebKit.so.4
#8  0x002f1fd7 in QWebSettings::enablePersistentStorage(QString const&) ()
   from /storage/WebKit/WebKitBuild/Release/lib/libQtWebKit.so.4
#9  0x08057962 in WebCore::DumpRenderTree::DumpRenderTree() ()
#10 0x08054573 in main ()

[Csaba Osztrogonác]

Comment on attachment 116518 [details]
Patch

Landed in http://trac.webkit.org/changeset/102776.

It works fine on the buildbot, on Ubuntus in real hardware, on the Amazon image. It fails only my VirtualBox Ubuntu VM, but it doesn't a big problem now.

[Csaba Osztrogonác]

Unfortunately it broke the MIPS and SH4 build. :-/

I have no idea how to fix MIPS and SH4 ... I tried it,
but it wasn't good: http://trac.webkit.org/changeset/102795

I don't want to roll out this patch, because I unskipped thousends of tests for the Qt5 platform which is very important.

Holger, could you check it, please?

[Simon Hausmann]

(In reply to comment #14)
> Unfortunately it broke the MIPS and SH4 build. :-/
> 
> I have no idea how to fix MIPS and SH4 ... I tried it,
> but it wasn't good: http://trac.webkit.org/changeset/102795
> 
> I don't want to roll out this patch, because I unskipped thousends of tests for the Qt5 platform which is very important.
> 
> Holger, could you check it, please?

It probably just requires a clean rebuild due to the change in DEFINES.

[Csaba Osztrogonác]

Fix landed in http://trac.webkit.org/changeset/102806

[Balazs Kelemen]

(In reply to comment #16)
> Fix landed in http://trac.webkit.org/changeset/102806

Thanks for handling the build Ossy. Btw, I wonder why was this last one necessary. Is it about precedence? (I assumed AND is stronger than OR but maybe qmake is crazy enough to believe in different mathematics.)

[Holger Freyther]

(In reply to comment #17)
> (In reply to comment #16)
> > Fix landed in http://trac.webkit.org/changeset/102806
> 
> Thanks for handling the build Ossy. Btw, I wonder why was this last one necessary. Is it about precedence? (I assumed AND is stronger than OR but maybe qmake is crazy enough to believe in different mathematics.)

The motivation was that fontconfig has nothing to do with SH4/MIPS but is more about the configuration of these two bots (both target QWS and not X11). And AFAIR the default configuration of an embedded build will not use fontconfig?

Without looking much at the context, how will DRT directly link to -lfontconfig? Who manipulates the LIBS?

[Balazs Kelemen]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Fix landed in http://trac.webkit.org/changeset/102806
> > 
> > Thanks for handling the build Ossy. Btw, I wonder why was this last one necessary. Is it about precedence? (I assumed AND is stronger than OR but maybe qmake is crazy enough to believe in different mathematics.)
> 
> The motivation was that fontconfig has nothing to do with SH4/MIPS but is more about the configuration of these two bots (both target QWS and not X11). And AFAIR the default configuration of an embedded build will not use fontconfig?
> 
> Without looking much at the context, how will DRT directly link to -lfontconfig? Who manipulates the LIBS?

I was thinking about http://trac.webkit.org/changeset/102824, the very last one :) Sorry for the confusion.