[Chromium] Random characters got rendered as empty boxes or with incorrect glyphs...

https://bugs.webkit.org/show_bug.cgi?id=76508

Patch by Kazuhiro Inaba <kinaba@chromium.org> on 2012-01-19
Reviewed by Kent Tamura.

Wrapped GetGlyphIndices() API calls so that when they failed we trigger font
loading outside the sandbox and retry the call.

No new auto tests since the bug involves the system's occasional cache behavior
and thus there's no reliable way to reproduce and test the situation.

* platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:
(WebCore::getGlyphIndices):
GDI call wrapper ensuring fonts to be loaded.
(WebCore::initSpaceGlyph):
Changed to use the wrapper function.
(WebCore::fillBMPGlyphs):
Changed to use the wrapper function.
Introduced scoped HDC management by HWndDC.
(WebCore::GlyphPage::fill):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@105393 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 53de2f1..84ee594 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,26 @@
+2012-01-19  Kazuhiro Inaba  <kinaba@chromium.org>
+
+        [Chromium] Random characters got rendered as empty boxes or with incorrect glyphs even when a font is present
+        https://bugs.webkit.org/show_bug.cgi?id=76508
+
+        Reviewed by Kent Tamura.
+
+        Wrapped GetGlyphIndices() API calls so that when they failed we trigger font
+        loading outside the sandbox and retry the call.
+
+        No new auto tests since the bug involves the system's occasional cache behavior
+        and thus there's no reliable way to reproduce and test the situation.
+
+        * platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:
+        (WebCore::getGlyphIndices):
+        GDI call wrapper ensuring fonts to be loaded.
+        (WebCore::initSpaceGlyph):
+        Changed to use the wrapper function.
+        (WebCore::fillBMPGlyphs):
+        Changed to use the wrapper function.
+        Introduced scoped HDC management by HWndDC.
+        (WebCore::GlyphPage::fill):
+
 2012-01-19  Adam Barth  <abarth@webkit.org>
 
         createAttributeNS should understand that "xmlns" is allowed in the http://www.w3.org/2000/xmlns/

diff --git a/Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp b/Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp
index f5ab221..42f45ab 100644 (file)
--- a/Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp
+++ b/Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009 Google Inc. All rights reserved.
+ * Copyright (c) 2008, 2009, 2012 Google Inc. All rights reserved.
  * 
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -34,6 +34,7 @@
 
 #include "Font.h"
 #include "GlyphPageTreeNode.h"
+#include "HWndDC.h"
 #include "PlatformSupport.h"
 #include "SimpleFontData.h"
 #include "SystemInfo.h"
@@ -49,15 +50,27 @@ static void fillEmptyGlyphs(GlyphPage* page)
         page->setGlyphDataForIndex(i, 0, 0);
 }
 
-// Lazily initializes space glyph
-static Glyph initSpaceGlyph(HDC dc, Glyph* spaceGlyph)
+// Convert characters to glyph ids by GetGlyphIndices(), during which, we
+// ensure the font is loaded in memory to make it work in a sandboxed process.
+static bool getGlyphIndices(HFONT font, HDC dc, const UChar* characters, unsigned charactersLength, WORD* glyphBuffer, DWORD flag)
 {
-    if (*spaceGlyph)
-        return *spaceGlyph;
+    if (GetGlyphIndices(dc, characters, charactersLength, glyphBuffer, flag) != GDI_ERROR)
+        return true;
+    if (PlatformSupport::ensureFontLoaded(font)) {
+        if (GetGlyphIndices(dc, characters, charactersLength, glyphBuffer, flag) != GDI_ERROR)
+            return true;
+        // FIXME: Handle gracefully the error if this call also fails.
+        // See http://crbug.com/6401
+        LOG_ERROR("Unable to get the glyph indices after second attempt");
+    }
+    return false;
+}
 
+// Initializes space glyph
+static bool initSpaceGlyph(HFONT font, HDC dc, Glyph* spaceGlyph)
+{
     static wchar_t space = ' ';
-    GetGlyphIndices(dc, &space, 1, spaceGlyph, 0);
-    return *spaceGlyph;
+    return getGlyphIndices(font, dc, &space, 1, spaceGlyph, 0);
 }
 
 // Fills |length| glyphs starting at |offset| in a |page| in the Basic 
@@ -68,27 +81,25 @@ static bool fillBMPGlyphs(unsigned offset,
                           unsigned length,
                           UChar* buffer,
                           GlyphPage* page,
-                          const SimpleFontData* fontData,
-                          bool recurse)
+                          const SimpleFontData* fontData)
 {
-    HDC dc = GetDC((HWND)0);
+    HWndDC dc((HWND)0);
     HGDIOBJ oldFont = SelectObject(dc, fontData->platformData().hfont());
 
     TEXTMETRIC tm = {0};
     if (!GetTextMetrics(dc, &tm)) {
-        SelectObject(dc, oldFont);
-        ReleaseDC(0, dc);
+        if (PlatformSupport::ensureFontLoaded(fontData->platformData().hfont())) {
+            if (!GetTextMetrics(dc, &tm)) {
+                // FIXME: Handle gracefully the error if this call also fails.
+                // See http://crbug.com/6401
+                LOG_ERROR("Unable to get the text metrics after second attempt");
 
-        if (recurse) {
-            if (PlatformSupport::ensureFontLoaded(fontData->platformData().hfont()))
-                return fillBMPGlyphs(offset, length, buffer, page, fontData, false);
-
-            fillEmptyGlyphs(page);
-            return false;
+                SelectObject(dc, oldFont);
+                fillEmptyGlyphs(page);
+                return false;
+            }
         } else {
-            // FIXME: Handle gracefully the error if this call also fails.
-            // See http://crbug.com/6401
-            LOG_ERROR("Unable to get the text metrics after second attempt");
+            SelectObject(dc, oldFont);
             fillEmptyGlyphs(page);
             return false;
         }
@@ -128,7 +139,11 @@ static bool fillBMPGlyphs(unsigned offset,
     // Also according to Jungshik and Hironori's suggestion and modification
     // we treat turetype and raster Font as different way when windows version
     // is less than Vista.
-    GetGlyphIndices(dc, buffer, length, localGlyphBuffer, GGI_MARK_NONEXISTING_GLYPHS);
+    if (!getGlyphIndices(fontData->platformData().hfont(), dc, buffer, length, localGlyphBuffer, GGI_MARK_NONEXISTING_GLYPHS)) {
+        SelectObject(dc, oldFont);
+        fillEmptyGlyphs(page);
+        return false;
+    }
 
     // Copy the output to the GlyphPage
     bool haveGlyphs = false;
@@ -138,6 +153,7 @@ static bool fillBMPGlyphs(unsigned offset,
         invalidGlyph = 0x1F;
 
     Glyph spaceGlyph = 0;  // Glyph for a space. Lazily filled.
+    bool spaceGlyphInitialized = false;
 
     for (unsigned i = 0; i < length; i++) {
         UChar c = buffer[i];
@@ -149,7 +165,12 @@ static bool fillBMPGlyphs(unsigned offset,
         if (Font::treatAsSpace(c)) {
             // Hard code the glyph indices for characters that should be
             // treated like spaces.
-            glyph = initSpaceGlyph(dc, &spaceGlyph);
+            if (!spaceGlyphInitialized) {
+                // If initSpaceGlyph fails, spaceGlyph stays 0 (= glyph is not present).
+                initSpaceGlyph(fontData->platformData().hfont(), dc, &spaceGlyph);
+                spaceGlyphInitialized = true;
+            }
+            glyph = spaceGlyph;
         } else if (glyph == invalidGlyph) {
             // WebKit expects both the glyph index and FontData
             // pointer to be 0 if the glyph is not present
@@ -161,7 +182,6 @@ static bool fillBMPGlyphs(unsigned offset,
     }
 
     SelectObject(dc, oldFont);
-    ReleaseDC(0, dc);
     return haveGlyphs;
 }
 
@@ -222,7 +242,7 @@ bool GlyphPage::fill(unsigned offset, unsigned length, UChar* characterBuffer,
     // FIXME: Add assertions to make sure that buffer is entirely in BMP
     // or entirely in non-BMP. 
     if (bufferLength == length)
-        return fillBMPGlyphs(offset, length, characterBuffer, this, fontData, true);
+        return fillBMPGlyphs(offset, length, characterBuffer, this, fontData);
 
     if (bufferLength == 2 * length) {
         // A non-BMP input buffer will be twice as long as output glyph buffer