Bug 196437 – Retain cycle between CSSFontSelector -> CSSFontFaceSet -> CSSFontFace -> CSSFontSelector

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

[Myles C. Maxfield]

Retain cycle between CSSFontSelector -> CSSFontFaceSet -> CSSFontFace -> CSSFontSelector

[Myles C. Maxfield]

Created attachment 366386 [details]
WIP

[Myles C. Maxfield]

Created attachment 366387 [details]
Dependency graph

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

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 ]

[Myles C. Maxfield]

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)

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Maybe <rdar://problem/46598332> ?

[Myles C. Maxfield]

<rdar://problem/46598332>

[Chris Dumez]

Created attachment 391930 [details]
Patch

[Chris Dumez]

Created attachment 391938 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391938 [details]
Patch

Clearing flags on attachment: 391938

Committed r257639: <https://trac.webkit.org/changeset/257639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Thanks for the fix!