gpu: Compositor management of GpuMemoryBuffer instances.

gpu: Compositor management of GpuMemoryBuffer instances.

This is a refactor of the CHROMIUM_image extension that allows
the compositor to map/unmap GpuMemoryBuffers on worker threads
and potentially also allocate these buffers on worker threads.

CreateImageCHROMIUM now takes an opaque ClientBuffer as first
parameter. ClientBuffer is implementation specific and in the
case of chromium a gfx::GpuMemoryBuffer. Or more specifically
a content::GpuMemoryBufferImpl when used with the content layer.

This allows the compositor to allocate and use GpuMemoryBuffers
without a GLES2Interface.

A GpuMemoryBufferManager interface has been introduced. This
interface provides a mechanism for the compositor to allocate
GpuMemoryBuffer instances in the renderer and browser process.

BUG=418553
Committed: https://crrev.com/22dd9298e4365cfbc3662e98c3b387fe7f3ce6a3
Cr-Commit-Position: refs/heads/master@{#299360}

[reveman, 2014-10-08 16:08:44]

reveman@chromium.org changed reviewers:
+ piman@chromium.org, vmpstr@chromium.org

[reveman, 2014-10-08 16:08:46]

I think this is ready for review now.

+vmpstr for cc
+piman for everything else

This looks big as it's touching a lot of files but it's not as bad as it looks.
Most changes are trivial plumbing changes for which no review is really
required.

[vmpstr, 2014-10-08 17:52:25]

cc looks good... From my understanding it's mostly just adding an extra
parameter in most cases (in cc at least)

https://codereview.chromium.org/634083002/diff/20001/cc/test/test_gpu_memory_...
File cc/test/test_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/634083002/diff/20001/cc/test/test_gpu_memory_...
cc/test/test_gpu_memory_buffer_manager.cc:25: virtual void Unmap() override {
mapped_ = false; }
DCHECK/ASSERT/EXPECT mapped_?

https://codereview.chromium.org/634083002/diff/60001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/634083002/diff/60001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:729: delete resource->gpu_memory_buffer;
Can you set this and the rest of the things above to NULL/nullptr after
deleting? It helps with identifying use-after-free better, and it allows other
if (gpu_memory_buffer) checks to do the right thing.

https://codereview.chromium.org/634083002/diff/80001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/634083002/diff/80001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:730: }
Somewhat unrelated, but should we also set allocated to false in here?

https://codereview.chromium.org/634083002/diff/80001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:963: resource->allocated = true;
If we set allocated to false when deleted, can we also DCHECK(!allocated) in the
destructor of the resource? Or is there some situations where we don't delete
the resource? I mean, there shouldn't be since we'd leak.

[reveman, 2014-10-08 19:15:31]

PTAL

https://codereview.chromium.org/634083002/diff/20001/cc/test/test_gpu_memory_...
File cc/test/test_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/634083002/diff/20001/cc/test/test_gpu_memory_...
cc/test/test_gpu_memory_buffer_manager.cc:25: virtual void Unmap() override {
mapped_ = false; }
On 2014/10/08 17:52:25, vmpstr wrote:
> DCHECK/ASSERT/EXPECT mapped_?

Done.

https://codereview.chromium.org/634083002/diff/60001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/634083002/diff/60001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:729: delete resource->gpu_memory_buffer;
On 2014/10/08 17:52:25, vmpstr wrote:
> Can you set this and the rest of the things above to NULL/nullptr after
> deleting? It helps with identifying use-after-free better, and it allows other
> if (gpu_memory_buffer) checks to do the right thing.

Done.

https://codereview.chromium.org/634083002/diff/80001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/634083002/diff/80001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:730: }
On 2014/10/08 17:52:25, vmpstr wrote:
> Somewhat unrelated, but should we also set allocated to false in here?

I don't think so. We're destroying the resource instance and we really shouldn't
be resetting things here. I can see how it's useful to do some of this here to
more easily detect use-after-free issues but we should be using smart pointers
for that type of thing instead. That's a separate issue though.

https://codereview.chromium.org/634083002/diff/80001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:963: resource->allocated = true;
On 2014/10/08 17:52:25, vmpstr wrote:
> If we set allocated to false when deleted, can we also DCHECK(!allocated) in
the
> destructor of the resource? Or is there some situations where we don't delete
> the resource? I mean, there shouldn't be since we'd leak.

Ideally we'd have the destructor do the cleanup work instead of
DeleteResourceInternal. Some work needed to get to that state though and I'd
rather not get that mixed up with this patch that is large enough as it is.

[vmpstr, 2014-10-08 19:31:13]

cc/ lgtm

https://codereview.chromium.org/634083002/diff/80001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/634083002/diff/80001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:730: }
On 2014/10/08 19:15:31, reveman wrote:
> On 2014/10/08 17:52:25, vmpstr wrote:
> > Somewhat unrelated, but should we also set allocated to false in here?
> 
> I don't think so. We're destroying the resource instance and we really
shouldn't
> be resetting things here. I can see how it's useful to do some of this here to
> more easily detect use-after-free issues but we should be using smart pointers
> for that type of thing instead. That's a separate issue though.

Acknowledged.

https://codereview.chromium.org/634083002/diff/80001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:963: resource->allocated = true;
On 2014/10/08 19:15:31, reveman wrote:
> On 2014/10/08 17:52:25, vmpstr wrote:
> > If we set allocated to false when deleted, can we also DCHECK(!allocated) in
> the
> > destructor of the resource? Or is there some situations where we don't
delete
> > the resource? I mean, there shouldn't be since we'd leak.
> 
> Ideally we'd have the destructor do the cleanup work instead of
> DeleteResourceInternal. Some work needed to get to that state though and I'd
> rather not get that mixed up with this patch that is large enough as it is.

Acknowledged.

[piman, 2014-10-09 21:33:02]

https://codereview.chromium.org/634083002/diff/200001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/634083002/diff/200001/cc/resources/resource_p...
cc/resources/resource_provider.cc:987: DCHECK(resource->image_id);
This DCHECK would actually trigger if the context is lost. What are the
consequences of a 0 image_id later? Does it need to be handled, or should we
remove the DCHECK?

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

https://codereview.chromium.org/634083002/diff/200001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:226:
BrowserGpuMemoryBufferManager::current()->set_gpu_client_id(gpu_client_id_);
Can BrowserGpuChannelHostFactory simply own the BrowserGpuMemoryBufferManager?

https://codereview.chromium.org/634083002/diff/200001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/634083002/diff/200001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:71: ->PostTask(
nit: BrowserThread::PostTask(BrowserThread::IO, ...)

https://codereview.chromium.org/634083002/diff/200001/content/common/child_pr...
File content/common/child_process_host_impl.cc (left):

https://codereview.chromium.org/634083002/diff/200001/content/common/child_pr...
content/common/child_process_host_impl.cc:257: OnAllocateGpuMemoryBuffer)
For consistency, if ChildProcessHostMsg_SyncAllocateGpuMemoryBuffer is only
available for renderers, should the ChildGpuMemoryBufferManager only be
available on RenderThreadImpl rather than ChildThread?

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

https://codereview.chromium.org/634083002/diff/200001/ui/gfx/gpu_memory_buffe...
ui/gfx/gpu_memory_buffer.h:16: #ifdef __cplusplus
I don't think you need to bother with the #ifdef __cplusplus here (or most
files), this file wouldn't compile in C.

Just extern "C" is fine (though I think it's not even needed).

(there's other files where this applies).

https://codereview.chromium.org/634083002/diff/200001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/634083002/diff/200001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:35: case gfx::GpuMemoryBuffer::RGBX_8888:
I may have missed it (this patch is big), but what makes this case NOTREACHED ?

https://codereview.chromium.org/634083002/diff/200001/ui/gl/gl_image_shared_m...
File ui/gl/gl_image_shared_memory.cc (right):

https://codereview.chromium.org/634083002/diff/200001/ui/gl/gl_image_shared_m...
ui/gl/gl_image_shared_memory.cc:38: if
(!duped_shared_memory->Map(GetSize().GetArea() * BytesPerPixel(format))) {
We should avoid GetSize().GetArea() * BytesPerPixel(format) because of potential
for overflow. We can use CheckedNumeric types like in e.g.
SharedBitmap::CheckedSizeInBytes to be safe (I realize you're just moving code,
but catching this now).

[reveman, 2014-10-10 02:20:16]

PTAL

https://codereview.chromium.org/634083002/diff/200001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/634083002/diff/200001/cc/resources/resource_p...
cc/resources/resource_provider.cc:987: DCHECK(resource->image_id);
On 2014/10/09 21:33:01, piman (Very slow to review) wrote:
> This DCHECK would actually trigger if the context is lost. What are the
> consequences of a 0 image_id later? Does it need to be handled, or should we
> remove the DCHECK?

Removed it. We might try to draw with this resource without first binding an
image to the texture id but that shouldn't matter if the context is lost.

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

https://codereview.chromium.org/634083002/diff/200001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:226:
BrowserGpuMemoryBufferManager::current()->set_gpu_client_id(gpu_client_id_);
On 2014/10/09 21:33:02, piman (Very slow to review) wrote:
> Can BrowserGpuChannelHostFactory simply own the BrowserGpuMemoryBufferManager?

Done.

I kept the static BrowserGpuMemoryBufferManager::current() function for now as I
suspect we'll need it for non-renderer child process support
(BrowserGpuMemoryBufferManager->HostGpuMemoryBufferManager) but can replace all
usage of that with
BrowserGpuChannelHostFactory::instance()->gpu_memory_buffer_manager() if you
feel strongly about it.

https://codereview.chromium.org/634083002/diff/200001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/634083002/diff/200001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:71: ->PostTask(
On 2014/10/09 21:33:02, piman (Very slow to review) wrote:
> nit: BrowserThread::PostTask(BrowserThread::IO, ...)

Done.

https://codereview.chromium.org/634083002/diff/200001/content/common/child_pr...
File content/common/child_process_host_impl.cc (left):

https://codereview.chromium.org/634083002/diff/200001/content/common/child_pr...
content/common/child_process_host_impl.cc:257: OnAllocateGpuMemoryBuffer)
On 2014/10/09 21:33:02, piman (Very slow to review) wrote:
> For consistency, if ChildProcessHostMsg_SyncAllocateGpuMemoryBuffer is only
> available for renderers, should the ChildGpuMemoryBufferManager only be
> available on RenderThreadImpl rather than ChildThread?

I added a working implementation for this to latest patch instead. It's limited
to shared memory backed GpuMemoryBuffers for now but that's better than nothing.
I also added a TODO as we should be adding support for other types asap.

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

https://codereview.chromium.org/634083002/diff/200001/ui/gfx/gpu_memory_buffe...
ui/gfx/gpu_memory_buffer.h:16: #ifdef __cplusplus
On 2014/10/09 21:33:02, piman (Very slow to review) wrote:
> I don't think you need to bother with the #ifdef __cplusplus here (or most
> files), this file wouldn't compile in C.
> 
> Just extern "C" is fine (though I think it's not even needed).
> 
> (there's other files where this applies).

Good point. Done here and everywhere else this applies.

https://codereview.chromium.org/634083002/diff/200001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/634083002/diff/200001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:35: case gfx::GpuMemoryBuffer::RGBX_8888:
On 2014/10/09 21:33:02, piman (Very slow to review) wrote:
> I may have missed it (this patch is big), but what makes this case NOTREACHED
?

Nice catch. GpuMemoryBufferImplSharedMemory will not create a buffer of this
format but there's nothing stopping a malicious renderer from passing an invalid
value to the GPU process.

Added a ValidFormat check to ::Initialize.

https://codereview.chromium.org/634083002/diff/200001/ui/gl/gl_image_shared_m...
File ui/gl/gl_image_shared_memory.cc (right):

https://codereview.chromium.org/634083002/diff/200001/ui/gl/gl_image_shared_m...
ui/gl/gl_image_shared_memory.cc:38: if
(!duped_shared_memory->Map(GetSize().GetArea() * BytesPerPixel(format))) {
On 2014/10/09 21:33:02, piman (Very slow to review) wrote:
> We should avoid GetSize().GetArea() * BytesPerPixel(format) because of
potential
> for overflow. We can use CheckedNumeric types like in e.g.
> SharedBitmap::CheckedSizeInBytes to be safe (I realize you're just moving
code,
> but catching this now).

Done.

[piman, 2014-10-10 02:58:42]

LGTM, thanks!

https://codereview.chromium.org/634083002/diff/290001/content/common/child_pr...
File content/common/child_process_host_impl.cc (right):

https://codereview.chromium.org/634083002/diff/290001/content/common/child_pr...
content/common/child_process_host_impl.cc:339: base::Unretained(this),
nit: can you add a comment that unretained is safe because the callback is
called synchronously for GpuMemoryBufferImplSharedMemory? Just so that this
isn't cargo-culted to other types later (where it might be wrong).

[reveman, 2014-10-10 12:47:40]

reveman@chromium.org changed reviewers:
+ boliu@chromium.org, jam@chromium.org, nasko@chromium.org, sky@chromium.org

[reveman, 2014-10-10 12:47:44]

+boliu for android_webview/
+jam for base/threading/
+sky for ui/ and mojo/
+nasko for *_messages.h

https://codereview.chromium.org/634083002/diff/290001/content/common/child_pr...
File content/common/child_process_host_impl.cc (right):

https://codereview.chromium.org/634083002/diff/290001/content/common/child_pr...
content/common/child_process_host_impl.cc:339: base::Unretained(this),
On 2014/10/10 02:58:42, piman (Very slow to review) wrote:
> nit: can you add a comment that unretained is safe because the callback is
> called synchronously for GpuMemoryBufferImplSharedMemory? Just so that this
> isn't cargo-culted to other types later (where it might be wrong).

Done.

[boliu, 2014-10-10 15:49:40]

android_webview lgtm

[sky, 2014-10-10 17:58:05]

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

[sky, 2014-10-10 17:58:07]

sky->jamesr for mojo
the rest, LGTM

[jamesr, 2014-10-10 18:02:29]

lgtm

[nasko, 2014-10-13 17:21:45]

*_messages.h LGTM

[reveman, 2014-10-13 17:22:44]

reveman@chromium.org changed reviewers:
+ jln@chromium.org

[reveman, 2014-10-13 17:22:45]

+jln

Ping for security review. Fyi, the only security related change in this patch is
the replacement of non-typed parameters with enums and implicit bounds checking
by IPC system instead of manually verifying these values on the browser side.

[reveman, 2014-10-13 17:25:01]

On 2014/10/13 17:21:45, nasko wrote:
> *_messages.h LGTM

thanks, ignore my last message :)

[danakj, 2014-10-13 17:46:10]

On 2014/10/13 17:25:01, reveman wrote:
> On 2014/10/13 17:21:45, nasko wrote:
> > *_messages.h LGTM
> 
> thanks, ignore my last message :)

Base rs-lgtm

[reveman, 2014-10-13 18:35:12]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-10-13 18:36:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634083002/1640001

[commit-bot: I haz the power, 2014-10-13 18:45:48]

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

[commit-bot: I haz the power, 2014-10-13 18:45:50]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[reveman, 2014-10-13 19:09:41]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-10-13 19:10:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634083002/2020001

[commit-bot: I haz the power, 2014-10-13 20:52:21]

Committed patchset #16 (id:2020001)

[commit-bot: I haz the power, 2014-10-13 20:53:05]

Patchset 16 (id:??) landed as
https://crrev.com/22dd9298e4365cfbc3662e98c3b387fe7f3ce6a3
Cr-Commit-Position: refs/heads/master@{#299360}