Bug 70390 - WebKit not ensure font load before calling Skia devices to draw during Chrome printing
Summary: WebKit not ensure font load before calling Skia devices to draw during Chrome...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL: http://code.google.com/p/chromium/iss...
Keywords:
Depends on: 71066
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-18 17:43 PDT by Arthur Hsu
Modified: 2011-10-27 22:37 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.03 KB, patch)
2011-10-18 17:46 PDT, Arthur Hsu
no flags Details | Formatted Diff | Diff
Patch (2.12 KB, patch)
2011-10-25 16:21 PDT, Arthur Hsu
no flags Details | Formatted Diff | Diff
Test HTML with a lot of fonts (49.84 KB, text/html)
2011-10-26 10:11 PDT, Arthur Hsu
no flags Details
Patch (2.27 KB, patch)
2011-10-26 14:45 PDT, Arthur Hsu
no flags Details | Formatted Diff | Diff
Patch (918 bytes, patch)
2011-10-27 14:56 PDT, Arthur Hsu
no flags Details | Formatted Diff | Diff
Patch (2.29 KB, patch)
2011-10-27 15:30 PDT, Arthur Hsu
no flags Details | Formatted Diff | Diff
Patch (2.40 KB, patch)
2011-10-27 18:23 PDT, Arthur Hsu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur Hsu 2011-10-18 17:43:36 PDT
This is related to https://bugs.webkit.org/show_bug.cgi?id=69370 (Chromium crash bug http://code.google.com/p/chromium/issues/detail?id=94421)

Fix in 69370 is defeated when USE(SKIA_TEXT) is undefined.  Even if Skia text is not used, Skia device (the PDF device in this case) can still be used.  Failed to ensure font load will cause same issue described in 69370.
Comment 1 Arthur Hsu 2011-10-18 17:46:11 PDT
Created attachment 111539 [details]
Patch
Comment 2 Mike Reed 2011-10-19 06:07:13 PDT
Is there a performance hit for always calling this (in the non-printing case)?

If we can't trust the painter's return value, should we change that function to be void?
Comment 3 Arthur Hsu 2011-10-19 10:35:47 PDT
There will be performance penalty.  It will fire an IPC call to browser process and load the font.  If the font is already in memory, it will be quick.  If the font has not loaded, or been swapped out by GDI, it will also hit I/O.

Skia SkCanvas::drawPosText returns void, so there's no way to tell from return values.  Either change the Skia signatures to return bool, or change WebKit signatures.  Mike, if you want this to be changed on Skia side, I can do it and revert this WebKit change.
Comment 4 James Robinson 2011-10-19 11:28:13 PDT
Comment on attachment 111539 [details]
Patch

The bug title says this is about printing. It looks like you are adding the IPC to the non-printing path as well, unless I'm misreading it.
Comment 5 Arthur Hsu 2011-10-19 11:30:54 PDT
Skia devices are used only during printing.  But the fix can affect non-printing path.
Comment 6 James Robinson 2011-10-19 11:33:36 PDT
Comment on attachment 111539 [details]
Patch

This needs hard performance data. Sync IPCs are not cheap.

Can you run the page cyclers on windows with and without this test and let us know what the performance impact is?  Clearing review until that data is available.
Comment 7 Arthur Hsu 2011-10-19 14:04:25 PDT
chromium@r106357, webkit@97838
Windows 7 Enterprise SP1, Xeon X5650@2.67GHz, 12GB RAM

Before patch

iterations	5
pages	1
milliseconds	18822
mean per set	3764.40
mean per page	3764.40
timer lag	2201.00
timer lag per page	440.20


After patch

iterations	5
pages	1
milliseconds	19640
mean per set	3928.00
mean per page	3928.00
timer lag	2318.00
timer lag per page	463.60
Comment 8 James Robinson 2011-10-19 15:46:06 PDT
Comment on attachment 111539 [details]
Patch

Which page cycler was that? I think you need to run some more iterations.

Based on that data alone, though, this would be a 5% performance regression, which is unacceptable.
Comment 9 Mike Reed 2011-10-20 05:18:11 PDT
can we trigger this call only if we're drawing to a printing device?
Comment 10 Arthur Hsu 2011-10-25 16:21:50 PDT
Created attachment 112423 [details]
Patch
Comment 11 James Robinson 2011-10-25 16:23:50 PDT
Comment on attachment 112423 [details]
Patch

You should know by now what I'm going to ask:

1.) What is the performance impact?
2.) This appears to be code that's used in the printing and non-printing codepaths. What is the correctness impact for the non-printing case and how can we test that?

R- until these issues are resolved.
Comment 12 Arthur Hsu 2011-10-25 17:50:52 PDT
Windows 7 Enterprise SP1, Xeon X5650@2.67GHz, 12GB RAM
Chrome r107193 WebKit r98359

intl1 20 iterations

Before patch
============
Summary

iterations	20
pages	56
milliseconds	1862633
mean per set	93131.65
mean per page	1663.07
timer lag	502446.00
timer lag per page	448.61

After patch
===========
Summary

iterations	20
pages	56
milliseconds	1862489
mean per set	93124.45
mean per page	1662.94
timer lag	497902.00
timer lag per page	444.56
Comment 13 Arthur Hsu 2011-10-25 17:55:44 PDT
Unless USE(SKIA_TEXT) is defined, non-printing case will not hit this code path (true for r97838 and r98359 that I tested).

When USE(SKIA_TEXT) is defined and the ensureFontLoad() is not called, Skia may or may not render correct text just as all other code paths that calls GDI GetOutlineGlyphMetrics() in Chrome sandbox.  Existing test cases for renderer shall be able to detect rendering errors.
Comment 14 Mike Reed 2011-10-26 05:07:20 PDT
This will slow down the non-printing case when SKIA_TEXT is defined, and that will be our default soon.

Ben and I have been struggling to find a reproducible case where the raster drawing fails, but we have not be able to. Please make this change conditional on printing, or help us find the repro-case which justifies it for the non-printing case.
Comment 15 Arthur Hsu 2011-10-26 10:11:38 PDT
Created attachment 112556 [details]
Test HTML with a lot of fonts
Comment 16 Arthur Hsu 2011-10-26 10:16:08 PDT
(In reply to comment #14)
> This will slow down the non-printing case when SKIA_TEXT is defined, and that will be our default soon.
> 
> Ben and I have been struggling to find a reproducible case where the raster drawing fails, but we have not be able to. Please make this change conditional on printing, or help us find the repro-case which justifies it for the non-printing case.

I have attached a sample.html that had revealed bugs for font caching and rendering.  Please give it a try.

Existing code base has an ensureFontLoad() in FontChromiumWin.cpp:398 Font::drawGlyphs().  Take that line out and you shall see the correctness issue.
Comment 17 Arthur Hsu 2011-10-26 11:55:39 PDT
(In reply to comment #14)
> This will slow down the non-printing case when SKIA_TEXT is defined, and that will be our default soon.
> 
> Ben and I have been struggling to find a reproducible case where the raster drawing fails, but we have not be able to. Please make this change conditional on printing, or help us find the repro-case which justifies it for the non-printing case.

Also, I don't see a way to justify whether a GraphicsContext is for printing or for screen rendering when it reaches glyph drawing.  If there's a way please let me know, thanks.
Comment 18 Mike Reed 2011-10-26 12:33:34 PDT
This is the test for printing, lifted from PlatformContext.cpp

    if (m_canvas->getTopDevice()->getDeviceCapabilities() & SkDevice::kVector_Capability) {
Comment 19 Arthur Hsu 2011-10-26 14:45:19 PDT
Created attachment 112599 [details]
Patch
Comment 20 James Robinson 2011-10-26 14:55:25 PDT
Comment on attachment 112599 [details]
Patch

I'm deferring to you on this one, Mike.  Are we sure this isn't an issue with raster devices?
Comment 21 Mike Reed 2011-10-27 05:58:04 PDT
We have never seen this sort of failure on raster, so I vote to be conservative and favor performance until we do. I like the latest version where we check for printing.
Comment 22 James Robinson 2011-10-27 11:22:37 PDT
Comment on attachment 112599 [details]
Patch

OK then.
Comment 23 WebKit Review Bot 2011-10-27 13:20:42 PDT
Comment on attachment 112599 [details]
Patch

Clearing flags on attachment: 112599

Committed r98626: <http://trac.webkit.org/changeset/98626>
Comment 24 WebKit Review Bot 2011-10-27 13:20:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Arthur Hsu 2011-10-27 14:56:08 PDT
Created attachment 112762 [details]
Patch
Comment 26 James Robinson 2011-10-27 15:07:08 PDT
The original patch broke the build and was reverted. Can you fold that build fix into the original patch and post that for review?

How did the original patch not compile?  Are you testing these patches on windows before posting them?
Comment 27 Arthur Hsu 2011-10-27 15:24:15 PDT
(In reply to comment #26)
> The original patch broke the build and was reverted. Can you fold that build fix into the original patch and post that for review?
> 
> How did the original patch not compile?  Are you testing these patches on windows before posting them?

I compiled and tested the code under chrome.  Will re-land patch and ask for review after try bots are really green (the last green is bogus).
Comment 28 Arthur Hsu 2011-10-27 15:30:28 PDT
Created attachment 112768 [details]
Patch
Comment 29 James Robinson 2011-10-27 15:53:55 PDT
(In reply to comment #27)
> > How did the original patch not compile?  Are you testing these patches on windows before posting them?
> 
> I compiled and tested the code under chrome.

On windows? And you did not see the compile error the bot did? We do not have an EWS bot here for cr-win, so there's no coverage there.  Are you sending a tryjob to the chromium trybots? If so could you provide a link here to that try run? I don't want to risk breaking all the windows bots again.
Comment 30 Arthur Hsu 2011-10-27 15:54:54 PDT
Comment on attachment 112768 [details]
Patch

Seems need to put review to ? for the bots to run.
Comment 31 James Robinson 2011-10-27 16:07:15 PDT
I don't know what you are trying to achieve. As I said before, there are no cr-win EWS bots here. You need to explain what you are doing for other engineers to know what to expect.
Comment 32 Arthur Hsu 2011-10-27 18:23:21 PDT
Created attachment 112802 [details]
Patch
Comment 33 Arthur Hsu 2011-10-27 18:25:59 PDT
new patch tested on http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/5

The bot failed in link stage, but it compiles okay.  The linking failure is due to bot can't delete existing exe files.
Comment 34 James Robinson 2011-10-27 18:27:09 PDT
You haven't answered my previous question - have *you* tested this fix in a chromium win environment?
Comment 35 Arthur Hsu 2011-10-27 18:34:26 PDT
(In reply to comment #34)
> You haven't answered my previous question - have *you* tested this fix in a chromium win environment?

As I said, it is compiled and tested okay locally.  Test environment is VMWare 6.5, Windows 7 pt-BR Professional, and the testing URL for reproducing this bug  is  http://wp.clicrbs.com.br/anonymus/2011/10/15/receita-peixe-dinamarques/?topo=52,1,1,,268,e268

I do wonder the mismatch is because I enabled precompiled headers locally, but the bots are not.  To avoid that, a try job is sent for the new patch and I make sure it compiles okay.
Comment 36 James Robinson 2011-10-27 18:36:31 PDT
Can you repro the original compile failure with precompiled headers off? If so then that seems like a more serious issue that should be escalated.
Comment 37 Arthur Hsu 2011-10-27 18:55:50 PDT
(In reply to comment #36)
> Can you repro the original compile failure with precompiled headers off? If so then that seems like a more serious issue that should be escalated.

Yes, I commented out the "chromium_win_pch" line from include.gypi, run gclient runhooks, and compiling chrome shows me errors.

Oddly, when I add that line back again, run gclient hooks, this time compilation of chrome failed.  I'm not sure what's going on.
Comment 38 James Robinson 2011-10-27 19:07:17 PDT
Comment on attachment 112802 [details]
Patch

Please bring that issue up like chromium-dev so other users are aware of it
Comment 39 WebKit Review Bot 2011-10-27 22:37:46 PDT
Comment on attachment 112802 [details]
Patch

Clearing flags on attachment: 112802

Committed r98692: <http://trac.webkit.org/changeset/98692>
Comment 40 WebKit Review Bot 2011-10-27 22:37:53 PDT
All reviewed patches have been landed.  Closing bug.