Bug 202793

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

Created attachment 380641 [details] Patch

What's the difference between this bug and https://bugs.webkit.org/show_bug.cgi?id=186759 ?

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.

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

(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 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?

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

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

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.

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

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.

I have a patch queue that I think is a viable route for getting this landed now, I'll be opening bugs for the smaller and least controversial parts to lay the foundations. Work can be seen in this branch: https://github.com/Cwiiis/webkit/commits/offscreen-canvas-text-worker-2

Created attachment 424904 [details] Patch

The dependent patches have done most of the work for this - this last patch adds a CSSFontSelector and FontCache to WorkerGlobalScope and just uses them as appropriate via the ScriptExecutionContext and FontSelector interfaces. After this patch, text functions now work on OffscreenCanvas in a Worker, but FontFace in Workers is still unimplemented - I'll open a separate bug for that.

Created attachment 424906 [details] Patch

Comment on attachment 424906 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424906&action=review I think this patch will be much smaller if we don’t change Document::fontSelector to return a pointer (see comments below). > Source/WebCore/css/parser/CSSPropertyParserHelpers.h:177 > const AtomString& genericFontFamilyFromValueID(CSSValueID); > +WebKitFontFamilyNames::FamilyNamesIndex genericFontFamilyIndexFromValueID(CSSValueID); Not thrilled with these "FromValueID" names. C++ overloading means we don’t need to include the types of parameters in function names unless they are needed for clarity and CSSValueID is a real enumeration type, not just a typedef for something like int. Maybe leave it off from the new function name. Maybe even remove it from the old? > Source/WebCore/dom/Document.cpp:2065 > + fontSelector()->loadPendingFonts(); This isn’t great. If we have a function that returns a pointer, normally that’s because it can be null. And if it can be null, normally we’d want to have a null check. > Source/WebCore/dom/Document.h:545 > - CSSFontSelector& fontSelector() { return m_fontSelector; } > - const CSSFontSelector& fontSelector() const { return m_fontSelector; } > + CSSFontSelector* fontSelector() final { return m_fontSelector.ptr(); } > + const CSSFontSelector* fontSelector() const { return m_fontSelector.ptr(); } Are we changing this to a pointer because this can be null? If so, then under what conditions? If not, then why are we making this a pointer? Lets find a way to still have a reference-returning function in Document even if we also need a virtual pointer-returning function in ScriptExecutionContext. It’s much harder to confidently program with functions that return pointers that just happen to have a "never returns null" guarantee. Returning a reference is a much clearer way to express that. > Source/WebCore/dom/ScriptExecutionContext.h:167 > + virtual CSSFontSelector* fontSelector() { return nullptr; } As I suggested above, I think we should consider giving this a different name. Maybe even one to emphasize it can be null or one that hints at what it means if it’s null. Sadly, the WebKit standard for such names is fontSelectorIfExists, which I find a clunky naming scheme. If we do give it a different name, we should be sure to make the Document override of this function private, so that clients who have a Document remember to call the one that returns a reference instead. > Source/WebCore/html/canvas/OffscreenCanvasRenderingContext2D.cpp:93 > + modifiableState().font.initialize(*context.fontSelector(), *fontCascade); What guarantees the fontSelector won’t be null here?

(In reply to Darin Adler from comment #17) > Comment on attachment 424906 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424906&action=review > > I think this patch will be much smaller if we don’t change > Document::fontSelector to return a pointer (see comments below). You're right, and I'm actually not certain why I made this change now... At least, I don't think there's a good reason for it, I don't think it can be null. I'll change this. Looks like the build error is something to do with incomplete types with the UniqueRefs added in bug 223997, so perhaps this is a good time to group those into a structure that's private to FontCache entirely and remove them from the header. Will fix/revise.

Created attachment 424942 [details] Patch

Comment on attachment 424942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424942&action=review > Source/WebCore/css/CSSFontSelector.h:127 > + RefPtr<FontCache> m_fontCache; Can this be a Ref instead of a RefPtr? Does it ever have to be null? > Source/WebCore/html/canvas/OffscreenCanvasRenderingContext2D.cpp:92 > + auto fontCascade = Style::resolveForFontRaw(*fontRaw, WTFMove(fontDescription), context); > + if (fontCascade) { Slightly better style to define inside the if: if (auto fontCascade = ... > Source/WebCore/platform/graphics/FontCache.cpp:121 > +struct FontDataCacheKeyHash { So frustrating! Another patch I need to merge with (since I am redoing some of this hashing code a bit). Anyway, I can handle it. > Source/WebCore/platform/graphics/FontCache.cpp:132 > + static const bool safeToCompareToEmptyOrDeleted = true; When touching code like this we can switch from const to constexpr. > Source/WebCore/platform/graphics/FontCache.cpp:160 > + WTF_MAKE_FAST_ALLOCATED; > +public: Should be WTF_MAKE_STRUCT_FAST_ALLOCATED and then no need for public: here. > Source/WebCore/platform/graphics/FontCache.h:71 > +struct FontDataCaches; Could consider making this struct be a member of FontCache instead of at the file level. Would have little effect, but I think be a bit clearer. > Source/WebCore/platform/graphics/FontCache.h:222 > + static Ref<FontCache> create(); Should have commented when we added reference counting. I don’t fully understand why we need multiple owners for a single FontCache, or ref/deref to help keep the cache alive during the destruction process. If neither is required, then this class could just be a normal one that we create and destroy at will, and store in a UniqueRef or unique_ptr if we want it on the heap. Don’t need reference counting just to have multiple instances of a class! > Source/WebCore/platform/graphics/FontCache.h:225 > + FontCache(); This constructor should probably be private if possible. If it’s reference counted then we don’t want someone accidentally creating one not on the heap outside the create function. > Source/WebCore/platform/graphics/FontCascade.cpp:171 > + auto& fontCache = fontSelector ? fontSelector->fontCache() : FontCache::singleton(); Seems like we might want a helper function that does this operation. It’s a repeated idiom. Just something that takes a possibly-null FontSelector* and returns the FontCache&. Inline if necessary. Having the helper function could help rid us of the urge to put it in a local variable, too. > Source/WebCore/platform/graphics/FontCascadeFonts.cpp:105 > + , m_generation(m_fontSelector ? m_fontSelector->fontCache().generation() : FontCache::singleton().generation()) Here. > Source/WebCore/platform/graphics/FontCascadeFonts.cpp:147 > + auto& fontCache = fontSelector ? fontSelector->fontCache() : FontCache::singleton(); Here. > Source/WebCore/platform/graphics/FontCascadeFonts.cpp:185 > + auto& fontCache = m_fontSelector ? m_fontSelector->fontCache() : FontCache::singleton(); Here. > Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp:141 > + auto& fontCache = fontSelector() ? fontSelector()->fontCache() : FontCache::singleton(); Here. > Source/WebCore/style/StyleResolveForFontRaw.cpp:55 > + return fontDescription.familyCount() == 1 && fontDescription.firstFamily() == familyNamesData->at(static_cast<unsigned>(FamilyNamesIndex::MonospaceFamily)); We typically compare family names with a special equality operator, not by actually doing == on two strings. (In my patch I was toying with the idea of a class, FontFamilyName, instead of special == and hash functions.) > Source/WebCore/style/StyleResolveForFontRaw.cpp:78 > + family = familyNamesData->at(static_cast<unsigned>(CSSPropertyParserHelpers::genericFontFamilyIndex(ident))); This index type thing is not great. Is there some way we can change familyNamesData so it’s easier to use the enum values with it without a static_cast at the call site each time? Maybe just add an overload for at? > Source/WebCore/style/StyleResolveForFontRaw.cpp:205 > + return static_cast<float>(CSSPrimitiveValue::computeUnzoomedNonCalcLengthDouble(length.type, length.value, CSSPropertyFontSize, &fontCascade.fontMetrics(), &fontCascade.fontDescription(), &fontCascade.fontDescription(), is<Document>(context) ? downcast<Document>(context).renderView() : nullptr)); Is the nullptr behavior OK for offscreen canvas here? I’d like to understand more why this is OK.

Addressed everything, except: (In reply to Darin Adler from comment #20) > Comment on attachment 424942 [details] > > Source/WebCore/platform/graphics/FontCache.h:222 > > + static Ref<FontCache> create(); > > Should have commented when we added reference counting. I don’t fully > understand why we need multiple owners for a single FontCache, or ref/deref > to help keep the cache alive during the destruction process. If neither is > required, then this class could just be a normal one that we create and > destroy at will, and store in a UniqueRef or unique_ptr if we want it on the > heap. Don’t need reference counting just to have multiple instances of a > class! This is because the FontCache is created by the ScriptExecutionContext, which can disappear before the CSSFontSelector, as fonts retain a reference to CSSFontSelector and require the FontCache to stay alive during that time. An alternative is for CSSFontSelector to own a FontCache but only for Workers, though I think I prefer it this way. I don't mind changing this though. > > Source/WebCore/style/StyleResolveForFontRaw.cpp:55 > > + return fontDescription.familyCount() == 1 && fontDescription.firstFamily() == familyNamesData->at(static_cast<unsigned>(FamilyNamesIndex::MonospaceFamily)); > > We typically compare family names with a special equality operator, not by > actually doing == on two strings. (In my patch I was toying with the idea of > a class, FontFamilyName, instead of special == and hash functions.) I've left this as is as it's copied from the same function on FontDescription. > > Source/WebCore/style/StyleResolveForFontRaw.cpp:78 > > + family = familyNamesData->at(static_cast<unsigned>(CSSPropertyParserHelpers::genericFontFamilyIndex(ident))); > > This index type thing is not great. Is there some way we can change > familyNamesData so it’s easier to use the enum values with it without a > static_cast at the call site each time? Maybe just add an overload for at? Actually, I've not dealt with this yet, but I will, or at least I'll try - I like the idea of overriding at(), though this will involve editing the generator again to do it nicely... > > Source/WebCore/style/StyleResolveForFontRaw.cpp:205 > > + return static_cast<float>(CSSPrimitiveValue::computeUnzoomedNonCalcLengthDouble(length.type, length.value, CSSPropertyFontSize, &fontCascade.fontMetrics(), &fontCascade.fontDescription(), &fontCascade.fontDescription(), is<Document>(context) ? downcast<Document>(context).renderView() : nullptr)); > > Is the nullptr behavior OK for offscreen canvas here? I’d like to understand > more why this is OK. The RenderView is only required for viewport units - while this isn't specifically mentioned in the specs, this seems iffy to me - OffscreenCanvas isn't really on a viewport... I think ideally this would work (I need to check Chrome's behaviour), but I'd like to deal with this separately if needed. I'll leave a comment about it.

Created attachment 425005 [details] Patch

Created attachment 425011 [details] Patch

Comment on attachment 425011 [details] Patch Added the comment I mentioned that I'd forgotten in the last patch - no code changes.

Committed r275420: <https://commits.webkit.org/r275420> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425011 [details].

<rdar://problem/76147794>

(In reply to Chris Lord from comment #21) > The RenderView is only required for viewport units - while this isn't > specifically mentioned in the specs, this seems iffy to me - OffscreenCanvas > isn't really on a viewport... I think ideally this would work (I need to > check Chrome's behaviour), but I'd like to deal with this separately if > needed. I'll leave a comment about it. Best thing to do for the project’s future is to add one or more test cases, I guess.