Changeset 92269 in webkit

Timestamp:

Aug 3, 2011 3:48:22 AM (13 years ago)

Author:

jeremy@chromium.org

Message:

[Chromium] Fix OOP font loading to work on 10.6.6 and above.
​https://bugs.webkit.org/show_bug.cgi?id=65543

In 10.6.6 the function used to get the unique ID for an NSFont in the
renderer was changed so it fails in the sandbox (it now tries to access
the on-disk font file). In order to work around this, we get the font
ID from the browser process.

To speed things up, we introduce 2 levels of caching in WebKit. A font
name cache where we can perform a quick lookup without the need for the
font id and a font id cache which we can only lookup in after getting
the unique ID from the browser process.

Reviewed by Kenneth Russell.

No new tests since this is not readily testable.

Source/WebCore:

• platform/chromium/PlatformBridge.h:
• platform/graphics/chromium/CrossProcessFontLoading.h:
• platform/graphics/chromium/CrossProcessFontLoading.mm:

(WebCore::MemoryActivatedFont::create):
(WebCore::MemoryActivatedFont::MemoryActivatedFont):
(WebCore::MemoryActivatedFont::~MemoryActivatedFont):

Source/WebKit/chromium:

• public/mac/WebSandboxSupport.h: Plumb font ID parameter through.
• src/PlatformBridge.cpp:

(WebCore::PlatformBridge::loadFont): ditto.

Location:

trunk/Source

Files:

• WebCore/ChangeLog (modified) (1 diff)
• WebCore/platform/chromium/PlatformBridge.h (modified) (1 diff)
• WebCore/platform/graphics/chromium/CrossProcessFontLoading.h (modified) (3 diffs)
• WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm (modified) (7 diffs)
• WebKit/chromium/ChangeLog (modified) (1 diff)
• WebKit/chromium/public/mac/WebSandboxSupport.h (modified) (2 diffs)
• WebKit/chromium/src/PlatformBridge.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-08-03  Jeremy Moskovich  <jeremy@chromium.org>
+
+        [Chromium] Fix OOP font loading to work on 10.6.6 and above.
+        https://bugs.webkit.org/show_bug.cgi?id=65543
+
+        In 10.6.6 the function used to get the unique ID for an NSFont in the 
+        renderer was changed so it fails in the sandbox (it now tries to access
+        the on-disk font file). In order to work around this, we get the font
+        ID from the browser process.
+
+        To speed things up, we introduce 2 levels of caching in WebKit. A font
+        name cache where we can perform a quick lookup without the need for the
+        font id and a font id cache which we can only lookup in after getting
+        the unique ID from the browser process.
+
+        Reviewed by Kenneth Russell.
+
+        No new tests since this is not readily testable.
+
+        * platform/chromium/PlatformBridge.h:
+        * platform/graphics/chromium/CrossProcessFontLoading.h:
+        * platform/graphics/chromium/CrossProcessFontLoading.mm:
+        (WebCore::MemoryActivatedFont::create):
+        (WebCore::MemoryActivatedFont::MemoryActivatedFont):
+        (WebCore::MemoryActivatedFont::~MemoryActivatedFont):
+
 2011-08-03  Hayato Ito  <hayato@chromium.org>
 

trunk/Source/WebCore/platform/chromium/PlatformBridge.h

@@ -152 +152 @@
 #endif
 #if OS(DARWIN)
-    static bool loadFont(NSFont* srcFont, ATSFontContainerRef* out);
+    static bool loadFont(NSFont* srcFont, ATSFontContainerRef*, uint32_t* fontID);
 #elif OS(UNIX)
     static void getRenderStyleForStrike(const char* family, int sizeAndStyle, FontRenderStyle* result);

trunk/Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.h

@@ -34 +34 @@
 #import <wtf/RefCounted.h>
 #import <wtf/RetainPtr.h>
+#import <wtf/text/WTFString.h>
 
 typedef struct CGFont* CGFontRef;
@@ -74 +75 @@
 public:
     // Use to create a new object, see docs on constructor below.
-    static PassRefPtr<MemoryActivatedFont> create(ATSFontContainerRef srcFontContainerRef, ATSFontContainerRef container);
+    static PassRefPtr<MemoryActivatedFont> create(uint32_t fontID, NSFont*, ATSFontContainerRef);
     ~MemoryActivatedFont();
     
@@ -88 +89 @@
     // container - a font container corresponding to an identical font that
     // we loaded cross-process.
-    MemoryActivatedFont(ATSFontContainerRef srcFontContainerRef, ATSFontContainerRef container);
+    MemoryActivatedFont(uint32_t fontID, NSFont*, ATSFontContainerRef);
 
     ATSFontContainerRef m_fontContainer;
     WTF::RetainPtr<CGFontRef> m_cgFont;
     ATSFontRef m_atsFontRef;
-    ATSFontContainerRef m_srcFontContainerRef;
+    uint32_t m_fontID;
+    WTF::String m_inSandboxHashKey;
 };
 

trunk/Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm

@@ -39 +39 @@
 namespace {
 
-typedef HashMap<ATSFontContainerRef, MemoryActivatedFont*> FontContainerRefMemoryFontHash;
+typedef HashMap<uint32, MemoryActivatedFont*> FontContainerRefMemoryFontHash;
+typedef HashMap<WTF::String, MemoryActivatedFont*> FontNameMemoryFontHash;
 
 // On 10.5, font loading is not blocked by the sandbox and thus there is no
@@ -55 +56 @@
 }
 
-FontContainerRefMemoryFontHash& fontCacheBySrcFontContainerRef()
-{
-    DEFINE_STATIC_LOCAL(FontContainerRefMemoryFontHash, srcFontRefCache, ());
-    return srcFontRefCache;
+// Caching:
+//
+// Requesting a font from the browser process is expensive and so is
+// "activating" said font.  Caching of loaded fonts is complicated by the fact
+// that it's impossible to get a unique identifier for the on-disk font file
+// from inside the sandboxed renderer process.
+// This means that when loading a font we need to round-trip through the browser
+// process in order to get the unique font file identifer which we might already
+// have activated and cached.
+//
+// In order to save as much work as we can, we maintain 2 levels of caching
+// for the font data:
+// 1. A dumb cache keyed by the font name/style (information we can determine
+// from inside the sandbox).
+// 2. A smarter cache keyed by the real "unique font id".
+//
+// In order to perform a lookup in #2 we need to consult with the browser to get
+// us the lookup key.  While this doesn't save us the font load, it does save
+// us font activation.
+//
+// It's important to remember that existing FontPlatformData objects are already
+// cached, so a cache miss in the code in this file isn't necessarily so bad.
+
+FontContainerRefMemoryFontHash& fontCacheByFontID()
+{
+    DEFINE_STATIC_LOCAL(FontContainerRefMemoryFontHash, srcFontIDCache, ());
+    return srcFontIDCache;
+}
+
+
+FontNameMemoryFontHash& fontCacheByFontName()
+{
+    DEFINE_STATIC_LOCAL(FontNameMemoryFontHash, srcFontNameCache, ());
+    return srcFontNameCache;
+}
+
+// Given a font specified by |srcFont|, use the information we can query in
+// the sandbox to construct a key which we hope will be as unique as possible
+// to the containing font file.
+WTF::String hashKeyFromNSFont(NSFont* srcFont)
+{
+    NSFontDescriptor* desc = [srcFont fontDescriptor];
+    NSFontSymbolicTraits traits = [desc symbolicTraits];
+    return WTF::String::format("%s %x", [[srcFont fontName] UTF8String], traits);
 }
 
@@ -86 +127 @@
 PassRefPtr<MemoryActivatedFont> loadFontFromBrowserProcess(NSFont* nsFont)
 {
+    // First try to lookup in our cache with the limited information we have.
+    WTF::String hashKey = hashKeyFromNSFont(nsFont);
+    RefPtr<MemoryActivatedFont> font(fontCacheByFontName().get(hashKey));
+    if (font)
+        return font;
+
     ATSFontContainerRef container;
+    uint32_t fontID;
     // Send cross-process request to load font.
-    if (!PlatformBridge::loadFont(nsFont, &container))
+    if (!PlatformBridge::loadFont(nsFont, &container, &fontID))
         return 0;
-    
-    ATSFontContainerRef srcFontContainerRef = fontContainerRefFromNSFont(nsFont);
-    if (!srcFontContainerRef) {
+
+    // Now that we have the fontID from the browser process, we can consult
+    // the ID cache.
+    font = fontCacheByFontID().get(fontID);
+    if (font) {
+        // We can safely discard the new container since we already have the
+        // font in our cache.
+        // FIXME: PlatformBridge::loadFont() should consult the id cache
+        // before activating the font.  Then we can save this activate/deactive
+        // dance altogether.
         ATSFontDeactivate(container, 0, kATSOptionFlagsDefault);
-        return 0;
-    }
-    
-    PassRefPtr<MemoryActivatedFont> font = adoptRef(fontCacheBySrcFontContainerRef().get(srcFontContainerRef));
-    if (font.get())
         return font;
-
-    return MemoryActivatedFont::create(srcFontContainerRef, container);
+    }
+
+    return MemoryActivatedFont::create(fontID, nsFont, container);
 }
 
 } // namespace
 
-PassRefPtr<MemoryActivatedFont> MemoryActivatedFont::create(ATSFontContainerRef srcFontContainerRef, ATSFontContainerRef container)
-{
-  MemoryActivatedFont* font = new MemoryActivatedFont(srcFontContainerRef, container);
+PassRefPtr<MemoryActivatedFont> MemoryActivatedFont::create(uint32_t fontID, NSFont* nsFont, ATSFontContainerRef container)
+{
+  MemoryActivatedFont* font = new MemoryActivatedFont(fontID, nsFont, container);
   if (!font->cgFont())  // Object construction failed.
   {
@@ -117 +168 @@
 }
 
-MemoryActivatedFont::MemoryActivatedFont(ATSFontContainerRef srcFontContainerRef, ATSFontContainerRef container)
+MemoryActivatedFont::MemoryActivatedFont(uint32_t fontID, NSFont* nsFont, ATSFontContainerRef container)
     : m_fontContainer(container)
     , m_atsFontRef(kATSFontRefUnspecified)
-    , m_srcFontContainerRef(srcFontContainerRef)
+    , m_fontID(fontID)
+    , m_inSandboxHashKey(hashKeyFromNSFont(nsFont))
 {
     if (!container)
@@ -140 +192 @@
     m_cgFont.adoptCF(CGFontCreateWithPlatformFont(&m_atsFontRef));
     
-    if (!m_cgFont.get())
-        return;
-    
-    // Add ourselves to cache.
-    fontCacheBySrcFontContainerRef().add(m_srcFontContainerRef, this);
+    if (!m_cgFont)
+        return;
+    
+    // Add ourselves to caches.
+    fontCacheByFontID().add(fontID, this);
+    fontCacheByFontName().add(m_inSandboxHashKey, this);
 }
 
@@ -151 +204 @@
 MemoryActivatedFont::~MemoryActivatedFont()
 {
-    if (m_cgFont.get()) {
+    if (m_cgFont) {
         // First remove ourselves from the caches.
-        ASSERT(fontCacheBySrcFontContainerRef().contains(m_srcFontContainerRef));
+        ASSERT(fontCacheByFontID().contains(m_fontID));
+        ASSERT(fontCacheByFontName().contains(m_inSandboxHashKey));
         
-        fontCacheBySrcFontContainerRef().remove(m_srcFontContainerRef);
+        fontCacheByFontID().remove(m_fontID);
+        fontCacheByFontName().remove(m_inSandboxHashKey);
         
         // Make sure the CGFont is destroyed before its font container.
@@ -193 +248 @@
         // Font loading was blocked by the Sandbox.
         m_inMemoryFont = loadFontFromBrowserProcess(outNSFont);
-        if (m_inMemoryFont.get()) {
+        if (m_inMemoryFont) {
             cgFont = m_inMemoryFont->cgFont();
             

trunk/Source/WebKit/chromium/ChangeLog

@@ +1 @@
+2011-08-03  Jeremy Moskovich  <jeremy@chromium.org>
+
+        [Chromium] Fix OOP font loading to work on 10.6.6 and above.
+        https://bugs.webkit.org/show_bug.cgi?id=65543
+
+        In 10.6.6 the function used to get the unique ID for an NSFont in the 
+        renderer was changed so it fails in the sandbox (it now tries to access
+        the on-disk font file). In order to work around this, we get the font
+        ID from the browser process.
+
+        To speed things up, we introduce 2 levels of caching in WebKit. A font
+        name cache where we can perform a quick lookup without the need for the
+        font id and a font id cache which we can only lookup in after getting
+        the unique ID from the browser process.
+
+        Reviewed by Kenneth Russell.
+
+        No new tests since this is not readily testable.
+
+        * public/mac/WebSandboxSupport.h: Plumb font ID parameter through.
+        * src/PlatformBridge.cpp:
+        (WebCore::PlatformBridge::loadFont): ditto.
+
 2011-08-03  Pavel Feldman  <pfeldman@chromium.org>
 

trunk/Source/WebKit/chromium/public/mac/WebSandboxSupport.h

@@ -47 +47 @@
     // Given an input font - |srcFont| [which can't be loaded due to sandbox
     // restrictions].  Return a font container belonging to an equivalent
-    // font file that can be used to access the font.
+    // font file that can be used to access the font and a unique identifier
+    // corresponding to the on-disk font file.
     //
     // Note that a font container may contain multiple fonts, the caller is
@@ -56 +57 @@
     //
     // Returns: true on success, false on error.
-    virtual bool loadFont(NSFont* srcFont, ATSFontContainerRef* out) = 0;
+    virtual bool loadFont(NSFont* srcFont, ATSFontContainerRef*, uint32_t* fontID) = 0;
 };
 

trunk/Source/WebKit/chromium/src/PlatformBridge.cpp

@@ -445 +445 @@
 
 #if OS(DARWIN)
-bool PlatformBridge::loadFont(NSFont* srcFont, ATSFontContainerRef* out)
+bool PlatformBridge::loadFont(NSFont* srcFont, ATSFontContainerRef* container, uint32_t* fontID)
 {
     WebSandboxSupport* ss = webKitClient()->sandboxSupport();
     if (ss)
-        return ss->loadFont(srcFont, out);
+        return ss->loadFont(srcFont, container, fontID);
 
     // This function should only be called in response to an error loading a
@@ -455 +455 @@
     // This by definition shouldn't happen if there is no sandbox support.
     ASSERT_NOT_REACHED();
-    *out = 0;
+    *container = 0;
+    *fontID = 0;
     return false;
 }