Handle missing font files more gracefully

microsoft · Dec 1, 2016 · 04cd65c · 04cd65c
1 parent 6f7e249
commit 04cd65c
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 15 deletions.
diff --git a/Frameworks/CoreGraphics/CGDataProvider.mm b/Frameworks/CoreGraphics/CGDataProvider.mm
@@ -66,7 +66,9 @@ - (NSUInteger)length {
 CGDataProviderRef CGDataProviderCreateWithURL(CFURLRef url) {
     NSString* path = [static_cast<NSURL*>(url) path];
     CGDataProvider* ret = [[CGDataProvider alloc] initWithContentsOfFile:path];
-    ret->filename = path;
+    if (ret) {
+        ret->filename = path;
+    }
 
     return ret;
 }

diff --git a/Frameworks/CoreGraphics/DWriteWrapper.mm b/Frameworks/CoreGraphics/DWriteWrapper.mm
@@ -716,8 +716,9 @@ HRESULT RuntimeClassInitialize() {
         return S_OK;
     }
 
-    void AddDatas(CFArrayRef fontDatas, CFArrayRef* errors) {
+    HRESULT AddDatas(CFArrayRef fontDatas, CFArrayRef* errors) {
         CFMutableArrayRef outErrors = nil;
+        HRESULT ret = S_OK;
         if (errors) {
             outErrors = CFArrayCreateMutable(nullptr, 0, &kCFTypeArrayCallBacks);
             *errors = outErrors;
@@ -729,19 +730,24 @@ void AddDatas(CFArrayRef fontDatas, CFArrayRef* errors) {
             if (data) {
                 if (CFSetContainsValue(m_fontDatasSet.get(), data)) {
                     __AppendErrorIfExists(outErrors, kCTFontManagerErrorAlreadyRegistered);
+                    ret = S_FALSE;
                 } else {
                     CFArrayAppendValue(m_fontDatas.get(), data);
                     CFSetAddValue(m_fontDatasSet.get(), data);
                     __AppendNullptrIfExists(outErrors);
                 }
             } else {
                 __AppendErrorIfExists(outErrors, kCTFontManagerErrorInvalidFontData);
+                ret = S_FALSE;
             }
         }
+
+        return ret;
     }
 
-    void RemoveDatas(CFArrayRef fontDatas, CFArrayRef* errors) {
+    HRESULT RemoveDatas(CFArrayRef fontDatas, CFArrayRef* errors) {
         CFMutableArrayRef outErrors = nil;
+        HRESULT ret = S_OK;
         if (errors) {
             outErrors = CFArrayCreateMutable(nullptr, 0, &kCFTypeArrayCallBacks);
             *errors = outErrors;
@@ -759,11 +765,15 @@ void RemoveDatas(CFArrayRef fontDatas, CFArrayRef* errors) {
                     __AppendNullptrIfExists(outErrors);
                 } else {
                     __AppendErrorIfExists(outErrors, kCTFontManagerErrorNotRegistered);
+                    ret = S_FALSE;
                 }
             } else {
                 __AppendErrorIfExists(outErrors, kCTFontManagerErrorInvalidFontData);
+                ret = S_FALSE;
             }
         }
+
+        return ret;
     }
 
     HRESULT STDMETHODCALLTYPE CreateEnumeratorFromKey(_In_ IDWriteFactory* factory,
@@ -802,7 +812,7 @@ static void __AppendNullptrIfExists(CFMutableArrayRef errors) {
 };
 
 // TLambda is a member function of our FontCollectionLoader which updates the internal state of the loader
-// TLambda :: (DWriteFontCollectionLoader -> CFArrayRef -> CFArrayRef*) -> void
+// TLambda :: (DWriteFontCollectionLoader -> CFArrayRef -> CFArrayRef*) -> HRESULT
 template <typename TLambda>
 static HRESULT __DWriteUpdateUserCreatedFontCollection(CFArrayRef datas, CFArrayRef* errors, TLambda&& func) {
     ComPtr<IDWriteFactory> dwriteFactory;
@@ -820,21 +830,20 @@ static HRESULT __DWriteUpdateUserCreatedFontCollection(CFArrayRef datas, CFArray
     RETURN_IF_FAILED(createdLoader);
 
     // Call member function to update loader's font data
-    func(loader.Get(), datas, errors);
+    // Still want to update font collection with whatever fonts succeeded, so return ret at end
+    HRESULT ret = func(loader.Get(), datas, errors);
 
     // DWrite won't create a new font collection unless we provide a new key each time
     // So every time we modify the font collection increment the key
     static size_t collectionKey = 0;
-    ++collectionKey;
-
     std::lock_guard<std::mutex> guard(_userCreatedFontCollectionMutex);
     RETURN_IF_FAILED(
-        dwriteFactory->CreateCustomFontCollection(loader.Get(), &(collectionKey), sizeof(collectionKey), &_userCreatedFontCollection));
+        dwriteFactory->CreateCustomFontCollection(loader.Get(), &(++collectionKey), sizeof(collectionKey), &_userCreatedFontCollection));
 
     // Update s_userFontPropertiesMap with new values
     s_userFontPropertiesMap = __CreatePropertiesMapForFontCollection(_userCreatedFontCollection.Get());
 
-    return S_OK;
+    return ret;
 }
 
 // Registers user defined fonts to a collection so they can be created later

diff --git a/Frameworks/CoreText/CTFontManager.mm b/Frameworks/CoreText/CTFontManager.mm
@@ -59,12 +59,11 @@ static bool __CTFontManagerUpdateWithFonts(CFArrayRef fontURLs, CTFontManagerSco
     woc::unique_cf<CFMutableArrayRef> fontDatas{ CFArrayCreateMutable(nullptr, count, &kCFTypeArrayCallBacks) };
     for (size_t i = 0; i < count; ++i) {
         NSData* data = [NSData dataWithContentsOfURL:static_cast<NSURL*>(CFArrayGetValueAtIndex(fontURLs, i))];
-        if (data != nullptr) {
-            CFArrayAppendValue(fontDatas.get(), (CFDataRef)data);
-        }
+        CFArrayAppendValue(fontDatas.get(), (CFDataRef)data);
     }
 
-    return SUCCEEDED(func(fontDatas.get(), errors));
+    // S_FALSE represents partial failure so cannot use SUCCEEDED macro
+    return func(fontDatas.get(), errors) == S_OK;
 }
 
 // Gets CFData from CGFontRef if available, which are passed into DWriteWrapper methods
@@ -84,7 +83,9 @@ static bool __CTFontManagerUpdateWithGraphicsFont(CGFontRef font, CFErrorRef _Nu
 
     woc::unique_cf<CFArrayRef> fontDatas{ CFArrayCreate(nullptr, (const void**)&data, 1, &kCFTypeArrayCallBacks) };
     CFArrayRef errors = nil;
-    bool ret = SUCCEEDED(func(fontDatas.get(), &errors));
+
+    // S_FALSE represents partial failure so cannot use SUCCEEDED macro
+    bool ret = func(fontDatas.get(), &errors) == S_OK;
     if (error != nil && errors != nil && CFArrayGetCount(errors) > 0L) {
         *error = (CFErrorRef)CFRetain(CFArrayGetValueAtIndex(errors, 0));
     }

diff --git a/tests/unittests/CoreText/CTFontManagerTests.mm b/tests/unittests/CoreText/CTFontManagerTests.mm
@@ -85,7 +85,19 @@
 TEST(CTFontManager, ShouldFailToUnregisterNonregisteredFonts) {
     NSURL* testFileURL = __GetURLFromPathRelativeToModuleDirectory(@"/data/WinObjC.ttf");
     CFErrorRef error = nullptr;
-    EXPECT_TRUE(CTFontManagerUnregisterFontsForURL((__bridge CFURLRef)testFileURL, kCTFontManagerScopeSession, &error));
+    EXPECT_FALSE(CTFontManagerUnregisterFontsForURL((__bridge CFURLRef)testFileURL, kCTFontManagerScopeSession, &error));
     EXPECT_NE(nullptr, error);
     EXPECT_EQ(kCTFontManagerErrorNotRegistered, CFErrorGetCode(error));
 }
+
+TEST(CTFontManager, ShouldFailToRegisterNonexistentFont) {
+    NSURL* testFileURL = __GetURLFromPathRelativeToModuleDirectory(@"/data/BlatantlyNonexistentFont.ttf");
+    CFErrorRef error = nullptr;
+    EXPECT_FALSE(CTFontManagerRegisterFontsForURL((__bridge CFURLRef)testFileURL, kCTFontManagerScopeSession, &error));
+    EXPECT_NE(nullptr, error);
+    EXPECT_EQ(kCTFontManagerErrorInvalidFontData, CFErrorGetCode(error));
+
+    EXPECT_FALSE(CTFontManagerUnregisterFontsForURL((__bridge CFURLRef)testFileURL, kCTFontManagerScopeSession, &error));
+    EXPECT_NE(nullptr, error);
+    EXPECT_EQ(kCTFontManagerErrorInvalidFontData, CFErrorGetCode(error));
+}