Split GpuProcessHost into GpuProcessHostUIShim, which runs on the UI...

Split GpuProcessHost into GpuProcessHostUIShim, which runs on the UI
thread, and GpuProcessHost, which now runs on the IO thread and
derives from ChildProcessHost. This split was necessary in order to
service synchronous messages from the renderer process. Moved message
handlers for GPU messages from renderer to browser from
BrowserRenderProcessHost to ResourceMessageFilter.

Stopped sending multiple ViewHostMsg_EstablishGpuChannel messages from
the same renderer if the connection was already established. Resetting
the channel was causing failures in Send, and every other page reload
containing WebGL content to fail. This cleanup will allow further
simplification in the GPU process, but this is being left for a
subsequent CL.

Fixed bug in sandboxing of GPU process. Fixed latent bugs in cleanup
code in GpuChannel and GpuChannelHost. Fixed crashes in
ChildProcessHost if resource_dispatcher_host_ was NULL. Fixed apparent
latent race conditions in creation of BackingStoreProxy and
VideoLayerProxy.

With these changes, WebGL content is running in the sandbox on both
Mac and Windows. Linux support will be added in a following CL.

BUG=29120
TEST=ran WebGL demos on Mac and Windows


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

[Ken Russell (switch to Gerrit), 2010-03-29 03:26:52]

Please review. Thanks.

[jam, 2010-03-29 06:30:54]

http://codereview.chromium.org/1546001/diff/23001/5004
File chrome/browser/child_process_host.cc (right):

http://codereview.chromium.org/1546001/diff/23001/5004#newcode83
chrome/browser/child_process_host.cc:83: if (resource_dispatcher_host_) {
did you see this actually being null?  because the io thread is shutdown before
the RDH is destroyed (and even ignoring that, i don't see any code that nulls
this out).

http://codereview.chromium.org/1546001/diff/23001/5006
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/1546001/diff/23001/5006#newcode20
chrome/browser/gpu_process_host.cc:20: class SendOnUIThreadTask : public Task {
nit: perhaps RouteOnUIThreadTask is more accurate

http://codereview.chromium.org/1546001/diff/23001/5006#newcode138
chrome/browser/gpu_process_host.cc:138: if (EnsureInitialized() && Send(new
GpuMsg_EstablishChannel(renderer_id)))
nit: when we have if/else we almost always use brace brackets

http://codereview.chromium.org/1546001/diff/23001/5006#newcode147
chrome/browser/gpu_process_host.cc:147: if (EnsureInitialized())
nit: this check seems redundant since it's also done inside Send()

http://codereview.chromium.org/1546001/diff/23001/5005
File chrome/browser/gpu_process_host_ui_shim.cc (right):

http://codereview.chromium.org/1546001/diff/23001/5005#newcode7
chrome/browser/gpu_process_host_ui_shim.cc:7: #include
"chrome/browser/gpu_process_host_ui_shim.h"
nit: this needs to be the first include.  this verifies that the header doesn't
depend on any other headers to compile, so we don't get surprise compile
failures if someone else includes this later

http://codereview.chromium.org/1546001/diff/23001/5005#newcode66
chrome/browser/gpu_process_host_ui_shim.cc:66:
CHECK(router_.RouteMessage(message));
I don't think this should be a check.  What if the there are race conditions
between a renderer process dying and a message for it arriving?  Or if a
corrupt/exploited GPU process send a message?  We don't want to crash the
browser for that.

http://codereview.chromium.org/1546001/diff/23001/5017
File chrome/browser/gpu_process_host_ui_shim.h (right):

http://codereview.chromium.org/1546001/diff/23001/5017#newcode47
chrome/browser/gpu_process_host_ui_shim.h:47: int last_routing_id_;
nit: member variables go last

http://codereview.chromium.org/1546001/diff/23001/5009
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/1546001/diff/23001/5009#newcode332
chrome/browser/renderer_host/resource_message_filter.h:332: // channel.
nit: we don't add comments for IPC message handlers, since that would duplicate
what's in the _messages_internal.h file.

[Ken Russell (switch to Gerrit), 2010-03-29 18:14:24]

http://codereview.chromium.org/1546001/diff/23001/5004
File chrome/browser/child_process_host.cc (right):

http://codereview.chromium.org/1546001/diff/23001/5004#newcode83
chrome/browser/child_process_host.cc:83: if (resource_dispatcher_host_) {
On 2010/03/29 06:30:55, John Abd-El-Malek wrote:
> did you see this actually being null?  because the io thread is shutdown
before
> the RDH is destroyed (and even ignoring that, i don't see any code that nulls
> this out).

Yes, I am explicitly passing NULL for the RDH to the ChildProcessHost
constructor, because it seemed there was no need to implement it for the GPU
process.

http://codereview.chromium.org/1546001/diff/23001/5006
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/1546001/diff/23001/5006#newcode20
chrome/browser/gpu_process_host.cc:20: class SendOnUIThreadTask : public Task {
On 2010/03/29 06:30:55, John Abd-El-Malek wrote:
> nit: perhaps RouteOnUIThreadTask is more accurate

Changed.

http://codereview.chromium.org/1546001/diff/23001/5006#newcode138
chrome/browser/gpu_process_host.cc:138: if (EnsureInitialized() && Send(new
GpuMsg_EstablishChannel(renderer_id)))
On 2010/03/29 06:30:55, John Abd-El-Malek wrote:
> nit: when we have if/else we almost always use brace brackets

Fixed. I've gotten too used to WebKit coding style.

http://codereview.chromium.org/1546001/diff/23001/5006#newcode147
chrome/browser/gpu_process_host.cc:147: if (EnsureInitialized())
On 2010/03/29 06:30:55, John Abd-El-Malek wrote:
> nit: this check seems redundant since it's also done inside Send()

Removed. Also removed unnecessary one in EstablishGpuChannel.

http://codereview.chromium.org/1546001/diff/23001/5005
File chrome/browser/gpu_process_host_ui_shim.cc (right):

http://codereview.chromium.org/1546001/diff/23001/5005#newcode7
chrome/browser/gpu_process_host_ui_shim.cc:7: #include
"chrome/browser/gpu_process_host_ui_shim.h"
On 2010/03/29 06:30:55, John Abd-El-Malek wrote:
> nit: this needs to be the first include.  this verifies that the header
doesn't
> depend on any other headers to compile, so we don't get surprise compile
> failures if someone else includes this later

Done.

http://codereview.chromium.org/1546001/diff/23001/5005#newcode66
chrome/browser/gpu_process_host_ui_shim.cc:66:
CHECK(router_.RouteMessage(message));
On 2010/03/29 06:30:55, John Abd-El-Malek wrote:
> I don't think this should be a check.  What if the there are race conditions
> between a renderer process dying and a message for it arriving?  Or if a
> corrupt/exploited GPU process send a message?  We don't want to crash the
> browser for that.

You're right. Removed.

http://codereview.chromium.org/1546001/diff/23001/5017
File chrome/browser/gpu_process_host_ui_shim.h (right):

http://codereview.chromium.org/1546001/diff/23001/5017#newcode47
chrome/browser/gpu_process_host_ui_shim.h:47: int last_routing_id_;
On 2010/03/29 06:30:55, John Abd-El-Malek wrote:
> nit: member variables go last

Fixed.

http://codereview.chromium.org/1546001/diff/23001/5009
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/1546001/diff/23001/5009#newcode332
chrome/browser/renderer_host/resource_message_filter.h:332: // channel.
On 2010/03/29 06:30:55, John Abd-El-Malek wrote:
> nit: we don't add comments for IPC message handlers, since that would
duplicate
> what's in the _messages_internal.h file.

Removed and double-checked that the desired documentation is in
gpu_messages_internal.h.

[jam, 2010-03-29 18:29:08]

lgtm

[Ken Russell (switch to Gerrit), 2010-03-29 19:43:36]

FYI: tested --enable-gpu-rendering on Linux with this patch; working fine.

[brettw, 2010-03-29 23:10:33]

LGTM, this is nicer. Just a few minor style nits.

http://codereview.chromium.org/1546001/diff/39001/40003
File chrome/browser/child_process_host.cc (right):

http://codereview.chromium.org/1546001/diff/39001/40003#newcode83
chrome/browser/child_process_host.cc:83: if (resource_dispatcher_host_) {
Style nit: don't use {} here to match the surrounding code.

http://codereview.chromium.org/1546001/diff/39001/40005
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/1546001/diff/39001/40005#newcode138
chrome/browser/gpu_process_host.cc:138: if (Send(new
GpuMsg_EstablishChannel(renderer_id))) {
Don't add {} here either (especially since this was just old code)

http://codereview.chromium.org/1546001/diff/39001/40004
File chrome/browser/gpu_process_host_ui_shim.cc (right):

http://codereview.chromium.org/1546001/diff/39001/40004#newcode26
chrome/browser/gpu_process_host_ui_shim.cc:26: }  // anonymous namespace
just write // namespace

[Ken Russell (switch to Gerrit), 2010-03-30 00:27:01]

http://codereview.chromium.org/1546001/diff/39001/40003
File chrome/browser/child_process_host.cc (right):

http://codereview.chromium.org/1546001/diff/39001/40003#newcode83
chrome/browser/child_process_host.cc:83: if (resource_dispatcher_host_) {
On 2010/03/29 23:10:33, brettw wrote:
> Style nit: don't use {} here to match the surrounding code.

Done.

http://codereview.chromium.org/1546001/diff/39001/40005
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/1546001/diff/39001/40005#newcode138
chrome/browser/gpu_process_host.cc:138: if (Send(new
GpuMsg_EstablishChannel(renderer_id))) {
On 2010/03/29 23:10:33, brettw wrote:
> Don't add {} here either (especially since this was just old code)

jam commented earlier that {} is preferred around if/else. I'm going to leave
this as is.

http://codereview.chromium.org/1546001/diff/39001/40004
File chrome/browser/gpu_process_host_ui_shim.cc (right):

http://codereview.chromium.org/1546001/diff/39001/40004#newcode26
chrome/browser/gpu_process_host_ui_shim.cc:26: }  // anonymous namespace
On 2010/03/29 23:10:33, brettw wrote:
> just write // namespace

Done.