[webkit-reviews] review denied: [Bug 202793] Implement text rendering on OffscreenCanvas in a Worker : [Attachment 380641] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 10 12:56:49 PDT 2019


Ryosuke Niwa <rniwa at webkit.org> has denied Chris Lord <clord at igalia.com>'s
request for review:
Bug 202793: Implement text rendering on OffscreenCanvas in a Worker
https://bugs.webkit.org/show_bug.cgi?id=202793

Attachment 380641: Patch

https://bugs.webkit.org/attachment.cgi?id=380641&action=review




--- Comment #4 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 380641
  --> https://bugs.webkit.org/attachment.cgi?id=380641
Patch

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

> Source/WebCore/css/CSSFontFace.cpp:107
> +	       const Settings* settings = globalScope ?
&globalScope->settings() : nullptr;

Settings object isn't thread safe. r- because of this.

> Source/WebCore/css/CSSFontFace.cpp:623
> +    if (m_fontSelector && !m_fontSelector->destroyed())

WebKit's term of art is stopped(), not destroyed() because that would imply
UAF.

> Source/WebCore/platform/graphics/FontCascade.cpp:253
> +    if (isMainThread() || fontCascadeCache().contains(hash)) {

It's not safe to ever access fontCascadeCache(), which is HashMap from a
different thread as it can result in UAF.
r- because of this.

> Source/WebCore/workers/WorkerGlobalScope.h:226
> +    const Ref<Settings> m_settings;

Settings object isn't ThreadSafeRefCounted.
r- because of this.


More information about the webkit-reviews mailing list