Bug 202793 - Implement text rendering on OffscreenCanvas in a Worker
Summary: Implement text rendering on OffscreenCanvas in a Worker
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords:
Depends on: 202626 219283 202794 202797
Blocks: 183720 186759
  Show dependency treegraph
 
Reported: 2019-10-10 06:10 PDT by Chris Lord
Modified: 2020-11-25 01:46 PST (History)
11 users (show)

See Also:


Attachments
Patch (92.57 KB, patch)
2019-10-10 08:13 PDT, Chris Lord
rniwa: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lord 2019-10-10 06:10:11 PDT
Bug 186759 tracks making font functions available on OffscreenCanvas, this bug tracks making them complete and fully available to use within workers.
Comment 1 Chris Lord 2019-10-10 06:14:11 PDT
(In reply to Chris Lord from comment #0)
> Bug 186759 tracks making font functions available on OffscreenCanvas, this
> bug tracks making them complete and fully available to use within workers.

Change of heart - Bug 186759 is the meta bug, this tracks implementing font functions on OffscreenCanvas in workers, a later bug will cover making fonts stylable within workers.
Comment 2 Chris Lord 2019-10-10 08:13:21 PDT
Created attachment 380641 [details]
Patch
Comment 3 Myles C. Maxfield 2019-10-10 11:23:04 PDT
What's the difference between this bug and https://bugs.webkit.org/show_bug.cgi?id=186759 ?
Comment 4 Ryosuke Niwa 2019-10-10 12:56:49 PDT
Comment on attachment 380641 [details]
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.
Comment 5 Myles C. Maxfield 2019-10-10 13:58:21 PDT
(In reply to Myles C. Maxfield from comment #3)
> What's the difference between this bug and
> https://bugs.webkit.org/show_bug.cgi?id=186759 ?

Oh, whoops, you answered this directly above. Sorry for not reading. Please disregard.
Comment 6 Chris Lord 2019-10-11 01:57:48 PDT
(In reply to Ryosuke Niwa from comment #4)
> Comment on attachment 380641 [details]
> 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.

This Settings object is only used within the thread it was created on.

> > 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.

Thanks, I'll make this change.

> > 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.

Thanks, good catch.

> > Source/WebCore/workers/WorkerGlobalScope.h:226
> > +    const Ref<Settings> m_settings;
> 
> Settings object isn't ThreadSafeRefCounted.
> r- because of this.

Does it need to be ThreadSafeRefCounted if it's created and used only within this thread?
Comment 7 Ryosuke Niwa 2019-10-11 02:47:39 PDT
Comment on attachment 380641 [details]
Patch

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

> Source/WebCore/workers/WorkerGlobalScope.cpp:84
> +    , m_settings(Settings::create(nullptr))

Oops, sorry, I somehow didn't see this critical piece of code.
How are values of this m_settings updated?
Comment 8 Ryosuke Niwa 2019-10-11 02:49:19 PDT
(In reply to Chris Lord from comment #6)
>
> > > Source/WebCore/workers/WorkerGlobalScope.h:226
> > > +    const Ref<Settings> m_settings;
> > 
> > Settings object isn't ThreadSafeRefCounted.
> > r- because of this.
> 
> Does it need to be ThreadSafeRefCounted if it's created and used only within
> this thread?

It needs to be if you're creating it in the main thread and passing it to the worker thread. How else are we going to initialize it with the right values? Note that settings values aren't always initialized to the "right" values when it's created. Many settings' values are set forth by WebKit's WebPreferences' values.
Comment 9 Chris Lord 2019-10-11 02:51:30 PDT
(In reply to Ryosuke Niwa from comment #8)
> (In reply to Chris Lord from comment #6)
> >
> > > > Source/WebCore/workers/WorkerGlobalScope.h:226
> > > > +    const Ref<Settings> m_settings;
> > > 
> > > Settings object isn't ThreadSafeRefCounted.
> > > r- because of this.
> > 
> > Does it need to be ThreadSafeRefCounted if it's created and used only within
> > this thread?
> 
> It needs to be if you're creating it in the main thread and passing it to
> the worker thread. How else are we going to initialize it with the right
> values? Note that settings values aren't always initialized to the "right"
> values when it's created. Many settings' values are set forth by WebKit's
> WebPreferences' values.

Thanks, knowing this I need to re-think how this is going to work (suggestions welcome).
Comment 10 Ryosuke Niwa 2019-10-11 03:17:19 PDT
I think you want to follow how online / offline / geolocation states are passed to the worker. While settings can be updated dynamically overtime, it's probably not the end of the world if we didn't update it once the worker had started. So you can probably just create Settings object in the main thread and hand it off to the worker thread when it's created.

Alternatively, you can create an object which contains a smaller set of settings that are used by OffscreenCanvas / worker. See CSSParserContext for an example.
Comment 11 Chris Lord 2019-10-24 05:02:15 PDT
(In reply to Ryosuke Niwa from comment #10)
> I think you want to follow how online / offline / geolocation states are
> passed to the worker. While settings can be updated dynamically overtime,
> it's probably not the end of the world if we didn't update it once the
> worker had started. So you can probably just create Settings object in the
> main thread and hand it off to the worker thread when it's created.
> 
> Alternatively, you can create an object which contains a smaller set of
> settings that are used by OffscreenCanvas / worker. See CSSParserContext for
> an example.

So I've just been looking at this - I guess the options here are:

- Make Settings ThreadSafeRefCounted, add main-thread asserts to all setter functions and share the object when creating a worker.
- Create a new Settings object and pass all relevant settings on creation (I guess we'd want a structure and helper functions to retrieve, store and set all relevant settings).
- Create a new type of Settings class that only holds the relevant settings for using with FontSelector (and which can be augmented with other settings workers may use in the future). This still involves retrieving all relevant settings from the Document's Settings object and passing them along when creating the worker. I guess this makes sense as an interface that both Settings and this new object can implement.

I'm leaning towards this third option, it feels like the cleanest and probably involves the fewest changes to existing files. Not sure what to call the interface though... WindowOrWorkerGlobalScopeSettings would fit in with current naming, with a new WorkerGlobalScopeSettings class. Seems a little wordy, but does what it says on the tin.
Comment 12 Chris Lord 2019-10-24 07:17:02 PDT
And after looking at implementing this, I realise how much more work needs to be done on the font support... I may refactor so that this settings change happens in bug 202525 instead and misses out the font settings for now, as this part is considerably harder.