Make FontListImpl RefCountedThreadSafe

Make FontListImpl RefCountedThreadSafe

FontListImpl has non-thread-safe ref count, and this CL replaces it
with thread safe ref count.

FontListImpl is held by FontList, and |font_cache_| in ResourceBundle
holds FontLists. |font_cache_| itself is accessed from multiple thread
with a protection of |images_and_fonts_lock_|, but its contents seem
exposed without any protection.

That implies there is a potential data race if a user of ResourceBundle
takes a FontList from GetFontList(), holds a copy of it, and
simultaneously touches |font_cache_| by ReloadLocaleResources on
another thread.

BUG=688072

[tzik, 2017-02-16 18:08:08]

Description was changed from

==========
Make FontListImpl RefCountedThreadSafe
==========

to

==========
Make FontListImpl RefCountedThreadSafe

FontListImpl has non-thread-safe ref count, and this CL replaces it
with thread safe ref count.

FontListImpl is held by FontList, and |font_cache_| in ResourceBundle
holds FontLists. |font_cache_| itself is accessed from multiple thread
with a protection of |images_and_fonts_lock_|, but its contents seem
exposed without any protection.

That implies there is a potential data race if a user of ResourceBundle
takes a FontList from GetFontList(), holds a copy of it, and
simultaneously touches |font_cache_| by ReloadLocaleResources on
another thread.

BUG=688072
==========

[tzik, 2017-02-16 18:08:08]

tzik@chromium.org changed reviewers:
+ sky@chromium.org

[tzik, 2017-02-16 18:08:20]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[tzik, 2017-02-16 18:08:29]

PTAL

[commit-bot: I haz the power, 2017-02-16 18:08:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-02-16 19:28:10]

sky@chromium.org changed reviewers:
+ yukishiino@chromium.org

[sky, 2017-02-16 19:28:10]

I'm sure your fix is correct, but it begs the question of whether FontListImpl
is really thread safe. It doesn't look like it is to me. +yukishiino

[commit-bot: I haz the power, 2017-02-16 20:20:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 20:20:18]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-02-17 06:18:27]

On 2017/02/16 19:28:10, sky wrote:
> I'm sure your fix is correct, but it begs the question of whether FontListImpl
> is really thread safe. It doesn't look like it is to me. +yukishiino

IIRC, FontList and FontListImpl were not designed to be thread-safe because
there was only a single thread that uses font lists to paint text.  Many
UI-related things were implemented based on an assumption that UI thread is
single-threaded, not limited to font lists.

If only ReloadLocaleResources matters, this CL makes sense to me.  Except for
ReloadLocaleResources, only a single thread should use font lists, so
FontListImpl does not need to be thread-safe.  But ref counting needs to be
thread safe.

Probably we may want a comment why we need thread-safe ref counting?

LGTM on my side.

[tzik, 2017-02-17 13:28:22]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[tzik, 2017-02-17 13:28:26]

On 2017/02/17 06:18:27, Yuki wrote:
> On 2017/02/16 19:28:10, sky wrote:
> > I'm sure your fix is correct, but it begs the question of whether
FontListImpl
> > is really thread safe. It doesn't look like it is to me. +yukishiino
> 
> IIRC, FontList and FontListImpl were not designed to be thread-safe because
> there was only a single thread that uses font lists to paint text.  Many
> UI-related things were implemented based on an assumption that UI thread is
> single-threaded, not limited to font lists.

Hmm, right. gfx::PlatformFont and gfx::Image are also problematic.
Let me fix gfx::PlatformFont in this CL too.

> 
> If only ReloadLocaleResources matters, this CL makes sense to me.  Except for
> ReloadLocaleResources, only a single thread should use font lists, so
> FontListImpl does not need to be thread-safe.  But ref counting needs to be
> thread safe.
> 
> Probably we may want a comment why we need thread-safe ref counting?
> 
> LGTM on my side.

[commit-bot: I haz the power, 2017-02-17 13:28:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-02-17 13:36:36]

On 2017/02/17 13:28:26, tzik wrote:
> On 2017/02/17 06:18:27, Yuki wrote:
> > On 2017/02/16 19:28:10, sky wrote:
> > > I'm sure your fix is correct, but it begs the question of whether
> FontListImpl
> > > is really thread safe. It doesn't look like it is to me. +yukishiino
> > 
> > IIRC, FontList and FontListImpl were not designed to be thread-safe because
> > there was only a single thread that uses font lists to paint text.  Many
> > UI-related things were implemented based on an assumption that UI thread is
> > single-threaded, not limited to font lists.
> 
> Hmm, right. gfx::PlatformFont and gfx::Image are also problematic.
> Let me fix gfx::PlatformFont in this CL too.
> 
> > 
> > If only ReloadLocaleResources matters, this CL makes sense to me.  Except
for
> > ReloadLocaleResources, only a single thread should use font lists, so
> > FontListImpl does not need to be thread-safe.  But ref counting needs to be
> > thread safe.
> > 
> > Probably we may want a comment why we need thread-safe ref counting?
> > 
> > LGTM on my side.

Still LGTM.

[commit-bot: I haz the power, 2017-02-17 15:50:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 15:50:49]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-17 19:15:43]

On Thu, Feb 16, 2017 at 10:18 PM,  <yukishiino@chromium.org> wrote:
> On 2017/02/16 19:28:10, sky wrote:
>> I'm sure your fix is correct, but it begs the question of whether
>> FontListImpl
>> is really thread safe. It doesn't look like it is to me. +yukishiino
>
> IIRC, FontList and FontListImpl were not designed to be thread-safe because
> there was only a single thread that uses font lists to paint text. Many
> UI-related things were implemented based on an assumption that UI thread is
> single-threaded, not limited to font lists.

Are you sure on this one? If that's the case, why bother with the lock at all?

>
> If only ReloadLocaleResources matters, this CL makes sense to me. Except for
> ReloadLocaleResources, only a single thread should use font lists, so
> FontListImpl does not need to be thread-safe. But ref counting needs to be
> thread safe.
>
> Probably we may want a comment why we need thread-safe ref counting?
>
> LGTM on my side.
>
>
> https://codereview.chromium.org/2695393003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yuki, 2017-02-20 03:19:15]

On 2017/02/17 19:15:43, sky wrote:
> On Thu, Feb 16, 2017 at 10:18 PM,  <mailto:yukishiino@chromium.org> wrote:
> > On 2017/02/16 19:28:10, sky wrote:
> >> I'm sure your fix is correct, but it begs the question of whether
> >> FontListImpl
> >> is really thread safe. It doesn't look like it is to me. +yukishiino
> >
> > IIRC, FontList and FontListImpl were not designed to be thread-safe because
> > there was only a single thread that uses font lists to paint text. Many
> > UI-related things were implemented based on an assumption that UI thread is
> > single-threaded, not limited to font lists.
> 
> Are you sure on this one? If that's the case, why bother with the lock at all?

I think it's true that we *had* an assumption that only UI thread uses font
lists.  Thus, we don't have a lock in font lists at this moment.

However, tzik@ found that a resource loader (in another thread) may create /
destroy font lists, although the resource loader never uses font list's API such
as GetBaseline() or GetCapHeight().  I still think it's true that the *actual*
use of font lists only happen in the UI thread.  However, we need to think about
thread-safety of creation / destruction of font lists with resource loaders in
another thread.

From a point of view of font lists, creation / destruction is exceptionally not
single-threaded.  This is the problem that tzik@ is trying to fix.

This is the reason why I don't think that we need to make everything in font
lists thread-safe.  We only need to make creation / destruction of font lists
thread-safe.

Please correct me if something is wrong.

[tzik, 2017-02-20 07:31:01]

tzik@chromium.org changed reviewers:
+ rsesek@chromium.org

[tzik, 2017-02-20 07:34:33]

On 2017/02/20 03:19:15, Yuki wrote:
> On 2017/02/17 19:15:43, sky wrote:
> > On Thu, Feb 16, 2017 at 10:18 PM,  <mailto:yukishiino@chromium.org> wrote:
> > > On 2017/02/16 19:28:10, sky wrote:
> > >> I'm sure your fix is correct, but it begs the question of whether
> > >> FontListImpl
> > >> is really thread safe. It doesn't look like it is to me. +yukishiino
> > >
> > > IIRC, FontList and FontListImpl were not designed to be thread-safe
because
> > > there was only a single thread that uses font lists to paint text. Many
> > > UI-related things were implemented based on an assumption that UI thread
is
> > > single-threaded, not limited to font lists.
> > 
> > Are you sure on this one? If that's the case, why bother with the lock at
all?
> 
> I think it's true that we *had* an assumption that only UI thread uses font
> lists.  Thus, we don't have a lock in font lists at this moment.
> 
> However, tzik@ found that a resource loader (in another thread) may create /
> destroy font lists, although the resource loader never uses font list's API
such
> as GetBaseline() or GetCapHeight().  I still think it's true that the *actual*
> use of font lists only happen in the UI thread.  However, we need to think
about
> thread-safety of creation / destruction of font lists with resource loaders in
> another thread.
> 
> From a point of view of font lists, creation / destruction is exceptionally
not
> single-threaded.  This is the problem that tzik@ is trying to fix.
> 
> This is the reason why I don't think that we need to make everything in font
> lists thread-safe.  We only need to make creation / destruction of font lists
> thread-safe.
> 
> Please correct me if something is wrong.

+rsesek. The discussion on the gfx::Font thread safety is parallel to gfx::Image
one.

https://codereview.chromium.org/2692343008/#msg9

On 2017/02/17 20:44:56, Robert Sesek wrote:
> I don't think it's this simple, unfortunately. The basic issue is that
> gfx::Image is not threadsafe and shouldn't be treated as such. Fixing the
> refcounting issue does not fix the actual thread safety problem. There's quite
a
> bit of history here:
> 
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/8LqVoXQ_2bo/cW0tH...
> https://bugs.chromium.org/p/chromium/issues/detail?id=468010

[tzik, 2017-02-20 07:40:32]

Fortunately or not, we have only a few number of the problematic cross thread
access to the images and fonts:
https://codereview.chromium.org/2699323002/

Let me try fixing the callers instead of making the ref counts thread safe.