Handle missing font files more gracefully #1486

aballway · 2016-12-01T20:03:52Z

Missing font files or otherwise incorrect font file data would cause a crash without producing a CFError or returning false.

This change is

rajsesh

In this case, these are tests and the failures in the build machine is because of missing test data. We need to understand why the test data files are not in the right place and fix that rather than trying to ignore them in the tests.

aballway · 2016-12-01T20:38:33Z

This isn't to fix the error in the build machine, rather the build machine error showed that my changes did not handle the error well and I lacked tests to check that, which is what this fixes.

msft-Jeyaram · 2016-12-01T20:43:24Z

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) {


this and CGDataProviderCreateWithFilename are pretty much the same logic.
re-use/refactor. rather than introducing the same code path.

This will all be changed for #1450 #WontFix

msft-Jeyaram · 2016-12-01T20:44:26Z

filename = nil;

nit: remove

Refers to: Frameworks/CoreGraphics/CGDataProvider.mm:33 in 04cd65c. [](commit_id = 04cd65c, deletion_comment = False)

msft-Jeyaram · 2016-12-01T20:45:10Z

Frameworks/CoreGraphics/DWriteWrapper.mm

        CFMutableArrayRef outErrors = nil;
+        HRESULT ret = S_OK;


ret [](start = 16, length = 3)

nit: result

msft-Jeyaram · 2016-12-01T20:48:07Z

Frameworks/CoreText/CTFontManager.mm

    }

-    return SUCCEEDED(func(fontDatas.get(), errors));
+    // S_FALSE represents partial failure so cannot use SUCCEEDED macro


why use S_FALSE?
Are you expecting partial failure?

It made more sense to use S_FALSE than E_* that might mean a more fatal failure than what we're experiencing

msft-Jeyaram · 2016-12-01T20:50:16Z

Frameworks/CoreGraphics/DWriteWrapper.mm

@@ -729,19 +730,24 @@ void AddDatas(CFArrayRef fontDatas, CFArrayRef* errors) {
            if (data) {
                if (CFSetContainsValue(m_fontDatasSet.get(), data)) {
                    __AppendErrorIfExists(outErrors, kCTFontManagerErrorAlreadyRegistered);
+                    ret = S_FALSE;


why does this have to be S_FALSE?
Are you expecting this to succeed, where you have failures?

This is created with a list of font files, so should one fail we need to return that it failed but still un(register) whatever fonts were valid

msft-Jeyaram · 2016-12-01T20:53:54Z

🕐

msft-Jeyaram · 2016-12-01T21:03:22Z

tests/unittests/CoreText/CTFontManagerTests.mm

+    EXPECT_FALSE(CTFontManagerUnregisterFontsForURL((__bridge CFURLRef)testFileURL, kCTFontManagerScopeSession, &error));
+    EXPECT_NE(nullptr, error);
+    EXPECT_EQ(kCTFontManagerErrorInvalidFontData, CFErrorGetCode(error));
+}


addition of registering/unregistering multiple fonts test would be useful.

+1, this is a good point that I missed in the previous PR

In reply to: 90536678 [](ancestors = 90536678)

msft-Jeyaram · 2016-12-01T21:03:55Z

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));


what happened here?
How does this behave on the ref platform?

It should return false, this was an error

msft-Jeyaram · 2016-12-01T21:14:38Z

tests/unittests/CoreText/CTFontManagerTests.mm

+    EXPECT_FALSE(CTFontManagerUnregisterFontsForURL((__bridge CFURLRef)testFileURL, kCTFontManagerScopeSession, &error));
+    EXPECT_NE(nullptr, error);
+    EXPECT_EQ(kCTFontManagerErrorInvalidFontData, CFErrorGetCode(error));
+}


Add a test to attempt to register the same font.

rajsesh · 2016-12-01T21:25:22Z

:face_palm: apologies for the confusion.

In reply to: 264287896 [](ancestors = 264287896)

msft-Jeyaram · 2016-12-01T22:56:42Z

Frameworks/CoreGraphics/DWriteWrapper.mm

@@ -667,7 +664,7 @@ HRESULT STDMETHODCALLTYPE GetCurrentFontFile(_Out_ IDWriteFontFile** fontFile) {
        RETURN_HR_IF(E_ILLEGAL_METHOD_CALL, m_location < 0 || m_location > CFArrayGetCount(m_fontDatas.get()));

        if (0 <= m_location && m_location < m_previouslyCreatedFiles->size()) {
-            *fontFile = m_previouslyCreatedFiles->at(m_location).Get();
+            m_previouslyCreatedFiles->at(m_location).CopyTo(fontFile);


opyTo(fontFile); [](start = 54, length = 16)

:D

ms-jihua · 2016-12-01T22:56:51Z

// TODO: #1250: Need to be able to load fonts from the app's bundle

iirc iOS behavior is to return nil for an unfound font, but OSX returns a default font. as such i'd like to keep this?

Refers to: Frameworks/CoreGraphics/DWriteWrapper.mm:411 in 04cd65c. [](commit_id = 04cd65c, deletion_comment = True)

ms-jihua

Just need to keep that comment

msft-Jeyaram · 2016-12-01T23:04:59Z

tests/unittests/CoreText/CTFontManagerTests.mm

+    woc::unique_cf<CTFontRef> secondFont{ CTFontCreateWithName(secondFontName.get(), 20, nullptr) };
+    EXPECT_NE(nullptr, secondFont);
+    StrongId<NSString> secondName = (NSString*)CTFontCopyFullName(secondFont.get());
+    EXPECT_OBJCEQ(@"WinObjC Italic", secondName);


you have repeated code here and in few other tests that do the same thing.
Consider using a general function. This can help add more tests in the future.

msft-Jeyaram · 2016-12-01T23:07:48Z

tests/unittests/CoreText/CTFontManagerTests.mm

+
+    woc::unique_cf<CTFontRef> firstFont{ CTFontCreateWithName(firstFontName.get(), 20, nullptr) };
+    EXPECT_NE(nullptr, firstFont);
+    StrongId<NSString> firstName = (NSString*)CTFontCopyFullName(firstFont.get());


tring> firstName = (NSString*)CTFontCopyFullName(firstFont.get()); [](start = 16, length = 66)

this feels wrong. The general = causes a retain??

It was a misunderstanding

Handle missing font files more gracefully

04cd65c

aballway assigned DHowett-MSFT and ms-jihua Dec 1, 2016

msftclas added the cla-not-required label Dec 1, 2016

rajsesh previously requested changes Dec 1, 2016

View reviewed changes

msft-Jeyaram reviewed Dec 1, 2016

View reviewed changes

msft-Jeyaram suggested changes Dec 1, 2016

View reviewed changes

msft-Jeyaram reviewed Dec 1, 2016

View reviewed changes

More tests, use ComPtr like a grown up

ee98009

msft-Jeyaram reviewed Dec 1, 2016

View reviewed changes

ms-jihua approved these changes Dec 1, 2016

View reviewed changes

msft-Jeyaram reviewed Dec 1, 2016

View reviewed changes

return comment

53503a9

msft-Jeyaram reviewed Dec 1, 2016

View reviewed changes

aballway added 2 commits December 1, 2016 15:10

Maybe one of these days I will stop leaking memory... some day

01f91d0

someday

583f639

msft-Jeyaram approved these changes Dec 1, 2016

View reviewed changes

aballway merged commit 56fe72e into microsoft:develop Dec 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle missing font files more gracefully #1486

Handle missing font files more gracefully #1486

aballway commented Dec 1, 2016 •

edited by rajsesh

rajsesh left a comment

aballway commented Dec 1, 2016

msft-Jeyaram Dec 1, 2016

aballway Dec 1, 2016

msft-Jeyaram commented Dec 1, 2016

msft-Jeyaram Dec 1, 2016

msft-Jeyaram Dec 1, 2016

aballway Dec 1, 2016

msft-Jeyaram Dec 1, 2016

aballway Dec 1, 2016

msft-Jeyaram commented Dec 1, 2016

msft-Jeyaram Dec 1, 2016

ms-jihua Dec 1, 2016

msft-Jeyaram Dec 1, 2016

aballway Dec 1, 2016

msft-Jeyaram Dec 1, 2016

rajsesh commented Dec 1, 2016

msft-Jeyaram Dec 1, 2016

ms-jihua commented Dec 1, 2016

ms-jihua left a comment

msft-Jeyaram Dec 1, 2016

msft-Jeyaram Dec 1, 2016

Handle missing font files more gracefully #1486

Handle missing font files more gracefully #1486

Conversation

aballway commented Dec 1, 2016 • edited by rajsesh

rajsesh left a comment

Choose a reason for hiding this comment

aballway commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajsesh commented Dec 1, 2016

Choose a reason for hiding this comment

ms-jihua commented Dec 1, 2016

ms-jihua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aballway commented Dec 1, 2016 •

edited by rajsesh