Bug 229839 – FontFaceSet.status needs to react to style changes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

FontFaceSet.status needs to react to style changes

[Myles C. Maxfield]

Created attachment 437212 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 437212 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        This is not a WPT test because it requires webfont/slow-ahem-loading.cgi.

Oh, I think I can actually turn this into a WPT test

[Myles C. Maxfield]

Comment on attachment 437212 [details]
Patch

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

>> LayoutTests/ChangeLog:8
>> +        This is not a WPT test because it requires webfont/slow-ahem-loading.cgi.
> 
> Oh, I think I can actually turn this into a WPT test

Oh, no, I can't, because the font may be cached, which will make the status update synchronously. So this needs internals.clearMemoryCache().

But I can at least make it less racey.

[Myles C. Maxfield]

Created attachment 437233 [details]
Patch

[Sam Weinig]

Comment on attachment 437233 [details]
Patch

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

> Source/WebCore/css/FontFaceSet.cpp:191
> +    auto protect = m_backing;

Hm. This clearly does the right thing, but I usually like to ensure we are actually in a Ref or RefPtr when using the protect idiom by doing something like:

Ref protect = backing;

just to make sure that if m_backing ever changes type this is still doing what we expect.

Since m_backing is a Ref, I guess the posibilities here are that either 'this' got deleted or m_backing got replaced and the original value got deleted. Would be interesting to try and add tests for either of those cases if possible.

[Myles C. Maxfield]

Comment on attachment 437233 [details]
Patch

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

>> Source/WebCore/css/FontFaceSet.cpp:191
>> +    auto protect = m_backing;
> 
> Hm. This clearly does the right thing, but I usually like to ensure we are actually in a Ref or RefPtr when using the protect idiom by doing something like:
> 
> Ref protect = backing;
> 
> just to make sure that if m_backing ever changes type this is still doing what we expect.
> 
> Since m_backing is a Ref, I guess the posibilities here are that either 'this' got deleted or m_backing got replaced and the original value got deleted. Would be interesting to try and add tests for either of those cases if possible.

Yeah. I'll land this with "Ref" instead of "auto" and I'll try to come up with a test that exercises the deletion problem in a follow-up.

[Myles C. Maxfield]

Committed r282015 (241320@main): <https://commits.webkit.org/241320@main>

[Radar WebKit Bug Importer]

<rdar://problem/82735622>

[Fujii Hironori]

fast/text/FontFaceSet-status-after-style-update.html is flaky.
https://results.webkit.org/?suite=layout-tests&test=fast%2Ftext%2FFontFaceSet-status-after-style-update.html

[Carlos Alberto Lopez Perez]

(In reply to Fujii Hironori from comment #9)
> fast/text/FontFaceSet-status-after-style-update.html is flaky.
> https://results.webkit.org/?suite=layout-
> tests&test=fast%2Ftext%2FFontFaceSet-status-after-style-update.html

Reported it on bug 229975