Bug 208351 – CSSFontFace should not need its m_fontSelector data member

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

[Chris Dumez]

CSSFontFace should not need its m_fontSelector data member. Instead, we should store all the bits of information we need on the CSSFontFace.

See https://bugs.webkit.org/attachment.cgi?id=366386 for a first attempt at this.

[Myles C. Maxfield]

See https://bugs.webkit.org/show_bug.cgi?id=196437#c3

[Myles C. Maxfield]

For calls like source->load(m_fontSelector.get()), we just use the font selector for 2 purposes:

1. Aggregating all font load requests across a single turn of the event loop, to start them all at the same time, and
2. To use m_document->cachedResourceLoader() to actually perform the load

Regarding 1: I don't understand why we do it, and it seems counterproductive to me. For comparison, if we have a webpage with multiple images, we don't wait to start loading images until the next turn of the event loop. I have no idea why fonts are special this way. My guess is it has something to do with trying to avoid running JS in the middle of layout, but I don't think that would be possible even if we started the load immediately. I think we should try to get rid of this aggregation and just start loading fonts immediately. The reason why I think we should change is this aggregation has caused multiple correctness bugs (see calls to m_document->cachedResourceLoader().incrementRequestCount(font)), which wouldn't have existed if we had just used the loading infrastructure the way it was intended to be used.

Regarding 2: Loading stuff shouldn't require the document. XHR doesn't require a document. We should migrate font loading to use the same mechanism that XHR uses (ThreadableLoader I think). If we do this, we can just load stuff without any dependencies, and won't need to go through the font selector at all.

[Chris Lord]

Created attachment 419996 [details]
Patch

[Chris Lord]

Created attachment 420109 [details]
Patch

[Chris Lord]

Comment on attachment 420109 [details]
Patch

erk, I forgot about client removal... Need to think about this some more.

[Chris Lord]

Created attachment 420115 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 420115 [details]
Patch

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

Thank you!

> Source/WebCore/css/CSSFontFace.cpp:81
> +                    document->fontSelector().willBeginLoadingFontSoon(*cachedFont);

Pretty sure document can be null here. See line 77 above. I think this occurs when the page uses the CSS Font Loading API to construct a FontFace object from script.

It's a bit scary that this didn't cause any tests to fail. I suggest either 1) adding a test which fails without a null check here, or 2) if it is impossible create such a test, modifying the signature of this function to take a Document& instead of a Document*.

> Source/WebCore/css/CSSFontFace.cpp:591
> +                source->load(document());

I thought we were going to migrate loading code to use ThreadableLoader instead of CachedResourceLoader?

> Source/WebCore/css/CSSFontSelector.cpp:376
> +    font.removeClient(*this);

This is a bit of a scary place to be doing this. I understand why it's correct, but it seems a bit brittle; we may want to add additional messages between the CachedFont and the CSSFontSelector, and it would be surprising if these messages stopped working just because the font loaded. What's the downside to instead running this line in the destructor of the FontSelector? That's what CSSFontFaceSource does.

This is relevant because we're interested in adding streaming support for fonts, which may involve additional messages between the CachedFont and the CSSFontSelector.

> Source/WebCore/css/CSSFontSelector.h:113
> +    using CachedFontClient::fontLoaded;

Why is this necessary?

[Chris Lord]

(In reply to Myles C. Maxfield from comment #7)
> Comment on attachment 420115 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420115&action=review
> 
> Thank you!

Thanks for the fast feedback!

> > Source/WebCore/css/CSSFontFace.cpp:81
> > +                    document->fontSelector().willBeginLoadingFontSoon(*cachedFont);
> 
> Pretty sure document can be null here. See line 77 above. I think this
> occurs when the page uses the CSS Font Loading API to construct a FontFace
> object from script.
> 
> It's a bit scary that this didn't cause any tests to fail. I suggest either
> 1) adding a test which fails without a null check here, or 2) if it is
> impossible create such a test, modifying the signature of this function to
> take a Document& instead of a Document*.

There was already a null check here that I haven't removed on line 79. I assume this is still ok? (aside; I also missed this initially and had a bunch of checking and asserts that I realised weren't necessary)

> > Source/WebCore/css/CSSFontFace.cpp:591
> > +                source->load(document());
> 
> I thought we were going to migrate loading code to use ThreadableLoader
> instead of CachedResourceLoader?

Absolutely, I figured this would just be a smaller patch that we could get in in the meantime - ThreadableLoader is next on my list :)

> > Source/WebCore/css/CSSFontSelector.cpp:376
> > +    font.removeClient(*this);
> 
> This is a bit of a scary place to be doing this. I understand why it's
> correct, but it seems a bit brittle; we may want to add additional messages
> between the CachedFont and the CSSFontSelector, and it would be surprising
> if these messages stopped working just because the font loaded. What's the
> downside to instead running this line in the destructor of the FontSelector?
> That's what CSSFontFaceSource does.
> 
> This is relevant because we're interested in adding streaming support for
> fonts, which may involve additional messages between the CachedFont and the
> CSSFontSelector.

I agree, I don't like this particularly, but I assumed that we wouldn't want a FontSelector to possibly accumulate a bunch of references unnecessarily... But I suppose this is unique to every Document, so I probably needn't have been concerned. Can certainly be removed - it already removes the clients in the destructor also, so this line can just be removed. I suppose we'd also rename m_fontsLoadingSoon to just m_fonts in that case.

> > Source/WebCore/css/CSSFontSelector.h:113
> > +    using CachedFontClient::fontLoaded;
> 
> Why is this necessary?

Both CSSFontFace::client and CachedFontClient have a fontLoaded function, so this is necessary to avoid warnings about hiding overloaded virtual functions on clang.

[Chris Lord]

Created attachment 420132 [details]
Patch

[Darin Adler]

Comment on attachment 420132 [details]
Patch

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

> Source/WebCore/css/CSSFontSelector.cpp:250
> +        face->opportunisticallyStartFontDataURLLoading(document());

Can document() really be null?

[Chris Lord]

(In reply to Darin Adler from comment #10)
> Comment on attachment 420132 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420132&action=review
> 
> > Source/WebCore/css/CSSFontSelector.cpp:250
> > +        face->opportunisticallyStartFontDataURLLoading(document());
> 
> Can document() really be null?

I don't know under what circumstances this actually happens, but the assumption was made that this can happen prior to this patch, so I don't really want to change it.

I would expect that perhaps this can happen when destruction of a Document happens - Document calls clearDocument on the FontSelector before the FontSelector is destroyed. I'm not sure where fonts may still be in use after this point, but I don't think this is the right bug to start testing/changing that assumption.

[Chris Lord]

Created attachment 420281 [details]
Patch

[EWS]

Committed r272849: <https://commits.webkit.org/r272849>

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

[Radar WebKit Bug Importer]

<rdar://problem/74346302>

[Darin Adler]

Comment on attachment 420132 [details]
Patch

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

>>> Source/WebCore/css/CSSFontSelector.cpp:250
>>> +        face->opportunisticallyStartFontDataURLLoading(document());
>> 
>> Can document() really be null?
> 
> I don't know under what circumstances this actually happens, but the assumption was made that this can happen prior to this patch, so I don't really want to change it.
> 
> I would expect that perhaps this can happen when destruction of a Document happens - Document calls clearDocument on the FontSelector before the FontSelector is destroyed. I'm not sure where fonts may still be in use after this point, but I don't think this is the right bug to start testing/changing that assumption.

Makes sense.

The alternative I would suggest would be to check document for null, and return early in that case. Assuming it is not valuable to start data URL loading *after* the font selector is removed from a document.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 222121

[Chris Lord]

(In reply to WebKit Commit Bot from comment #16)
> Re-opened since this is blocked by bug 222121

My best guess for this is that not disconnecting the CachedFontClient after load means we're hitting multiple load signals and causing pending fonts to load early, breaking coalescing (the secondary thought is that if that's the case, I'm a little surprised this is a noticeable regression and also surprised that the coalescing in FontSelector is really what controls the coalescing of font download requests).

I'll look into this.

[Myles C. Maxfield]

(In reply to Chris Lord from comment #17)
> (In reply to WebKit Commit Bot from comment #16)
> > Re-opened since this is blocked by bug 222121
> 
> My best guess for this is that not disconnecting the CachedFontClient after
> load means we're hitting multiple load signals and causing pending fonts to
> load early, breaking coalescing (the secondary thought is that if that's the
> case, I'm a little surprised this is a noticeable regression and also
> surprised that the coalescing in FontSelector is really what controls the
> coalescing of font download requests).
> 
> I'll look into this.

Not only did it increase page load time, but we actually also got a very common crash which this patch seemed to cause:

>  1 com.apple.WebCore              0x019ac3de WebCore::CachedResourceClientWalker<WebCore::CachedFontClient>::next() + 0
   2 com.apple.WebCore              0x012b61a2 WebCore::CSSFontFaceSource::load(WebCore::Document*) + 0
   3 com.apple.WebCore              0x012adbba WebCore::CSSFontFace::pump(WebCore::ExternalResourceDownloadPolicy) + 0
   4 com.apple.WebCore              0x012ae3ff WebCore::CSSFontFace::font(WebCore::FontDescription const&, bool, bool, WebCore::ExternalResourceDownloadPolicy) + 0
   5 com.apple.WebCore              0x012e2737 WebCore::CSSFontAccessor::font(WebCore::ExternalResourceDownloadPolicy) const + 0
   6 com.apple.WebCore              0x01b6d824 WebCore::FontRanges::glyphDataForCharacter(int, WebCore::ExternalResourceDownloadPolicy) const + 0
   7 com.apple.WebCore              0x01b65747 WebCore::FontCascadeFonts::glyphDataForVariant(int, WebCore::FontCascadeDescription const&, WebCore::FontVariant, unsigned int) + 0
   8 com.apple.WebCore              0x01b60299 WebCore::FontCascadeFonts::glyphDataForCharacter(int, WebCore::FontCascadeDescription const&, WebCore::FontVariant) + 0
   9 com.apple.WebCore              0x01bb48f4 WebCore::WidthIterator::advance(unsigned int, WebCore::GlyphBuffer&) + 0

This crash started appearing on February 15, the same day this patch landed, and the backtrace indicates a codepath that this patch added (CSSFontFaceSource::load() calling CachedFont::requestLoad() which has the CachedResourceClientWalker loop)

[Myles C. Maxfield]

Ironically, we actually got a _second_ very common crash which this patch seemed to cause:

>  1 com.apple.WebCore              0x019b5a55 WebCore::CachedResource::removeClient(WebCore::CachedResourceClient&) + 0
   2 com.apple.WebCore              0x00062e94 WebCore::CSSFontSelector::clearDocument() + 0
   3 com.apple.WebCore              0x01403f9c WebCore::Document::removedLastRef() + 0
   4 com.apple.WebCore              0x00065e86 WebCore::HTMLCollection::~HTMLCollection() + 0
   5 com.apple.WebCore              0x013dc9bf WebCore::ClassCollection::~ClassCollection() + 0
   6 com.apple.JavaScriptCore       0x00cf8df2 void JSC::MarkedBlock::Handle::specializedSweep<true, (JSC::MarkedBlock::Handle::EmptyMode)0, (JSC::MarkedBlock::Handle::SweepMode)0, (JSC::MarkedBlock::Handle::SweepDestructionMode)1, (JSC::MarkedBlock::Handle::ScribbleMode)0, (JSC::MarkedBlock::Handle::NewlyAllocatedMode)1, (JSC::MarkedBlock::Handle::MarksMode)0, JSC::JSDestructibleObjectDestroyFunc>(JSC::FreeList*, JSC::MarkedBlock::Handle::EmptyMode, JSC::MarkedBlock::Handle::SweepMode, JSC::MarkedBlock::Handle::SweepDestructionMode, JSC::MarkedBlock::Handle::ScribbleMode, JSC::MarkedBlock::Handle::NewlyAllocatedMode, JSC::MarkedBlock::Handle::MarksMode, JSC::JSDestructibleObjectDestroyFunc const&) + 0

This crash also started appearing on February 15, the same day this patch landed, and the backtrace indicates a codepath that this patch added (CSSFontSelector::clearDocument() calling font->removeClient(*this))

[Chris Lord]

Created attachment 421190 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 421190 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Move the m_fontSelector member of CSSFontFace onto CSSFontFaceSource,
> +        the only place where it's actually required.

Please explain how this fixes the perf regression and the crashes in the previous version of the patch.

[Chris Lord]

(In reply to Simon Fraser (smfr) from comment #21)
> Comment on attachment 421190 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421190&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Move the m_fontSelector member of CSSFontFace onto CSSFontFaceSource,
> > +        the only place where it's actually required.
> 
> Please explain how this fixes the perf regression and the crashes in the
> previous version of the patch.

Judging from the test failures, it isn't there yet, but this is a much less invasive way of removing the class member.

Where as the previous patch introduced a new callback and added the FontSelector as a listener to every external font source, with those fonts tracked by a HashSet in the FontSelector; this patch only moves the FontSelector weak reference from CSSFontFace to CSSFontFaceSource (the only place where it is required).

The crash in the previous patch I believe was coming from either a stale CachedFont reference on FontSelector, or a stale FontSelector reference in the listeners list on CachedFont, so this patch not using that mechanism side-steps introducing it.

[Chris Lord]

Created attachment 421301 [details]
Patch

[Chris Lord]

Having fun and games with this patch. This ought to be much simpler and more straight-forward, but I'm getting test differences locally that I can't immediately explain... I guess some ordering of events has changed somewhere, but I've not explicitly changed it. It doesn't make sense...

[Chris Dumez]

Comment on attachment 421301 [details]
Patch

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

> Source/WebCore/css/CSSFontFace.cpp:84
> +            source = makeUnique<CSSFontFaceSource>(fontFace, item.resource(), *fontFaceElement);

Do we know for sure that fontFaceElement is actually non-null here? It is not obvious from looking at the code.

> Source/WebCore/css/CSSFontFaceSource.cpp:98
> +    , m_svgFontFaceElement(makeWeakPtr(&fontFace))

FYI, makeWeakPtr(fontFace) works and would be more efficient as it avoids an unnecessary null-check.

[Chris Lord]

(In reply to Chris Dumez from comment #25)
> Comment on attachment 421301 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421301&action=review
> 
> > Source/WebCore/css/CSSFontFace.cpp:84
> > +            source = makeUnique<CSSFontFaceSource>(fontFace, item.resource(), *fontFaceElement);
> 
> Do we know for sure that fontFaceElement is actually non-null here? It is
> not obvious from looking at the code.

Ugh, thank you, I've obviously been looking at this for too long.

> > Source/WebCore/css/CSSFontFaceSource.cpp:98
> > +    , m_svgFontFaceElement(makeWeakPtr(&fontFace))
> 
> FYI, makeWeakPtr(fontFace) works and would be more efficient as it avoids an
> unnecessary null-check.

Thanks.

[Chris Lord]

Created attachment 421398 [details]
Patch

[Chris Lord]

I don't think that failure on mac-wk2 (and maybe mac-debug-wk1?) has anything to do with this patch, I'll retry a bit later. I think this is ready for review as it is.

[Darin Adler]

Comment on attachment 421398 [details]
Patch

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

> Source/WebCore/css/CSSFontFace.cpp:85
> +            source = fontFaceElement ? makeUnique<CSSFontFaceSource>(fontFace, item.resource(), *fontFaceElement) :
> +                makeUnique<CSSFontFaceSource>(fontFace, item.resource());

We often put the ":" at the beginning of the second line rather than the end of the first line. Also, our coding style calls for braces when a single line statement becomes two lines, even though it’s still only a single statement.

> Source/WebCore/css/CSSFontFaceSource.h:93
> +    WeakPtr<CSSFontSelector> m_fontSelector { nullptr }; // For remote fonts, to orchestrate loading.

No need for nullptr, smart pointers get initialized to null without that.

> Source/WebCore/css/CSSFontFaceSource.h:97
> +    RefPtr<JSC::ArrayBufferView> m_immediateSource { nullptr };

Ditto.

> Source/WebCore/css/CSSFontFaceSource.h:100
> +    WeakPtr<SVGFontFaceElement> m_svgFontFaceElement { nullptr };

Ditto.

[Chris Lord]

Created attachment 421817 [details]
Patch

[Chris Lord]

For reference, performance tests were run privately and no regression was shown for this patch.

[EWS]

Committed r273650: <https://commits.webkit.org/r273650>

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

[Ryosuke Niwa]

Comment on attachment 421817 [details]
Patch

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

> Source/WebCore/css/CSSFontFace.cpp:590
> -                source->load(m_fontSelector.get());
> +                source->load(document());

It's very unnerving that this is getting a raw pointer of Document and passing it to load.
We should probably store Document in a RefPtr before calling load here.
FWIW, the previous incarnation of this patch which got rolled out seems to have introduced a UAF security bug.

[Darin Adler]

Comment on attachment 421817 [details]
Patch

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

>> Source/WebCore/css/CSSFontFace.cpp:590
>> +                source->load(document());
> 
> It's very unnerving that this is getting a raw pointer of Document and passing it to load.
> We should probably store Document in a RefPtr before calling load here.
> FWIW, the previous incarnation of this patch which got rolled out seems to have introduced a UAF security bug.

One way of writing that is this:

    source->load(makeRefPtr(document()).get());

But I am not sure how I would know when to do this makeRefPtr dance. Ryosuke, how should I think about when it’s needed?

[Ryosuke Niwa]

(In reply to Darin Adler from comment #34)
> Comment on attachment 421817 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421817&action=review
> 
> >> Source/WebCore/css/CSSFontFace.cpp:590
> >> +                source->load(document());
> > 
> > It's very unnerving that this is getting a raw pointer of Document and passing it to load.
> > We should probably store Document in a RefPtr before calling load here.
> > FWIW, the previous incarnation of this patch which got rolled out seems to have introduced a UAF security bug.
> 
> One way of writing that is this:
> 
>     source->load(makeRefPtr(document()).get());
> 
> But I am not sure how I would know when to do this makeRefPtr dance.
> Ryosuke, how should I think about when it’s needed?

The rule of thumb we've come up so far is this:
https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html

1. Every data member to a ref counted object must use either Ref, RefPtr, or WeakPtr.
   webkit.NoUncountedMemberChecker <https://releases.llvm.org/11.0.1/tools/clang/docs/analyzer/checkers.html#webkit-nouncountedmemberchecker>
2. Every ref counted base class, if it has a derived class, must define a virtual destructor.
   webkit.RefCntblBaseVirtualDtor <https://releases.llvm.org/11.0.1/tools/clang/docs/analyzer/checkers.html#webkit-refcntblbasevirtualdtor>
3. Every ref counted object passed to a non-trivial function as an argument (including "this" pointer)
   should be stored as a Ref or RefPtr in the caller’s local scope
   unless it's an argument to the caller itself by transitive property [1].
   alpha.webkit.UncountedCallArgsChecker <https://releases.llvm.org/11.0.1/tools/clang/docs/analyzer/checkers.html#alpha-webkit-uncountedcallargschecker>
4. Every ref counted object must be captured using Ref or RefPtr for lambda.