Bug 53427 - [Qt][WK2] Canvas tests make css2.1 ones failing
: [Qt][WK2] Canvas tests make css2.1 ones failing
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit2
: 528+ (Nightly build)
: PC All
: P3 Normal
Assigned To: Zoltan Herczeg
: LayoutTestFailure, Qt, QtTriaged
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-31 08:48 PST by Balazs Kelemen
Modified: 2012-02-03 08:28 PST (History)
6 users (show)

See Also:


Attachments
patch (3.74 KB, patch)
2011-06-08 23:58 PDT, Zoltan Herczeg
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 2011-01-31 08:48:46 PST
Pretty annoying bug. If one runs canvas tests than all of the css2.1 tests failing with WTR.
Without the canvas tests there is only 3 failure in css2.1. The whole story happening only on
the bot's 32 bit Debian environment. On the same machine in a 64 bit chroot there is no such
problem.
Comment 1 Balazs Kelemen 2011-02-01 04:44:24 PST
I have reproduced the problem on a different 32 bit Linux environment.
Since the css2.1 tests are testing more basic and so important parts,
I think it would be better to skip the canvas tests for now. I am going
to upload the patch.
Comment 2 Balazs Kelemen 2011-02-01 05:00:20 PST
After a look of the number of the css2.1 and the canvas tests, and some
discussion with Ossy I have rejected the idea of skipping the canvas tests.
Comment 3 Balazs Kelemen 2011-02-01 05:03:50 PST
Some snippet from the output of run-webkit-tests -2 --verbose

running canvas/philip/tests/type.extend.html -> succeeded
running canvas/philip/tests/type.name.html -> succeeded
running canvas/philip/tests/type.replace.html -> succeeded
running css1/box_properties/float_on_text_elements.html -> failed
running css1/classification/white_space.html -> failed
running css1/units/zero-duration-without-units.html -> succeeded
running css2.1/t09-c5526c-display-00-e.html -> failed
running css2.1/t0402-c71-fwd-parsing-00-f.html -> failed
running css2.1/t0402-c71-fwd-parsing-01-f.html -> failed
running css2.1/t0402-c71-fwd-parsing-02-f.html -> failed
running css2.1/t0402-c71-fwd-parsing-03-f.html -> failed
running css2.1/t0402-c71-fwd-parsing-04-f.html -> failed

... (lot of failing css2.1 tests)

running css2.1/t1204-root-e.html -> failed
running css2.1/t1205-c561-list-displ-00-b.html -> failed
| Closing DumpTool |
| Opening DumpTool |
running css2.1/t1205-c563-list-type-00-b.html -> succeeded
running css2.1/t1205-c563-list-type-01-b.html -> succeeded
running css2.1/t1205-c564-list-img-00-b-g.html -> succeeded
running css2.1/t1205-c565-list-pos-00-b.html -> succeeded

... (almost all the css2.1 tests runs successfully from here)
Comment 4 Zoltan Herczeg 2011-05-27 06:14:53 PDT
QApplication::fontMetrics().averageCharWidth() doubles during the running of the test. Something affects this member. Do you have any idea?
Comment 5 Zoltan Herczeg 2011-05-27 06:24:35 PDT
(Besides QApplication::setFont() which is not happen)
Comment 6 Zoltan Herczeg 2011-05-30 05:36:25 PDT
Big progress! The increase happen during:

WebCore/bindings/js/GCController.cpp: gcTimerFired()

Don't ask how I found it ;) I suspect a Qt specific object do bad things during its destructor.
Comment 7 Zoltan Herczeg 2011-05-31 07:42:34 PDT
Another big step:

Source/WebCore/html/HTMLCanvasElement.cpp
HTMLCanvasElement::~HTMLCanvasElement()

m_context.clear();

Can cause this. Tee conditions are not yet known.
Comment 8 Zoltan Herczeg 2011-06-02 04:54:30 PDT
FontCustomPlatformDataQt.cpp

QFontDatabase::removeApplicationFont(m_handle); - is actually successful in case of WebKit2's DRT, but fails with normal DRT. I think the normal DRT has an extra reference to the font, which keeps it alive (so the fail hides the issue).
Comment 9 Zoltan Herczeg 2011-06-02 04:57:09 PDT
Zecke, you made this code before. Maybe you have some idea.
Comment 10 Zoltan Herczeg 2011-06-02 06:32:42 PDT
If I comment out the mentioned line, the tests are passed.

 FontCustomPlatformData::~FontCustomPlatformData()
 {
-    QFontDatabase::removeApplicationFont(m_handle);
+//    QFontDatabase::removeApplicationFont(m_handle);
 }
Comment 11 Zoltan Herczeg 2011-06-08 03:59:35 PDT
Now I know whz the tests are passed in DRT:

DumpRenderTree::open() calls QFontDatabase::removeAllApplicationFonts()

this call hides this particular bug. Since the Qt port moves to QRawFont,  QFontDatabase will be dropped from the source, it is enough to mimic the DRT behaviour in WKTR for now, even if it is wrong.

I have no better idea at the moment.
Comment 12 Zoltan Herczeg 2011-06-08 23:58:20 PDT
Created attachment 96555 [details]
patch

Ugly, but this is how DRT works, and just hides the bug. We need to refactor and fix the platform font management code in the future.
Comment 13 Csaba Osztrogonác 2011-06-09 01:37:41 PDT
Comment on attachment 96555 [details]
patch

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

You're right, it is a little ugly, but necessary workaround 
to make WK2 tests work. r=me to go ahead with some typo fix.

Is there any bug report for moving to QRawFont?

> LayoutTests/ChangeLog:9
> +        * platform/qt-wk2/Skipped:
> +

+ Unskip canvas tests.

> Tools/ChangeLog:8
> +        Adding a workaround for this issue by mimicking the behaviour of DumpRenderTree.

s/mimicking/mimicing
Comment 14 Zoltan Herczeg 2011-06-09 01:49:13 PDT
Landed in http://trac.webkit.org/changeset/88435
Comment 15 Csaba Osztrogonác 2011-11-11 05:31:57 PST
Reopen, because this bug is appeared again after migrating from Qt 4.8 to Qt 5.0, because workaround doesn't work anymore.
Comment 16 Balazs Kelemen 2011-11-16 09:15:25 PST
(In reply to comment #15)
> Reopen, because this bug is appeared again after migrating from Qt 4.8 to Qt 5.0, because workaround doesn't work anymore.

Actually we still have FontDatabase::removeAllApplicationFonts - maybe the Qt5 implementation is not sufficient, need to check - but it came out that we have a much more basic issue which is that we don't use the test fonts with Qt5. Tracked in https://bugs.webkit.org/show_bug.cgi?id=72513.
Comment 17 Jesus Sanchez-Palencia 2012-02-03 07:16:04 PST
Is this related to the work Pierre has being doing with raw fonts?!
Comment 18 Balazs Kelemen 2012-02-03 08:28:56 PST
(In reply to comment #17)
> Is this related to the work Pierre has being doing with raw fonts?!

Not really. This bug has been solved long time ago thanks to Zoltan. The new failures was not a reappearing of that but another bug that has also been fixed.