Summary: | Retain cycle between CSSFontSelector -> CSSFontFaceSet -> CSSFontFace -> CSSFontSelector | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||
Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, cdumez, commit-queue, ddkilzer, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, koivisto, macpherson, menard, rniwa, webkit-bug-importer, ysuzuki | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=208351 | ||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2019-03-31 23:05:26 PDT
Created attachment 366386 [details]
WIP
Created attachment 366387 [details]
Dependency graph
CSSFontFace::m_fontSelector is used for 4 purposes: 1) When a font loads, it calls CSSFontSelector::fontLoaded(), so the font selector can dispatchInvalidationCallbacks(). 2) AllowsUserInstalleFonts, FontLoadTimingOverride, and ShouldIgnoreFontLoadCompletions are queried from the document's settings 3) Starting the font load requires the document->cachedResourceLoader() 4) The FontFace object's getters need to updateStyleIfNeeded() to pull down any CSSOM modifications that may have happened in the past. The reason we can't use a simple Client callback for 3) is that load() should still work even if the FontFace isn't attached to any FontFaceSet. It needs to issue a network request even if there is no Document involved. XHR does this via a ThreadableLoader. We should figure out whether we can use ThreadableLoader too. Regressions: Unexpected text-only failures (2) fast/css/font-face-multiple-remote-sources.html [ Failure ] svg/batik/text/xmlSpace.svg [ Failure ] Regressions: Unexpected crashes (1) fast/text/font-face-set-cssom.html [ Crash ] fast/text/font-face-set-cssom.html: 0 com.apple.JavaScriptCore 0x00000001688d7fd0 WTFCrash + 16 (Assertions.cpp:305) 1 com.apple.WebCore 0x000000015000c0cb WTFCrashWithInfo(int, char const*, char const*, int) + 27 2 com.apple.WebCore 0x00000001522d7219 WTF::HashTableConstIterator<WebCore::CSSFontFace::Client*, WebCore::CSSFontFace::Client*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::CSSFontFace::Client*>, WTF::HashTraits<WebCore::CSSFontFace::Client*>, WTF::HashTraits<WebCore::CSSFontFace::Client*> >::checkValidity() const + 105 (HashTable.h:213) 3 com.apple.WebCore 0x00000001522d7389 WTF::HashTableConstIterator<WebCore::CSSFontFace::Client*, WebCore::CSSFontFace::Client*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::CSSFontFace::Client*>, WTF::HashTraits<WebCore::CSSFontFace::Client*>, WTF::HashTraits<WebCore::CSSFontFace::Client*> >::operator++() + 25 (HashTable.h:181) 4 com.apple.WebCore 0x00000001522d6733 WTF::HashTableConstIteratorAdapter<WTF::HashTable<WebCore::CSSFontFace::Client*, WebCore::CSSFontFace::Client*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::CSSFontFace::Client*>, WTF::HashTraits<WebCore::CSSFontFace::Client*>, WTF::HashTraits<WebCore::CSSFontFace::Client*> >, WebCore::CSSFontFace::Client*>::operator++() + 35 (HashTable.h:1506) 5 com.apple.WebCore 0x00000001522c99de void WebCore::iterateClients<WebCore::CSSFontFace::updateStyleIfNeeded()::$_29>(WTF::HashSet<WebCore::CSSFontFace::Client*, WTF::PtrHash<WebCore::CSSFontFace::Client*>, WTF::HashTraits<WebCore::CSSFontFace::Client*> >&, WebCore::CSSFontFace::updateStyleIfNeeded()::$_29) + 382 (CSSFontFace.cpp:58) 6 com.apple.WebCore 0x00000001522c9859 WebCore::CSSFontFace::updateStyleIfNeeded() + 25 (CSSFontFace.cpp:803) 7 com.apple.WebCore 0x000000015239c866 WebCore::FontFace::family() const + 54 (FontFace.cpp:301) Comment on attachment 366386 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=366386&action=review > Source/WebCore/css/CSSFontFaceSet.h:124 > - HashSet<CSSFontFaceSetClient*> m_clients; > + HashSet<Client*> m_clients; It's not great that all these Client interfaces rely on implementations to remove themselves. At minimum, ~Client should delete itself from CSSFontFaceSet. But we should just try making Client inherit from CanMakeWeakPtr and then just use WeakPtr<Client> here instead. Maybe <rdar://problem/46598332> ? Created attachment 391930 [details]
Patch
Created attachment 391938 [details]
Patch
Comment on attachment 391938 [details] Patch Clearing flags on attachment: 391938 Committed r257639: <https://trac.webkit.org/changeset/257639> All reviewed patches have been landed. Closing bug. Thanks for the fix! |