Bug 180062 – [Cocoa] Add SPI to disallow user-installed fonts

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[Cocoa] Add SPI to disallow user-installed fonts

[Myles C. Maxfield]

Created attachment 327718 [details]
WIP

[Myles C. Maxfield]

Created attachment 327724 [details]
WIP

[EWS Watchlist]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 22 files


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

[EWS Watchlist]

Comment on attachment 327724 [details]
WIP

Attachment 327724 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5384514

New failing tests:
fast/text/user-installed-disable.html

[EWS Watchlist]

Created attachment 327730 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Myles C. Maxfield]

Created attachment 327761 [details]
WIP

[EWS Watchlist]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 40 files


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

[Myles C. Maxfield]

Created attachment 327821 [details]
WIP

[EWS Watchlist]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 40 files


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

[Myles C. Maxfield]

My test font is insufficient because it has the same traits as real Helvetica, and the font selection algorithm currently selects the first font that CoreText returns. CoreText returns both Helveticas, with the system one first, which means that my fake Helvetica isn't used at all.

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #10)
> My test font is insufficient because it has the same traits as real
> Helvetica, and the font selection algorithm currently selects the first font
> that CoreText returns. CoreText returns both Helveticas, with the system one
> first, which means that my fake Helvetica isn't used at all.

The system Helvetica has weights 400, 300, and 700. If my fake Helvetica has weight 500, it should be able to be used.

[Myles C. Maxfield]

Created attachment 327830 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 40 files


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

[EWS Watchlist]

Comment on attachment 327830 [details]
Patch

Attachment 327830 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5399492

New failing tests:
fast/text/user-installed-fonts/shadow.html

[EWS Watchlist]

Created attachment 327840 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Myles C. Maxfield]

Created attachment 327862 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 40 files


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

[EWS Watchlist]

Comment on attachment 327862 [details]
Patch

Attachment 327862 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5404954

New failing tests:
fast/text/user-installed-fonts/shadow.html

[EWS Watchlist]

Created attachment 327873 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Myles C. Maxfield]

Created attachment 328027 [details]
WIP

[Myles C. Maxfield]

<rdar://problem/35042408>

[Myles C. Maxfield]

CTFontManagerRegisterFontsForURL() is returning false on iOS

[Myles C. Maxfield]

iOS won't allow registering a font with a duplicate PostScript name.

[Myles C. Maxfield]

Created attachment 328553 [details]
Needs iOS test update

[EWS Watchlist]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 42 files


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

[EWS Watchlist]

Comment on attachment 328553 [details]
Needs iOS test update

Attachment 328553 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5512216

New failing tests:
fast/text/user-installed-fonts/shadow.html

[EWS Watchlist]

Created attachment 328560 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Myles C. Maxfield]

Created attachment 328609 [details]
Patch

[Myles C. Maxfield]

Created attachment 328640 [details]
Patch

[Myles C. Maxfield]

Created attachment 328650 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 328650 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.h:205
> +    Vector<FontSelectionCapabilities> getFontSelectionCapabilitiesInFamily(const AtomicString&, bool shouldDisallowUserInstalledFonts);

why not use the AllowUserInstalledFonts enum?

> Source/WebCore/platform/graphics/FontDescription.h:117
> +    bool mayRepresentUserInstalledFont() const { return m_mayRepresentUserInstalledFont; }

AllowUserInstalledFonts?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:989
> +    AllowUserInstalledFonts allowUserInstalledFonts;

Why not m_

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1179
> +Vector<FontSelectionCapabilities> FontCache::getFontSelectionCapabilitiesInFamily(const AtomicString& familyName, bool shouldDisallowUserInstalledFonts)

AllowUserInstalledFonts?

> Source/WebCore/testing/InternalSettings.cpp:735
> +ExceptionOr<void> InternalSettings::setShouldDisallowUserInstalledFonts(bool shouldDisallow)
> +{
> +    if (!m_page)
> +        return Exception { InvalidAccessError };
> +    settings().setShouldDisallowUserInstalledFonts(shouldDisallow);
> +    return { };
> +}
> +

Not sure why you need this. Settings get exposed to tests already without any code.

> Source/WebCore/testing/InternalSettings.idl:105
> +    [MayThrowException] void setShouldDisallowUserInstalledFonts(boolean shouldDisallow);

Don't think you need this.

> Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:118
> +@property (nonatomic, setter=_setShouldDisallowUserInstalledFonts:) BOOL _shouldDisallowUserInstalledFonts WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

"shouldDisallow" or just "disallows"?

Is it a question of "allowing" them, or just using them? Maybe "setUsesUserInstalledFonts"?

> Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm:139
> +    auto bufferSize = WKStringGetMaximumUTF8CStringSize(configuration);
> +    char characterBuffer[bufferSize];
> +    WKStringGetUTF8CString(configuration, characterBuffer, bufferSize);

Don't we have WKStringRef -> NSString helpers?

> Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm:153
> +    NSMutableArray *fontsToRemove = [NSMutableArray arrayWithCapacity:0];

[NSMutableArray array];

[Myles C. Maxfield]

Comment on attachment 328650 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontDescription.h:117
>> +    bool mayRepresentUserInstalledFont() const { return m_mayRepresentUserInstalledFont; }
> 
> AllowUserInstalledFonts?

This one is a little different because it represents whether or not this object may represent a user installed font, rather than whether or not user installed fonts are allowed in general.

>> Source/WebCore/testing/InternalSettings.idl:105
>> +    [MayThrowException] void setShouldDisallowUserInstalledFonts(boolean shouldDisallow);
> 
> Don't think you need this.

Wow it's magic!

>> Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:118
>> +@property (nonatomic, setter=_setShouldDisallowUserInstalledFonts:) BOOL _shouldDisallowUserInstalledFonts WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> "shouldDisallow" or just "disallows"?
> 
> Is it a question of "allowing" them, or just using them? Maybe "setUsesUserInstalledFonts"?

I thought about this, but chose "should" because "disallow" sounds like an immediate verb, rather than setting a bit of state which gets used subsequently.

>> Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm:139
>> +    WKStringGetUTF8CString(configuration, characterBuffer, bufferSize);
> 
> Don't we have WKStringRef -> NSString helpers?

Yep, in WKStringCF.h

[Myles C. Maxfield]

Created attachment 328715 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 328715 [details]
Patch for committing

Clearing flags on attachment: 328715

Committed r225641: <https://trac.webkit.org/changeset/225641>

[Fujii Hironori]

Filed Bug 180575 to fix GTK port WebKitTestRunner.

[Matt Lewis]

This caused a consistent Image only failure on the test fast/text/user-installed-fonts/shadow-postscript-family.html on all platforms except Windows.

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Ftext%2Fuser-installed-fonts%2Fshadow-postscript-family.html
https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/builds/1606

[Matt Lewis]

(In reply to Matt Lewis from comment #36)
> This caused a consistent Image only failure on the test
> fast/text/user-installed-fonts/shadow-postscript-family.html on all
> platforms except Windows.
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=fast%2Ftext%2Fuser-installed-fonts%2Fshadow-
> postscript-family.html
> https://build.webkit.org/builders/
> Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/builds/1606

The failure is only showing up on iOS Simulator due to expectations set.

[Myles C. Maxfield]

(In reply to Matt Lewis from comment #37)
> (In reply to Matt Lewis from comment #36)
> > This caused a consistent Image only failure on the test
> > fast/text/user-installed-fonts/shadow-postscript-family.html on all
> > platforms except Windows.
> > 
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=fast%2Ftext%2Fuser-installed-fonts%2Fshadow-
> > postscript-family.html
> > https://build.webkit.org/builders/
> > Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/builds/1606
> 
> The failure is only showing up on iOS Simulator due to expectations set.

I probably just messed up the expectations. Please don't roll this out quite yet - I can probably fix this today.

[Myles C. Maxfield]

The flakiness dashboard shows that it is failing on WK1, which is expected.

It's also failing on iOS WK2, which is unexpected. Investigating now.

[Matt Lewis]

Myles, Do you have any update on this?

[Myles C. Maxfield]

(In reply to Matt Lewis from comment #40)
> Myles, Do you have any update on this?

https://trac.webkit.org/changeset/228147/webkit