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.
Created attachment 111539 [details] Patch
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?
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 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.
Skia devices are used only during printing. But the fix can affect non-printing path.
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.
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 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.
can we trigger this call only if we're drawing to a printing device?
Created attachment 112423 [details] Patch
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.
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
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.
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.
Created attachment 112556 [details] Test HTML with a lot of fonts
(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.
(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.
This is the test for printing, lifted from PlatformContext.cpp if (m_canvas->getTopDevice()->getDeviceCapabilities() & SkDevice::kVector_Capability) {
Created attachment 112599 [details] Patch
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?
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 on attachment 112599 [details] Patch OK then.
Comment on attachment 112599 [details] Patch Clearing flags on attachment: 112599 Committed r98626: <http://trac.webkit.org/changeset/98626>
All reviewed patches have been landed. Closing bug.
Created attachment 112762 [details] Patch
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?
(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).
Created attachment 112768 [details] Patch
(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 on attachment 112768 [details] Patch Seems need to put review to ? for the bots to run.
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.
Created attachment 112802 [details] Patch
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.
You haven't answered my previous question - have *you* tested this fix in a chromium win environment?
(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.
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.
(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 on attachment 112802 [details] Patch Please bring that issue up like chromium-dev so other users are aware of it
Comment on attachment 112802 [details] Patch Clearing flags on attachment: 112802 Committed r98692: <http://trac.webkit.org/changeset/98692>