Refactor GpuDataManagerImpl to make it thread-safe, now and forever.

Refactor GpuDataManagerImpl to make it thread-safe, now and forever.

The original impl of GpuDataManagerImpl is thread-safe, but gradurally it regressed.

In order to make sure this class is thread-safe in the future, we move all code to GpuDataManagerImplPrivate, and make GpuDataManagerImpl a simple wrapper around GpuDataManagerImplPrivate's public functions, where each function call is guarded by lock, thus thread-safe.

BUG=232556
TEST=asan bots no longer crashes as described in crbug.com/232556
R=joi@chromium.org, kbr@chromium.org, piman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199530

[Zhenyao Mo, 2013-05-09 00:50:33]

Ken, please review.

Tested with --force-compositing-mode --enable-threaded-compositing, no functions
in this class are called at each frame.

[dmichael (off chromium), 2013-05-09 15:53:51]

https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.h (right):

https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
drive-by suggestion: To make kbr's review easier, you could try uploading with:
git cl upload --similarity=20
(or some number smaller than 50)
It should be possible to make this file show up as "A+" instead of "A" so that
the parts that aren't changed from gpu_data_manager_impl don't show up as
changed.

[jam, 2013-05-09 16:17:07]

https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.cc:21: #define
DELEGATE_TO_PRIVATE_0(name) \
driveby:

all these macros seem to break two style guides that we use:
-classes should be generally used on one thread only
"Our approach is to use message passing as the way of communicating between
threads. We discourage locking and threadsafe objects. Instead, objects live on
only one thread, we pass messages between threads for communication, and we use
callback interfaces (implemented by message passing) for most cross-thread
requests."
http://dev.chromium.org/developers/design-documents/threading

-avoiding macros to generate C++ code unless it's the only way (i.e. low level
code like bindings, IPC)

[Zhenyao Mo, 2013-05-09 18:07:26]

https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.cc:21: #define
DELEGATE_TO_PRIVATE_0(name) \
On 2013/05/09 16:17:07, jam wrote:
> driveby:
> 
> all these macros seem to break two style guides that we use:
> -classes should be generally used on one thread only
> "Our approach is to use message passing as the way of communicating between
> threads. We discourage locking and threadsafe objects. Instead, objects live
on
> only one thread, we pass messages between threads for communication, and we
use
> callback interfaces (implemented by message passing) for most cross-thread
> requests."
> http://dev.chromium.org/developers/design-documents/threading

This class provides critical information for multiple threads, extensions,
compositing threads, etc. These queries need to be synchronous. That's why locks
and ThreadSafeObserverList were used before, but were unsuccessful because new
CLs easily broke the original careful design.  That's the motivation of this CL.

> 
> -avoiding macros to generate C++ code unless it's the only way (i.e. low level
> code like bindings, IPC)

OK, will remove macros.

https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.h (right):

https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/05/09 15:53:51, dmichael wrote:
> drive-by suggestion: To make kbr's review easier, you could try uploading
with:
> git cl upload --similarity=20
> (or some number smaller than 50)
> It should be possible to make this file show up as "A+" instead of "A" so that
> the parts that aren't changed from gpu_data_manager_impl don't show up as
> changed.

is there a way to do this through svn/gcl?

[dmichael (off chromium), 2013-05-09 18:11:35]

On Thu, May 9, 2013 at 12:07 PM, <zmo@chromium.org> wrote:

>
> https://codereview.chromium.**org/14794006/diff/47003/**
>
content/browser/gpu/gpu_data_**manager_impl.cc<https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_data_manager_impl.cc>
> File content/browser/gpu/gpu_data_**manager_impl.cc (right):
>
> https://codereview.chromium.**org/14794006/diff/47003/**
>
content/browser/gpu/gpu_data_**manager_impl.cc#newcode21<https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_data_manager_impl.cc#newcode21>
> content/browser/gpu/gpu_data_**manager_impl.cc:21: #define
> DELEGATE_TO_PRIVATE_0(name) \
> On 2013/05/09 16:17:07, jam wrote:
>
>> driveby:
>>
>
>  all these macros seem to break two style guides that we use:
>> -classes should be generally used on one thread only
>> "Our approach is to use message passing as the way of communicating
>>
> between
>
>> threads. We discourage locking and threadsafe objects. Instead,
>>
> objects live on
>
>> only one thread, we pass messages between threads for communication,
>>
> and we use
>
>> callback interfaces (implemented by message passing) for most
>>
> cross-thread
>
>> requests."
>>
http://dev.chromium.org/**developers/design-documents/**threading<http://dev....
>>
>
> This class provides critical information for multiple threads,
> extensions, compositing threads, etc. These queries need to be
> synchronous. That's why locks and ThreadSafeObserverList were used
> before, but were unsuccessful because new CLs easily broke the original
> careful design.  That's the motivation of this CL.
>
>
>
>  -avoiding macros to generate C++ code unless it's the only way (i.e.
>>
> low level
>
>> code like bindings, IPC)
>>
>
> OK, will remove macros.
>
>
> https://codereview.chromium.**org/14794006/diff/47003/**
>
content/browser/gpu/gpu_data_**manager_impl_private.h<https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_data_manager_impl_private.h>
> File content/browser/gpu/gpu_data_**manager_impl_private.h (right):
>
> https://codereview.chromium.**org/14794006/diff/47003/**
>
content/browser/gpu/gpu_data_**manager_impl_private.h#**newcode1<https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_data_manager_impl_private.h#newcode1>
> content/browser/gpu/gpu_data_**manager_impl_private.h:1: // Copyright (c)
> 2013 The Chromium Authors. All rights reserved.
> On 2013/05/09 15:53:51, dmichael wrote:
>
>> drive-by suggestion: To make kbr's review easier, you could try
>>
> uploading with:
>
>> git cl upload --similarity=20
>> (or some number smaller than 50)
>> It should be possible to make this file show up as "A+" instead of "A"
>>
> so that
>
>> the parts that aren't changed from gpu_data_manager_impl don't show up
>>
> as
>
>> changed.
>>
>
> is there a way to do this through svn/gcl?
>
If you're using svn, you should use "svn cp". svn tracks file moves and
copies for you, but you have to do it through svn.

If you've done that, and you're still not getting the diff you want, maybe
try:
"gcl upload --git_similarity 20"
(I found this from running "gcl help upload"; I'm not sure if it means what
I think it means.)


>
>
https://codereview.chromium.**org/14794006/<https://codereview.chromium.org/1...
>

[Zhenyao Mo, 2013-05-09 18:20:57]

On 2013/05/09 18:11:35, dmichael wrote:
> On Thu, May 9, 2013 at 12:07 PM, <mailto:zmo@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/14794006/diff/47003/**
> >
>
content/browser/gpu/gpu_data_**manager_impl.cc<https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_data_manager_impl.cc>
> > File content/browser/gpu/gpu_data_**manager_impl.cc (right):
> >
> > https://codereview.chromium.**org/14794006/diff/47003/**
> >
>
content/browser/gpu/gpu_data_**manager_impl.cc#newcode21<https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_data_manager_impl.cc#newcode21>
> > content/browser/gpu/gpu_data_**manager_impl.cc:21: #define
> > DELEGATE_TO_PRIVATE_0(name) \
> > On 2013/05/09 16:17:07, jam wrote:
> >
> >> driveby:
> >>
> >
> >  all these macros seem to break two style guides that we use:
> >> -classes should be generally used on one thread only
> >> "Our approach is to use message passing as the way of communicating
> >>
> > between
> >
> >> threads. We discourage locking and threadsafe objects. Instead,
> >>
> > objects live on
> >
> >> only one thread, we pass messages between threads for communication,
> >>
> > and we use
> >
> >> callback interfaces (implemented by message passing) for most
> >>
> > cross-thread
> >
> >> requests."
> >>
>
http://dev.chromium.org/**developers/design-documents/**threading%3Chttp://de...>
> >>
> >
> > This class provides critical information for multiple threads,
> > extensions, compositing threads, etc. These queries need to be
> > synchronous. That's why locks and ThreadSafeObserverList were used
> > before, but were unsuccessful because new CLs easily broke the original
> > careful design.  That's the motivation of this CL.
> >
> >
> >
> >  -avoiding macros to generate C++ code unless it's the only way (i.e.
> >>
> > low level
> >
> >> code like bindings, IPC)
> >>
> >
> > OK, will remove macros.
> >
> >
> > https://codereview.chromium.**org/14794006/diff/47003/**
> >
>
content/browser/gpu/gpu_data_**manager_impl_private.h<https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_data_manager_impl_private.h>
> > File content/browser/gpu/gpu_data_**manager_impl_private.h (right):
> >
> > https://codereview.chromium.**org/14794006/diff/47003/**
> >
>
content/browser/gpu/gpu_data_**manager_impl_private.h#**newcode1<https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_data_manager_impl_private.h#newcode1>
> > content/browser/gpu/gpu_data_**manager_impl_private.h:1: // Copyright (c)
> > 2013 The Chromium Authors. All rights reserved.
> > On 2013/05/09 15:53:51, dmichael wrote:
> >
> >> drive-by suggestion: To make kbr's review easier, you could try
> >>
> > uploading with:
> >
> >> git cl upload --similarity=20
> >> (or some number smaller than 50)
> >> It should be possible to make this file show up as "A+" instead of "A"
> >>
> > so that
> >
> >> the parts that aren't changed from gpu_data_manager_impl don't show up
> >>
> > as
> >
> >> changed.
> >>
> >
> > is there a way to do this through svn/gcl?
> >
> If you're using svn, you should use "svn cp". svn tracks file moves and
> copies for you, but you have to do it through svn.
> 
> If you've done that, and you're still not getting the diff you want, maybe
> try:
> "gcl upload --git_similarity 20"
> (I found this from running "gcl help upload"; I'm not sure if it means what
> I think it means.)
> 
> 
> >
> >
>
https://codereview.chromium.**org/14794006/%3Chttps://codereview.chromium.org...>
> >

FYI: gcl upload --git_similarity doesn't work for me, but I use svn cp and now
the diff is much better.  Thanks for the advice.

[Ken Russell (switch to Gerrit), 2013-05-09 18:33:52]

https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://codereview.chromium.org/14794006/diff/47003/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.cc:21: #define
DELEGATE_TO_PRIVATE_0(name) \
On 2013/05/09 18:07:26, Zhenyao Mo wrote:
> On 2013/05/09 16:17:07, jam wrote:
> > driveby:
> > 
> > all these macros seem to break two style guides that we use:
> > -classes should be generally used on one thread only
> > "Our approach is to use message passing as the way of communicating between
> > threads. We discourage locking and threadsafe objects. Instead, objects live
> on
> > only one thread, we pass messages between threads for communication, and we
> use
> > callback interfaces (implemented by message passing) for most cross-thread
> > requests."
> > http://dev.chromium.org/developers/design-documents/threading
> 
> This class provides critical information for multiple threads, extensions,
> compositing threads, etc. These queries need to be synchronous. That's why
locks
> and ThreadSafeObserverList were used before, but were unsuccessful because new
> CLs easily broke the original careful design.  That's the motivation of this
CL.

Agree that this class is better structured and more maintainable using locks for
thread safety. Using PostTask will balloon the code and make it much harder to
prove correctness.

> 
> > 
> > -avoiding macros to generate C++ code unless it's the only way (i.e. low
level
> > code like bindings, IPC)
> 
> OK, will remove macros.

Personally I don't mind the macros.

[Zhenyao Mo, 2013-05-09 19:50:58]

Further digging through the coding styles about macro, it seems like the main
reasons we should avoid macros do not apply here.

1) The macros here are in cc file, not h file, so they are very local.
2) There is no magic in the macros, just very simple delegation.

Using macros here makes the code much easier to read and maintain in the future.

[Zhenyao Mo, 2013-05-09 19:54:01]

kbr: the try bots results are in patch set 6. I don't think the failures are
related.

patch set 7 is exactly the same as patch set 6, but of clearer diff.

Please take a look.

[Zhenyao Mo, 2013-05-09 19:55:09]

piman: content/ owner.

[jam, 2013-05-09 20:09:58]

On 2013/05/09 19:50:58, Zhenyao Mo wrote:
> Further digging through the coding styles about macro, it seems like the main
> reasons we should avoid macros do not apply here.
> 
> 1) The macros here are in cc file, not h file, so they are very local.
> 2) There is no magic in the macros, just very simple delegation.
> 
> Using macros here makes the code much easier to read and maintain in the
future.

the style guide says to avoid them where possible, i.e. "Be very cautious with
macros. Prefer inline functions, enums, and const variables to macros." and "But
before using a macro, consider carefully whether there's a non-macro way to
achieve the same result."

In low-level code like bind/ipc, there's no way to do that other than with
macros. in this place, macros aren't necessary.

In general, in chrome code we avoid using macros to generate code. this has been
advocated for a long time in the code base, which is why you'll find very few
examples of this (other than low-level code per above).

[piman, 2013-05-09 20:18:23]

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.cc:21: #define
DELEGATE_TO_PRIVATE_0(name) \
The macros seem unnecessary. You have 10 of them, vs only about 40 calls, you
don't get much reuse from most of them.
It impairs readability/navigation/debugging. It's also against the style guide.

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl.h (right):

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.h:176: mutable base::Lock lock_;
nit: it's nice to put the lock above the members it protects, for 2 reasons:
1- it gives good symmetry with the call sites, where you take the lock
(autolock) before accessing the variable
2- it ensures the lock outlives the member, which can sometimes avoid subtle
bugs.

[Ken Russell (switch to Gerrit), 2013-05-09 20:21:03]

On 2013/05/09 20:09:58, jam wrote:
> On 2013/05/09 19:50:58, Zhenyao Mo wrote:
> > Further digging through the coding styles about macro, it seems like the
main
> > reasons we should avoid macros do not apply here.
> > 
> > 1) The macros here are in cc file, not h file, so they are very local.
> > 2) There is no magic in the macros, just very simple delegation.
> > 
> > Using macros here makes the code much easier to read and maintain in the
> future.
> 
> the style guide says to avoid them where possible, i.e. "Be very cautious with
> macros. Prefer inline functions, enums, and const variables to macros." and
"But
> before using a macro, consider carefully whether there's a non-macro way to
> achieve the same result."
> 
> In low-level code like bind/ipc, there's no way to do that other than with
> macros. in this place, macros aren't necessary.
> 
> In general, in chrome code we avoid using macros to generate code. this has
been
> advocated for a long time in the code base, which is why you'll find very few
> examples of this (other than low-level code per above).

I trust Mo's judgment that using macros here will reduce the amount of code that
needs to be maintained by hand. Mo, if you try writing these methods by hand and
find that it is actually workable, please update the CL.

[Ken Russell (switch to Gerrit), 2013-05-09 20:21:16]

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:626: // ProcessCrashed is
thread-safe.
It's hard to reason about the correctness here. What about instead posting this
message to the GpuDataManagerImpl which owns this GpuDataManagerImplPrivate?

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:681: // Notify3DAPIBlocked
is thread-safe.
Same observation here.

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.h (right):

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.h:24: class CONTENT_EXPORT
GpuDataManagerImplPrivate {
Does this class really need to be CONTENT_EXPORT?

Also, is there any way to guard against accidentally subclassing it?

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_unittest.cc (right):

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_unittest.cc:63: class
GpuDataManagerImplTest : public testing::Test {
This file should be renamed to gpu_data_manager_impl_private_unittest.cc and
this class renamed.

[Zhenyao Mo, 2013-05-09 23:35:12]

Revised. please take another look.

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.cc:21: #define
DELEGATE_TO_PRIVATE_0(name) \
On 2013/05/09 20:18:23, piman wrote:
> The macros seem unnecessary. You have 10 of them, vs only about 40 calls, you
> don't get much reuse from most of them.
> It impairs readability/navigation/debugging. It's also against the style
guide.

OK, removed macros.

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl.h (right):

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.h:176: mutable base::Lock lock_;
On 2013/05/09 20:18:23, piman wrote:
> nit: it's nice to put the lock above the members it protects, for 2 reasons:
> 1- it gives good symmetry with the call sites, where you take the lock
> (autolock) before accessing the variable
> 2- it ensures the lock outlives the member, which can sometimes avoid subtle
> bugs.

Done.

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:626: // ProcessCrashed is
thread-safe.
On 2013/05/09 20:21:16, kbr wrote:
> It's hard to reason about the correctness here. What about instead posting
this
> message to the GpuDataManagerImpl which owns this GpuDataManagerImplPrivate?

Done.

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:681: // Notify3DAPIBlocked
is thread-safe.
On 2013/05/09 20:21:16, kbr wrote:
> Same observation here.

Done.

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.h (right):

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.h:24: class CONTENT_EXPORT
GpuDataManagerImplPrivate {
On 2013/05/09 20:21:16, kbr wrote:
> Does this class really need to be CONTENT_EXPORT?

Removed.

> 
> Also, is there any way to guard against accidentally subclassing it?

Make constructor private and add a Create() function.

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_unittest.cc (right):

https://codereview.chromium.org/14794006/diff/62002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_unittest.cc:63: class
GpuDataManagerImplTest : public testing::Test {
On 2013/05/09 20:21:16, kbr wrote:
> This file should be renamed to gpu_data_manager_impl_private_unittest.cc and
> this class renamed.

Done.

[jam, 2013-05-09 23:50:12]

now that there are no macros, it seems that having another private class isn't
needed. if every method starts with "base::AutoLock auto_lock(lock_);" then it
should be obvious to a dev/reviewer that this should be added to new methods. it
seems that one could forget to add the autolock in the current wrapper class as
easily as they could if it was all one class.

[Zhenyao Mo, 2013-05-09 23:57:02]

On 2013/05/09 23:50:12, jam wrote:
> now that there are no macros, it seems that having another private class isn't
> needed. if every method starts with "base::AutoLock auto_lock(lock_);" then it
> should be obvious to a dev/reviewer that this should be added to new methods.
it
> seems that one could forget to add the autolock in the current wrapper class
as
> easily as they could if it was all one class.

Not really.  We only use AutoLock for the public methods, this is because we
don't allow reentrant locks, and original GpuDataManagerImpl functions call each
other.

That's why we get all the public functions in a separate class and do the
AutoLock there, whereas the Private class do not have to worry about locks.

If combined, the mechanism won't be apparent as private functions don't have
AutoLock. Future modification can easily break the thread-safety again.

[Ken Russell (switch to Gerrit), 2013-05-10 00:04:34]

On 2013/05/09 23:57:02, Zhenyao Mo wrote:
> On 2013/05/09 23:50:12, jam wrote:
> > now that there are no macros, it seems that having another private class
isn't
> > needed. if every method starts with "base::AutoLock auto_lock(lock_);" then
it
> > should be obvious to a dev/reviewer that this should be added to new
methods.
> it
> > seems that one could forget to add the autolock in the current wrapper class
> as
> > easily as they could if it was all one class.
> 
> Not really.  We only use AutoLock for the public methods, this is because we
> don't allow reentrant locks, and original GpuDataManagerImpl functions call
each
> other.
> 
> That's why we get all the public functions in a separate class and do the
> AutoLock there, whereas the Private class do not have to worry about locks.
> 
> If combined, the mechanism won't be apparent as private functions don't have
> AutoLock. Future modification can easily break the thread-safety again.

100% agree with what Mo said.

LGTM

[Zhenyao Mo, 2013-05-10 00:16:49]

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.h (right):

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.h:24: class CONTENT_EXPORT
GpuDataManagerImplPrivate {
It turns out this is needed in component build for content_unittests

[Zhenyao Mo, 2013-05-10 00:42:44]

piman: owner review.

[piman, 2013-05-10 00:59:36]

There are some patterns that bother me. Because the lock is not obvious in those
places, it's definitely hard to reason about them.

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:407:
video_memory_usage_stats);
This is called with the lock held. What's to prevent the callee from re-entering
back into GpuDataManager?
Also, what's to prevent the callee from blocking/taking long time, that would
leave the UI and IO threads blocked if they access the GpuDataManager?

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:630:
base::Unretained(owner_),
Why is Unretained safe? Please add a comment, because that's not obvious at all
to me.

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:636: exit_code);
Same concern here with calling up with lock held.

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:683:
base::Unretained(owner_), url, render_process_id,
same concern here with unretained

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:930: url,
render_process_id, render_view_id, requester);
Same concern here with calling out with lock held.

[Zhenyao Mo, 2013-05-10 02:25:21]

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:407:
video_memory_usage_stats);
On 2013/05/10 00:59:36, piman wrote:
> This is called with the lock held. What's to prevent the callee from
re-entering
> back into GpuDataManager?
> Also, what's to prevent the callee from blocking/taking long time, that would
> leave the UI and IO threads blocked if they access the GpuDataManager?

Good point.  Thanks for catching this.

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:630:
base::Unretained(owner_),
On 2013/05/10 00:59:36, piman wrote:
> Why is Unretained safe? Please add a comment, because that's not obvious at
all
> to me.

I think so.  Add a comment.

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:636: exit_code);
On 2013/05/10 00:59:36, piman wrote:
> Same concern here with calling up with lock held.

Done.

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:683:
base::Unretained(owner_), url, render_process_id,
On 2013/05/10 00:59:36, piman wrote:
> same concern here with unretained

Done.

https://codereview.chromium.org/14794006/diff/61009/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:930: url,
render_process_id, render_view_id, requester);
On 2013/05/10 00:59:36, piman wrote:
> Same concern here with calling out with lock held.

Done.

[piman, 2013-05-10 02:50:28]

https://codereview.chromium.org/14794006/diff/86001/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl.h (right):

https://codereview.chromium.org/14794006/diff/86001/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.h:179: // if the owner
GpuDataManagerImpl is null.
In which case would the owner be NULL though? I don't see what resets it.

https://codereview.chromium.org/14794006/diff/86001/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.h:180: // This should only be usedf by
GpuDataManagerImplPrivate where
typo: usedf

https://codereview.chromium.org/14794006/diff/86001/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.h:199: GpuDataManagerImpl* owner_;
nit: DISALLOW_COPY_AND_ASSIGN

[Zhenyao Mo, 2013-05-10 04:27:04]

On 2013/05/10 02:50:28, piman wrote:
>
https://codereview.chromium.org/14794006/diff/86001/content/browser/gpu/gpu_d...
> File content/browser/gpu/gpu_data_manager_impl.h (right):
> 
>
https://codereview.chromium.org/14794006/diff/86001/content/browser/gpu/gpu_d...
> content/browser/gpu/gpu_data_manager_impl.h:179: // if the owner
> GpuDataManagerImpl is null.
> In which case would the owner be NULL though? I don't see what resets it.

In GpuDataManagerImplPrivateTest we directly new GpuDataManagerImplPrivate, in
which case owner_ is null.  We do test the Notify code in the tests, which will
be a problem if we just assume owner_->lock_ is valid.

> 
>
https://codereview.chromium.org/14794006/diff/86001/content/browser/gpu/gpu_d...
> content/browser/gpu/gpu_data_manager_impl.h:180: // This should only be usedf
by
> GpuDataManagerImplPrivate where
> typo: usedf
> 
>
https://codereview.chromium.org/14794006/diff/86001/content/browser/gpu/gpu_d...
> content/browser/gpu/gpu_data_manager_impl.h:199: GpuDataManagerImpl* owner_;
> nit: DISALLOW_COPY_AND_ASSIGN

[Zhenyao Mo, 2013-05-10 17:36:18]

piman: please take a look again.

https://codereview.chromium.org/14794006/diff/86001/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl.h (right):

https://codereview.chromium.org/14794006/diff/86001/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.h:180: // This should only be usedf by
GpuDataManagerImplPrivate where
On 2013/05/10 02:50:28, piman wrote:
> typo: usedf

Done.

https://codereview.chromium.org/14794006/diff/86001/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.h:199: GpuDataManagerImpl* owner_;
On 2013/05/10 02:50:28, piman wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

[Zhenyao Mo, 2013-05-10 17:53:15]

Please wait, I just realized another issue.  Need to do more work before ready
for review.

[Zhenyao Mo, 2013-05-10 18:33:32]

https://codereview.chromium.org/14794006/diff/101001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (left):

https://codereview.chromium.org/14794006/diff/101001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:675:
gpu_switch_callbacks_[i].Run();
This is another place that callbacks can re-enter GpuDataManagerImpl lock.

To make this thread-safe, we have to make this callback list also a
ThreadSafeObserverList, i.e., duplicate all the GpuDataManagerObserverList code.

Originally we didn't make this part of the GpuDataManagerObserverList is that
gpu switching event is only handled on the content side at the moment, and
GpuDataManagerObserver is in content/public.

However, the amount of code duplication and readability over weighs adding a
public content API that's only accessed in content side.

[Zhenyao Mo, 2013-05-10 18:41:16]

Now it's ready for review.

joi: content/public owner.
piman: can you do the final review?

Thank you.

[Zhenyao Mo, 2013-05-10 18:48:19]

Also Ken, the code changed some since your LGTM, so maybe you should also have a
quick look.

[piman, 2013-05-10 19:11:31]

lgtm

[Jói, 2013-05-10 20:34:56]

LGTM for content/public.

[Zhenyao Mo, 2013-05-10 20:43:08]

Committed patchset #12 manually as r199530 (presubmit successful).

[Ken Russell (switch to Gerrit), 2013-05-10 23:59:19]

LGTM

Very happy that the GPU switch callback was merged into the observer class.

@bajones, your Chromium-side CL for dynamically disabling multisampling will
need to be adjusted slightly.

https://codereview.chromium.org/14794006/diff/109001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/14794006/diff/109001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:625: {
This is a little hard to read. Instead of the explicit "return" above, consider
writing this as a proper if/else.

https://codereview.chromium.org/14794006/diff/109001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:881: {
This block doesn't seem necessary.

[Max Vujovic (mvujovic), 2013-05-13 18:45:11]

Looks like this change might have caused the crash described here:
https://code.google.com/p/chromium/issues/detail?id=240365