Bug 202548 – FontFaceSet's ready promise is not always resolved

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

When we do layout on an element, FontRanges::glyphDataForCharacter() will cause the first available font to start loading, but will continue looking at subsequent fonts to see if there's a good one we can render while the load is happening. When looking for a fallback font, it calls FontRanges::Range::font() with a ExternalResourceDownloadPolicy set to Forbid. This is fine, except that a side effect of calling this function is that the CSSFontFace marks itself as Loading, which means document.fonts.ready is deferred. Then, the load finishes, and the subsequent CSSFontFace is never actually used, meaning it never exits the Loading state, which means document.fonts.ready never fires.

[Myles C. Maxfield]

Created attachment 380167 [details]
Test

[Myles C. Maxfield]

This was discovered during Chris's investigation of https://bugs.webkit.org/show_bug.cgi?id=202476.

[Myles C. Maxfield]

The fix for this is likely in CSSFontFace::pump() where it setStatus(Status::Loading) right before source->load().

[Myles C. Maxfield]

Created attachment 380169 [details]
Patch

[Chris Dumez]

Comment on attachment 380169 [details]
Patch

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

> Source/WebCore/css/CSSFontFace.cpp:740
> +                if (policy == ExternalResourceDownloadPolicy::Allow && m_status == Status::Pending)

I have one concern.

Imagine the case where this is a local font and this local font ends up being successful and Policy is ExternalResourceDownloadPolicy::Forbid. Previously, we would do:
1. Status update: Pending -> Loading
2. Call load()
3. Then below, source->status() returns Success
4. Status update: Loading -> Success (See switch below).

After your change:
1. Call load() without status update, so we are Pending
2. Then below, source->status() returns Success
3. Status update: Pending -> Loading

Now we're stuck in Loading state even though our source successfully loaded, right?

[Chris Dumez]

Comment on attachment 380169 [details]
Patch

cq- to make sure this does not inadvertently committed before answering my comment. If my comment is inaccurate, feel free to override and cq+.

[Myles C. Maxfield]

Comment on attachment 380169 [details]
Patch

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

>> Source/WebCore/css/CSSFontFace.cpp:740
>> +                if (policy == ExternalResourceDownloadPolicy::Allow && m_status == Status::Pending)
> 
> I have one concern.
> 
> Imagine the case where this is a local font and this local font ends up being successful and Policy is ExternalResourceDownloadPolicy::Forbid. Previously, we would do:
> 1. Status update: Pending -> Loading
> 2. Call load()
> 3. Then below, source->status() returns Success
> 4. Status update: Loading -> Success (See switch below).
> 
> After your change:
> 1. Call load() without status update, so we are Pending
> 2. Then below, source->status() returns Success
> 3. Status update: Pending -> Loading
> 
> Now we're stuck in Loading state even though our source successfully loaded, right?

The next two lines after the Pending -> Loading transition is

            if (m_status == Status::Loading || m_status == Status::TimedOut)
                setStatus(Status::Success);

We just set the status to Loading, so we take this "if" branch and set the status to success.

I'll add a test.

[Myles C. Maxfield]

Committed r250983: <https://trac.webkit.org/changeset/250983>

[Radar WebKit Bug Importer]

<rdar://problem/56165070>

[Chris Dumez]

Comment on attachment 380169 [details]
Patch

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

>>> Source/WebCore/css/CSSFontFace.cpp:740
>>> +                if (policy == ExternalResourceDownloadPolicy::Allow && m_status == Status::Pending)
>> 
>> I have one concern.
>> 
>> Imagine the case where this is a local font and this local font ends up being successful and Policy is ExternalResourceDownloadPolicy::Forbid. Previously, we would do:
>> 1. Status update: Pending -> Loading
>> 2. Call load()
>> 3. Then below, source->status() returns Success
>> 4. Status update: Loading -> Success (See switch below).
>> 
>> After your change:
>> 1. Call load() without status update, so we are Pending
>> 2. Then below, source->status() returns Success
>> 3. Status update: Pending -> Loading
>> 
>> Now we're stuck in Loading state even though our source successfully loaded, right?
> 
> The next two lines after the Pending -> Loading transition is
> 
>             if (m_status == Status::Loading || m_status == Status::TimedOut)
>                 setStatus(Status::Success);
> 
> We just set the status to Loading, so we take this "if" branch and set the status to success.
> 
> I'll add a test.

Oh, duh. Sorry about the noise.