Blacklist bad GPU drivers: currenly we disable all gpu related features if th...

Blacklist bad GPU drivers: currenly we disable all gpu related features if a (os, device, driver) configuration is on the blacklist.

BUG=58182
TEST=unittest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68948

[brettw, 2010-12-03 00:11:38]

http://codereview.chromium.org/5612002/diff/1/chrome/browser/gpu_blacklist.h
File chrome/browser/gpu_blacklist.h (right):

http://codereview.chromium.org/5612002/diff/1/chrome/browser/gpu_blacklist.h#...
chrome/browser/gpu_blacklist.h:16: #include "base/values.h"
I think you can forward declare DictionaryValue instead of including this in the
header.

http://codereview.chromium.org/5612002/diff/1/chrome/browser/gpu_blacklist.h#...
chrome/browser/gpu_blacklist.h:19: #include "chrome/common/gpu_info.h"
This can be forward declared.

http://codereview.chromium.org/5612002/diff/1/chrome/browser/gpu_blacklist.h#...
chrome/browser/gpu_blacklist.h:75: scoped_ptr<Version> version_;
You can forward declare Version if you add a non-implicit destructor for this
class implemented in the .cc file.

http://codereview.chromium.org/5612002/diff/1/chrome/browser/gpu_process_host.h
File chrome/browser/gpu_process_host.h (right):

http://codereview.chromium.org/5612002/diff/1/chrome/browser/gpu_process_host...
chrome/browser/gpu_process_host.h:15: #include "chrome/browser/gpu_blacklist.h"
I think this can be forward declared instead.

http://codereview.chromium.org/5612002/diff/1/chrome/common/gpu_param_traits.h
File chrome/common/gpu_param_traits.h (right):

http://codereview.chromium.org/5612002/diff/1/chrome/common/gpu_param_traits....
chrome/common/gpu_param_traits.h:14: #include
"chrome/common/gpu_feature_flags.h"
Can this be forward declared instead of introducing more headers?

http://codereview.chromium.org/5612002/diff/1/chrome/renderer/render_thread.h
File chrome/renderer/render_thread.h (right):

http://codereview.chromium.org/5612002/diff/1/chrome/renderer/render_thread.h...
chrome/renderer/render_thread.h:19: #include "chrome/common/gpu_feature_flags.h"
Can the class be forward declared instead of introducing more headers?

[Zhenyao Mo, 2010-12-03 18:29:57]

A second patch will be uploaded.

http://codereview.chromium.org/5612002/diff/1/chrome/browser/gpu_blacklist.h
File chrome/browser/gpu_blacklist.h (right):

http://codereview.chromium.org/5612002/diff/1/chrome/browser/gpu_blacklist.h#...
chrome/browser/gpu_blacklist.h:16: #include "base/values.h"
On 2010/12/03 00:11:38, brettw wrote:
> I think you can forward declare DictionaryValue instead of including this in
the
> header.

Done.

http://codereview.chromium.org/5612002/diff/1/chrome/browser/gpu_blacklist.h#...
chrome/browser/gpu_blacklist.h:19: #include "chrome/common/gpu_info.h"
On 2010/12/03 00:11:38, brettw wrote:
> This can be forward declared.

Done.

http://codereview.chromium.org/5612002/diff/1/chrome/browser/gpu_blacklist.h#...
chrome/browser/gpu_blacklist.h:75: scoped_ptr<Version> version_;
On 2010/12/03 00:11:38, brettw wrote:
> You can forward declare Version if you add a non-implicit destructor for this
> class implemented in the .cc file.

Done.

http://codereview.chromium.org/5612002/diff/1/chrome/browser/gpu_process_host.h
File chrome/browser/gpu_process_host.h (right):

http://codereview.chromium.org/5612002/diff/1/chrome/browser/gpu_process_host...
chrome/browser/gpu_process_host.h:15: #include "chrome/browser/gpu_blacklist.h"
On 2010/12/03 00:11:38, brettw wrote:
> I think this can be forward declared instead.

Done.

http://codereview.chromium.org/5612002/diff/1/chrome/common/gpu_param_traits.h
File chrome/common/gpu_param_traits.h (right):

http://codereview.chromium.org/5612002/diff/1/chrome/common/gpu_param_traits....
chrome/common/gpu_param_traits.h:14: #include
"chrome/common/gpu_feature_flags.h"
Can't do it here.

On 2010/12/03 00:11:38, brettw wrote:
> Can this be forward declared instead of introducing more headers?

http://codereview.chromium.org/5612002/diff/1/chrome/renderer/render_thread.h
File chrome/renderer/render_thread.h (right):

http://codereview.chromium.org/5612002/diff/1/chrome/renderer/render_thread.h...
chrome/renderer/render_thread.h:19: #include "chrome/common/gpu_feature_flags.h"
On 2010/12/03 00:11:38, brettw wrote:
> Can the class be forward declared instead of introducing more headers?

Done.

[brettw, 2010-12-06 17:38:13]

Thanks! I just did a style drive-by, so your "real" reviewers should review it
now :)

[vangelis, 2010-12-08 00:17:38]

Looks good! Just a few inline comments.

http://codereview.chromium.org/5612002/diff/36001/chrome/browser/gpu_blacklis...
File chrome/browser/gpu_blacklist.cc (right):

http://codereview.chromium.org/5612002/diff/36001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.cc:132: std::string os_type = "";
I think the default value for an std::string is the empty string so I doubt you
need to initialize them.  Here and below.

http://codereview.chromium.org/5612002/diff/36001/chrome/browser/gpu_blacklist.h
File chrome/browser/gpu_blacklist.h (right):

http://codereview.chromium.org/5612002/diff/36001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:149: static OsType StringToOsType(const
std::string& os);
nit: it might make more sense to define this as a method of the OsInfo class
instead.

http://codereview.chromium.org/5612002/diff/36001/chrome/browser/resources/gp...
File chrome/browser/resources/gpu_blacklist.json (right):

http://codereview.chromium.org/5612002/diff/36001/chrome/browser/resources/gp...
chrome/browser/resources/gpu_blacklist.json:1: {
It would be useful to put a comment in the file describing the format of the
blacklist and give examples so that folks that need to update it know what to
do.

http://codereview.chromium.org/5612002/diff/36001/chrome/common/gpu_feature_f...
File chrome/common/gpu_feature_flags.h (right):

http://codereview.chromium.org/5612002/diff/36001/chrome/common/gpu_feature_f...
chrome/common/gpu_feature_flags.h:20: kGpuFeatureAny,
it's not clear what kGpuFeatureAny really does and I don't see it being handled
in the .cc file.  Maybe remove?

http://codereview.chromium.org/5612002/diff/36001/chrome/common/gpu_feature_f...
chrome/common/gpu_feature_flags.h:53: bool
is_accelerated_2d_canvas_blacklisted_;
It would be cleaner to use a single bitfield (e.g. an unsigned int) for all the
features instead of separate bools so that additional features can be added
without changing function signatures. If more features are added then the
SetFlags method will get increasingly confusing as you'll be passing several
floats.

http://codereview.chromium.org/5612002/diff/36001/chrome/common/gpu_messages.cc
File chrome/common/gpu_messages.cc (right):

http://codereview.chromium.org/5612002/diff/36001/chrome/common/gpu_messages....
chrome/common/gpu_messages.cc:145: ret = ret && ReadParam(m, iter,
&is_accelerated_compositing_blacklisted);
nit: combine all three in a single statement that &&'s all the ReadParams.

http://codereview.chromium.org/5612002/diff/36001/chrome/renderer/render_thre...
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/5612002/diff/36001/chrome/renderer/render_thre...
chrome/renderer/render_thread.cc:1093: if
(gpu_feature_flags.is_accelerated_2d_canvas_blacklisted() ||
It would be worth putting a comment here mentioning that for now we don't
establish a gpu channel if any of the features is blacklisted.

[Ken Russell (switch to Gerrit), 2010-12-08 05:04:15]

http://codereview.chromium.org/5612002/diff/36001/chrome/renderer/render_thre...
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/5612002/diff/36001/chrome/renderer/render_thre...
chrome/renderer/render_thread.cc:1095: gpu_feature_flags.is_webgl_blacklisted())
{
There's an architectural problem with doing these checks here, which is that the
renderer process is untrusted. A compromised renderer might cause these bits to
be set to false even though the browser process said that it isn't supposed to
be allowed to establish a channel to the GPU process.

I think this CL should be restructured to send the blacklist information from
the browser to the GPU process in GpuProcessHost::EstablishGpuChannel. The GPU
process should then determine whether to create a channel or not. If not, then
send back an invalid channel handle. GpuProcessHost::OnChannelEstablished and
RenderThread::OnGpuChannelEstablished then shouldn't need to be modified at all.

We won't be able to support blacklisting of individual features with this new
structure, at least not by preventing a GPU channel from being created for this
renderer process. There is one GPU channel per renderer, and when a new "OpenGL"
context is created for an API like WebGL or accelerated canvas 2D, the channel
is used to create a new command buffer. The creation of those command buffers is
eventually going to have to check these blacklist flags, but since we're
starting off by disabling GPU support if any of these flags are set, the safest
initial solution is what you've done here, to just prevent the GPU channel from
being established. It's only where we make the choice to not establish the
channel that's the concern.

[Zhenyao Mo, 2010-12-09 00:06:06]

Also, if any of the features are blacklisted, browser will kill the gpu channel
(instead of killing it in renderer as in the previous patch).

http://codereview.chromium.org/5612002/diff/36001/chrome/browser/gpu_blacklis...
File chrome/browser/gpu_blacklist.cc (right):

http://codereview.chromium.org/5612002/diff/36001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.cc:132: std::string os_type = "";
On 2010/12/08 00:17:38, vangelis wrote:
> I think the default value for an std::string is the empty string so I doubt
you
> need to initialize them.  Here and below.

Done.

http://codereview.chromium.org/5612002/diff/36001/chrome/browser/gpu_blacklist.h
File chrome/browser/gpu_blacklist.h (right):

http://codereview.chromium.org/5612002/diff/36001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:149: static OsType StringToOsType(const
std::string& os);
On 2010/12/08 00:17:38, vangelis wrote:
> nit: it might make more sense to define this as a method of the OsInfo class
> instead.

Done.

http://codereview.chromium.org/5612002/diff/36001/chrome/browser/resources/gp...
File chrome/browser/resources/gpu_blacklist.json (right):

http://codereview.chromium.org/5612002/diff/36001/chrome/browser/resources/gp...
chrome/browser/resources/gpu_blacklist.json:1: {
As I discussed with Vangelis offline, we will add documentation on
gpu_blacklist.h instead of here, as this json file will be loaded by browser
process, so the smaller the file, the better.

On 2010/12/08 00:17:38, vangelis wrote:
> It would be useful to put a comment in the file describing the format of the
> blacklist and give examples so that folks that need to update it know what to
> do.

http://codereview.chromium.org/5612002/diff/36001/chrome/common/gpu_feature_f...
File chrome/common/gpu_feature_flags.h (right):

http://codereview.chromium.org/5612002/diff/36001/chrome/common/gpu_feature_f...
chrome/common/gpu_feature_flags.h:20: kGpuFeatureAny,
On 2010/12/08 00:17:38, vangelis wrote:
> it's not clear what kGpuFeatureAny really does and I don't see it being
handled
> in the .cc file.  Maybe remove? 

Done.

http://codereview.chromium.org/5612002/diff/36001/chrome/common/gpu_feature_f...
chrome/common/gpu_feature_flags.h:53: bool
is_accelerated_2d_canvas_blacklisted_;
On 2010/12/08 00:17:38, vangelis wrote:
> It would be cleaner to use a single bitfield (e.g. an unsigned int) for all
the
> features instead of separate bools so that additional features can be added
> without changing function signatures. If more features are added then the
> SetFlags method will get increasingly confusing as you'll be passing several
> floats.

Done.

http://codereview.chromium.org/5612002/diff/36001/chrome/common/gpu_messages.cc
File chrome/common/gpu_messages.cc (right):

http://codereview.chromium.org/5612002/diff/36001/chrome/common/gpu_messages....
chrome/common/gpu_messages.cc:145: ret = ret && ReadParam(m, iter,
&is_accelerated_compositing_blacklisted);
On 2010/12/08 00:17:38, vangelis wrote:
> nit: combine all three in a single statement that &&'s all the ReadParams. 

Done.

http://codereview.chromium.org/5612002/diff/36001/chrome/renderer/render_thre...
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/5612002/diff/36001/chrome/renderer/render_thre...
chrome/renderer/render_thread.cc:1093: if
(gpu_feature_flags.is_accelerated_2d_canvas_blacklisted() ||
On 2010/12/08 00:17:38, vangelis wrote:
> It would be worth putting a comment here mentioning that for now we don't
> establish a gpu channel if any of the features is blacklisted.

Done.

[Ken Russell (switch to Gerrit), 2010-12-10 00:53:00]

This approach looks better to me. LGTM.

[vangelis, 2010-12-10 18:17:24]

Looks good. Just a couple of smaller comments and it's good to go.

http://codereview.chromium.org/5612002/diff/65001/chrome/browser/gpu_blacklist.h
File chrome/browser/gpu_blacklist.h (right):

http://codereview.chromium.org/5612002/diff/65001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:31: //    expanded in the future).
Should we add an "all" feature that turns off everything?  Also, I think you
should mention here that in the current implementation, there's no way to turn
off individual features just to make sure there's no confusion.

http://codereview.chromium.org/5612002/diff/65001/chrome/common/gpu_feature_f...
File chrome/common/gpu_feature_flags.h (right):

http://codereview.chromium.org/5612002/diff/65001/chrome/common/gpu_feature_f...
chrome/common/gpu_feature_flags.h:19: kGpuFeatureAccelerated2dCanvas = 1,
Since you're expecting to be able to do binary operations with these, it might
be better to define their values as:

1 << 0
1 << 1
1 << 2

etc.  to make sure they stay as powers of two.

[Timur Iskhodzhanov, 2010-12-12 13:26:48]

This patch has made all the UI Valgrind bots red on the memory waterfall
Please watch the memory waterfall status after commiting!

On 2010/12/10 18:17:24, vangelis wrote:
> Looks good. Just a couple of smaller comments and it's good to go.
> 
>
http://codereview.chromium.org/5612002/diff/65001/chrome/browser/gpu_blacklist.h
> File chrome/browser/gpu_blacklist.h (right):
> 
>
http://codereview.chromium.org/5612002/diff/65001/chrome/browser/gpu_blacklis...
> chrome/browser/gpu_blacklist.h:31: //    expanded in the future).
> Should we add an "all" feature that turns off everything?  Also, I think you
> should mention here that in the current implementation, there's no way to turn
> off individual features just to make sure there's no confusion.
> 
>
http://codereview.chromium.org/5612002/diff/65001/chrome/common/gpu_feature_f...
> File chrome/common/gpu_feature_flags.h (right):
> 
>
http://codereview.chromium.org/5612002/diff/65001/chrome/common/gpu_feature_f...
> chrome/common/gpu_feature_flags.h:19: kGpuFeatureAccelerated2dCanvas = 1,
> Since you're expecting to be able to do binary operations with these, it might
> be better to define their values as:
> 
> 1 << 0
> 1 << 1
> 1 << 2
> 
> etc.  to make sure they stay as powers of two.