Avoid double CTFont creation by passing through data descriptor

The out-of-process font loader, now that it has moved to using
CTFontManagerCreateFontDescriptorFromData, does not need to create a
full CTFontRef anymore, but can instead wrap the out-of-process font
data into a CTFontDescriptorRef built from data. Doing that avoids
creating an extra CTFont copy in font_platform_data_mac, as we can merge
the data descriptor with the cascade list attributes, and only then
create the CTFontRef from it.

Bug: 1033478
Change-Id: I025c265caf472578fd77a11acbc4b23a1d8fffb2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2013289
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Auto-Submit: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735353}

content/browser/sandbox_mac_unittest.mm

@@ -211,13 +211,13 @@ MULTIPROCESS_TEST_MAIN(FontLoadingProcess) {
       font_shmem->Clone(mojo::SharedBufferHandle::AccessMode::READ_ONLY);
   CHECK(shmem_handle.is_valid());
 
-  base::ScopedCFTypeRef<CTFontRef> ctfont_base;
-  CHECK(FontLoader::CTFontRefFromBuffer(
-      std::move(shmem_handle), font_data_length, ctfont_base.InitializeInto()));
-  CHECK(ctfont_base);
+  base::ScopedCFTypeRef<CTFontDescriptorRef> data_descriptor;
+  CHECK(FontLoader::CTFontDescriptorFromBuffer(
+      std::move(shmem_handle), font_data_length, &data_descriptor));
+  CHECK(data_descriptor);
 
-  base::ScopedCFTypeRef<CTFontRef> sized_ctfont(CTFontCreateCopyWithAttributes(
-      ctfont_base.get(), 16.0, nullptr, nullptr));
+  base::ScopedCFTypeRef<CTFontRef> sized_ctfont(
+      CTFontCreateWithFontDescriptor(data_descriptor.get(), 16.0, nullptr));
   CHECK(sized_ctfont);
 
   // Do something with the font to make sure it's loaded.

content/child/child_process_sandbox_support_impl_mac.cc

@@ -29,8 +29,9 @@ WebSandboxSupportMac::WebSandboxSupportMac() {
 
 WebSandboxSupportMac::~WebSandboxSupportMac() = default;
 
-bool WebSandboxSupportMac::LoadFont(CTFontRef font,
-                                    CTFontRef* out,
+bool WebSandboxSupportMac::LoadFont(
+    CTFontRef font,
+    base::ScopedCFTypeRef<CTFontDescriptorRef>* out_descriptor,
     uint32_t* font_id) {
   if (!sandbox_support_)
     return false;
@@ -43,7 +44,7 @@ bool WebSandboxSupportMac::LoadFont(CTFontRef font,
                  *font_id > 0 && font_data.is_valid();
   if (!success) {
     DLOG(ERROR) << "Bad response from LoadFont() for " << font_name;
-    *out = nullptr;
+    out_descriptor->reset();
     *font_id = 0;
     return false;
   }
@@ -55,8 +56,9 @@ bool WebSandboxSupportMac::LoadFont(CTFontRef font,
   // TODO(jeremy): Need to call back into the requesting process to make sure
   // that the font isn't already activated, based on the font id.  If it's
   // already activated, don't reactivate it here - https://crbug.com/72727 .
-  return FontLoader::CTFontRefFromBuffer(
-      std::move(font_data), static_cast<uint32_t>(font_data_size), out);
+  return FontLoader::CTFontDescriptorFromBuffer(
+      std::move(font_data), static_cast<uint32_t>(font_data_size),
+      out_descriptor);
 }
 
 SkColor WebSandboxSupportMac::GetSystemColor(blink::MacSystemColorID color_id) {

content/child/child_process_sandbox_support_impl_mac.h

@@ -7,6 +7,7 @@
 
 #include <CoreText/CoreText.h>
 
+#include "base/mac/scoped_cftyperef.h"
 #include "base/memory/read_only_shared_memory_region.h"
 #include "base/memory/shared_memory_mapping.h"
 #include "content/common/sandbox_support_mac.mojom.h"
@@ -24,7 +25,9 @@ class WebSandboxSupportMac : public blink::WebSandboxSupport {
   ~WebSandboxSupportMac() override;
 
   // blink::WebSandboxSupport:
-  bool LoadFont(CTFontRef font, CTFontRef* out, uint32_t* font_id) override;
+  bool LoadFont(CTFontRef font,
+                base::ScopedCFTypeRef<CTFontDescriptorRef>* out_descriptor,
+                uint32_t* font_id) override;
   SkColor GetSystemColor(blink::MacSystemColorID color_id) override;
 
  private:

content/common/mac/font_loader.h

@@ -11,6 +11,7 @@
 #include <memory>
 
 #include "base/callback_forward.h"
+#include "base/mac/scoped_cftyperef.h"
 #include "content/common/content_export.h"
 #include "mojo/public/cpp/system/buffer.h"
 
@@ -49,19 +50,20 @@ class FontLoader {
                        LoadedCallback callback);
 
   // Given a shared memory buffer containing the raw data for a font file, load
-  // the font and return a CTFontRef.
+  // the font and turn them into a CTFontDescriptor.
   //
   // |data| - A shared memory handle pointing to the raw data from a font file.
   // |data_size| - Size of |data|.
   //
   // On return:
   //  returns true on success, false on failure.
-  //  |out| - A CTFontRef corresponding to the designated font.
+  //  |out| - A CTFontDescriptorRef corresponding to the designated font buffer.
   //  The caller is responsible for releasing this value via CFRelease().
   CONTENT_EXPORT
-  static bool CTFontRefFromBuffer(mojo::ScopedSharedBufferHandle font_data,
+  static bool CTFontDescriptorFromBuffer(
+      mojo::ScopedSharedBufferHandle font_data,
       uint32_t font_data_size,
-                                  CTFontRef* out);
+      base::ScopedCFTypeRef<CTFontDescriptorRef>* out_descriptor);
 
   CONTENT_EXPORT
   static std::unique_ptr<ResultInternal> LoadFontForTesting(

content/common/mac/font_loader.mm

@@ -141,10 +141,11 @@ void FontLoader::LoadFont(const base::string16& font_name,
 }
 
 // static
-bool FontLoader::CTFontRefFromBuffer(mojo::ScopedSharedBufferHandle font_data,
+bool FontLoader::CTFontDescriptorFromBuffer(
+    mojo::ScopedSharedBufferHandle font_data,
     uint32_t font_data_size,
-                                     CTFontRef* out) {
-  *out = NULL;
+    base::ScopedCFTypeRef<CTFontDescriptorRef>* out_descriptor) {
+  out_descriptor->reset();
   mojo::ScopedSharedBufferMapping mapping = font_data->Map(font_data_size);
   if (!mapping)
     return false;
@@ -152,16 +153,11 @@ bool FontLoader::CTFontRefFromBuffer(mojo::ScopedSharedBufferHandle font_data,
   NSData* data = [NSData dataWithBytes:mapping.get() length:font_data_size];
   base::ScopedCFTypeRef<CTFontDescriptorRef> data_descriptor(
       CTFontManagerCreateFontDescriptorFromData(base::mac::NSToCFCast(data)));
-  base::ScopedCFTypeRef<CGDataProviderRef> provider(
-      CGDataProviderCreateWithCFData(base::mac::NSToCFCast(data)));
-  if (!provider)
-    return false;
-
-  *out = CTFontCreateWithFontDescriptor(data_descriptor.get(), 0, nullptr);
 
-  if (*out == NULL)
+  if (!data_descriptor)
     return false;
 
+  *out_descriptor = std::move(data_descriptor);
   return true;
 }
 

third_party/blink/public/platform/DEPS

@@ -8,6 +8,7 @@ include_rules = [
     "+base/location.h",
     "+base/logging.h",
     "+base/macros.h",
+    "+base/mac/scoped_cftyperef.h",
     "+base/memory/ref_counted.h",
     "+base/memory/scoped_refptr.h",
     "+base/memory/weak_ptr.h",

third_party/blink/public/platform/mac/web_sandbox_support.h

@@ -31,6 +31,7 @@
 #ifndef THIRD_PARTY_BLINK_PUBLIC_PLATFORM_MAC_WEB_SANDBOX_SUPPORT_H_
 #define THIRD_PARTY_BLINK_PUBLIC_PLATFORM_MAC_WEB_SANDBOX_SUPPORT_H_
 
+#include "base/mac/scoped_cftyperef.h"
 #include "third_party/blink/public/common/sandbox_support/sandbox_support_mac.h"
 #include "third_party/skia/include/core/SkColor.h"
 
@@ -44,16 +45,14 @@ class WebSandboxSupport {
   virtual ~WebSandboxSupport() {}
 
   // Given an input font - |srcFont| [which can't be loaded due to sandbox
-  // restrictions]. Return a font belonging to an equivalent font file
-  // that can be used to access the font and a unique identifier corresponding
-  // to the on-disk font file.
-  //
-  // If this function succeeds, the caller assumes ownership of the |out|
-  // parameter and must call CFRelease() on the CTFontRef.
+  // restrictions]. Return a font descriptor based on data belonging to an
+  // equivalent font file that can be used to access the font and a unique
+  // identifier corresponding to the on-disk font file.
   //
   // Returns: true on success, false on error.
-  virtual bool LoadFont(CTFontRef src_font,
-                        CTFontRef* out,
+  virtual bool LoadFont(
+      CTFontRef src_font,
+      base::ScopedCFTypeRef<CTFontDescriptorRef>* out_descriptor,
       uint32_t* font_id) = 0;
 
   // Returns the system's preferred value for a named color.

third_party/blink/renderer/platform/fonts/mac/font_platform_data_mac.mm

@@ -27,7 +27,6 @@
 #import <AvailabilityMacros.h>
 
 #include "base/mac/foundation_util.h"
-#include "base/mac/scoped_cftyperef.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/stl_util.h"
 #import "third_party/blink/public/platform/mac/web_sandbox_support.h"
@@ -59,10 +58,10 @@ static bool CanLoadInProcess(NSFont* ns_font) {
   return ![font_name isEqualToString:@"LastResort"];
 }
 
-static CTFontDescriptorRef CascadeToLastResortFontDescriptor() {
-  static CTFontDescriptorRef descriptor;
-  if (descriptor)
-    return descriptor;
+static CFDictionaryRef CascadeToLastResortFontAttributes() {
+  static CFDictionaryRef attributes;
+  if (attributes)
+    return attributes;
 
   base::ScopedCFTypeRef<CTFontDescriptorRef> last_resort(
       CTFontDescriptorCreateWithNameAndSize(CFSTR("LastResort"), 0));
@@ -73,13 +72,10 @@ static CTFontDescriptorRef CascadeToLastResortFontDescriptor() {
 
   const void* keys[] = {kCTFontCascadeListAttribute};
   const void* values[] = {values_array};
-  base::ScopedCFTypeRef<CFDictionaryRef> attributes(CFDictionaryCreate(
+  attributes = CFDictionaryCreate(
       kCFAllocatorDefault, keys, values, base::size(keys),
-      &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
-
-  descriptor = CTFontDescriptorCreateWithAttributes(attributes);
-
-  return descriptor;
+      &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks);
+  return attributes;
 }
 
 static sk_sp<SkTypeface> LoadFromBrowserProcess(NSFont* ns_font,
@@ -94,19 +90,22 @@ static sk_sp<SkTypeface> LoadFromBrowserProcess(NSFont* ns_font,
     return nullptr;
   }
 
-  CTFontRef loaded_ct_font;
+  base::ScopedCFTypeRef<CTFontDescriptorRef> loaded_data_descriptor;
   uint32_t font_id;
   if (!sandbox_support->LoadFont(base::mac::NSToCFCast(ns_font),
-                                 &loaded_ct_font, &font_id)) {
+                                 &loaded_data_descriptor, &font_id)) {
     // TODO crbug.com/461279: Make this appear in the inspector console?
     DLOG(ERROR)
         << "Loading user font \"" << [[ns_font familyName] UTF8String]
         << "\" from non system location failed. Corrupt or missing font file?";
     return nullptr;
   }
-  base::ScopedCFTypeRef<CTFontRef> ct_font_base(loaded_ct_font);
-  base::ScopedCFTypeRef<CTFontRef> ct_font(CTFontCreateCopyWithAttributes(
-      ct_font_base, text_size, 0, CascadeToLastResortFontDescriptor()));
+
+  base::ScopedCFTypeRef<CTFontDescriptorRef> data_descriptor_with_cascade(
+      CTFontDescriptorCreateCopyWithAttributes(
+          loaded_data_descriptor, CascadeToLastResortFontAttributes()));
+  base::ScopedCFTypeRef<CTFontRef> ct_font(CTFontCreateWithFontDescriptor(
+      data_descriptor_with_cascade.get(), text_size, 0));
   sk_sp<SkTypeface> return_font(SkCreateTypefaceFromCTFont(ct_font));
 
   if (!return_font.get())