[webkit-dev] Implementing OffscreenCanvas

[Ryosuke Niwa]

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.

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

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.

- R. Niwa
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20191011/0cecf48c/attachment.html>