Created attachment 362234 [details]
Patch v1

<rdar://problem/48147429>

Comment on attachment 362234 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=362234&action=review

> Tools/ChangeLog:13
> +        CTFontManagerRegisterFontsForURL() to ignore any errors, thereby
> +        fixing the leaks.

Tangentially, what are the errors? Doesn't seem like we should be expecting any, so it may be an indication of something pretty terrible.

Created attachment 362303 [details]
Patch v2

Now prints out error message when registration fails.

(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 362234 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362234&action=review
> 
> > Tools/ChangeLog:13
> > +        CTFontManagerRegisterFontsForURL() to ignore any errors, thereby
> > +        fixing the leaks.
> 
> Tangentially, what are the errors? Doesn't seem like we should be expecting
> any, so it may be an indication of something pretty terrible.

Only these four tests trigger the error:

$BUILD_ROOT/layout-test-results/fast/text/user-installed-fonts/shadow-family-stderr.txt

2019-02-18 10:29:46.488 com.apple.WebKit.WebContent.Development[62401:23370792] Failed to activate fake Helvetica: Error Domain=com.apple.CoreText.CTFontManagerErrorDomain Code=105 "Could not register the font file(s)" UserInfo={NSDescription=Could not register the font file(s), CTFontManagerErrorFontURLs=(
    "file://$HOME/Library/Developer/CoreSimulator/Devices/453C9A70-C13B-4D78-AE6E-FE1A9F04643A/data/Containers/Bundle/Application/8D8B896F-A06F-4ED2-B157-C87593F9B230/WebKitTestRunnerApp.app/PlugIns/WebKitTestRunnerInjectedBundle.bundle/FakeHelvetica-Helvetica-500.ttf"
)}

$BUILD_ROOT/layout-test-results/fast/text/user-installed-fonts/shadow-stderr.txt

2019-02-18 10:29:48.071 com.apple.WebKit.WebContent.Development[62401:23370792] Failed to activate fake Helvetica: Error Domain=com.apple.CoreText.CTFontManagerErrorDomain Code=105 "Could not register the font file(s)" UserInfo={NSDescription=Could not register the font file(s), CTFontManagerErrorFontURLs=(
    "file://$HOME/Library/Developer/CoreSimulator/Devices/453C9A70-C13B-4D78-AE6E-FE1A9F04643A/data/Containers/Bundle/Application/8D8B896F-A06F-4ED2-B157-C87593F9B230/WebKitTestRunnerApp.app/PlugIns/WebKitTestRunnerInjectedBundle.bundle/FakeHelvetica-Helvetica-400.ttf"
)}

$BUILD_ROOT/layout-test-results/fast/text/user-installed-fonts/shadow-family-disable-stderr.txt

2019-02-18 10:29:46.240 com.apple.WebKit.WebContent.Development[62401:23370792] Failed to activate fake Helvetica: Error Domain=com.apple.CoreText.CTFontManagerErrorDomain Code=105 "Could not register the font file(s)" UserInfo={NSDescription=Could not register the font file(s), CTFontManagerErrorFontURLs=(
    "file://$HOME/Library/Developer/CoreSimulator/Devices/453C9A70-C13B-4D78-AE6E-FE1A9F04643A/data/Containers/Bundle/Application/8D8B896F-A06F-4ED2-B157-C87593F9B230/WebKitTestRunnerApp.app/PlugIns/WebKitTestRunnerInjectedBundle.bundle/FakeHelvetica-Helvetica-500.ttf"
)}

$BUILD_ROOT/layout-test-results/fast/text/user-installed-fonts/shadow-disable-stderr.txt

2019-02-18 10:29:45.988 com.apple.WebKit.WebContent.Development[62401:23370792] Failed to activate fake Helvetica: Error Domain=com.apple.CoreText.CTFontManagerErrorDomain Code=105 "Could not register the font file(s)" UserInfo={NSDescription=Could not register the font file(s), CTFontManagerErrorFontURLs=(
    "file://$HOME/Library/Developer/CoreSimulator/Devices/453C9A70-C13B-4D78-AE6E-FE1A9F04643A/data/Containers/Bundle/Application/8D8B896F-A06F-4ED2-B157-C87593F9B230/WebKitTestRunnerApp.app/PlugIns/WebKitTestRunnerInjectedBundle.bundle/FakeHelvetica-Helvetica-400.ttf"
)}

Note that these files DO exist at this path, too.

Are they being registered more than once?

(In reply to David Kilzer (:ddkilzer) from comment #5)
> (In reply to Alexey Proskuryakov from comment #3)
> > Comment on attachment 362234 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=362234&action=review
> > 
> > > Tools/ChangeLog:13
> > > +        CTFontManagerRegisterFontsForURL() to ignore any errors, thereby
> > > +        fixing the leaks.
> > 
> > Tangentially, what are the errors? Doesn't seem like we should be expecting
> > any, so it may be an indication of something pretty terrible.
> 
> Only these four tests trigger the error:
> 
> $BUILD_ROOT/layout-test-results/fast/text/user-installed-fonts/shadow-family-
> stderr.txt
> 
> 2019-02-18 10:29:46.488
> com.apple.WebKit.WebContent.Development[62401:23370792] Failed to activate
> fake Helvetica: Error Domain=com.apple.CoreText.CTFontManagerErrorDomain
> Code=105 "Could not register the font file(s)" UserInfo={NSDescription=Could
> not register the font file(s), CTFontManagerErrorFontURLs=(
>    
> "file://$HOME/Library/Developer/CoreSimulator/Devices/453C9A70-C13B-4D78-
> AE6E-FE1A9F04643A/data/Containers/Bundle/Application/8D8B896F-A06F-4ED2-B157-
> C87593F9B230/WebKitTestRunnerApp.app/PlugIns/WebKitTestRunnerInjectedBundle.
> bundle/FakeHelvetica-Helvetica-500.ttf"
> )}
> 
> $BUILD_ROOT/layout-test-results/fast/text/user-installed-fonts/shadow-stderr.
> txt
> 
> 2019-02-18 10:29:48.071
> com.apple.WebKit.WebContent.Development[62401:23370792] Failed to activate
> fake Helvetica: Error Domain=com.apple.CoreText.CTFontManagerErrorDomain
> Code=105 "Could not register the font file(s)" UserInfo={NSDescription=Could
> not register the font file(s), CTFontManagerErrorFontURLs=(
>    
> "file://$HOME/Library/Developer/CoreSimulator/Devices/453C9A70-C13B-4D78-
> AE6E-FE1A9F04643A/data/Containers/Bundle/Application/8D8B896F-A06F-4ED2-B157-
> C87593F9B230/WebKitTestRunnerApp.app/PlugIns/WebKitTestRunnerInjectedBundle.
> bundle/FakeHelvetica-Helvetica-400.ttf"
> )}
> 
> $BUILD_ROOT/layout-test-results/fast/text/user-installed-fonts/shadow-family-
> disable-stderr.txt
> 
> 2019-02-18 10:29:46.240
> com.apple.WebKit.WebContent.Development[62401:23370792] Failed to activate
> fake Helvetica: Error Domain=com.apple.CoreText.CTFontManagerErrorDomain
> Code=105 "Could not register the font file(s)" UserInfo={NSDescription=Could
> not register the font file(s), CTFontManagerErrorFontURLs=(
>    
> "file://$HOME/Library/Developer/CoreSimulator/Devices/453C9A70-C13B-4D78-
> AE6E-FE1A9F04643A/data/Containers/Bundle/Application/8D8B896F-A06F-4ED2-B157-
> C87593F9B230/WebKitTestRunnerApp.app/PlugIns/WebKitTestRunnerInjectedBundle.
> bundle/FakeHelvetica-Helvetica-500.ttf"
> )}
> 
> $BUILD_ROOT/layout-test-results/fast/text/user-installed-fonts/shadow-
> disable-stderr.txt
> 
> 2019-02-18 10:29:45.988
> com.apple.WebKit.WebContent.Development[62401:23370792] Failed to activate
> fake Helvetica: Error Domain=com.apple.CoreText.CTFontManagerErrorDomain
> Code=105 "Could not register the font file(s)" UserInfo={NSDescription=Could
> not register the font file(s), CTFontManagerErrorFontURLs=(
>    
> "file://$HOME/Library/Developer/CoreSimulator/Devices/453C9A70-C13B-4D78-
> AE6E-FE1A9F04643A/data/Containers/Bundle/Application/8D8B896F-A06F-4ED2-B157-
> C87593F9B230/WebKitTestRunnerApp.app/PlugIns/WebKitTestRunnerInjectedBundle.
> bundle/FakeHelvetica-Helvetica-400.ttf"
> )}
> 
> Note that these files DO exist at this path, too.
> 
> Are they being registered more than once?

Myles, is this expected?

(In reply to David Kilzer (:ddkilzer) from comment #6)
> Myles, is this expected?

Nope.

(In reply to Myles C. Maxfield from comment #7)
> (In reply to David Kilzer (:ddkilzer) from comment #6)
> > Myles, is this expected?
> 
> Nope.

Should this block landing this patch since this fixes the leak, and adds stderr output to these tests?

Seems like the right behavior here would probably be to RELEASE_ASSERT. But first the issue has to be fixed of course.

(In reply to Alexey Proskuryakov from comment #9)
> Seems like the right behavior here would probably be to RELEASE_ASSERT. But
> first the issue has to be fixed of course.

According to LayoutTests/platform/ios/TestExpectations which was added for Bug 180062 in r225641, this is expected behavior on iOS:

+# User-installed font infrastructure is ony present on certain OSes.
+webkit.org/b/180062 fast/text/user-installed-fonts/disable.html [ ImageOnlyFailure ]
+webkit.org/b/180062 fast/text/user-installed-fonts/shadow-family.html [ ImageOnlyFailure ]
+webkit.org/b/180062 fast/text/user-installed-fonts/shadow-postscript.html [ ImageOnlyFailure ]
+webkit.org/b/180062 fast/text/user-installed-fonts/shadow.html [ ImageOnlyFailure ]

In fact, it's only supposed to work on Mojave and above on WebKit2:

LayoutTests/platform/mac-wk1/TestExpectations

+# User-installed fonts test infrastructure is not present in WK1
+webkit.org/b/180062 fast/text/user-installed-fonts [ ImageOnlyFailure ]

LayoutTests/platform/mac/TestExpectations

+# User-installed font infrastructure is ony present on certain OSes.
+webkit.org/b/180062 [ ElCapitan Sierra HighSierra ] fast/text/user-installed-fonts/disable.html [ ImageOnlyFailure ]
+webkit.org/b/180062 [ ElCapitan Sierra HighSierra ] fast/text/user-installed-fonts/shadow-postscript.html [ ImageOnlyFailure ]
+webkit.org/b/180062 [ ElCapitan Sierra HighSierra ] fast/text/user-installed-fonts/shadow.html [ ImageOnlyFailure ]

So do we change the code to prevent the CFErrorRef objects from being created (going back to Patch v1 / Attachment #362234 [details])?  Or get the errors, but only report them on macOS Mojave and newer?

Another alternative is to ifdef out the code where it's not going to work.

Created attachment 362389 [details]
Patch v3

Only log errors in installFakeHelvetica() if supported platform.

(In reply to David Kilzer (:ddkilzer) from comment #12)
> Created attachment 362389 [details]
> Patch v3
> 
> Only log errors in installFakeHelvetica() if supported platform.

Patch v3 only logs on supported platforms.  Should we just make the whole WTR::installFakeHelvetica() method do nothing except on macOS Mojave or newer?

Attachment 362389 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm:146:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to Build Bot from comment #14)
> Attachment 362389 [details] did not pass style-queue:
> 
> 
> ERROR:
> Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm:146: 
> Misplaced OS version check. Please use a named macro in wtf/Platform.h,
> wtf/FeatureDefines.h, or an appropriate internal file. 
> [build/version_check] [5]
> Total errors found: 1 in 2 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

+ #if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400

So this warning means we should make up a brand new macro for wtf/Platform.h for this specific case?

Or should we just change the macro to `#if USE(APPKIT)` because eventually this code will work on all macOS versions, and the expectations in LayoutTests/platform/mac/TestExpectations (see Comment #10) will eventually be removed?

Perhaps we already have a feature macro for this? Adding one for testing code would be silly, but testing code features usually correspond to real features.

Comment on attachment 362389 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=362389&action=review

> Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm:92
> +                if (!CTFontManagerRegisterFontsForURL((CFURLRef)fontURL, kCTFontManagerScopeProcess, &error)) {

Shouldn't we be using C++-style casts?

> Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm:147
> +        // Registering shadow fonts is only supported on macOS Mojave or newer. See Bugs 180062 & 194761.

I think it's supposed to work on iOS, but we shouldn't block this patch on that. We can open a new bug to investigate why it doesn't work on iOS.

Comment on attachment 362389 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=362389&action=review

>> Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm:92
>> +                if (!CTFontManagerRegisterFontsForURL((CFURLRef)fontURL, kCTFontManagerScopeProcess, &error)) {
> 
> Shouldn't we be using C++-style casts?

Will fix.

>> Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm:147
>> +        // Registering shadow fonts is only supported on macOS Mojave or newer. See Bugs 180062 & 194761.
> 
> I think it's supposed to work on iOS, but we shouldn't block this patch on that. We can open a new bug to investigate why it doesn't work on iOS.

It appears to work on iOS, but not iOS Simulator.  I will file a radar to track this internally.

Comment on attachment 362389 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=362389&action=review

>>> Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm:92
>>> +                if (!CTFontManagerRegisterFontsForURL((CFURLRef)fontURL, kCTFontManagerScopeProcess, &error)) {
>> 
>> Shouldn't we be using C++-style casts?
> 
> Will fix.

Actually, to make these ARC-compliant (for a future switch to use ARC in this project), we have to use C-style casts:  (__bridge CFTypeRef).

Using static_cast<__bridge CFTypeRef>() doesn't work.  :(

Committed r241920: <https://trac.webkit.org/changeset/241920>

(In reply to David Kilzer (:ddkilzer) from comment #19)
> Comment on attachment 362389 [details]
> Patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362389&action=review
> 
> >>> Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm:92
> >>> +                if (!CTFontManagerRegisterFontsForURL((CFURLRef)fontURL, kCTFontManagerScopeProcess, &error)) {
> >> 
> >> Shouldn't we be using C++-style casts?
> > 
> > Will fix.
> 
> Actually, to make these ARC-compliant (for a future switch to use ARC in
> this project), we have to use C-style casts:  (__bridge CFTypeRef).
> 
> Using static_cast<__bridge CFTypeRef>() doesn't work.  :(

👎