[webkit-dev] Implementing OffscreenCanvas

[Chris Lord]

On 2019-10-10 23:15, Ryosuke Niwa wrote:
> On Thu, Oct 10, 2019 at 2:07 PM Myles C. Maxfield
> <mmaxfield at apple.com> wrote:
> 
>>> On Oct 10, 2019, at 12:57 PM, Ryosuke Niwa <rniwa at webkit.org>
>>> wrote:
>>>
>>> Hi Chris,
>>>
>>> I'm excited that you're working on OffscreenCanvas because I think
>>> it would be a valuable feature
>>
>> Me too!!!
>>
>>> , and I'm confident we can come up with a strategy to limit its
>>> privacy & security risk as we see fit.
>>>
>>> However, many of your patches seem to ignore the fact most of
>>> WebCore objects aren't thread safe. For example, CSS parser and
>>> the entire CSS object model aren't designed to used in non-main
>>> thread. Regardless of how ready Linux ports might be, we can't
>>> land or enable this feature in WebKit until all thread safety
>>> issues are sorted out.
>>>
>>> Unfortunately, I can't make a time commitment to review & find
>>> every thread safety issue in your patches. Please work with
>>> relevant experts and go over your code changes.
>>
>> I’d be happy to work with you on this.
>>
>>> For example, it's never safe to an object that's RefCounted in
>>> multiple threads because RefCounted isn't thread safe. One would
>>> have to use ThreadSafeRefCounted. It's never safe to use
>>> AtomString from one another in another because AtomString has a
>>> pool of strings per thread. For that matter, it's never safe to
>>> use a single String object from two or more threads because String
>>> itself is RefCounted and isn't thread safe. It's not not okay to
>>> do readonly access to basic container types like Vector, HashMap,
>>> etc... because they don't guarantee atomic update of internal data
>>> structures and doing so can result in UAF.

To the best of my knowledge, I've taken this into account (it's hard to
see this without applying the whole patch series, as later patches
assume the changes made with respect to thread-safety in the earlier
patches). There are of course bound to be things I've missed - and I'm
also open to taking a different strategy on certain things too, I'm
fairly new to the WebKit codebase (well, I'm assuming having worked on
it around 10 years ago doesn't apply too much anymore :)) and I imagine
I've taken some naive approaches in places that someone more experienced
would be able to correct me on.

>>> I think the hardest part of this project is validating that
>>> enabling this feature wouldn't introduce dozens of new thread
>>> safety issues and thereby security vulnerabilities.
>>
>> Sounds like this this is a good candidate for a feature flag.
> 
> Yeah, in fact, this should really have a compile time flag in addition
> to runtime flag, and it should be default off until we've done enough
> fuzzing. The thread unsafe code can be turned into an attack gadget if
> it exists at all in production binary.

This sounds good to me, I'll file a bug and write a patch for this. I
assume there are ways of enabling features when tests are run? While a
user (or a developer) using WebKit wouldn't want this feature to be
enabled, I think it should be enabled for (some) test runs as soon as
possible to give us an indication of any issues that exist at the
moment.


>> On Thu, Oct 10, 2019 at 4:23 AM Chris Lord <clord at igalia.com> wrote:
>>
>> I've spent the last month or so 'finishing' the implementation of
>> OffscreenCanvas[1], based on Žan Doberšek's work from a year
>> ago[2].
>> OffscreenCanvas is an API for being able to use canvas drawing
>> without a
>> visible canvas, and from within Workers. It's supported by Blink and
>> has
>> partial support in Gecko.
>>
>> It's at the point now where I'd consider it a finished draft - it is
>> almost fully implemented and passes the majority of relevant tests
>> in a
>> debug build without crashing, but has some areas that need
>> completion on
>> other platforms (async drawing on non-Linux) and some missing parts
>> (Web
>> Inspector, ImageBitmapRenderingContext). It almost certainly needs
>> reworking in places.
>>
>> My work is on GitHub[3] - I'd like to solicit reviews and comment.
>> Some
>> of the bugs hanging off [2] have patches that need review and I
>> think
>> are near ready to being landable as the foundation of this work. It
>> is
>> broadly split up like so:
>>
>> - Refactor to move functionality from HTMLCanvasElement to
>> CanvasBase
>> - Refactor to not unnecessarily require HTMLCanvasElement in places
>> - Implement OffscreenCanvas functionality
>> - Make font loading/styling usable from a Worker and without a
>> Document
>> - Implement AnimationFrameProvider on DedicatedWorkerGlobalScope
>> - Implement asynchronous drawing updates on placeholder canvases
>>
>> I expect the font-related stuff to be the most contentious, and my
>> AnimationFrameProvider implementation may be too trivial (but might
>> be
>> ok for a first go?)
>>
>> All feedback appreciated. Best regards,
>>
>> Chris
>>
>> [1]
>>
> https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-interface
>> [2] https://bugs.webkit.org/show_bug.cgi?id=183720
>> [3] https://github.com/Cwiiis/webkit/tree/offscreen-canvas
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev at lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev at lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev