Bug 199769 – Make sure to set kCTFontFallbackOptionAttribute to kCTFontFallbackOptionSystem for system fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[youenn fablet]

This ensures that these system fonts will not resort to user installed fonts in case of a missing character.

[youenn fablet]

<rdar://problem/49390297>

[youenn fablet]

Created attachment 374055 [details]
Patch

[youenn fablet]

Created attachment 374062 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 374062 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1536
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000)

No need for iOS version check, we only support building for 12.2 and newer).

I *think* that this check as is disables the feature on watchOS and tvOS, which may or may not be desirable. But let’s be explicit about it if we want this.

[Myles C. Maxfield]

Comment on attachment 374062 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1537
> +#define HAVE_DISALLOW_USER_INSTALLED_FONTS 1

“Have disallowed” doesn’t make much sense.

[youenn fablet]

Thanks for the review, I'll update it accordingly.

I tried coming with a test but failed to do it so far, if anybody has an idea, let me know.

It also seems that styles keep a shouldAllowUserInstalledFonts() that is not always in sync with the corresponding setting when changed through internals API.

[youenn fablet]

(In reply to Alexey Proskuryakov from comment #4)
> Comment on attachment 374062 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374062&action=review
> 
> > Source/WTF/wtf/Platform.h:1536
> > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000)
> 
> No need for iOS version check, we only support building for 12.2 and newer).

OK

> I *think* that this check as is disables the feature on watchOS and tvOS,
> which may or may not be desirable. But let’s be explicit about it if we want
> this.

I think it is fine as is.

> > Source/WTF/wtf/Platform.h:1537
> > +#define HAVE_DISALLOW_USER_INSTALLED_FONTS 1
> 
> “Have disallowed” doesn’t make much sense.

Will change to HAVE(CAN_DISALLOW_USER_INSTALLED_FONTS)

[youenn fablet]

Created attachment 374125 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374125 [details]
Patch

Rejecting attachment 374125 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 374125, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/12746368

[youenn fablet]

Created attachment 374169 [details]
Patch for landing

[youenn fablet]

Created attachment 374195 [details]
patch

[youenn fablet]

Created attachment 374213 [details]
With tests

[youenn fablet]

Created attachment 374218 [details]
With tests

[Myles C. Maxfield]

Comment on attachment 374218 [details]
With tests

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:842
> +#if !HAVE(CAN_DISALLOW_USER_INSTALLED_FONTS)

Nit: How about HAVE(DISALLOWABLE_USER_INSTALLED_FONTS)

> Tools/WebKitTestRunner/TestController.cpp:1399
> +        else if (key == "allowUserInstalledFonts")

I'm amazed this didn't exist already.

[Myles C. Maxfield]

Comment on attachment 374218 [details]
With tests

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:573
> +        return createFontForInstalledFonts(originalFont, fontDescription.shouldAllowUserInstalledFonts());

This should have already been called before this function. Why not put this call at the end of lookupFallbackFont()? It would be more performant there.

[Myles C. Maxfield]

Comment on attachment 374218 [details]
With tests

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

> Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:99
> +		41D5B62622DD9D36000F4C4A /* FakeHelvetica-SingleExtendedCharacter.ttf in Resources */ = {isa = PBXBuildFile; fileRef = 41D5B62522DD9D36000F4C4A /* FakeHelvetica-SingleExtendedCharacter.ttf */; };

Woah, how did you build this font?

[youenn fablet]

(In reply to Myles C. Maxfield from comment #15)
> Comment on attachment 374218 [details]
> With tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374218&action=review
> 
> > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:573
> > +        return createFontForInstalledFonts(originalFont, fontDescription.shouldAllowUserInstalledFonts());
> 
> This should have already been called before this function. Why not put this
> call at the end of lookupFallbackFont()? It would be more performant there.

That is the first thing I did to verify this was the issue.

I moved this wrapping so that this was making sure preparePlatformFont would always return a 'safe' font.
For instance, there is no guarantee to have the same fontDescription.shouldAllowUserInstalledFonts value.
I guess we could beef-up the optimisation if-check to validate this, or use an ASSERT.

Is it perf sensitive?

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374218&action=review
> 
> > Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:99
> > +		41D5B62622DD9D36000F4C4A /* FakeHelvetica-SingleExtendedCharacter.ttf in Resources */ = {isa = PBXBuildFile; fileRef = 41D5B62522DD9D36000F4C4A /* FakeHelvetica-SingleExtendedCharacter.ttf */; };
> 
> Woah, how did you build this font?

I used FontForge, my first and probably last work on editing fonts.

[youenn fablet]

(In reply to Myles C. Maxfield from comment #14)
> Comment on attachment 374218 [details]
> With tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374218&action=review
> 
> > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:842
> > +#if !HAVE(CAN_DISALLOW_USER_INSTALLED_FONTS)
> 
> Nit: How about HAVE(DISALLOWABLE_USER_INSTALLED_FONTS)

OK

> > Tools/WebKitTestRunner/TestController.cpp:1399
> > +        else if (key == "allowUserInstalledFonts")
> 
> I'm amazed this didn't exist already.

I think we should probably modify all existing tests to use this one and not the internals settings one.

[youenn fablet]

(In reply to youenn fablet from comment #17)
> (In reply to Myles C. Maxfield from comment #15)
> > Comment on attachment 374218 [details]
> > With tests
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=374218&action=review
> > 
> > > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:573
> > > +        return createFontForInstalledFonts(originalFont, fontDescription.shouldAllowUserInstalledFonts());
> > 
> > This should have already been called before this function. Why not put this
> > call at the end of lookupFallbackFont()? It would be more performant there.
> 
> That is the first thing I did to verify this was the issue.
> 
> I moved this wrapping so that this was making sure preparePlatformFont would
> always return a 'safe' font.
> For instance, there is no guarantee to have the same
> fontDescription.shouldAllowUserInstalledFonts value.
> I guess we could beef-up the optimisation if-check to validate this, or use
> an ASSERT.
> 
> Is it perf sensitive?

Or maybe optimize createFontForInstalledFonts in case font already has the right attribute.

[Myles C. Maxfield]

Comment on attachment 374218 [details]
With tests

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

>>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:573
>>> +        return createFontForInstalledFonts(originalFont, fontDescription.shouldAllowUserInstalledFonts());
>> 
>> This should have already been called before this function. Why not put this call at the end of lookupFallbackFont()? It would be more performant there.
> 
> That is the first thing I did to verify this was the issue.
> 
> I moved this wrapping so that this was making sure preparePlatformFont would always return a 'safe' font.
> For instance, there is no guarantee to have the same fontDescription.shouldAllowUserInstalledFonts value.
> I guess we could beef-up the optimisation if-check to validate this, or use an ASSERT.
> 
> Is it perf sensitive?

Creating fonts affects PLT and is perf sensitive.

I like the ASSERT() idea. Can we put the createFontForInstalledFonts() call in lookupFallbackFont() and add an ASSERT() in preparePlatformFont()?

>>> Tools/WebKitTestRunner/TestController.cpp:1399
>>> +        else if (key == "allowUserInstalledFonts")
>> 
>> I'm amazed this didn't exist already.
> 
> I think we should probably modify all existing tests to use this one and not the internals settings one.

Is there an internals settings one? There's an internals function to install some fonts, but none that I can see to disable user-installed fonts.

In general, I believe internals is preferable to the <!-- webkit-test-runner [ allowUserInstalledFonts=false ] --> thing. If we can migrate to that, we should. (Also, moving to internals should require 0 lines of code since all Settings are already exposed to internals already. You should already just be able to say

if (window.internals)
    window.internals.settings.setShouldAllowUserInstalledFonts(false);

And it should just work on ToT.

[youenn fablet]

> In general, I believe internals is preferable to the <!-- webkit-test-runner
> [ allowUserInstalledFonts=false ] --> thing. If we can migrate to that, we
> should. (Also, moving to internals should require 0 lines of code since all
> Settings are already exposed to internals already. You should already just
> be able to say
> 
> if (window.internals)
>     window.internals.settings.setShouldAllowUserInstalledFonts(false);
> 
> And it should just work on ToT.

internals.settings.setShouldAllowUserInstalledFonts is affecting the setting and will adjust the setting in some of the places it is stored but not all.
Some rendering code is sticking with the old initial value for instance.
Hence why I went with the webkit-test-runner option.

[Myles C. Maxfield]

(In reply to youenn fablet from comment #21)
> > In general, I believe internals is preferable to the <!-- webkit-test-runner
> > [ allowUserInstalledFonts=false ] --> thing. If we can migrate to that, we
> > should. (Also, moving to internals should require 0 lines of code since all
> > Settings are already exposed to internals already. You should already just
> > be able to say
> > 
> > if (window.internals)
> >     window.internals.settings.setShouldAllowUserInstalledFonts(false);
> > 
> > And it should just work on ToT.
> 
> internals.settings.setShouldAllowUserInstalledFonts is affecting the setting
> and will adjust the setting in some of the places it is stored but not all.
> Some rendering code is sticking with the old initial value for instance.
> Hence why I went with the webkit-test-runner option.

I'm surprised at the idea that, because something is buggy, we should build a new way of doing the same thing, rather than just fixing the bug(s). Especially because the replacement method is less preferable than the original method.

[youenn fablet]

Created attachment 374252 [details]
Patch

[youenn fablet]

Created attachment 374263 [details]
Patch

[youenn fablet]

Created attachment 374281 [details]
Patch

[youenn fablet]

(In reply to youenn fablet from comment #25)
> Created attachment 374281 [details]
> Patch

Made sure that the cached font descriptors in FontDatabase have exactly the right fallback option.

[Myles C. Maxfield]

Comment on attachment 374281 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:572
> +        return createFontForInstalledFonts(originalFont, fontDescription.shouldAllowUserInstalledFonts());

I thought we needed to guard this; only call it when it's actually necessary.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:882
> +            addAttributesForInstalledFonts(attributes.get(), allowUserInstalledFonts);

I don't think this is actually necessary. You're already calling this in preparePlatformFont().

[youenn fablet]

Comment on attachment 374281 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:572
>> +        return createFontForInstalledFonts(originalFont, fontDescription.shouldAllowUserInstalledFonts());
> 
> I thought we needed to guard this; only call it when it's actually necessary.

createFontForInstalledFonts is a no-op if the fallback attribute is matching the policy.

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:882
>> +            addAttributesForInstalledFonts(attributes.get(), allowUserInstalledFonts);
> 
> I don't think this is actually necessary. You're already calling this in preparePlatformFont().

It does help as preparePlatformFont will in most cases become a no-op, especially since we are caching these descriptors, this ensure we do not change the attribute again and again in preparePlatformFont

[WebKit Commit Bot]

Comment on attachment 374281 [details]
Patch

Clearing flags on attachment: 374281

Committed r247566: <https://trac.webkit.org/changeset/247566>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.