FontFaceSet.has() needs to react to style changes
Created attachment 437238 [details] Patch
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
Created attachment 437239 [details] Patch
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?
Created attachment 437305 [details] Patch
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.
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.
Created attachment 437337 [details] Patch
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
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].
<rdar://problem/82916442>