gpu: Remove Create/DeleteImage IPC by adding an X11_PIXMAP_BUFFER GpuMemoryBuffer type.

gpu: Remove Create/DeleteImage IPC by adding an X11_PIXMAP_BUFFER GpuMemoryBuffer type.

This adds a new GpuMemoryBuffer type that can be used to create
a GpuMemoryBuffer from an existing X11 pixmap.

This removes Create/DeleteImage IPC and reduces complexity
significantly as it allows the ImageManager to be moved to
the decoder and simply track images.

A new platform dependent GpuMemoryBufferFactory interface
is introduced to allow this new type of buffer to be created
on the GPU service side. To avoid the need for any global
variables, this factory instance is also responsible for
creating GLImage instances.

The old factory interface used by android_webview is renamed
InProcessGpuMemoryBufferFactory until it can be removed in
favor of this new interface.

BUG=368716
TEST=gpu_unittests, gl_tests --gtest_filter=GpuMemoryBufferTest.Lifecycle

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

[reveman, 2014-06-12 20:29:00]

This is a significant step towards removing Create/DeleteImage IPC. With
addition of a X11_PIXMAP buffer type we can simply remove Create/DeleteImage
once Create/DestroyGpuMemoryBuffer IPC has been properly implemented on the GPU
process side.

Please take a look.

[alexst (slow to review), 2014-06-12 22:29:22]

https://codereview.chromium.org/331723003/diff/1/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host.h (right):

https://codereview.chromium.org/331723003/diff/1/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host.h:130: int client_id,
I don't know that it's super important for this particular CL, but
GpuMemoryBufferHandle.global_id was meant to store a pair of <image_id,
client_id>. Maybe it makes sense to start using those here instead of also
adding them to all the functions.

[piman, 2014-06-13 01:31:27]

So, now, any client (including untrusted ones) can send a
GpuCommandBufferMsg_RegisterGpuMemoryBuffer and bind any pixmap on the system to
a texture, and read its bits. Probably not wanted....

Should we blacklist (or, better, whitelist?) handle types in
GpuCommandBufferStub::OnRegisterGpuMemoryBuffer?


Alternatively, we could make the browser's channel privileged somehow and only
allow that one to create X11-type GpuMemoryBuffers, and then remove the
CreateImage path on the GpuChannelHostFactory, moving it to
CommandBufferProxyImpl. That way:
- it removes a bunch of logic, the CommandBufferProxyImpl implementation would
be much simpler I think (wrt synchronization issues)
- it removes the problem of the image ids colliding with the GpuMemoryBuffer ids

https://codereview.chromium.org/331723003/diff/1/ui/gl/gl_image_glx.cc
File ui/gl/gl_image_glx.cc (right):

https://codereview.chromium.org/331723003/diff/1/ui/gl/gl_image_glx.cc#newcod...
ui/gl/gl_image_glx.cc:127: DCHECK_EQ(size_.ToString(),
ActualPixmapSize(buffer.pixmap).ToString());
DCHECKs, or return failure instead?

[reveman, 2014-06-13 16:45:11]

I was trying to keep the amount of changes minimal in the initial patch but that
was clearly a bad idea. Thanks for noticing this major flaw and sorry for the
confusion about how this is supposed to work.

Latest patch includes what would have been the follow up patch.

Here's how the general gpu process side buffer creation flow works:

1. Browser process creates a GPU memory buffer using the CreateImage (soon to be
replaced by CreateGpuMemoryBuffer) IPC. This is and have always been limited to
the browser process.
1.2. This creates a global "ID -> buffer" mapping on the GPU process side. The
ID is owned by specific client.
2. A renderer can use an ID that it owns to registers a GPU memory buffer with a
command buffer. This will create a GLImage that can be bound to texture.

In the case of CreateImage, step 2 is done as part of handling the CreateImage
IPC but we'll remove all that and instead use CreateGpuMemoryBuffer IPC in a
follow up patch.

PTAL

https://codereview.chromium.org/331723003/diff/1/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host.h (right):

https://codereview.chromium.org/331723003/diff/1/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host.h:130: int client_id,
On 2014/06/12 22:29:22, alexst wrote:
> I don't know that it's super important for this particular CL, but
> GpuMemoryBufferHandle.global_id was meant to store a pair of <image_id,
> client_id>. Maybe it makes sense to start using those here instead of also
> adding them to all the functions.

We could use GpuMemoryBufferId::secondary_id instead but with all this code very
soon being removed I don't think it matters. I prefer to keep client_id for now
to keep things simple.

Fyi, the primary_id that's part of GpuMemoryBufferId is not an image id. It's a
gpu memory buffer id. The important difference is that the first is global and
an image id is specific to a command buffer. The image id is the result of
registering a gpu memory buffer with a specific command buffer and creating an
image object.

https://codereview.chromium.org/331723003/diff/1/ui/gl/gl_image_glx.cc
File ui/gl/gl_image_glx.cc (right):

https://codereview.chromium.org/331723003/diff/1/ui/gl/gl_image_glx.cc#newcod...
ui/gl/gl_image_glx.cc:127: DCHECK_EQ(size_.ToString(),
ActualPixmapSize(buffer.pixmap).ToString());
On 2014/06/13 01:31:27, piman wrote:
> DCHECKs, or return failure instead?

I'm using DCHECKs to avoid unnecessary round-trips to the X server in release
builds. Probably not a big difference on chromeos where the X server is
typically not busy but I think it can make a difference and generally makes
sense on Linux where we have no idea what other X clients are doing.

FYI, the only way these DCHECKs can fail is a misbehaving browser process. I
renderer shouldn't be able to do anything that will affect these.

https://codereview.chromium.org/331723003/diff/20001/ui/gl/x11_pixmap_tracker.h
File ui/gl/x11_pixmap_tracker.h (right):

https://codereview.chromium.org/331723003/diff/20001/ui/gl/x11_pixmap_tracker...
ui/gl/x11_pixmap_tracker.h:6: #define UI_GL_X11_PIXMAP_TRACKER_H_
Maybe move this into a ui/gl/x/ sub directory..

[piman, 2014-06-13 17:14:21]

https://codereview.chromium.org/331723003/diff/1/ui/gl/gl_image_glx.cc
File ui/gl/gl_image_glx.cc (right):

https://codereview.chromium.org/331723003/diff/1/ui/gl/gl_image_glx.cc#newcod...
ui/gl/gl_image_glx.cc:127: DCHECK_EQ(size_.ToString(),
ActualPixmapSize(buffer.pixmap).ToString());
On 2014/06/13 16:45:11, reveman wrote:
> On 2014/06/13 01:31:27, piman wrote:
> > DCHECKs, or return failure instead?
> 
> I'm using DCHECKs to avoid unnecessary round-trips to the X server in release
> builds. Probably not a big difference on chromeos where the X server is
> typically not busy but I think it can make a difference and generally makes
> sense on Linux where we have no idea what other X clients are doing.
> 
> FYI, the only way these DCHECKs can fail is a misbehaving browser process. I
> renderer shouldn't be able to do anything that will affect these.

Ok.

https://codereview.chromium.org/331723003/diff/20001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/331723003/diff/20001/content/common/gpu/gpu_c...
content/common/gpu/gpu_channel_manager.cc:70: LAZY_INSTANCE_INITIALIZER;
Why not simply making it a member of GpuChannelManager?

[reveman, 2014-06-13 18:50:34]

https://codereview.chromium.org/331723003/diff/20001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/331723003/diff/20001/content/common/gpu/gpu_c...
content/common/gpu/gpu_channel_manager.cc:70: LAZY_INSTANCE_INITIALIZER;
On 2014/06/13 17:14:21, piman wrote:
> Why not simply making it a member of GpuChannelManager?

No other reason than hiding as much as possible in the .cc file. I think the
implementation should move to a separate file if we make this a member. That's
fine with me though and maybe the best solution. Another alternative is to just
have the GpuChannelManager class implement this interface. Latest patch does
that. All these options are OK with me as long as we're consistent in how we
track other types of objects that can back gpu memory buffers. ie. surface
textures.

Let me know what you prefer.

[alexst (slow to review), 2014-06-13 19:15:57]

https://codereview.chromium.org/331723003/diff/40001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_channel_manager.h (right):

https://codereview.chromium.org/331723003/diff/40001/content/common/gpu/gpu_c...
content/common/gpu/gpu_channel_manager.h:65: public gfx::X11PixmapTracker
I like this general style, but as you mentioned there will be a number of these
to implement, and I'm not sure crowding them all into the ChannelManager class
is the best thing to do. 

I'm already adding SurfaceTextureTracker and EGLPixmapTracker, and before too
long, we will probably have a DMABufTracker, etc.

Maybe we have a separate class called something like
GpuMemoryBufferHandleTrackerImpl that would be a member here and would implement
all these interfaces.

I would even volunteer to add it as a followup if we think it's a good way to go
because I already have it for SurfaceTexture.

[reveman, 2014-06-13 19:27:57]

https://codereview.chromium.org/331723003/diff/40001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_channel_manager.h (right):

https://codereview.chromium.org/331723003/diff/40001/content/common/gpu/gpu_c...
content/common/gpu/gpu_channel_manager.h:65: public gfx::X11PixmapTracker
On 2014/06/13 19:15:57, alexst wrote:
> I like this general style, but as you mentioned there will be a number of
these
> to implement, and I'm not sure crowding them all into the ChannelManager class
> is the best thing to do. 

I agree. This was a bad idea.

> 
> I'm already adding SurfaceTextureTracker and EGLPixmapTracker, and before too
> long, we will probably have a DMABufTracker, etc.
> 
> Maybe we have a separate class called something like
> GpuMemoryBufferHandleTrackerImpl that would be a member here and would
implement
> all these interfaces.
> 
> I would even volunteer to add it as a followup if we think it's a good way to
go
> because I already have it for SurfaceTexture.

I'd also like to avoid a lot of these ifdefs. A platform specific
GpuMemoryBufferFactory interface is really what we want here but I was hoping we
could save that to a follow up patch. Let me try to implement it as part of this
patch and then we can see if it's possible to split this up somehow.

[reveman, 2014-06-13 20:55:06]

https://codereview.chromium.org/331723003/diff/40001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_channel_manager.h (right):

https://codereview.chromium.org/331723003/diff/40001/content/common/gpu/gpu_c...
content/common/gpu/gpu_channel_manager.h:65: public gfx::X11PixmapTracker
On 2014/06/13 19:27:56, reveman wrote:
> On 2014/06/13 19:15:57, alexst wrote:
> > I like this general style, but as you mentioned there will be a number of
> these
> > to implement, and I'm not sure crowding them all into the ChannelManager
class
> > is the best thing to do. 
> 
> I agree. This was a bad idea.
> 
> > 
> > I'm already adding SurfaceTextureTracker and EGLPixmapTracker, and before
too
> > long, we will probably have a DMABufTracker, etc.
> > 
> > Maybe we have a separate class called something like
> > GpuMemoryBufferHandleTrackerImpl that would be a member here and would
> implement
> > all these interfaces.
> > 
> > I would even volunteer to add it as a followup if we think it's a good way
to
> go
> > because I already have it for SurfaceTexture.
> 
> I'd also like to avoid a lot of these ifdefs. A platform specific
> GpuMemoryBufferFactory interface is really what we want here but I was hoping
we
> could save that to a follow up patch. Let me try to implement it as part of
this
> patch and then we can see if it's possible to split this up somehow.

Take a look at latest patch. It adds a GpuMemoryBufferFactory interface on the
GPU process side that can be used for all this platform/implementation specific
code. This makes it pretty clear how the GpuMemoryBufferFactoryHost code is
connected to the GPU process.

[piman, 2014-06-13 21:04:16]

On 2014/06/13 20:55:06, reveman wrote:
>
https://codereview.chromium.org/331723003/diff/40001/content/common/gpu/gpu_c...
> File content/common/gpu/gpu_channel_manager.h (right):
> 
>
https://codereview.chromium.org/331723003/diff/40001/content/common/gpu/gpu_c...
> content/common/gpu/gpu_channel_manager.h:65: public gfx::X11PixmapTracker
> On 2014/06/13 19:27:56, reveman wrote:
> > On 2014/06/13 19:15:57, alexst wrote:
> > > I like this general style, but as you mentioned there will be a number of
> > these
> > > to implement, and I'm not sure crowding them all into the ChannelManager
> class
> > > is the best thing to do. 
> > 
> > I agree. This was a bad idea.
> > 
> > > 
> > > I'm already adding SurfaceTextureTracker and EGLPixmapTracker, and before
> too
> > > long, we will probably have a DMABufTracker, etc.
> > > 
> > > Maybe we have a separate class called something like
> > > GpuMemoryBufferHandleTrackerImpl that would be a member here and would
> > implement
> > > all these interfaces.
> > > 
> > > I would even volunteer to add it as a followup if we think it's a good way
> to
> > go
> > > because I already have it for SurfaceTexture.
> > 
> > I'd also like to avoid a lot of these ifdefs. A platform specific
> > GpuMemoryBufferFactory interface is really what we want here but I was
hoping
> we
> > could save that to a follow up patch. Let me try to implement it as part of
> this
> > patch and then we can see if it's possible to split this up somehow.
> 
> Take a look at latest patch. It adds a GpuMemoryBufferFactory interface on the
> GPU process side that can be used for all this platform/implementation
specific
> code. This makes it pretty clear how the GpuMemoryBufferFactoryHost code is
> connected to the GPU process.

I prefer members to globals. globals are a pain, even if it seems to make things
easier right now, you always have to pay the cost later. We just had to remove
globals from ui/compositor and that was painful. I think we can make
GpuMemoryBufferFactory a member of GpuChannelManager and pass it wherever we
need it.

[alexst (slow to review), 2014-06-17 03:23:17]

Pending Antoine's remarks, lgtm.

[reveman, 2014-06-17 20:36:16]

On 2014/06/13 21:04:16, piman wrote:
> On 2014/06/13 20:55:06, reveman wrote:
> >
>
https://codereview.chromium.org/331723003/diff/40001/content/common/gpu/gpu_c...
> > File content/common/gpu/gpu_channel_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/331723003/diff/40001/content/common/gpu/gpu_c...
> > content/common/gpu/gpu_channel_manager.h:65: public gfx::X11PixmapTracker
> > On 2014/06/13 19:27:56, reveman wrote:
> > > On 2014/06/13 19:15:57, alexst wrote:
> > > > I like this general style, but as you mentioned there will be a number
of
> > > these
> > > > to implement, and I'm not sure crowding them all into the ChannelManager
> > class
> > > > is the best thing to do. 
> > > 
> > > I agree. This was a bad idea.
> > > 
> > > > 
> > > > I'm already adding SurfaceTextureTracker and EGLPixmapTracker, and
before
> > too
> > > > long, we will probably have a DMABufTracker, etc.
> > > > 
> > > > Maybe we have a separate class called something like
> > > > GpuMemoryBufferHandleTrackerImpl that would be a member here and would
> > > implement
> > > > all these interfaces.
> > > > 
> > > > I would even volunteer to add it as a followup if we think it's a good
way
> > to
> > > go
> > > > because I already have it for SurfaceTexture.
> > > 
> > > I'd also like to avoid a lot of these ifdefs. A platform specific
> > > GpuMemoryBufferFactory interface is really what we want here but I was
> hoping
> > we
> > > could save that to a follow up patch. Let me try to implement it as part
of
> > this
> > > patch and then we can see if it's possible to split this up somehow.
> > 
> > Take a look at latest patch. It adds a GpuMemoryBufferFactory interface on
the
> > GPU process side that can be used for all this platform/implementation
> specific
> > code. This makes it pretty clear how the GpuMemoryBufferFactoryHost code is
> > connected to the GPU process.
> 
> I prefer members to globals. globals are a pain, even if it seems to make
things
> easier right now, you always have to pay the cost later. We just had to remove
> globals from ui/compositor and that was painful. I think we can make
> GpuMemoryBufferFactory a member of GpuChannelManager and pass it wherever we
> need it.

There's no GpuMemoryBufferFactory instance in latest patch. Just the interface
that exists for the purpose of avoiding ifdefs and making the code cleaner. The
GpuMemoryBufferFactoryX11Pixmap interface, used to register X11 pixmaps, use a
global variable internally as a mechanism for providing X11 pixmaps to the GLX
specific GLImage type. It would be nice to not use a global variable for this
but I'm not sure what's the best way to accomplish that. Maybe
RequestAccessToGpuMemoryBuffer could transform a secure handle into an insecure
handle instead of just returning true/false. I need some more time to think
about this.

As alexst is blocked on this and so is fixing crbug.com/375507, I'd suggest that
we land something like the current version of this patch for now and look at
removing the use of the global variable in the X11 pixmap factory as a follow
up.

[piman, 2014-06-17 23:38:11]

https://codereview.chromium.org/331723003/diff/60001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/331723003/diff/60001/content/common/gpu/gpu_c...
content/common/gpu/gpu_command_buffer_stub.cc:943: }
Because XID is a field of handle, if you grab the XID here from the map, then
you don't need to access that map from ui/gl at all, so you don't need to pass
state via a global (since you can simply stash the map on the
GpuChannelManager). fewer interfaces, less code.

https://codereview.chromium.org/331723003/diff/60001/content/common/gpu/gpu_m...
File content/common/gpu/gpu_memory_buffer_factory_x11_pixmap.h (right):

https://codereview.chromium.org/331723003/diff/60001/content/common/gpu/gpu_m...
content/common/gpu/gpu_memory_buffer_factory_x11_pixmap.h:22: virtual
~GpuMemoryBufferFactoryX11Pixmap();
nit: remove. You're not creating instances of this class, and you're not
defining this destructor.

[reveman, 2014-06-18 22:06:28]

https://codereview.chromium.org/331723003/diff/60001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/331723003/diff/60001/content/common/gpu/gpu_c...
content/common/gpu/gpu_command_buffer_stub.cc:943: }
On 2014/06/17 23:38:11, piman wrote:
> Because XID is a field of handle, if you grab the XID here from the map, then
> you don't need to access that map from ui/gl at all, so you don't need to pass
> state via a global (since you can simply stash the map on the
> GpuChannelManager). fewer interfaces, less code.

That works in the case of X11 pixmaps and XIDs but it would be a different story
with surface textures for example. I'm not sure it's a good idea to turn the
handle into some arbitrary struct for passing data between the stub and the
GLImage system as it's supposed to represent a serializable reference to a gpu
memory buffer. Maybe we need some new type of in-process gpu memory buffer
handle that can be used here..

https://codereview.chromium.org/331723003/diff/60001/content/common/gpu/gpu_m...
File content/common/gpu/gpu_memory_buffer_factory_x11_pixmap.h (right):

https://codereview.chromium.org/331723003/diff/60001/content/common/gpu/gpu_m...
content/common/gpu/gpu_memory_buffer_factory_x11_pixmap.h:22: virtual
~GpuMemoryBufferFactoryX11Pixmap();
On 2014/06/17 23:38:11, piman wrote:
> nit: remove. You're not creating instances of this class, and you're not
> defining this destructor.

Added an empty protected implementation as I'd rather not make it possible to
create an instance of this class.

[piman, 2014-06-18 22:14:36]

https://codereview.chromium.org/331723003/diff/60001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/331723003/diff/60001/content/common/gpu/gpu_c...
content/common/gpu/gpu_command_buffer_stub.cc:943: }
On 2014/06/18 22:06:28, reveman wrote:
> On 2014/06/17 23:38:11, piman wrote:
> > Because XID is a field of handle, if you grab the XID here from the map,
then
> > you don't need to access that map from ui/gl at all, so you don't need to
pass
> > state via a global (since you can simply stash the map on the
> > GpuChannelManager). fewer interfaces, less code.
> 
> That works in the case of X11 pixmaps and XIDs but it would be a different
story
> with surface textures for example. I'm not sure it's a good idea to turn the
> handle into some arbitrary struct for passing data between the stub and the
> GLImage system as it's supposed to represent a serializable reference to a gpu
> memory buffer. Maybe we need some new type of in-process gpu memory buffer
> handle that can be used here..

If the GpuMemoryBufferHandle doesn't include the native handle, then I'm not
sure about the parts of this CL that pass the XID through it for the CreateImage
path.

Either way, if we need a data structure to do the GpuMemoryBufferHandle->native
handle mapping, that can be owned by the GpuChannelManager and passed explicitly
to GLImage::CreateGLImageForGpuMemoryBuffer. No need for globals for that.

[alexst (slow to review), 2014-06-18 22:54:50]

https://codereview.chromium.org/331723003/diff/80001/content/common/gpu/gpu_m...
File content/common/gpu/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/331723003/diff/80001/content/common/gpu/gpu_m...
content/common/gpu/gpu_memory_buffer_factory.h:15: static void Initialize();
What if we made this static GpuMemoryBufferFactory*
CreateGpuMemoryBufferFactory(), which allows platform specific allocation, and
stored the instance in channel_manager and injected it where necessary.

Then below functions would no longer be static and we can add another method
along the lines of

GpuProcessMemoryBuffer* AcquireMemoryBuffer(int primary_id, int secondary_id)

and have

struct GpuProcessMemoryBuffer {
#ifdef X11
 XID xid;
#endif
#ifdef ANDROID
 SurfaceTexture surface_texture;
#endif
//etc.
}

This should give us an in-process handle and remove globals.

[piman, 2014-06-18 23:02:07]

On Wed, Jun 18, 2014 at 3:54 PM, <alexst@chromium.org> wrote:

>
> https://codereview.chromium.org/331723003/diff/80001/
> content/common/gpu/gpu_memory_buffer_factory.h
> File content/common/gpu/gpu_memory_buffer_factory.h (right):
>
> https://codereview.chromium.org/331723003/diff/80001/
> content/common/gpu/gpu_memory_buffer_factory.h#newcode15
> content/common/gpu/gpu_memory_buffer_factory.h:15: static void
> Initialize();
> What if we made this static GpuMemoryBufferFactory*
> CreateGpuMemoryBufferFactory(), which allows platform specific
> allocation, and stored the instance in channel_manager and injected it
> where necessary.
>
> Then below functions would no longer be static and we can add another
> method along the lines of
>
> GpuProcessMemoryBuffer* AcquireMemoryBuffer(int primary_id, int
> secondary_id)
>
> and have
>
> struct GpuProcessMemoryBuffer {
> #ifdef X11
>  XID xid;
> #endif
> #ifdef ANDROID
>  SurfaceTexture surface_texture;
> #endif
> //etc.
> }
>
> This should give us an in-process handle and remove globals.
>

SGTM.

>
> https://codereview.chromium.org/331723003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2014-06-19 14:56:12]

https://codereview.chromium.org/331723003/diff/80001/content/common/gpu/gpu_m...
File content/common/gpu/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/331723003/diff/80001/content/common/gpu/gpu_m...
content/common/gpu/gpu_memory_buffer_factory.h:15: static void Initialize();
On 2014/06/18 22:54:49, alexst wrote:
> What if we made this static GpuMemoryBufferFactory*
> CreateGpuMemoryBufferFactory(), which allows platform specific allocation, and
> stored the instance in channel_manager and injected it where necessary.
> 
> Then below functions would no longer be static and we can add another method
> along the lines of
> 
> GpuProcessMemoryBuffer* AcquireMemoryBuffer(int primary_id, int secondary_id)
> 
> and have
> 
> struct GpuProcessMemoryBuffer {
> #ifdef X11
>  XID xid;
> #endif
> #ifdef ANDROID
>  SurfaceTexture surface_texture;
> #endif
> //etc.
> }
> 
> This should give us an in-process handle and remove globals.

Not a fan of the idea of another struct with ifdef-ed fields. It makes sense for
the handle that needs to be used with IPC but there shouldn't be necessary to
use this for an in-process handle.

Maybe we could do something like this:

class GpuMemoryBufferFactory {
 public:
  static scoped_ptr<GpuMemoryBufferFactory> Create();

  virtual bool CreateGpuMemoryBuffer(
      const gfx::GpuMemoryBufferHandle& handle, ...);
  virtual void DestroyGpuMemoryBuffer(
      const gfx::GpuMemoryBufferHandle& handle);

  scoped_refptr<gfx::GLImage> AcquireImage(
      const gfx::GpuMemoryBufferHandle& handle);
}

and kill the GLimage factory in ui/gl.

[reveman, 2014-06-19 15:04:08]

On 2014/06/18 22:14:36, piman wrote:
>
https://codereview.chromium.org/331723003/diff/60001/content/common/gpu/gpu_c...
> File content/common/gpu/gpu_command_buffer_stub.cc (right):
> 
>
https://codereview.chromium.org/331723003/diff/60001/content/common/gpu/gpu_c...
> content/common/gpu/gpu_command_buffer_stub.cc:943: }
> On 2014/06/18 22:06:28, reveman wrote:
> > On 2014/06/17 23:38:11, piman wrote:
> > > Because XID is a field of handle, if you grab the XID here from the map,
> then
> > > you don't need to access that map from ui/gl at all, so you don't need to
> pass
> > > state via a global (since you can simply stash the map on the
> > > GpuChannelManager). fewer interfaces, less code.
> > 
> > That works in the case of X11 pixmaps and XIDs but it would be a different
> story
> > with surface textures for example. I'm not sure it's a good idea to turn the
> > handle into some arbitrary struct for passing data between the stub and the
> > GLImage system as it's supposed to represent a serializable reference to a
gpu
> > memory buffer. Maybe we need some new type of in-process gpu memory buffer
> > handle that can be used here..
> 
> If the GpuMemoryBufferHandle doesn't include the native handle, then I'm not
> sure about the parts of this CL that pass the XID through it for the
CreateImage
> path.

FYI, the X11 pixmap case is unique as it creates a GpuMemoryBuffer from an
existing X11 pixmap (there wouldn't be am XID pixmap field in the handle if the
pixmap was created on the gpu process side). We could take advantage of that in
this patch but I think that's a mistake as it only solves the problem for this
patch and we'll need to redesign it again for surface texture and io surface
support.

> 
> Either way, if we need a data structure to do the
GpuMemoryBufferHandle->native
> handle mapping, that can be owned by the GpuChannelManager and passed
explicitly
> to GLImage::CreateGLImageForGpuMemoryBuffer. No need for globals for that.

[alexst (slow to review), 2014-06-19 16:08:20]

On 2014/06/19 14:56:12, reveman wrote:
>
https://codereview.chromium.org/331723003/diff/80001/content/common/gpu/gpu_m...
> File content/common/gpu/gpu_memory_buffer_factory.h (right):
> 
>
https://codereview.chromium.org/331723003/diff/80001/content/common/gpu/gpu_m...
> content/common/gpu/gpu_memory_buffer_factory.h:15: static void Initialize();
> On 2014/06/18 22:54:49, alexst wrote:
> > What if we made this static GpuMemoryBufferFactory*
> > CreateGpuMemoryBufferFactory(), which allows platform specific allocation,
and
> > stored the instance in channel_manager and injected it where necessary.
> > 
> > Then below functions would no longer be static and we can add another method
> > along the lines of
> > 
> > GpuProcessMemoryBuffer* AcquireMemoryBuffer(int primary_id, int
secondary_id)
> > 
> > and have
> > 
> > struct GpuProcessMemoryBuffer {
> > #ifdef X11
> >  XID xid;
> > #endif
> > #ifdef ANDROID
> >  SurfaceTexture surface_texture;
> > #endif
> > //etc.
> > }
> > 
> > This should give us an in-process handle and remove globals.
> 
> Not a fan of the idea of another struct with ifdef-ed fields. It makes sense
for
> the handle that needs to be used with IPC but there shouldn't be necessary to
> use this for an in-process handle.
> 
> Maybe we could do something like this:
> 
> class GpuMemoryBufferFactory {
>  public:
>   static scoped_ptr<GpuMemoryBufferFactory> Create();
> 
>   virtual bool CreateGpuMemoryBuffer(
>       const gfx::GpuMemoryBufferHandle& handle, ...);
>   virtual void DestroyGpuMemoryBuffer(
>       const gfx::GpuMemoryBufferHandle& handle);
> 
>   scoped_refptr<gfx::GLImage> AcquireImage(
>       const gfx::GpuMemoryBufferHandle& handle);
> }
> 
> and kill the GLimage factory in ui/gl.

This looks good as well. The reason I brought up the struct was because I need
to get the native handle back from the GLImage
so that I can give it to the display controller for scanout.

That's only ozone specific though, so let's not worry about it here.

[no sievers, 2014-07-01 22:49:07]

https://codereview.chromium.org/331723003/diff/120001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/331723003/diff/120001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:476:
CreateGpuMemoryBufferCallbackMap::iterator iter =
I wonder if all of this code really belongs in GpuProcessHostUIShim.
Not a big deal but makes it slightly confusing that it doesn't end up sending
through gpu_channel_ but looks up the browser-gpu channel instead.

https://codereview.chromium.org/331723003/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/331723003/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2697: caps.map_image = true;
should probably get rid of this capability flag

https://codereview.chromium.org/331723003/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/331723003/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:607: // TODO: Use
content::GpuMemoryBufferImpl::Create() to create this instance.
And for this we have to somehow unify the code in
android_webview/browser/gpu_memory_buffer_factory_impl.cc with
content/common/gpu/gpu_memory_buffer_factory_android.cc.

The main reason for this being under android_webview/ is probably
g_gl_draw_functions. We can change it to whatever works with this patch. For
example, we could override the whole factory (or just the one creator function -
everything else could be wrapped in callbacks) at runtime.

https://codereview.chromium.org/331723003/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:636: // TODO: Use
content::GpuMemoryBufferFactory::CreateImageForGpuMemoryBuffer()
So this is not a problem or is it? Looks like the factory already supports
ANDROID_NATIVE_BUFFER, and we'd just have to create a factory instance here.

[reveman, 2014-07-02 14:47:54]

https://codereview.chromium.org/331723003/diff/120001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/331723003/diff/120001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:476:
CreateGpuMemoryBufferCallbackMap::iterator iter =
On 2014/07/01 22:49:07, sievers wrote:
> I wonder if all of this code really belongs in GpuProcessHostUIShim.
> Not a big deal but makes it slightly confusing that it doesn't end up sending
> through gpu_channel_ but looks up the browser-gpu channel instead.

Yes, you're probably right about that. Let's save that change for later though.

https://codereview.chromium.org/331723003/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/331723003/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2697: caps.map_image = true;
On 2014/07/01 22:49:07, sievers wrote:
> should probably get rid of this capability flag

I think we still need this for ppapi.

https://codereview.chromium.org/331723003/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/331723003/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:607: // TODO: Use
content::GpuMemoryBufferImpl::Create() to create this instance.
On 2014/07/01 22:49:07, sievers wrote:
> And for this we have to somehow unify the code in
> android_webview/browser/gpu_memory_buffer_factory_impl.cc with
> content/common/gpu/gpu_memory_buffer_factory_android.cc.
> 
> The main reason for this being under android_webview/ is probably
> g_gl_draw_functions. We can change it to whatever works with this patch. For
> example, we could override the whole factory (or just the one creator function
-
> everything else could be wrapped in callbacks) at runtime. 

I'm not too worried about unifying them, I'll figure out a clean way to do that.

The problem is that the GpuMemoryBufferImpl code lives in content/ while
InProcessCommandBuffer (unlike CommandBufferProxy) code lives in gpu/. I'm not
sure how to best bridge that. A few ideas:

- Move InProcessCommandBuffer to content/

- Implement GpuControl in content/ instead of by InProcessCommandBuffer

- Add a Create/DestroyGpuMemoryBuffer function to GpuControlService and allow
GpuControlService to be implemented in content/

- Move all GpuMemoryBufferImpl and GpuMemoryBufferFactory code to gpu/ instead
of content/ (I'm not a fan of this approach as the code needs to do client ID
validation for secure multi-process usage which I think is content/ specific)

https://codereview.chromium.org/331723003/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:636: // TODO: Use
content::GpuMemoryBufferFactory::CreateImageForGpuMemoryBuffer()
On 2014/07/01 22:49:07, sievers wrote:
> So this is not a problem or is it? Looks like the factory already supports
> ANDROID_NATIVE_BUFFER, and we'd just have to create a factory instance here.

Same problem here. GpuMemoryBufferFactory lives in content/ for what I think is
a good reason.

[no sievers, 2014-07-03 00:20:43]

https://codereview.chromium.org/331723003/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/331723003/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:607: // TODO: Use
content::GpuMemoryBufferImpl::Create() to create this instance.
On 2014/07/02 14:47:54, reveman wrote:
> On 2014/07/01 22:49:07, sievers wrote:
> > And for this we have to somehow unify the code in
> > android_webview/browser/gpu_memory_buffer_factory_impl.cc with
> > content/common/gpu/gpu_memory_buffer_factory_android.cc.
> > 
> > The main reason for this being under android_webview/ is probably
> > g_gl_draw_functions. We can change it to whatever works with this patch. For
> > example, we could override the whole factory (or just the one creator
function
> -
> > everything else could be wrapped in callbacks) at runtime. 
> 
> I'm not too worried about unifying them, I'll figure out a clean way to do
that.
> 
> The problem is that the GpuMemoryBufferImpl code lives in content/ while
> InProcessCommandBuffer (unlike CommandBufferProxy) code lives in gpu/. I'm not
> sure how to best bridge that. A few ideas:
> 
> - Move InProcessCommandBuffer to content/
> 
> - Implement GpuControl in content/ instead of by InProcessCommandBuffer
> 
> - Add a Create/DestroyGpuMemoryBuffer function to GpuControlService and allow
> GpuControlService to be implemented in content/
> 
> - Move all GpuMemoryBufferImpl and GpuMemoryBufferFactory code to gpu/ instead
> of content/ (I'm not a fan of this approach as the code needs to do client ID
> validation for secure multi-process usage which I think is content/ specific)

I like the latter two ideas better. Having things in content/ limits how things
can be used, incl. in testing. For example, there might also be some value in
having GpuMemoryBuffer live inside gpu/ so it can be tested, let's say, with
gl_tests. Do you think we can maybe just move the check for cross-process
validation to some other place/call-site in content/?

[reveman, 2014-07-10 22:33:12]

Ok, I think this is ready for another review. I decided to wait  with the
unification of gpu memory buffer allocation mechanism used for android_webview
and other platforms as it turned out a bit complicated and this patch is large
enough as it is.

Please take a look.

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:304: int32* id) {
Note: changes to this function and DestroyGpuMemoryBuffer are only to make
content::CommandBufferProxyImpl::CreateGpuMemoryBuffer and
gpu::InProcessCommandBuffer::CreateGpuMemoryBuffer more consistent.

[no sievers, 2014-07-11 18:33:29]

LGTM. nice cleanup.

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:339: return
gpu_memory_buffers_.get(new_id);
nit: can avoid the lookup here

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:948: if (image_manager)
Should we fail rightaway or dcheck if there is no image manager?

https://codereview.chromium.org/331723003/diff/160001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/331723003/diff/160001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:632: return
gpu_memory_buffers_.get(new_id);
nit: avoid lookup

https://codereview.chromium.org/331723003/diff/160001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_manager.cc (right):

https://codereview.chromium.org/331723003/diff/160001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_manager.cc:46: 
I've been wanting to use InProcessCommandBuffer for gl_tests so we can probably
eventually get rid of a lot of extra code in here. I guess for now this works to
make the existing tests happy.

https://codereview.chromium.org/331723003/diff/160001/ui/gfx/gpu_memory_buffer.h
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/331723003/diff/160001/ui/gfx/gpu_memory_buffe...
ui/gfx/gpu_memory_buffer.h:58: long buffer_id;
nit: why change this?

[alexst (slow to review), 2014-07-11 19:50:03]

Nice, lgtm!

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_android.cc (right):

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_android.cc:22: NOTREACHED();
Maybe NOTIMPLEMENTED() is better? It could be reached if somebody made the call
on the browser side.

[danakj, 2014-07-11 20:20:24]

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_android.cc (right):

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_android.cc:22: NOTREACHED();
On 2014/07/11 19:50:03, alexst wrote:
> Maybe NOTIMPLEMENTED() is better? It could be reached if somebody made the
call
> on the browser side.

I'd agree if the renderer process can cause this, but the browser should be
trusted code to not do weird things?

[piman, 2014-07-11 20:36:28]

Great cleanup, thanks.

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:948: if (image_manager)
On 2014/07/11 18:33:29, sievers wrote:
> Should we fail rightaway or dcheck if there is no image manager?

It can only be NULL for mock GLES2Decoders in tests, right? Those wouldn't call
into this? Can we DCHECK instead?

[reveman, 2014-07-11 21:08:06]

+boliu for android_webview/
+cevans for content/common/*_messages.h
+sky for mojo/

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:339: return
gpu_memory_buffers_.get(new_id);
On 2014/07/11 18:33:29, sievers wrote:
> nit: can avoid the lookup here

Done.

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:948: if (image_manager)
On 2014/07/11 20:36:27, piman (slow to review) wrote:
> On 2014/07/11 18:33:29, sievers wrote:
> > Should we fail rightaway or dcheck if there is no image manager?
> 
> It can only be NULL for mock GLES2Decoders in tests, right? Those wouldn't
call
> into this? Can we DCHECK instead?

Yes, I just moved this code without thinking too much about it. We should be
able to assume that an ImageManager instance exists here. Changed to a DCHECK.

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:958: if (image_manager)
and we now DCHECK here too

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_android.cc (right):

https://codereview.chromium.org/331723003/diff/160001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_android.cc:22: NOTREACHED();
On 2014/07/11 20:20:24, danakj wrote:
> On 2014/07/11 19:50:03, alexst wrote:
> > Maybe NOTIMPLEMENTED() is better? It could be reached if somebody made the
> call
> > on the browser side.

There's no missing implementation here as the browser needs to pick a supported
buffer type. There are no supported types yet so if the browser triggers a call
to this, the problem is not missing implementation but incorrect usage by the
browser.

> 
> I'd agree if the renderer process can cause this, but the browser should be
> trusted code to not do weird things?

The renderer can't cause this.

https://codereview.chromium.org/331723003/diff/160001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/331723003/diff/160001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:632: return
gpu_memory_buffers_.get(new_id);
On 2014/07/11 18:33:29, sievers wrote:
> nit: avoid lookup

Done.

https://codereview.chromium.org/331723003/diff/160001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_manager.cc (right):

https://codereview.chromium.org/331723003/diff/160001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_manager.cc:46: 
On 2014/07/11 18:33:29, sievers wrote:
> I've been wanting to use InProcessCommandBuffer for gl_tests so we can
probably
> eventually get rid of a lot of extra code in here. I guess for now this works
to
> make the existing tests happy.

Acknowledged.

https://codereview.chromium.org/331723003/diff/160001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_manager.cc:345: return
gpu_memory_buffers_.get(new_id);
Changed this to be consistent with
CommandBufferProxyImpl::CreateGpuMemoryBuffer().

https://codereview.chromium.org/331723003/diff/160001/ui/gfx/gpu_memory_buffer.h
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/331723003/diff/160001/ui/gfx/gpu_memory_buffe...
ui/gfx/gpu_memory_buffer.h:58: long buffer_id;
On 2014/07/11 18:33:29, sievers wrote:
> nit: why change this?

Everything in this struct is expected to be serialized and potentially used to
identify the buffer by another process. While "EGLClientBuffer" which is a
"void*" could be used as an identifier, that's pretty nasty. Long term we'll use
|global_id| (or |handle| if we add more generic gralloc support that works in
out of process mode) and the factory will do a lookup just like the X11Pixmap
factory does. I think buffer_id is fine for now and a big improvement over
EGLClientBuffer.

[sky, 2014-07-11 22:08:33]

sky->jamesr (I don't know this code)

[sky, 2014-07-11 22:09:01]

And jamesr, it's mojo/ you need to look at.

[jamesr, 2014-07-11 22:09:45]

mojo lgtm once piman's happy

[boliu, 2014-07-12 00:17:01]

android_webview lgtm

[piman, 2014-07-16 17:06:30]

lgtm

[reveman, 2014-07-16 17:08:25]

ping for security review

[jschuh, 2014-07-16 17:22:04]

Sorry for the delay, I've been buried. It looks safe from a generic security
standpoint, but I'm not really familiar with X so I'd prefer if jln or jorgelo
signed off rather than me.

[reveman, 2014-07-16 18:34:51]

On 2014/07/16 17:22:04, Justin Schuh wrote:
> Sorry for the delay, I've been buried. It looks safe from a generic security
> standpoint, but I'm not really familiar with X so I'd prefer if jln or jorgelo
> signed off rather than me.

In case it makes it easier to understand the impact of this patch from a
security perspective;

The feature being modified here allows the browser process to use X11 pixmaps as
textures when using the GPU process for producing output. In the past, we had
separate IPC for this but the patch unifies this with the GpuMemoryBuffer code
by introducing an X11_PIXMAP type of buffer. Access to X11 pixmaps must be
restricted to the browser process. Prior to this patch, this was enforced by
having the Create/DeleteImage IPC do all the work and simply limit access to the
GPU process GLImage instance to contexts created through the same gpu channel by
having one image manager per channel. After this patch, we first create a
GpuMemoryBuffer instance using browser only IPC and then create the GLImage
instance using IPC available also to the renderer. We make sure that a
compromised renderer can't create this GLImage instance by checking if the
client id given when creating the GpuMemoryBuffer matches the id of the client
that is trying to create a GLImage from it. This is the same security mechanism
we use for other types of GpuMemoryBuffers.

[jschuh, 2014-07-17 00:15:34]

jln or jorgelo, mind taking a look?

[Jorge Lucangeli Obes, 2014-07-17 23:21:47]

On 2014/07/17 00:15:34, Justin Schuh wrote:
> jln or jorgelo, mind taking a look?

lgtm from the explanation above.

[reveman, 2014-07-18 02:02:23]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-07-18 02:04:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reveman@chromium.org/331723003/260001

[commit-bot: I haz the power, 2014-07-18 02:09:50]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[commit-bot: I haz the power, 2014-07-18 02:13:36]

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

[commit-bot: I haz the power, 2014-07-18 02:13:38]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[reveman, 2014-07-18 02:22:52]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-07-18 02:23:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reveman@chromium.org/331723003/280001

[commit-bot: I haz the power, 2014-07-18 06:13:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-18 06:17:37]

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

[commit-bot: I haz the power, 2014-07-18 06:17:38]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[jschuh, 2014-07-18 15:25:58]

rubberstamp lgtm for ipc security

[reveman, 2014-07-18 15:26:47]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-07-18 15:28:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reveman@chromium.org/331723003/280001

[commit-bot: I haz the power, 2014-07-18 15:41:07]

Change committed as 284097