Bug 187936 - [Cocoa] Stop crashing in lastResortFallbackFont()
Summary: [Cocoa] Stop crashing in lastResortFallbackFont()
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:
Blocks:
 
Reported: 2018-07-23 17:32 PDT by Myles C. Maxfield
Modified: 2018-08-01 09:54 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.36 KB, patch)
2018-07-23 17:39 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (4.48 KB, patch)
2018-07-23 17:47 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (4.49 KB, patch)
2018-07-23 17:47 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (4.49 KB, patch)
2018-07-23 17:48 PDT, Myles C. Maxfield
jonlee: review+
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 2018-07-23 17:32:45 PDT
[Cocoa] Stop crashing in lastResortFallbackFont()
Comment 1 Myles C. Maxfield 2018-07-23 17:39:17 PDT
Created attachment 345629 [details]
Patch
Comment 2 Myles C. Maxfield 2018-07-23 17:47:04 PDT
Created attachment 345631 [details]
Patch
Comment 3 Myles C. Maxfield 2018-07-23 17:47:59 PDT
Created attachment 345632 [details]
Patch
Comment 4 Myles C. Maxfield 2018-07-23 17:48:33 PDT
Created attachment 345633 [details]
Patch
Comment 5 Jon Lee 2018-07-23 18:54:10 PDT
Comment on attachment 345633 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:-1491
> -    return *fontForFamily(fontDescription, AtomicString("Lucida Grande", AtomicString::ConstructFromLiteral), nullptr, nullptr, { }, false);

The comment above seems no longer to be true. Remove or update?
Comment 6 Myles C. Maxfield 2018-07-24 11:18:15 PDT
<rdar://problem/42249550>
Comment 7 Myles C. Maxfield 2018-07-24 11:34:31 PDT
Committed r234158: <https://trac.webkit.org/changeset/234158>
Comment 8 Darin Adler 2018-07-28 22:35:26 PDT
Comment on attachment 345633 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1490
>      // The Times fallback will almost always work, but in the highly unusual case where
>      // the user doesn't have it, we fall back on Lucida Grande because that's
>      // guaranteed to be there, according to Nathan Taylor. This is good enough
>      // to avoid a crash at least.

I think it’s crazy that we are still using "Lucida Grande" as part of our "last resort". I believe I originally wrote this comment, and it was correct long ago when Lucida Grande was the system font on Mac. It was never true on iOS. Nowadays, not only is using it insufficient, it’s not even a good idea to try it after "Times". We should get rid of this. Just Times and then the last resort.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1498
> +    auto font = adoptCF(CTFontCreateWithName(CFSTR("Helvetica"), fontDescription.computedPixelSize(), nullptr));

Is this really helpful? When do we have "Helvetica" but not "Times"?
Comment 9 Myles C. Maxfield 2018-07-31 11:47:32 PDT
Comment on attachment 345633 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1498
>> +    auto font = adoptCF(CTFontCreateWithName(CFSTR("Helvetica"), fontDescription.computedPixelSize(), nullptr));
> 
> Is this really helpful? When do we have "Helvetica" but not "Times"?

This was a recommendation from the CoreText team.
Comment 10 Darin Adler 2018-08-01 09:54:22 PDT
Comment on attachment 345633 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1498
>>> +    auto font = adoptCF(CTFontCreateWithName(CFSTR("Helvetica"), fontDescription.computedPixelSize(), nullptr));
>> 
>> Is this really helpful? When do we have "Helvetica" but not "Times"?
> 
> This was a recommendation from the CoreText team.

So now I know who recommended it, but not why!