Bug 223033 – cachedCGColor() and nsColor() are not thread-safe

• Component: Platform
• Status: RESOLVED FIXED

[Sam Weinig]

cachedCGColor() is not thread-safe, and will break if used of OffscreenCanvas or multi-queue display list replaying.

[Radar WebKit Bug Importer]

<rdar://problem/75317764>

[Said Abou-Hallawa]

Created attachment 426285 [details]
Patch

[Chris Dumez]

Comment on attachment 426285 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:145
> +    LockHolder lockHolder(&cachedCGColorLock);

You static initializations above are not thread-safe either as far as I know.

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 426285 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426285&action=review
> 
> > Source/WebCore/platform/graphics/cg/ColorCG.cpp:145
> > +    LockHolder lockHolder(&cachedCGColorLock);
> 
> You static initializations above are not thread-safe either as far as I know.

*Your*

[Yusuke Suzuki]

Comment on attachment 426285 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:145
>>> +    LockHolder lockHolder(&cachedCGColorLock);
>> 
>> You static initializations above are not thread-safe either as far as I know.
> 
> *Your*

Is it correct? Lock has constexpr constructor, and compiler initializes the content in __DATA section.

[Chris Dumez]

(In reply to Yusuke Suzuki from comment #5)
> Comment on attachment 426285 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426285&action=review
> 
> >>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:145
> >>> +    LockHolder lockHolder(&cachedCGColorLock);
> >> 
> >> You static initializations above are not thread-safe either as far as I know.
> > 
> > *Your*
> 
> Is it correct? Lock has constexpr constructor, and compiler initializes the
> content in __DATA section.

To clarify, I was talking about these kind of static initialization that happen before your lock, in the same function:
```
static NeverDestroyed<RetainPtr<CGColorRef>> transparentCGColor = createCGColor(color);
            return transparentCGColor.get().get();
```

[Yusuke Suzuki]

Comment on attachment 426285 [details]
Patch

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

>>>>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:145
>>>>> +    LockHolder lockHolder(&cachedCGColorLock);
>>>> 
>>>> You static initializations above are not thread-safe either as far as I know.
>>> 
>>> *Your*
>> 
>> Is it correct? Lock has constexpr constructor, and compiler initializes the content in __DATA section.
> 
> To clarify, I was talking about these kind of static initialization that happen before your lock, in the same function:
> ```
> static NeverDestroyed<RetainPtr<CGColorRef>> transparentCGColor = createCGColor(color);
>             return transparentCGColor.get().get();
> ```

Right. Using std::call_once & LazyNeverDestroyed<> would be better here.

[Said Abou-Hallawa]

Created attachment 426288 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 426285 [details]
Patch

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

>>>>>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:145
>>>>>> +    LockHolder lockHolder(&cachedCGColorLock);
>>>>> 
>>>>> You static initializations above are not thread-safe either as far as I know.
>>>> 
>>>> *Your*
>>> 
>>> Is it correct? Lock has constexpr constructor, and compiler initializes the content in __DATA section.
>> 
>> To clarify, I was talking about these kind of static initialization that happen before your lock, in the same function:
>> ```
>> static NeverDestroyed<RetainPtr<CGColorRef>> transparentCGColor = createCGColor(color);
>>             return transparentCGColor.get().get();
>> ```
> 
> Right. Using std::call_once & LazyNeverDestroyed<> would be better here.

You are right. The call to createCGColor() in the statement:

    static NeverDestroyed<RetainPtr<CGColorRef>> transparentCGColor = createCGColor(color);

happens only once when this code is first hit. Subsequent calls will use the static cached value 'transparentCGColor'.

[Chris Dumez]

Do we need to perf-test this (MotionMark for e.g.)? Seems like this might be a hot function although I am not familiar with this code.

[Sam Weinig]

You may also want to do the same for nsColor() in ColorMac.mm, not sure if that is used from a background thread or not, but it unlikely to be hot and has the same problem.

[Sam Weinig]

It would also be good if we could avoid the lock in the case of the transparent/white/black cases, as those represent a large percentage of the calls (last time I measured). 

Using std::call_once / LazyNeverDestroyed would allow the lock to be moved below the fast case.

[Chris Dumez]

(In reply to Sam Weinig from comment #12)
> It would also be good if we could avoid the lock in the case of the
> transparent/white/black cases, as those represent a large percentage of the
> calls (last time I measured). 
> 
> Using std::call_once / LazyNeverDestroyed would allow the lock to be moved
> below the fast case.

I Agree. That is also was Yusuke suggested earlier.

[Darin Adler]

Comment on attachment 426288 [details]
Patch

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

Is the additional slowness from a lock in this function acceptable?

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:128
> +    static Lock cachedCGColorLock;
> +    LockHolder lockHolder(&cachedCGColorLock);

I suggest locking *after* the fast path special cases for transparentBlack, black, and white. The lock is not needed for that code.

[Darin Adler]

Oh, I see that you already discussed all of this above.

[Darin Adler]

Comment on attachment 426288 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:128
>> +    LockHolder lockHolder(&cachedCGColorLock);
> 
> I suggest locking *after* the fast path special cases for transparentBlack, black, and white. The lock is not needed for that code.

I guess I’m wrong about that since we don’t use thread-safe statics. Even those need the locking the way this is currently written. That’s unfortunate and fixable if we needed to fix it.

[Said Abou-Hallawa]

Comment on attachment 426288 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:128
>>> +    LockHolder lockHolder(&cachedCGColorLock);
>> 
>> I suggest locking *after* the fast path special cases for transparentBlack, black, and white. The lock is not needed for that code.
> 
> I guess I’m wrong about that since we don’t use thread-safe statics. Even those need the locking the way this is currently written. That’s unfortunate and fixable if we needed to fix it.

I do not know what "thread-safe statics" means. Also in this page https://en.cppreference.com/w/cpp/thread/call_once, I read this statement 

"Initialization of function-local statics is guaranteed to occur only once even when called from multiple threads, and may be more efficient than the equivalent code using std::call_once."

Does not this mean this statement will be safely executed once?

    static NeverDestroyed<RetainPtr<CGColorRef>> whiteCGColor = createCGColor(color);

[Chris Dumez]

(In reply to Said Abou-Hallawa from comment #17)
> Comment on attachment 426288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426288&action=review
> 
> >>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:128
> >>> +    LockHolder lockHolder(&cachedCGColorLock);
> >> 
> >> I suggest locking *after* the fast path special cases for transparentBlack, black, and white. The lock is not needed for that code.
> > 
> > I guess I’m wrong about that since we don’t use thread-safe statics. Even those need the locking the way this is currently written. That’s unfortunate and fixable if we needed to fix it.
> 
> I do not know what "thread-safe statics" means. Also in this page
> https://en.cppreference.com/w/cpp/thread/call_once, I read this statement 
> 
> "Initialization of function-local statics is guaranteed to occur only once
> even when called from multiple threads, and may be more efficient than the
> equivalent code using std::call_once."
> 
> Does not this mean this statement will be safely executed once?
> 
>     static NeverDestroyed<RetainPtr<CGColorRef>> whiteCGColor =
> createCGColor(color);

No because we compile with -fno-threadsafe-statics for perf reasons.

[Chris Dumez]

(In reply to Chris Dumez from comment #18)
> (In reply to Said Abou-Hallawa from comment #17)
> > Comment on attachment 426288 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=426288&action=review
> > 
> > >>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:128
> > >>> +    LockHolder lockHolder(&cachedCGColorLock);
> > >> 
> > >> I suggest locking *after* the fast path special cases for transparentBlack, black, and white. The lock is not needed for that code.
> > > 
> > > I guess I’m wrong about that since we don’t use thread-safe statics. Even those need the locking the way this is currently written. That’s unfortunate and fixable if we needed to fix it.
> > 
> > I do not know what "thread-safe statics" means. Also in this page
> > https://en.cppreference.com/w/cpp/thread/call_once, I read this statement 
> > 
> > "Initialization of function-local statics is guaranteed to occur only once
> > even when called from multiple threads, and may be more efficient than the
> > equivalent code using std::call_once."
> > 
> > Does not this mean this statement will be safely executed once?
> > 
> >     static NeverDestroyed<RetainPtr<CGColorRef>> whiteCGColor =
> > createCGColor(color);
> 
> No because we compile with -fno-threadsafe-statics for perf reasons.

AFAIK, the recommended pattern in WebKit for local statics when multiple threads are involved is LazyNeverDestroyed + std::call_once.

[Darin Adler]

(In reply to Said Abou-Hallawa from comment #17)
> I do not know what "thread-safe statics" means.

C++ normally guarantees that initialization of static local variables <https://en.cppreference.com/w/cpp/language/storage_duration#Static_local_variables> happens exactly once. Compilers like clang refer to that C++ language feature as "thread-safe statics". In the WebKit project we turn that language feature off, putting the compiler into a non-standard mode that is more efficient, by compiling with clang's "-fno-threadsafe-statics", for example.

It would be good for us to some day do the performance measurement work to prove that we can move to the standard C++ semantics without a major performance cost. If we could do that, we could then simplify a lot of our C++ code, which does extra work to get the thread safety when we need it.

[Said Abou-Hallawa]

Created attachment 426482 [details]
Patch

[Darin Adler]

Comment on attachment 426482 [details]
Patch

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

What about the NSColor method?

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:134
> +            std::call_once(onceFlag, [&color] {
> +                transparentCGColor.construct(createCGColor(color));
> +            });

We don’t really need to capture color here. Could write this instead:

    std::call(onceFlag, [] {
        transparentCGColor.construct(createCGColor(Color::transparentBlack));
    });

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:142
> +            std::call_once(onceFlag, [&color] {
> +                blackCGColor.construct(createCGColor(color));
> +            });

Ditto.

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:150
> +            std::call_once(onceFlag, [&color] {
> +                whiteCGColor.construct(createCGColor(color));
> +            });

Ditto.

[Darin Adler]

Comment on attachment 426482 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:134
>> +            });
> 
> We don’t really need to capture color here. Could write this instead:
> 
>     std::call(onceFlag, [] {
>         transparentCGColor.construct(createCGColor(Color::transparentBlack));
>     });

Wait, we need to capture transparentCGColor. I don’t understand how the code in the patch successfully compiles.

[Darin Adler]

Comment on attachment 426482 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:157
> +    LockHolder lockHolder(&cachedCGColorLock);

I think the preferred idiom is:

    auto holder = holdLock(cachedCGColorLock);

[Chris Dumez]

Comment on attachment 426482 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:134
>>> +            });
>> 
>> We don’t really need to capture color here. Could write this instead:
>> 
>>     std::call(onceFlag, [] {
>>         transparentCGColor.construct(createCGColor(Color::transparentBlack));
>>     });
> 
> Wait, we need to capture transparentCGColor. I don’t understand how the code in the patch successfully compiles.

AFAIK, you don't need to capture static variables in lambdas.

[Darin Adler]

Comment on attachment 426482 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:134
>>>> +            });
>>> 
>>> We don’t really need to capture color here. Could write this instead:
>>> 
>>>     std::call(onceFlag, [] {
>>>         transparentCGColor.construct(createCGColor(Color::transparentBlack));
>>>     });
>> 
>> Wait, we need to capture transparentCGColor. I don’t understand how the code in the patch successfully compiles.
> 
> AFAIK, you don't need to capture static variables in lambdas.

OK, got it! Then my comment stands as-is.

[Said Abou-Hallawa]

Created attachment 426497 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 426482 [details]
Patch

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

I did similar changes in nsColor().

>>>>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:134
>>>>> +            });
>>>> 
>>>> We don’t really need to capture color here. Could write this instead:
>>>> 
>>>>     std::call(onceFlag, [] {
>>>>         transparentCGColor.construct(createCGColor(Color::transparentBlack));
>>>>     });
>>> 
>>> Wait, we need to capture transparentCGColor. I don’t understand how the code in the patch successfully compiles.
>> 
>> AFAIK, you don't need to capture static variables in lambdas.
> 
> OK, got it! Then my comment stands as-is.

Done.

>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:157
>> +    LockHolder lockHolder(&cachedCGColorLock);
> 
> I think the preferred idiom is:
> 
>     auto holder = holdLock(cachedCGColorLock);

Done.

[Chris Dumez]

Comment on attachment 426497 [details]
Patch

Looks correct. I hope this doesn't regress perf though.

[Said Abou-Hallawa]

Created attachment 426510 [details]
Patch

Fix win port

[EWS]

Committed r276283 (236765@main): <https://commits.webkit.org/236765@main>

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

[Darin Adler]

This doesn’t work yet, because the return values are unretained objects that can be released from the cache on any other thread.

[Chris Dumez]

(In reply to Darin Adler from comment #32)
> This doesn’t work yet, because the return values are unretained objects that
> can be released from the cache on any other thread.

Indeed, will follow up via Bug 231909.