Bug 1455569 - Handle variation settings of system-installed variation fonts when creating CTFont from CGFont on High Sierra. r=lsalzman
authorJonathan Kew <jkew@mozilla.com>
Fri, 20 Apr 2018 17:18:03 +0100
changeset 468301 32b9dbc4159082cadb88f9f87948a867ac41104d
parent 468300 033299f2733900eb0da9b5b4d814aea39ce552d4
child 468302 070eb9701963ab1a00e3cbe6931f21e80f7d6aa5
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslsalzman
bugs1455569
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1455569 - Handle variation settings of system-installed variation fonts when creating CTFont from CGFont on High Sierra. r=lsalzman
gfx/2d/ScaledFontMac.cpp
gfx/2d/UnscaledFontMac.h
gfx/cairo/cairo/src/cairo-quartz-font.c
gfx/skia/skia/src/ports/SkFontHost_mac.cpp
gfx/thebes/gfxCoreTextShaper.cpp
gfx/thebes/gfxMacFont.cpp
gfx/thebes/gfxMacFont.h
widget/cocoa/nsCocoaFeatures.h
widget/cocoa/nsCocoaFeatures.mm
--- a/gfx/2d/ScaledFontMac.cpp
+++ b/gfx/2d/ScaledFontMac.cpp
@@ -70,50 +70,71 @@ private:
 };
 
 ScaledFontMac::CTFontDrawGlyphsFuncT* ScaledFontMac::CTFontDrawGlyphsPtr = nullptr;
 bool ScaledFontMac::sSymbolLookupDone = false;
 
 // Helper to create a CTFont from a CGFont, copying any variations that were
 // set on the original CGFont.
 static CTFontRef
-CreateCTFontFromCGFontWithVariations(CGFontRef aCGFont, CGFloat aSize)
+CreateCTFontFromCGFontWithVariations(CGFontRef aCGFont, CGFloat aSize,
+                                     bool aInstalledFont)
 {
     // Avoid calling potentially buggy variation APIs on pre-Sierra macOS
     // versions (see bug 1331683).
     //
     // And on HighSierra, CTFontCreateWithGraphicsFont properly carries over
     // variation settings from the CGFont to CTFont, so we don't need to do
     // the extra work here -- and this seems to avoid Core Text crashiness
     // seen in bug 1454094.
     //
-    // So we only need to do this "the hard way" on Sierra; on other releases,
-    // just let the standard CTFont function do its thing.
-    if (!nsCocoaFeatures::OnSierraExactly()) {
-        return CTFontCreateWithGraphicsFont(aCGFont, aSize, nullptr, nullptr);
-    }
+    // However, for installed fonts it seems we DO need to copy the variations
+    // explicitly even on 10.13, otherwise fonts fail to render (as in bug
+    // 1455494) when non-default values are used. Fortunately, the crash
+    // mentioned above occurs with data fonts, not (AFAICT) with system-
+    // installed fonts.
+    //
+    // So we only need to do this "the hard way" on Sierra, and for installed
+    // fonts on HighSierra+; otherwise, just let the standard CTFont function
+    // do its thing.
+    //
+    // NOTE in case this ever needs further adjustment: there is similar logic
+    // in four places in the tree (sadly):
+    //    CreateCTFontFromCGFontWithVariations in gfxMacFont.cpp
+    //    CreateCTFontFromCGFontWithVariations in ScaledFontMac.cpp
+    //    CreateCTFontFromCGFontWithVariations in cairo-quartz-font.c
+    //    ctfont_create_exact_copy in SkFontHost_mac.cpp
 
-    CFDictionaryRef vars = CGFontCopyVariations(aCGFont);
     CTFontRef ctFont;
-    if (vars) {
-        CFDictionaryRef varAttr =
-            CFDictionaryCreate(nullptr,
-                               (const void**)&kCTFontVariationAttribute,
-                               (const void**)&vars, 1,
-                               &kCFTypeDictionaryKeyCallBacks,
-                               &kCFTypeDictionaryValueCallBacks);
-        CFRelease(vars);
+    if (nsCocoaFeatures::OnSierraExactly() ||
+        (aInstalledFont && nsCocoaFeatures::OnHighSierraOrLater())) {
+        CFDictionaryRef vars = CGFontCopyVariations(aCGFont);
+        if (vars) {
+            CFDictionaryRef varAttr =
+                CFDictionaryCreate(nullptr,
+                                   (const void**)&kCTFontVariationAttribute,
+                                   (const void**)&vars, 1,
+                                   &kCFTypeDictionaryKeyCallBacks,
+                                   &kCFTypeDictionaryValueCallBacks);
+            CFRelease(vars);
 
-        CTFontDescriptorRef varDesc = CTFontDescriptorCreateWithAttributes(varAttr);
-        CFRelease(varAttr);
+            CTFontDescriptorRef varDesc =
+                CTFontDescriptorCreateWithAttributes(varAttr);
+            CFRelease(varAttr);
 
-        ctFont = CTFontCreateWithGraphicsFont(aCGFont, aSize, nullptr, varDesc);
-        CFRelease(varDesc);
+            ctFont = CTFontCreateWithGraphicsFont(aCGFont, aSize, nullptr,
+                                                  varDesc);
+            CFRelease(varDesc);
+        } else {
+            ctFont = CTFontCreateWithGraphicsFont(aCGFont, aSize, nullptr,
+                                                  nullptr);
+        }
     } else {
-        ctFont = CTFontCreateWithGraphicsFont(aCGFont, aSize, nullptr, nullptr);
+        ctFont = CTFontCreateWithGraphicsFont(aCGFont, aSize, nullptr,
+                                              nullptr);
     }
     return ctFont;
 }
 
 ScaledFontMac::ScaledFontMac(CGFontRef aFont,
                              const RefPtr<UnscaledFont>& aUnscaledFont,
                              Float aSize,
                              bool aOwnsFont,
@@ -134,17 +155,19 @@ ScaledFontMac::ScaledFontMac(CGFontRef a
 
   if (!aOwnsFont) {
     // XXX: should we be taking a reference
     CGFontRetain(aFont);
   }
 
   if (CTFontDrawGlyphsPtr != nullptr) {
     // only create mCTFont if we're going to be using the CTFontDrawGlyphs API
-    mCTFont = CreateCTFontFromCGFontWithVariations(aFont, aSize);
+    auto unscaledMac = static_cast<UnscaledFontMac*>(aUnscaledFont.get());
+    bool dataFont = unscaledMac->IsDataFont();
+    mCTFont = CreateCTFontFromCGFontWithVariations(aFont, aSize, !dataFont);
   } else {
     mCTFont = nullptr;
   }
 }
 
 ScaledFontMac::~ScaledFontMac()
 {
   if (mCTFont) {
@@ -155,17 +178,20 @@ ScaledFontMac::~ScaledFontMac()
 
 #ifdef USE_SKIA
 SkTypeface* ScaledFontMac::GetSkTypeface()
 {
   if (!mTypeface) {
     if (mCTFont) {
       mTypeface = SkCreateTypefaceFromCTFont(mCTFont);
     } else {
-      CTFontRef fontFace = CreateCTFontFromCGFontWithVariations(mFont, mSize);
+      auto unscaledMac = static_cast<UnscaledFontMac*>(GetUnscaledFont().get());
+      bool dataFont = unscaledMac->IsDataFont();
+      CTFontRef fontFace =
+        CreateCTFontFromCGFontWithVariations(mFont, mSize, !dataFont);
       mTypeface = SkCreateTypefaceFromCTFont(fontFace);
       CFRelease(fontFace);
     }
   }
   return mTypeface;
 }
 #endif
 
--- a/gfx/2d/UnscaledFontMac.h
+++ b/gfx/2d/UnscaledFontMac.h
@@ -35,16 +35,18 @@ public:
   }
 
   FontType GetType() const override { return FontType::MAC; }
 
   CGFontRef GetFont() const { return mFont; }
 
   bool GetFontFileData(FontFileDataOutput aDataCallback, void *aBaton) override;
 
+  bool IsDataFont() const { return mIsDataFont; }
+
   already_AddRefed<ScaledFont>
     CreateScaledFont(Float aGlyphSize,
                      const uint8_t* aInstanceData,
                      uint32_t aInstanceDataLength,
                      const FontVariation* aVariations,
                      uint32_t aNumVariations) override;
 
   static CGFontRef
--- a/gfx/cairo/cairo/src/cairo-quartz-font.c
+++ b/gfx/cairo/cairo/src/cairo-quartz-font.c
@@ -348,16 +348,27 @@ CreateCTFontFromCGFontWithVariations(CGF
     //
     // And on HighSierra, CTFontCreateWithGraphicsFont properly carries over
     // variation settings from the CGFont to CTFont, so we don't need to do
     // the extra work here -- and this seems to avoid Core Text crashiness
     // seen in bug 1454094.
     //
     // So we only need to do this "the hard way" on Sierra; on other releases,
     // just let the standard CTFont function do its thing.
+    //
+    // NOTE in case this ever needs further adjustment: there is similar logic
+    // in four places in the tree (sadly):
+    //    CreateCTFontFromCGFontWithVariations in gfxMacFont.cpp
+    //    CreateCTFontFromCGFontWithVariations in ScaledFontMac.cpp
+    //    CreateCTFontFromCGFontWithVariations in cairo-quartz-font.c
+    //    ctfont_create_exact_copy in SkFontHost_mac.cpp
+    //
+    // XXX Does this need to behave differently for installed fonts on High
+    // Sierra, as in other similar places (bug 1455569)?
+
     if (!Gecko_OnSierraExactly()) {
         return CTFontCreateWithGraphicsFont(aCGFont, aSize, NULL, NULL);
     }
 
     CFDictionaryRef vars = CGFontCopyVariations(aCGFont);
     CTFontRef ctFont;
     if (vars) {
         CFDictionaryRef varAttr =
--- a/gfx/skia/skia/src/ports/SkFontHost_mac.cpp
+++ b/gfx/skia/skia/src/ports/SkFontHost_mac.cpp
@@ -752,16 +752,17 @@ private:
 
 // CTFontCreateCopyWithAttributes or CTFontCreateCopyWithSymbolicTraits cannot be used on 10.10
 // and later, as they will return different underlying fonts depending on the size requested.
 // It is not possible to use descriptors with CTFontCreateWithFontDescriptor, since that does not
 // work with non-system fonts. As a result, create the strike specific CTFonts from the underlying
 // CGFont.
 #ifdef MOZ_SKIA
 extern "C" bool Gecko_OnSierraExactly();
+extern "C" bool Gecko_OnHighSierraOrLater();
 #endif
 static UniqueCFRef<CTFontRef> ctfont_create_exact_copy(CTFontRef baseFont, CGFloat textSize,
                                                        const CGAffineTransform* transform)
 {
     UniqueCFRef<CGFontRef> baseCGFont(CTFontCopyGraphicsFont(baseFont, nullptr));
 
     // The last parameter (CTFontDescriptorRef attributes) *must* be nullptr.
     // If non-nullptr then with fonts with variation axes, the copy will fail in
@@ -776,19 +777,51 @@ static UniqueCFRef<CTFontRef> ctfont_cre
     // Avoid calling potentially buggy variation APIs on pre-Sierra macOS
     // versions (see bug 1331683).
     //
     // And on HighSierra, CTFontCreateWithGraphicsFont properly carries over
     // variation settings from the CGFont to CTFont, so we don't need to do
     // the extra work here -- and this seems to avoid Core Text crashiness
     // seen in bug 1454094.
     //
-    // So we only need to do this "the hard way" on Sierra; on other releases,
-    // just let the standard CTFont function do its thing.
-    if (Gecko_OnSierraExactly())
+    // However, for installed fonts it seems we DO need to copy the variations
+    // explicitly even on 10.13, otherwise fonts fail to render (as in bug
+    // 1455494) when non-default values are used. Fortunately, the crash
+    // mentioned above occurs with data fonts, not (AFAICT) with system-
+    // installed fonts.
+    //
+    // So we only need to do this "the hard way" on Sierra, and for installed
+    // fonts on HighSierra+; otherwise, just let the standard CTFont function
+    // do its thing.
+    //
+    // NOTE in case this ever needs further adjustment: there is similar logic
+    // in four places in the tree (sadly):
+    //    CreateCTFontFromCGFontWithVariations in gfxMacFont.cpp
+    //    CreateCTFontFromCGFontWithVariations in ScaledFontMac.cpp
+    //    CreateCTFontFromCGFontWithVariations in cairo-quartz-font.c
+    //    ctfont_create_exact_copy in SkFontHost_mac.cpp
+
+    // To figure out if a font is installed locally or used from a @font-face
+    // resource, we check whether its descriptor can provide a URL. This will
+    // be present for installed fonts, but not for those activated from an
+    // in-memory resource.
+    auto IsInstalledFont = [](CTFontRef aFont) {
+        CTFontDescriptorRef desc = CTFontCopyFontDescriptor(aFont);
+        CFTypeRef attr = CTFontDescriptorCopyAttribute(desc, kCTFontURLAttribute);
+        CFRelease(desc);
+        bool result = false;
+        if (attr) {
+            result = true;
+            CFRelease(attr);
+        }
+        return result;
+    };
+
+    if (Gecko_OnSierraExactly() ||
+        (Gecko_OnHighSierraOrLater() && IsInstalledFont(baseFont)))
 #endif
     {
         // Not UniqueCFRef<> because CGFontCopyVariations can return null!
         CFDictionaryRef variations = CGFontCopyVariations(baseCGFont.get());
         if (variations) {
             UniqueCFRef<CFDictionaryRef>
                 varAttr(CFDictionaryCreate(nullptr,
                                            (const void**)&kCTFontVariationAttribute,
--- a/gfx/thebes/gfxCoreTextShaper.cpp
+++ b/gfx/thebes/gfxCoreTextShaper.cpp
@@ -661,18 +661,21 @@ gfxCoreTextShaper::GetFeaturesDescriptor
     }
     return sFeaturesDescriptor[aFeatureFlags];
 }
 
 CTFontRef
 gfxCoreTextShaper::CreateCTFontWithFeatures(CGFloat aSize,
                                             CTFontDescriptorRef aDescriptor)
 {
+    const gfxFontEntry* fe = mFont->GetFontEntry();
+    bool isInstalledFont = !fe->IsUserFont() || fe->IsLocalUserFont();
     CGFontRef cgFont = static_cast<gfxMacFont*>(mFont)->GetCGFontRef();
     return gfxMacFont::CreateCTFontFromCGFontWithVariations(cgFont, aSize,
+                                                            isInstalledFont,
                                                             aDescriptor);
 }
 
 void
 gfxCoreTextShaper::Shutdown() // [static]
 {
     for (size_t i = 0; i < kMaxFontInstances; i++) {
         if (sFeaturesDescriptor[i] != nullptr) {
--- a/gfx/thebes/gfxMacFont.cpp
+++ b/gfx/thebes/gfxMacFont.cpp
@@ -416,54 +416,74 @@ gfxMacFont::GetCharWidth(CFDataRef aCmap
 
     return 0;
 }
 
 /* static */
 CTFontRef
 gfxMacFont::CreateCTFontFromCGFontWithVariations(CGFontRef aCGFont,
                                                  CGFloat aSize,
+                                                 bool aInstalledFont,
                                                  CTFontDescriptorRef aFontDesc)
 {
     // Avoid calling potentially buggy variation APIs on pre-Sierra macOS
     // versions (see bug 1331683).
     //
     // And on HighSierra, CTFontCreateWithGraphicsFont properly carries over
     // variation settings from the CGFont to CTFont, so we don't need to do
     // the extra work here -- and this seems to avoid Core Text crashiness
     // seen in bug 1454094.
     //
-    // So we only need to do this "the hard way" on Sierra; on other releases,
-    // just let the standard CTFont function do its thing.
-    if (!nsCocoaFeatures::OnSierraExactly()) {
-        return CTFontCreateWithGraphicsFont(aCGFont, aSize, nullptr, aFontDesc);
-    }
+    // However, for installed fonts it seems we DO need to copy the variations
+    // explicitly even on 10.13, otherwise fonts fail to render (as in bug
+    // 1455494) when non-default values are used. Fortunately, the crash
+    // mentioned above occurs with data fonts, not (AFAICT) with system-
+    // installed fonts.
+    //
+    // So we only need to do this "the hard way" on Sierra, and on HighSierra
+    // for system-installed fonts; in other cases just let the standard CTFont
+    // function do its thing.
+    //
+    // NOTE in case this ever needs further adjustment: there is similar logic
+    // in four places in the tree (sadly):
+    //    CreateCTFontFromCGFontWithVariations in gfxMacFont.cpp
+    //    CreateCTFontFromCGFontWithVariations in ScaledFontMac.cpp
+    //    CreateCTFontFromCGFontWithVariations in cairo-quartz-font.c
+    //    ctfont_create_exact_copy in SkFontHost_mac.cpp
 
-    CFDictionaryRef variations = ::CGFontCopyVariations(aCGFont);
     CTFontRef ctFont;
-    if (variations) {
-        CFDictionaryRef varAttr =
-            ::CFDictionaryCreate(nullptr,
-                                 (const void**)&kCTFontVariationAttribute,
-                                 (const void**)&variations, 1,
-                                 &kCFTypeDictionaryKeyCallBacks,
-                                 &kCFTypeDictionaryValueCallBacks);
-        ::CFRelease(variations);
+    if (nsCocoaFeatures::OnSierraExactly() ||
+        (aInstalledFont && nsCocoaFeatures::OnHighSierraOrLater())) {
+        CFDictionaryRef variations = ::CGFontCopyVariations(aCGFont);
+        if (variations) {
+            CFDictionaryRef varAttr =
+                ::CFDictionaryCreate(nullptr,
+                                     (const void**)&kCTFontVariationAttribute,
+                                     (const void**)&variations, 1,
+                                     &kCFTypeDictionaryKeyCallBacks,
+                                     &kCFTypeDictionaryValueCallBacks);
+            ::CFRelease(variations);
 
-        CTFontDescriptorRef varDesc = aFontDesc
-            ? ::CTFontDescriptorCreateCopyWithAttributes(aFontDesc, varAttr)
-            : ::CTFontDescriptorCreateWithAttributes(varAttr);
-        ::CFRelease(varAttr);
+            CTFontDescriptorRef varDesc = aFontDesc
+                ? ::CTFontDescriptorCreateCopyWithAttributes(aFontDesc, varAttr)
+                : ::CTFontDescriptorCreateWithAttributes(varAttr);
+            ::CFRelease(varAttr);
 
-        ctFont = ::CTFontCreateWithGraphicsFont(aCGFont, aSize, nullptr, varDesc);
-        ::CFRelease(varDesc);
+            ctFont = ::CTFontCreateWithGraphicsFont(aCGFont, aSize, nullptr,
+                                                    varDesc);
+            ::CFRelease(varDesc);
+        } else {
+            ctFont = ::CTFontCreateWithGraphicsFont(aCGFont, aSize, nullptr,
+                                                    aFontDesc);
+        }
     } else {
         ctFont = ::CTFontCreateWithGraphicsFont(aCGFont, aSize, nullptr,
                                                 aFontDesc);
     }
+
     return ctFont;
 }
 
 int32_t
 gfxMacFont::GetGlyphWidth(DrawTarget& aDrawTarget, uint16_t aGID)
 {
     if (mVariationFont) {
         // Avoid a potential Core Text crash (bug 1450209) by using
@@ -471,17 +491,20 @@ gfxMacFont::GetGlyphWidth(DrawTarget& aD
         // fonts, but those won't have variations, so it's OK.
         int cgAdvance;
         if (::CGFontGetGlyphAdvances(mCGFont, &aGID, 1, &cgAdvance)) {
             return cgAdvance * mFUnitsConvFactor * 0x10000;
         }
     }
 
     if (!mCTFont) {
-        mCTFont = CreateCTFontFromCGFontWithVariations(mCGFont, mAdjustedSize);
+        bool isInstalledFont =
+            !mFontEntry->IsUserFont() || mFontEntry->IsLocalUserFont();
+        mCTFont = CreateCTFontFromCGFontWithVariations(mCGFont, mAdjustedSize,
+                                                       isInstalledFont);
         if (!mCTFont) { // shouldn't happen, but let's be safe
             NS_WARNING("failed to create CTFontRef to measure glyph width");
             return 0;
         }
     }
 
     CGSize advance;
     ::CTFontGetAdvancesForGlyphs(mCTFont, kCTFontDefaultOrientation, &aGID,
--- a/gfx/thebes/gfxMacFont.h
+++ b/gfx/thebes/gfxMacFont.h
@@ -63,16 +63,17 @@ public:
     FontType GetType() const override { return FONT_TYPE_MAC; }
 
     // Helper to create a CTFont from a CGFont, with optional font descriptor
     // (for features), and copying any variations that were set on the CGFont.
     // This is public so that gfxCoreTextShaper can also use it.
     static CTFontRef
     CreateCTFontFromCGFontWithVariations(CGFontRef aCGFont,
                                          CGFloat aSize,
+                                         bool aInstalledFont,
                                          CTFontDescriptorRef aFontDesc = nullptr);
 
 protected:
     const Metrics& GetHorizontalMetrics() override {
         return mMetrics;
     }
 
     // override to prefer CoreText shaping with fonts that depend on AAT
--- a/widget/cocoa/nsCocoaFeatures.h
+++ b/widget/cocoa/nsCocoaFeatures.h
@@ -40,11 +40,12 @@ private:
   static void InitializeVersionNumbers();
 
   static int32_t mOSXVersion;
 };
 
 // C-callable helper for cairo-quartz-font.c and SkFontHost_mac.cpp
 extern "C" {
     bool Gecko_OnSierraExactly();
+    bool Gecko_OnHighSierraOrLater();
 }
 
 #endif // nsCocoaFeatures_h_
--- a/widget/cocoa/nsCocoaFeatures.mm
+++ b/widget/cocoa/nsCocoaFeatures.mm
@@ -183,13 +183,19 @@ nsCocoaFeatures::OnSierraExactly()
 
 /* Version of OnSierraExactly as global function callable from cairo & skia */
 bool
 Gecko_OnSierraExactly()
 {
     return nsCocoaFeatures::OnSierraExactly();
 }
 
+bool
+Gecko_OnHighSierraOrLater()
+{
+    return nsCocoaFeatures::OnHighSierraOrLater();
+}
+
 /* static */ bool
 nsCocoaFeatures::IsAtLeastVersion(int32_t aMajor, int32_t aMinor, int32_t aBugFix)
 {
     return OSXVersion() >= GetVersion(aMajor, aMinor, aBugFix);
 }