Bug 219283 – Make FontCache and FontCascadeCache thread-safe

• Component: Text
• Status: NEW

[Chris Lord]

For OffscreenCanvas and possibly GPUProcess, FontCache needs to be thread-safe. It looks like there is an amount of thread-safety for iOS, but that should be extended to all platforms.

[Radar WebKit Bug Importer]

<rdar://problem/71887176>

[Chris Lord]

Created attachment 422578 [details]
Patch

[Chris Lord]

Created attachment 422687 [details]
Patch

[Chris Lord]

Created attachment 422829 [details]
Patch

[Chris Lord]

Created attachment 423029 [details]
Patch

[Chris Lord]

Comment on attachment 423029 [details]
Patch

Just to note, I realise the ChangeLog message is inadequate, I just don't want to needlessly retrigger EWS tests. Pretend it says this:

"Remove the iOS-specific lock in FontCache and add a platform-independent lock that guards functions that use data that isn't safe for concurrent access. Make similar changes in some related classes so that FontCache becomes usable off the main thread."

Also, for what it's worth, I've tested using a simple, canvas-based, font loading/drawing performance test and there's no discernable difference after applying this patch. I would certainly make sure this gets broader testing than that before even considering committing it though.

[Darin Adler]

Comment on attachment 423029 [details]
Patch

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

I’m not sure this has the thread safety right. It’s good to do the locking more consistently but I don’t understand the thread safety implications of using AtomString in the internals here.

I need to understand more of the solid foundations of how this is thread-safe. I’d assume that more cross-thread copying would be needed to turn strings into a form that can be safely used across multiple threads guarded by a lock. If someone passes in a string it can only do ref/deref on one thread. The cross-thread copy could be used inside a lock on multiple threads.

None of this helps much with AtomString.

"More locking" is clear, but the rest of the story still seems not quite done.

> Source/WebCore/ChangeLog:9
> +        Make the FontCache lock not iOS-specific and use it in more places.

I think one thing we should be clear on is that the iOS FontCache locking was *only* to make it safe enough to use across the web thread and the main thread (both return true for isMainThread) in certain ways.

These changes now aim to make this safe to use from multiple threads, including non-web, non-main threads on iOS.

So this is not just about generalizing the iOS locking as if iOS was "ahead" and other platforms are catching up, it’s about adding a thorough locking strategy, which happens to allow us to remove the partial locking strategy that was only useful on iOS.

I know you wrote *better* change log text in the bug, but I wanted to clarify this situation and suggest even-better framing for the change.

> Source/WebCore/platform/graphics/FontCache.cpp:49
> -
>  

Remove one more blank line please.

> Source/WebCore/platform/graphics/FontCache.cpp:142
> +const String* FontCache::alternateFamilyName(const AtomString& familyName)

We should return just String or perhaps Optional<ASCIILiteral>. If it is String, there’s no need to work so hard to avoid reference count churn since we’re going to take a reference to the string anyway in the function that calls this function. Since String has a convenient null value, we don’t need to use a pointer or Optional just to add a null value. ASCIILiteral does need the Optional, and might be a good solution depending on how this actually is used by callers.

Changing the return type will clean up this family of functions a bit.

> Source/WebCore/platform/graphics/FontCache.cpp:144
> +    static NeverDestroyed<const String> arial("Arial", String::ConstructFromLiteral);

These global variables are only OK because these strings are only used internally, never returned from any function, and all use is guarded by the font lock. It’s frustrating that there is nothing that helps make this clear. If anyone called this function outside the lock we’d have a super-subtle bug!

Not 100% sure that it’s an important optimization to use ConstructFromLiteral instead of just using the "Arial"_s syntax to make an ASCIILiteral, also unsure how important the global variable optimization is.

If we use ASCIILiteral all those questions just go away and the need for global variables goes away too.

> Source/WebCore/platform/graphics/FontCache.cpp:153
> +    const String* platformSpecificAlternate = platformAlternateFamilyName(familyName);
> +    if (platformSpecificAlternate)
>          return platformSpecificAlternate;

Lets scope this local inside the if statement in cases like this:

    if (auto alternate = platformAlternateFamilyName(familyName); !alternate.isNull())
        return alternate;

> Source/WebCore/platform/graphics/FontCache.cpp:220
> +            const String* alternateName = alternateFamilyName(familyName);
> +            if (alternateName) {

Please scope this in the if statement:

    if (auto& alternateName = alternateFamilyName(familyName); !alternateName.isNull()) {

> Source/WebCore/platform/graphics/FontCache.cpp:221
> +                FontPlatformData* fontPlatformDataForAlternateName = getCachedFontPlatformData(fontDescription, *alternateName, fontFaceFeatures, fontFaceCapabilities, true);

I do not think it is thread-safe to turn a String into an AtomString here. I don’t understand why it’s OK for us to pass alternateName here. What prevents the global AtomString table from causing multiple threads to ref/deref the same object at the same time? Once we make an AtomString, unrelated code on another thread can also get that same string, and it’s then no longer safe to ref/deref it on more than one thread.

Now that I realize that alternateName is being passed to an AtomString constructor, I think it’s going to be expensive to keep looking up the names in the AtomString function every time this is called. Also, it seems the alternateFamilyName functions can just return ASCIILiteral if they are going to have to be turned into an AtomString every time anyway. Using a String that is not the AtomString has no performance advantage of value; just costs extra reference count churn for no good reason.

Or since this function *takes* an AtomString it seems like this is main-thread-only code. If it is, then why all the changes to the alternateFamilyName functions? That seems unrelated to the new locking and should be done in a separate patch with its own rationale.

Also, maybe use auto on this line.

> Source/WebCore/platform/graphics/FontCache.h:293
> +    static const String* alternateFamilyName(const AtomString&);
> +    static const String* platformAlternateFamilyName(const AtomString&);

These should just return String; no reason to use a pointer for this. (See comments above.)

> Source/WebCore/platform/graphics/FontCascadeDescription.cpp:107
> +    if (family1.isEmpty() || family2.isEmpty())
> +        return family1.isEmpty() && family2.isEmpty();

Why is this change needed? ASCIICaseInsensitiveHash::equal should already handle empty strings correctly. Is this about null strings perhaps?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1431
> +const String* FontCache::platformAlternateFamilyName(const AtomString& familyName)

Same comments as above apply.

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:695
> +const String* FontCache::platformAlternateFamilyName(const AtomString& familyName)

Same thing again.

[Chris Lord]

Created attachment 423162 [details]
Patch

[Chris Lord]

Created attachment 423175 [details]
Patch

[Chris Lord]

Created attachment 423177 [details]
Patch

[Chris Lord]

I've addressed some of the simpler comments from the review in comment #7, I'm still considering the string usage. I've not marked this patch for review.

[Chris Lord]

Created attachment 423318 [details]
Patch

[Chris Lord]

Created attachment 423321 [details]
Patch

[Chris Lord]

Comment on attachment 423321 [details]
Patch

I think this is at least ready for another review round and further discussion.

I followed Darin's advice re changing the API for FontCache::platformAlternateFamilyName and that did indeed simplify things along that path. I also took a look through FontCache a bit more carefully and made sure to use isolated copies of strings that it expects to keep and use from multiple threads.

I think we could all feel more comfortable with this patch if there were more opportunities to assert if something behaves incorrectly, but no ideas are immediately coming to mind.

Feedback very much appreciated.

[Darin Adler]

Comment on attachment 423321 [details]
Patch

Crashes on EWS for mac-debug-wk1 seem like actual bugs in this patch; please investigate. I may review before those are addressed, or I may wait.

[Darin Adler]

(In reply to Chris Lord from comment #14)
> I think we could all feel more comfortable with this patch if there were
> more opportunities to assert if something behaves incorrectly, but no ideas
> are immediately coming to mind.

A way to make us more confident would be to tighten the encapsulation. The smaller the class is that does isolatedCopy, the better. Could make a "class within a class" focused just on that issue with a minimal interface, then decorate with something more full featured for the publicly exposed class.

[Chris Lord]

Created attachment 423473 [details]
Patch

[Chris Lord]

Created attachment 423476 [details]
Patch

[Chris Lord]

Created attachment 423487 [details]
Patch

[Chris Lord]

Comment on attachment 423487 [details]
Patch

I've condensed the ChangeLog entry and annotated the changes made in each set of files for ease of reviewing/discussion.

[Darin Adler]

Comment on attachment 423487 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.cpp:187
>      // IE disregards "@" regardless of the orientatoin, so we follow the behavior.

While touching this, I think we should fix the spelling of "orientatoin".

> Source/WebCore/platform/graphics/FontCache.cpp:189
> +    const String& familyName = (passedFamilyName.isEmpty() || passedFamilyName[0] != '@') ?
> +        passedFamilyName : String(passedFamilyName.impl()->substring(1));

String(passedFamilyName.impl()->substring(1)) is the same thing as passedFamilyName.substring(1); we can write it the simpler way. String subscripting has built in length checking, and passing an index past the end of the string returns a null character. String::substring has a special case when passed 0 to just return the whole string. Combining those three things this can be written:

    String familyName = passedFamilyName.substring(passedFamilyName[0] == '@' ? 1 : 0);

I think I like that one-liner better.

> Source/WebCore/platform/graphics/FontCache.cpp:418
> +        key.families.uncheckedAppend(description.familyAt(i).string().isolatedCopy());

For performance, it’s a shame that we make all the isolated copies just to do a cache lookup. It would be better for performance to structure things so the isolated copies are only made after failing to find a cached font in FontCache::retrieveOrAddCachedFonts.

[Darin Adler]

Comment on attachment 423487 [details]
Patch

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

Looking good overall.

Strategy seems great! Details may not quite be right yet. Need better explanation of what we did and why for me to form an opinion.

I’m a little confused about each reference counted object. Why does each need a thread-safe reference count, and why is it OK to only make the reference count thread-safe, no need to do make other operations on the object guarded, atomic, or thread-safe.

What was the strategy for which functions to hold locks in? What’s the rule of thumb we are following?

Please also consider the "less isolated copying" strategy I mention below.

>> Source/WebCore/platform/graphics/FontCache.cpp:418
>> +        key.families.uncheckedAppend(description.familyAt(i).string().isolatedCopy());
> 
> For performance, it’s a shame that we make all the isolated copies just to do a cache lookup. It would be better for performance to structure things so the isolated copies are only made after failing to find a cached font in FontCache::retrieveOrAddCachedFonts.

We should consider this. I don’t think that a key should intrinsically always be an isolated copy. Instead we should add an "isolatedCopy" function or an "isolate" function that works in place to the class and use it at the right time.

This should help *significantly* with performance.

> Source/WebCore/platform/graphics/FontCache.cpp:503
> +    auto locker = holdLock(m_lock);

Seems obviously unsafe to guard a single *global* clients set with a lock that is per-FontCache instance. We’d need to change gClients to be a data member, perhaps?

> Source/WebCore/platform/graphics/FontCascadeDescription.cpp:107
> +    if (family1.isNull() || family2.isNull())
> +        return false;

Why exactly do we need this change?

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:181
> +    // Check again with lock held. This allows us to have a lock-less fast path in the common case that
> +    // this fallback range has already been realized without introducing a race condition.

The general rule of thumb is that double checked lock patterns don’t work. I’m not sure why it works here.

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:213
> +    return m_realizedFallbackRanges.last();

Does not seem safe to return a FontRanges object here given that it contains a Ref<FontAccessor>, which uses a non-thread-safe reference count.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:642
>      static NeverDestroyed<String> arabic(MAKE_STATIC_STRING_IMPL("Arabic"));
>      static NeverDestroyed<String> pashto(MAKE_STATIC_STRING_IMPL("Pashto"));
>      static NeverDestroyed<String> urdu(MAKE_STATIC_STRING_IMPL("Urdu"));
>      static String* matchWords[3] = { &arabic.get(), &pashto.get(), &urdu.get() };

These globals are dangerous. Two different threads can race trying to initialize the same globals. These need to be something that doesn’t need initialization, like using constexpr.

There is no need to use String here. There is a containsIgnoringASCIICase function that takes a StringView; these can just be const char* or ASCIILiteral, and we can call StringView(family).containsIgnoringASCIICase(matchWord). All that strange String* stuff is not needed.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:323
>          return *font;

Not new: This should be font.releaseNonNull(), not *font, to avoid reference count churn.

[Chris Lord]

Created attachment 423625 [details]
Patch

[Chris Lord]

Created attachment 423627 [details]
Patch

[Chris Lord]

Created attachment 423629 [details]
Patch

[Chris Lord]

Created attachment 423631 [details]
Patch

[Chris Lord]

(In reply to Darin Adler from comment #22)
> Comment on attachment 423487 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423487&action=review
> 
> Looking good overall.
> 
> Strategy seems great! Details may not quite be right yet. Need better
> explanation of what we did and why for me to form an opinion.
> 
> I’m a little confused about each reference counted object. Why does each
> need a thread-safe reference count, and why is it OK to only make the
> reference count thread-safe, no need to do make other operations on the
> object guarded, atomic, or thread-safe.
> 
> What was the strategy for which functions to hold locks in? What’s the rule
> of thumb we are following?

Which method I've chosen has generally been guided by an object's expected use and the data it holds - so FontCache, being a main-thread singleton cache gets locking, but things like Font-related objects, which will most of the time be taken temporarily and used in hot-paths, use locking where necessary, and where possible I've avoided it to maintain performance. I don't think I've used any atomic members explicitly, only via ThreadSafeRefCounted - for the classes that have just become ThreadSafeRefCounted and nothing else changed, it's usually because they're really just immutable data structures and thus thread-safe after creation.

I have a local branch with OffscreenCanvas text enabled which also helps a lot in determining problem areas. The patch as it is allows text to work on OffscreenCanvas without any obvious flaws, but I don't expect that it's perfect just yet. This will likely take some iteration, but I'm eager to get in as many of the big, non-breaking changes as possible to minimise the amount of time maintaining out-of-tree patches. I'm hoping with this as a start that other interested stakeholders (both in OffscreenCanvas and a thread-safe FontCache) might help with feedback, testing and further work.

> Please also consider the "less isolated copying" strategy I mention below.
> 
> >> Source/WebCore/platform/graphics/FontCache.cpp:418
> >> +        key.families.uncheckedAppend(description.familyAt(i).string().isolatedCopy());
> > 
> > For performance, it’s a shame that we make all the isolated copies just to do a cache lookup. It would be better for performance to structure things so the isolated copies are only made after failing to find a cached font in FontCache::retrieveOrAddCachedFonts.
> 
> We should consider this. I don’t think that a key should intrinsically
> always be an isolated copy. Instead we should add an "isolatedCopy" function
> or an "isolate" function that works in place to the class and use it at the
> right time.
> 
> This should help *significantly* with performance.

I took your advice, and of course you're right - where as that version of the patch measured about a 2.2% performance decrease in my local font performance test, this change made that a 1.6% deficit, and then changes to FontCascadeFonts with regards to locking (now lock-free in the most common uses) made up for the rest. I still expect this patch to be some kind of performance regression, but I'll ask Per Arne to run tests once we've gotten to a place we're happy with with respect to the code.

> > Source/WebCore/platform/graphics/FontCache.cpp:503
> > +    auto locker = holdLock(m_lock);
> 
> Seems obviously unsafe to guard a single *global* clients set with a lock
> that is per-FontCache instance. We’d need to change gClients to be a data
> member, perhaps?

You're right, I'm not sure how I managed to miss this - I've introduced a static lock associated with the platform data cache that guards the global variables within this file. I did consider making them member variables, but I could see somewhat why they weren't, and where possible, I'd like to favour the smaller change.

> > Source/WebCore/platform/graphics/FontCascadeDescription.cpp:107
> > +    if (family1.isNull() || family2.isNull())
> > +        return false;
> 
> Why exactly do we need this change?

Changing from AtomString to String, ASCIICaseInsensitiveHash::equal doesn't handle null strings. I've returned false here to match the very similar situation found in FontPlatformDataCacheKey::operator== in FontCache.cpp.

> > Source/WebCore/platform/graphics/FontCascadeFonts.cpp:181
> > +    // Check again with lock held. This allows us to have a lock-less fast path in the common case that
> > +    // this fallback range has already been realized without introducing a race condition.
> 
> The general rule of thumb is that double checked lock patterns don’t work.
> I’m not sure why it works here.

You're right and this has disappeared - the current code is what I was really intending for. The general thought here though is that for the lifetime of FontCascadeFonts, the array being checked only ever grows in size... But as I write this, I realise of course that this is a Vector, not an array, and that it growing in size may invalidate previous elements, so this needs to change. I think given that this is commonly expected to be just size==1, this could be some kind of embedded linked list or something along those lines.

> > Source/WebCore/platform/graphics/FontCascadeFonts.cpp:213
> > +    return m_realizedFallbackRanges.last();
> 
> Does not seem safe to return a FontRanges object here given that it contains
> a Ref<FontAccessor>, which uses a non-thread-safe reference count.

Again, you're right, this isn't safe - it turns out that only three functions were ever called on the FontRanges object and they were all short-lived, so instead of exposing that, I've added accessors for those functions to this class so that they can be accessed while the FontCascadeFonts is guaranteed to be alive.

> > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:642
> >      static NeverDestroyed<String> arabic(MAKE_STATIC_STRING_IMPL("Arabic"));
> >      static NeverDestroyed<String> pashto(MAKE_STATIC_STRING_IMPL("Pashto"));
> >      static NeverDestroyed<String> urdu(MAKE_STATIC_STRING_IMPL("Urdu"));
> >      static String* matchWords[3] = { &arabic.get(), &pashto.get(), &urdu.get() };
> 
> These globals are dangerous. Two different threads can race trying to
> initialize the same globals. These need to be something that doesn’t need
> initialization, like using constexpr.
> 
> There is no need to use String here. There is a containsIgnoringASCIICase
> function that takes a StringView; these can just be const char* or
> ASCIILiteral, and we can call
> StringView(family).containsIgnoringASCIICase(matchWord). All that strange
> String* stuff is not needed.

I've made this change, but is this true? I thought the modern-ish C++ standard guaranteed that static variable initialisation was thread-safe?

> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:323
> >          return *font;
> 
> Not new: This should be font.releaseNonNull(), not *font, to avoid reference
> count churn.

Change made.

[Chris Lord]

Comment on attachment 423631 [details]
Patch

I don't expect this review to be granted necessarily given my last comment (or indeed given any other issues that the patch may contain), but I'd still very much appreciate any more feedback you have.

[Darin Adler]

(In reply to Chris Lord from comment #27)
> ASCIICaseInsensitiveHash::equal doesn't
> handle null strings.

Very strange choice on the part of the person who wrote that. (Me, probably.)

> I've returned false here to match the very similar
> situation found in FontPlatformDataCacheKey::operator== in FontCache.cpp.

I would have suggested returning true if both are null. A semantic where something is not equal to itself might be OK, but I think it's unnecessarily confusing to program with.

> I thought the modern-ish C++
> standard guaranteed that static variable initialisation was thread-safe?

C++ does indeed guarantee this and has for a very long time, but not in the WebKit project. At least on the Apple platforms, WebKit is compiled with thread safety for static variable initialization turned off, for performance reasons. Some day we might be able to solve the performance problem some other way and reverse that decision.

[Darin Adler]

(In reply to Darin Adler from comment #29)
> (In reply to Chris Lord from comment #27)
> > I've returned false here to match the very similar
> > situation found in FontPlatformDataCacheKey::operator== in FontCache.cpp.
> 
> I would have suggested returning true if both are null. A semantic where
> something is not equal to itself might be OK, but I think it's unnecessarily
> confusing to program with.

Ah, seems you missed a subtle point about FontPlatformDataCacheKey::operator== that makes it different from what we did here. That function returns false when both are null *after* a checking if the two impl pointers are equal. If they are equal, it returns true. When both strings are null, both have a null impl pointer, so that function will return true, not false, in that case.

[Darin Adler]

Comment on attachment 423631 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.cpp:69
> +        , m_family(family.isolatedCopy())

I think we should do something similar to what we did for FontCascadeCacheKey, and isolate when storing the key rather than isolating when we create the key for lookup. The change would be in FontCache::getCachedFontPlatformData.

> Source/WebCore/platform/graphics/FontCascadeDescription.cpp:117
>      return ASCIICaseInsensitiveHash::hash(family);

This function will crash when passed a null string. I don’t fully understand why we need null checks in familyNamesAreEqual and not here.

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:122
> +    auto locker = holdLockIf(m_lock, !m_realizedFallbackRanges.size());

Could use isEmpty() here instead of !size().

Also, probably needs a "why" comment explaining why we definitely need a lock when the realized fallback ranges vector is not empty, but definitely don’t need one if it is empty. For example, can another thread change m_realizedFallbackRanges?

This feels like a "bad code smell" and seems unlikely to be 100% correct, but it might just be something too subtle for me to figure out without a comment.

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:214
> +    auto locker = holdLockIf(m_lock, m_realizedFallbackRanges.size() <= index);

May need a "why" comment?

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:216
> +    auto& ranges = realizeFallbackRangesAt(description, index);
> +    return ranges.isNull();

Better as a one-liner?

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:221
> +    auto locker = holdLockIf(m_lock, m_realizedFallbackRanges.size() <= index);

May need a "why" comment?

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:223
> +    auto& ranges = realizeFallbackRangesAt(description, index);
> +    return ranges.fontForCharacter(character);

Better as a one-liner?

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:228
> +    auto locker = holdLockIf(m_lock, !m_realizedFallbackRanges.size());

May need a "why" comment?

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:230
> +    auto& ranges = realizeFallbackRangesAt(description, 0);
> +    return ranges.fontForFirstRange();

Better as a one-liner?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:641
> +    for (auto& matchWord : matchWords) {

Could use "auto" instead of "auto&".

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:642
> +        if (equalIgnoringASCIICase(familyMatcher, StringView(matchWord)))

Probably don’t need an explicit cast to StringView here.

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:376
> +    static NeverDestroyed<String> fallbackFontName;

Seems like an unsafe use of a global.

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:393
> +    static NeverDestroyed<const String> fallbackFonts[] = {
> +        String("Times New Roman", String::ConstructFromLiteral),
> +        String("Microsoft Sans Serif", String::ConstructFromLiteral),
> +        String("Tahoma", String::ConstructFromLiteral),
> +        String("Lucida Sans Unicode", String::ConstructFromLiteral),
> +        String("Arial", String::ConstructFromLiteral)
>      };

Seems like an unsafe use of a global.

[Chris Lord]

Created attachment 423707 [details]
Patch

[Chris Lord]

I've added some asserts that I expect may be hit in FontCascadeDescription (which is why I added the null checks at all) - I'll use the EWS results to fix the call-site and leave the asserts there.

[Chris Lord]

All comments addressed, except...

(In reply to Darin Adler from comment #31)
> > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:642
> > +        if (equalIgnoringASCIICase(familyMatcher, StringView(matchWord)))
> 
> Probably don’t need an explicit cast to StringView here.

I think we do? The String(ASCIILiteral) constructor is private and the StringView(ASCIILiteral) constructor is declared explicit. Unless I'm misunderstanding how this works of course, which is highlight likely.

[Chris Lord]

Created attachment 423711 [details]
Patch

[Chris Lord]

Created attachment 423716 [details]
Patch

[Chris Lord]

Comment on attachment 423716 [details]
Patch

Ok, disregard my previous comment - the null check is required and I've made it more explicit and corrected the null string misapprehension in familyNamesAreEqual. I think we're ready for another round again :)

With regards to the locking in FontCascadeFonts, I have added a comment, but I'm not married to this particular way of doing it if we can think of something better. It does seem this part of the code has an impact on performance; locking unconditionally here has a noticeable effect, so it would be good to come up with a solution that could avoid locking in the common case.

[Darin Adler]

(In reply to Chris Lord from comment #34)
> The String(ASCIILiteral) constructor is private

No, it’s not private. WTF_EXPORT_PRIVATE means that it is exported but not as part of "API" for use outside WebKit. Not related to the similarly named concept of private members in a class.

> the
> StringView(ASCIILiteral) constructor is declared explicit

Not insisting on doing it in this patch: We should fix this. The ASCIILiteral -> StringView construction is inexpensive (just computes a string length) and lossless; it should not be explicit.

[Chris Lord]

(In reply to Darin Adler from comment #38)
> (In reply to Chris Lord from comment #34)
> > The String(ASCIILiteral) constructor is private
> 
> No, it’s not private. WTF_EXPORT_PRIVATE means that it is exported but not
> as part of "API" for use outside WebKit. Not related to the similarly named
> concept of private members in a class.
> 
> > the
> > StringView(ASCIILiteral) constructor is declared explicit
> 
> Not insisting on doing it in this patch: We should fix this. The
> ASCIILiteral -> StringView construction is inexpensive (just computes a
> string length) and lossless; it should not be explicit.

Ah, I see - perhaps it was made explicit to disambiguate between the String and StringView constructors in that case?

[Darin Adler]

(In reply to Chris Lord from comment #39)
> Ah, I see - perhaps it was made explicit to disambiguate between the String
> and StringView constructors in that case?

Maybe, but seems unlikely. The same would come up with many other String/StringView constructors with the same argument type. And the String constructor is the one that does memory allocation, so it’s the less-preferred one when there is ambiguity, so if this is an effect to avoid ambiguity, it's an ill advised one.

[Chris Lord]

Created attachment 423999 [details]
Patch

[Chris Lord]

(In reply to Chris Lord from comment #41)
> Created attachment 423999 [details]
> Patch

This contains just one small revision in FontCacheFreeType.cpp where we were accessing the global font family name atoms (which, correctly, asserts when done on the wrong thread).

[Chris Lord]

Comment on attachment 423999 [details]
Patch

I'm removing the review request - FontCascadeFonts still holds a reference to CSSFontSelector in this patch and that class is definitely not safe to use away from its creation thread. I need to address this before we carry on.

[Darin Adler]

Comment on attachment 423999 [details]
Patch

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

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:341
> +    if (family == familyNamesData->at(static_cast<unsigned>(FamilyNamesIndex::StandardFamily)) || familyNamesData->at(static_cast<unsigned>(FamilyNamesIndex::SerifFamily)))

This is missing a "family ==" after "||". Makes me wonder what our test coverage is.

[Chris Lord]

Created attachment 424127 [details]
Patch

[Chris Lord]

Created attachment 424128 [details]
Patch

[Chris Lord]

Created attachment 424129 [details]
Patch

[Chris Dumez]

Comment on attachment 424129 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Make FontCache and FontCascadeCache thread-safe

This changelog does not explain why we're doing this.
This has a perf cost so we'd need a good reason.

> Source/WebCore/dom/ActiveDOMObject.cpp:79
> +    ASSERT(canCurrentThreadAccessThreadLocalData(m_creationThread));

This change looks a bit scary. ActiveDOMObject should always get destroyed on their context thread. I don't think making this change is OK and it is likely covering up a bug.

> Source/WebCore/platform/graphics/FontCache.cpp:93
> +    void isolate() { m_family = m_family.isolatedCopy(); }

WebKit has a pattern for this. The function is called isolatedCopy() and it returns a new Object. It is important to follow the pattern so that CrossThreadCopier works as expected with all types.

> Source/WebCore/platform/graphics/FontCache.cpp:429
> +static void isolateFontCascadeCacheKey(FontCascadeCacheKey& key)

Ditto.

> Source/WebCore/platform/graphics/FontCascadeFonts.h:75
> +    bool fallbackRangesAreNull(const FontCascadeDescription&, unsigned);

Can this function be const?

> Source/WebCore/platform/graphics/FontCascadeFonts.h:76
> +    const Font* fontForCharacter(const FontCascadeDescription&, unsigned fallbackRangesIndex, UChar32);

Can this function be const?

> Source/WebCore/platform/graphics/FontCascadeFonts.h:77
> +    const Font& fontForFirstRange(const FontCascadeDescription&);

Can this function be const?

> Source/WebCore/platform/graphics/FontCascadeFonts.h:85
> +    const Font* findBestFallbackFont(const FontCascadeDescription&, UChar32);

Can this function be const?

> Source/WebCore/platform/graphics/FontCascadeFonts.h:89
> +    WEBCORE_EXPORT const FontRanges& realizeFallbackRangesAt(const FontCascadeDescription&, unsigned fallbackIndex);

Can this function be const?

> Source/WebCore/platform/graphics/FontCascadeFonts.h:91
> +    StdList<FontRanges> m_realizedFallbackRanges;

Why replace the Vector with a StdList?

> Source/WebCore/platform/graphics/FontCascadeFonts.h:140
> +    auto locker = holdLock(m_lock);

This does not look thread-safe. The locker should likely be at the beginning of the function, before you check if m_cachedPrimaryFont is null. Double-checked locking is not safe in C++.

[Chris Lord]

(In reply to Chris Dumez from comment #48)
> Comment on attachment 424129 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424129&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Make FontCache and FontCascadeCache thread-safe
> 
> This changelog does not explain why we're doing this.
> This has a perf cost so we'd need a good reason.

I've added a note that this is for OffscreenCanvas (I guess this could also be for GPUProcess too? I don't know enough about that to comment though, but someone mentioned it in the past)

> > Source/WebCore/dom/ActiveDOMObject.cpp:79
> > +    ASSERT(canCurrentThreadAccessThreadLocalData(m_creationThread));
> 
> This change looks a bit scary. ActiveDOMObject should always get destroyed
> on their context thread. I don't think making this change is OK and it is
> likely covering up a bug.

It is, but I think I've realised an alternative here that might work ok - the problem here is that FontCascadeFonts holds a reference to CSSFontSelector, and once text functions are enabled on OffscreenCanvas, it's possible for it to end up unrefing after Document/WorkerGlobalScope have been destroyed on a different thread.

> > Source/WebCore/platform/graphics/FontCache.cpp:93
> > +    void isolate() { m_family = m_family.isolatedCopy(); }
> 
> WebKit has a pattern for this. The function is called isolatedCopy() and it
> returns a new Object. It is important to follow the pattern so that
> CrossThreadCopier works as expected with all types.
> 
> > Source/WebCore/platform/graphics/FontCache.cpp:429
> > +static void isolateFontCascadeCacheKey(FontCascadeCacheKey& key)
> 
> Ditto.

Ok, will change.

> > Source/WebCore/platform/graphics/FontCascadeFonts.h:75
> > +    bool fallbackRangesAreNull(const FontCascadeDescription&, unsigned);
> 
> Can this function be const?
> 
> > Source/WebCore/platform/graphics/FontCascadeFonts.h:76
> > +    const Font* fontForCharacter(const FontCascadeDescription&, unsigned fallbackRangesIndex, UChar32);
> 
> Can this function be const?
> 
> > Source/WebCore/platform/graphics/FontCascadeFonts.h:77
> > +    const Font& fontForFirstRange(const FontCascadeDescription&);
> 
> Can this function be const?
> 
> > Source/WebCore/platform/graphics/FontCascadeFonts.h:85
> > +    const Font* findBestFallbackFont(const FontCascadeDescription&, UChar32);
> 
> Can this function be const?
> 
> > Source/WebCore/platform/graphics/FontCascadeFonts.h:89
> > +    WEBCORE_EXPORT const FontRanges& realizeFallbackRangesAt(const FontCascadeDescription&, unsigned fallbackIndex);
> 
> Can this function be const?

None of these functions can be const I don't think because they all call realizeFallbackRangesAt.

> > Source/WebCore/platform/graphics/FontCascadeFonts.h:91
> > +    StdList<FontRanges> m_realizedFallbackRanges;
> 
> Why replace the Vector with a StdList?

This is to enable access of earlier nodes in the list without locking - the comment at the top of realizeFallbackRangesAt tries to explain this.

> > Source/WebCore/platform/graphics/FontCascadeFonts.h:140
> > +    auto locker = holdLock(m_lock);
> 
> This does not look thread-safe. The locker should likely be at the beginning
> of the function, before you check if m_cachedPrimaryFont is null.
> Double-checked locking is not safe in C++.

You're right, I'll replace this with std::call_once.

[Chris Lord]

(In reply to Chris Lord from comment #49)
> (In reply to Chris Dumez from comment #48)
> > > Source/WebCore/platform/graphics/FontCascadeFonts.h:140
> > > +    auto locker = holdLock(m_lock);
> > 
> > This does not look thread-safe. The locker should likely be at the beginning
> > of the function, before you check if m_cachedPrimaryFont is null.
> > Double-checked locking is not safe in C++.
> 
> You're right, I'll replace this with std::call_once.

I misunderstood how this part of the code is expected to work, call_once can't work here. We really don't want to incur the lock penalty here if possible though, I'm working on this...

[Chris Lord]

Created attachment 424154 [details]
Patch

[Chris Lord]

Comment on attachment 424154 [details]
Patch

I would prefer to wait until this is green, but I'm taking a day off tomorrow and I'm hoping that even if it isn't green, that whatever fails is something minor... All review/feedback/comment appreciated!

[Chris Lord]

Comment on attachment 424154 [details]
Patch

After some discussion, we're taking a different route with this - I'm leaving this bug open for now in case GPUProcess needs this and this patch is helpful.