Summary: | FontFaceSet.status needs to react to style changes | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||
Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | clopez, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, Hironori.Fujii, macpherson, menard, sam, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=229975 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 229850, 229911, 229975 | ||||||||
Attachments: |
|
Description
Myles C. Maxfield
2021-09-02 16:21:10 PDT
Created attachment 437212 [details]
Patch
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 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. Created attachment 437233 [details]
Patch
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. 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. Committed r282015 (241320@main): <https://commits.webkit.org/241320@main> 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 (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 |