Bug 70390 – WebKit not ensure font load before calling Skia devices to draw during Chrome printing

• Component: Platform
• Status: RESOLVED FIXED

[Arthur Hsu]

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.

[Arthur Hsu]

Created attachment 111539 [details]
Patch

[Mike Reed]

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?

[Arthur Hsu]

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.

[James Robinson]

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.

[Arthur Hsu]

Skia devices are used only during printing.  But the fix can affect non-printing path.

[James Robinson]

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.

[Arthur Hsu]

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

[James Robinson]

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.

[Mike Reed]

can we trigger this call only if we're drawing to a printing device?

[Arthur Hsu]

Created attachment 112423 [details]
Patch

[James Robinson]

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.

[Arthur Hsu]

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

[Arthur Hsu]

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.

[Mike Reed]

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.

[Arthur Hsu]

Created attachment 112556 [details]
Test HTML with a lot of fonts

[Arthur Hsu]

(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.

[Arthur Hsu]

(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.

[Mike Reed]

This is the test for printing, lifted from PlatformContext.cpp

    if (m_canvas->getTopDevice()->getDeviceCapabilities() & SkDevice::kVector_Capability) {

[Arthur Hsu]

Created attachment 112599 [details]
Patch

[James Robinson]

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?

[Mike Reed]

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.

[James Robinson]

Comment on attachment 112599 [details]
Patch

OK then.

[WebKit Review Bot]

Comment on attachment 112599 [details]
Patch

Clearing flags on attachment: 112599

Committed r98626: <http://trac.webkit.org/changeset/98626>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Arthur Hsu]

Created attachment 112762 [details]
Patch

[James Robinson]

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?

[Arthur Hsu]

(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).

[Arthur Hsu]

Created attachment 112768 [details]
Patch

[James Robinson]

(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.

[Arthur Hsu]

Comment on attachment 112768 [details]
Patch

Seems need to put review to ? for the bots to run.

[James Robinson]

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.

[Arthur Hsu]

Created attachment 112802 [details]
Patch

[Arthur Hsu]

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.

[James Robinson]

You haven't answered my previous question - have *you* tested this fix in a chromium win environment?

[Arthur Hsu]

(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.

[James Robinson]

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.

[Arthur Hsu]

(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.

[James Robinson]

Comment on attachment 112802 [details]
Patch

Please bring that issue up like chromium-dev so other users are aware of it

[WebKit Review Bot]

Comment on attachment 112802 [details]
Patch

Clearing flags on attachment: 112802

Committed r98692: <http://trac.webkit.org/changeset/98692>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.