From 04cd65c72c9cc8746fdc537c1fbf8beaa6df721f Mon Sep 17 00:00:00 2001 From: AJ Ballway Date: Thu, 1 Dec 2016 11:51:41 -0800 Subject: [PATCH] Handle missing font files more gracefully --- Frameworks/CoreGraphics/CGDataProvider.mm | 4 ++- Frameworks/CoreGraphics/DWriteWrapper.mm | 25 +++++++++++++------ Frameworks/CoreText/CTFontManager.mm | 11 ++++---- .../unittests/CoreText/CTFontManagerTests.mm | 14 ++++++++++- 4 files changed, 39 insertions(+), 15 deletions(-) diff --git a/Frameworks/CoreGraphics/CGDataProvider.mm b/Frameworks/CoreGraphics/CGDataProvider.mm index 3c73764c80..e27e401f74 100644 --- a/Frameworks/CoreGraphics/CGDataProvider.mm +++ b/Frameworks/CoreGraphics/CGDataProvider.mm @@ -66,7 +66,9 @@ - (NSUInteger)length { CGDataProviderRef CGDataProviderCreateWithURL(CFURLRef url) { NSString* path = [static_cast(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 index cf1428a5df..08b5770807 100644 --- 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,6 +730,7 @@ 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); @@ -736,12 +738,16 @@ void AddDatas(CFArrayRef fontDatas, CFArrayRef* errors) { } } 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 static HRESULT __DWriteUpdateUserCreatedFontCollection(CFArrayRef datas, CFArrayRef* errors, TLambda&& func) { ComPtr 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 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 index 544604dc83..7df73502e5 100644 --- a/Frameworks/CoreText/CTFontManager.mm +++ b/Frameworks/CoreText/CTFontManager.mm @@ -59,12 +59,11 @@ static bool __CTFontManagerUpdateWithFonts(CFArrayRef fontURLs, CTFontManagerSco woc::unique_cf fontDatas{ CFArrayCreateMutable(nullptr, count, &kCFTypeArrayCallBacks) }; for (size_t i = 0; i < count; ++i) { NSData* data = [NSData dataWithContentsOfURL:static_cast(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 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 index 56d65a7515..df64d3c3d2 100644 --- 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)); +} \ No newline at end of file