Bug 180062 - [Cocoa] Add SPI to disallow user-installed fonts
Summary: [Cocoa] Add SPI to disallow user-installed fonts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 180164
Blocks:
  Show dependency treegraph
 
Reported: 2017-11-27 17:40 PST by Myles C. Maxfield
Modified: 2019-02-19 06:56 PST (History)
7 users (show)

See Also:


Attachments
WIP (26.34 KB, patch)
2017-11-27 17:42 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (34.98 KB, patch)
2017-11-27 20:58 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.16 MB, application/zip)
2017-11-27 22:28 PST, EWS Watchlist
no flags Details
WIP (52.65 KB, patch)
2017-11-28 10:23 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (52.65 KB, patch)
2017-11-28 20:14 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (51.95 KB, patch)
2017-11-28 22:29 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.23 MB, application/zip)
2017-11-28 23:59 PST, EWS Watchlist
no flags Details
Patch (52.04 KB, patch)
2017-11-29 09:22 PST, Myles C. Maxfield
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.25 MB, application/zip)
2017-11-29 10:50 PST, EWS Watchlist
no flags Details
WIP (48.42 KB, patch)
2017-11-30 14:19 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Needs iOS test update (54.17 KB, patch)
2017-12-06 00:09 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.25 MB, application/zip)
2017-12-06 01:37 PST, EWS Watchlist
no flags Details
Patch (89.18 KB, patch)
2017-12-06 12:44 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (89.24 KB, patch)
2017-12-06 15:54 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (90.69 KB, patch)
2017-12-06 16:31 PST, Myles C. Maxfield
simon.fraser: review+
Details | Formatted Diff | Diff
Patch for committing (88.81 KB, patch)
2017-12-07 12:00 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2017-11-27 17:40:26 PST
[Cocoa] Add SPI to disallow user-installed fonts
Comment 1 Myles C. Maxfield 2017-11-27 17:42:26 PST
Created attachment 327718 [details]
WIP
Comment 2 Myles C. Maxfield 2017-11-27 20:58:03 PST
Created attachment 327724 [details]
WIP
Comment 3 EWS Watchlist 2017-11-27 20:59:58 PST
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.
Comment 4 EWS Watchlist 2017-11-27 22:28:22 PST
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
Comment 5 EWS Watchlist 2017-11-27 22:28:23 PST
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
Comment 6 Myles C. Maxfield 2017-11-28 10:23:29 PST
Created attachment 327761 [details]
WIP
Comment 7 EWS Watchlist 2017-11-28 10:26:12 PST
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.
Comment 8 Myles C. Maxfield 2017-11-28 20:14:27 PST
Created attachment 327821 [details]
WIP
Comment 9 EWS Watchlist 2017-11-28 20:16:16 PST
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.
Comment 10 Myles C. Maxfield 2017-11-28 22:07:44 PST
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.
Comment 11 Myles C. Maxfield 2017-11-28 22:11:20 PST
(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.
Comment 12 Myles C. Maxfield 2017-11-28 22:29:00 PST
Created attachment 327830 [details]
Patch
Comment 13 EWS Watchlist 2017-11-28 22:31:14 PST
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.
Comment 14 EWS Watchlist 2017-11-28 23:59:00 PST
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
Comment 15 EWS Watchlist 2017-11-28 23:59:01 PST
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
Comment 16 Myles C. Maxfield 2017-11-29 09:22:15 PST
Created attachment 327862 [details]
Patch
Comment 17 EWS Watchlist 2017-11-29 09:24:06 PST
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.
Comment 18 EWS Watchlist 2017-11-29 10:50:22 PST
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
Comment 19 EWS Watchlist 2017-11-29 10:50:23 PST
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
Comment 20 Myles C. Maxfield 2017-11-30 14:19:28 PST
Created attachment 328027 [details]
WIP
Comment 21 Myles C. Maxfield 2017-12-05 15:35:21 PST
<rdar://problem/35042408>
Comment 22 Myles C. Maxfield 2017-12-05 17:17:56 PST
CTFontManagerRegisterFontsForURL() is returning false on iOS
Comment 23 Myles C. Maxfield 2017-12-06 00:07:14 PST
iOS won't allow registering a font with a duplicate PostScript name.
Comment 24 Myles C. Maxfield 2017-12-06 00:09:00 PST
Created attachment 328553 [details]
Needs iOS test update
Comment 25 EWS Watchlist 2017-12-06 00:10:30 PST
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.
Comment 26 EWS Watchlist 2017-12-06 01:37:37 PST
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
Comment 27 EWS Watchlist 2017-12-06 01:37:38 PST
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
Comment 28 Myles C. Maxfield 2017-12-06 12:44:47 PST
Created attachment 328609 [details]
Patch
Comment 29 Myles C. Maxfield 2017-12-06 15:54:38 PST
Created attachment 328640 [details]
Patch
Comment 30 Myles C. Maxfield 2017-12-06 16:31:16 PST
Created attachment 328650 [details]
Patch
Comment 31 Simon Fraser (smfr) 2017-12-06 17:30:47 PST
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];
Comment 32 Myles C. Maxfield 2017-12-07 11:55:06 PST
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
Comment 33 Myles C. Maxfield 2017-12-07 12:00:29 PST
Created attachment 328715 [details]
Patch for committing
Comment 34 WebKit Commit Bot 2017-12-07 12:29:08 PST
Comment on attachment 328715 [details]
Patch for committing

Clearing flags on attachment: 328715

Committed r225641: <https://trac.webkit.org/changeset/225641>
Comment 35 Fujii Hironori 2017-12-08 00:28:10 PST
Filed Bug 180575 to fix GTK port WebKitTestRunner.
Comment 36 Matt Lewis 2017-12-13 09:37:38 PST
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
Comment 37 Matt Lewis 2017-12-13 09:50:30 PST
(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.
Comment 38 Myles C. Maxfield 2017-12-13 13:09:37 PST
(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.
Comment 39 Myles C. Maxfield 2017-12-19 19:09:38 PST
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.
Comment 40 Matt Lewis 2018-02-08 12:49:26 PST
Myles, Do you have any update on this?
Comment 41 Myles C. Maxfield 2018-02-09 17:28:12 PST
(In reply to Matt Lewis from comment #40)
> Myles, Do you have any update on this?

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