Bug 233971 - add assertions to check Font objects are used only on a single thread
Summary: add assertions to check Font objects are used only on a single thread
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Cameron McCormack (:heycam)
Keywords: InRadar
Depends on:
Blocks: 233488
  Show dependency treegraph
Reported: 2021-12-07 21:18 PST by Cameron McCormack (:heycam)
Modified: 2021-12-14 21:19 PST (History)
6 users (show)

See Also:

WIP (33.31 KB, patch)
2021-12-07 21:25 PST, Cameron McCormack (:heycam)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2021-12-07 21:18:19 PST
As part of implementing OffscreenCanvas, bug 202793 changed FontCache from a singleton to have an instance on the main thread and one on every worker thread.  In this model, Font objects are also created and used on a single thread.  I think we should add some assertions that Font objects are touched only on the thread that created them.
Comment 1 Cameron McCormack (:heycam) 2021-12-07 21:25:59 PST
Created attachment 446287 [details]

Here's an incomplete patch that shows one approach, using the bits I'm adding in bug 233967. Unfortunately it's pretty noisy since the WTF_GUARDED_BY_LOCK attribute needs to be added on every member variable. An alternative would be to bundle up all the data members into a struct and have a single member variable with WTF_GUARDED_BY_LOCK on it, but that would cause corresponding noise on all member variable references. Sadly wrapping all the data members in an anonymous struct prevents WTF_GUARDED_BY_LOCK from being used on it in a single place. (It would be nice if a single annotation could be added on the class itself.)
Comment 2 Simon Fraser (smfr) 2021-12-07 22:34:11 PST
I think the title is wrong?
Comment 3 Chris Lord 2021-12-08 02:18:41 PST
This seems a bit overkill to me vs just guarding on the functions that provide Font objects - there's no point where we transfer Fonts between threads or access Fonts from different threads from the FontCache that provided them are there?
Comment 4 Cameron McCormack (:heycam) 2021-12-08 13:41:13 PST
Guarding on the places that provide the Font objects might be a better balance between  safety (and ability to rely on the static analysis attributes) and noisiness.

And you're right, I don't think we do explicitly transfer Fonts between threads.  I guess I just want to protect against future code changes that might provide a different way to get access to Font objects.

A different approach could be to have some handle class that wraps a raw Font*, asserts in its operator->, and doesn't provide a way to get at the raw pointer.  The static constructor functions are the only way to create Font instances and would return Ref<FontHandle>.
Comment 5 Radar WebKit Bug Importer 2021-12-14 21:19:14 PST