FontFaceSet.has() needs to react to style changes

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

FontFaceSet.has() needs to react to style changes

[Myles C. Maxfield]

Created attachment 437238 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Myles C. Maxfield]

Created attachment 437239 [details]
Patch

[Sam Weinig]

Comment on attachment 437239 [details]
Patch

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

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

Same as the others, would use

Ref protect = m_backing;

> Source/WebCore/css/FontFaceSet.cpp:113
>      return m_backing->hasFace(face.backing());

Seems like this should use protect to?

[Myles C. Maxfield]

Created attachment 437305 [details]
Patch

[Darin Adler]

Comment on attachment 437305 [details]
Patch

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

I don’t understand why m_backing is not sufficient protection and why we need a local variable.

> Source/WebCore/css/FontFaceSet.cpp:113
> -    return m_backing->hasFace(face.backing());
> +    Ref protect = m_backing;
> +    if (face.backing().cssConnection())
> +        protect->updateStyleIfNeeded();
> +    return protect->hasFace(face.backing());

I don’t understand why m_backing is not sufficient protection and why we need a local variable. A "protect" idiom should be very rarely needed. Instead the top level callers of functions need to use Ref or RefPtr.

If FontFaceSet *itself* might be dereferenced as a side effect of updating style, then the correct question is who could call this on a FontFaceSet without holding a reference to the FontFaceSet. If the call is from JavaScript, then the JavaScript bindings themselves will be holding a reference to the FontFaceSet, so it would have to be a call from within WebCore.

[Myles C. Maxfield]

Comment on attachment 437305 [details]
Patch

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

>> Source/WebCore/css/FontFaceSet.cpp:113
>> +    return protect->hasFace(face.backing());
> 
> I don’t understand why m_backing is not sufficient protection and why we need a local variable. A "protect" idiom should be very rarely needed. Instead the top level callers of functions need to use Ref or RefPtr.
> 
> If FontFaceSet *itself* might be dereferenced as a side effect of updating style, then the correct question is who could call this on a FontFaceSet without holding a reference to the FontFaceSet. If the call is from JavaScript, then the JavaScript bindings themselves will be holding a reference to the FontFaceSet, so it would have to be a call from within WebCore.

You're right - the only callers of this are from JS. So, JS will maintain a ref to FontFaceSet, and FontFaceSet will maintain a ref to CSSFontFaceSet. So this doesn't need to be protected.

[Myles C. Maxfield]

Created attachment 437337 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 437305 [details]
Patch

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

>>> Source/WebCore/css/FontFaceSet.cpp:113
>>> +    return protect->hasFace(face.backing());
>> 
>> I don’t understand why m_backing is not sufficient protection and why we need a local variable. A "protect" idiom should be very rarely needed. Instead the top level callers of functions need to use Ref or RefPtr.
>> 
>> If FontFaceSet *itself* might be dereferenced as a side effect of updating style, then the correct question is who could call this on a FontFaceSet without holding a reference to the FontFaceSet. If the call is from JavaScript, then the JavaScript bindings themselves will be holding a reference to the FontFaceSet, so it would have to be a call from within WebCore.
> 
> You're right - the only callers of this are from JS. So, JS will maintain a ref to FontFaceSet, and FontFaceSet will maintain a ref to CSSFontFaceSet. So this doesn't need to be protected.

https://bugs.webkit.org/show_bug.cgi?id=229911

[EWS]

Committed r282204 (241491@main): <https://commits.webkit.org/241491@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437337 [details].

[Radar WebKit Bug Importer]

<rdar://problem/82916442>