[webkit-dev] Implementing OffscreenCanvas

[Chris Lord]

On 2019-10-11 12:13, Ryosuke Niwa wrote:
> On Fri, Oct 11, 2019 at 2:09 AM Chris Lord <clord at igalia.com> wrote:
> 
>> 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 you want Antti's input most here since many of the classes
> you're touching in CSS is maintained by Antti these days.
> 
> The thing that worries me most about this feature is the thread
> safety. Historically, we had many thread safety issues regarding
> Timer, RefCounted objects, WeakPtr, etc... Chris (Dumez) and I have
> been addressing some of those issues more systematically (e.g.
> https://trac.webkit.org/r241499 & https://trac.webkit.org/r248113)
> because we seem to keep making same mistakes across the codebase. The
> key here is to really make which objects are used concurrently or in
> non-main threads obvious to whomever reading the code in the future.
> 
> I can think of a few ways to do that. For one, we should be adding
> lots of thread safety assertions. Assertions are better than comments
> because they're obvious to readers and they warn us whenever they get
> out of date or we make mistakes. In fact, I discourage adding comments
> about thread safety; if you find yourself doing that, then it's time
> to refactor code such that the code is obviously multi-threaded or
> concurrent by the virtue of the way things are structured or make it
> not matter because the code is naturally thread safe. Just as an
> example, one of your patches made CSSValuePool's member functions
> thread safe but it left the rest of code still access
> CSSValuePool::singleton() object. This is problematic because
> singleton objects are usually shared across threads. A better approach
> is to have each caller explicitly get a specific instance of
> CSSValuePool from some other object; e.g. CSSParser's member variable.
> This will make it so that the code that needs to run concurrently
> would not rely on singleton objects, and whatever new code which gets
> introduced to CSSValuePool or CSSParser would naturally be thread safe
> so long as the author follows the same convention. Finally, we can add
> an assertion in CSSValuePool::singleton() to make sure it's only
> called in the thread. This way, anyone who makes a mistake of calling
> this singleton function in CSSParser, or elsewhere which can be used
> by the offscreen canvas code in worker would hit this assertion.

Just an update on this, CSS parsing largely happens via static
functions, so there's no one place to add a CSSValuePool member variable
- however, I did add it where applicable and added alternative functions
to all of the CSS parsing static functions that end up in using a
CSSValuePool with versions of the function that accept a given
CSSValuePool. I wonder if we want to remove the versions that don't to
prevent accidental usage of CSSValuePool::singleton()?

>>>>> 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.
> 
> If you added a compile time flag, then you'd have to manually enable
> that for testing. Historically, we've done things like adding a
> special buildbot builder which runs tests with a certain feature
> enabled. You could also opt to turn on the compile time flag on by
> default on GTK+ port assuming the rest of GTK+ port maintainers are
> okay with that.

So I was just looking at adding a setting, but looks like it's already
behind a runtime-setting that's controlled by the experimental-features
flag, so I guess we're ok as it is?

> For this feature, you may also want to setup a bot or some machine
> which runs ThreadSanitizer:
> https://clang.llvm.org/docs/ThreadSanitizer.html
> 
> I'd definitely recommend doing extensive fuzzing to make sure we've
> sorted out all the thread safety issues before turning it on by
> default though especially because offscreen canvas isn't a kind of
> feature normal layout tests or WPT tests would exercise so just
> running them would probably be not enough to catch all nasty thread
> safety issues.

We're looking into setting up an extra test machine with
ThreadSanitizer, we'll have a look at fuzzing too. I'll post when we've
made some headway.

Cheers,

Chris