gpu: Associate all GpuMemoryBuffers with unique IDs.

gpu: Associate all GpuMemoryBuffers with unique IDs.

This makes the GpuMemoryBufferManager instance on the browser
side allocate ids. The result is that each id is unique and
that makes management much easier. This also allows some more
concrete handling of the client id used for GPU service side
allocation of buffers.

BUG=

Committed: https://crrev.com/510a6e058b4b3783dd7c9af18572d9a97fb257f6
Cr-Commit-Position: refs/heads/master@{#302726}

[reveman, 2014-11-04 00:20:58]

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

[piman, 2014-11-04 01:07:26]

This is great, simplifies things quite a bit.
I'm still confused about the Android bits though. It seems like there should be
either more changes or some of the changes are not right... It's hard to follow
though, I may be wrong.

https://codereview.chromium.org/685983005/diff/1/content/browser/android/brow...
File content/browser/android/browser_surface_texture_manager.cc (right):

https://codereview.chromium.org/685983005/diff/1/content/browser/android/brow...
content/browser/android/browser_surface_texture_manager.cc:99: const int
kBrowserClientId = 0;
Why 0, where does it come from?
Can RegisterSurfaceTexture be called with client_id == 0? From where?

If/when Android uses out-of-process gpu, doesn't this need to be
BrowserGpuChannelHostFactory::GetGpuChannelId() ?

https://codereview.chromium.org/685983005/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/685983005/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:288:
int surfaceTextureId, int clientId) {
Does this need to change too? Can we really remove clientId from everywhere?

[piman, 2014-11-04 01:22:46]

https://codereview.chromium.org/685983005/diff/1/content/browser/android/brow...
File content/browser/android/browser_surface_texture_manager.cc (right):

https://codereview.chromium.org/685983005/diff/1/content/browser/android/brow...
content/browser/android/browser_surface_texture_manager.cc:99: const int
kBrowserClientId = 0;
On 2014/11/04 01:07:26, piman (Very slow to review) wrote:
> Why 0, where does it come from?
> Can RegisterSurfaceTexture be called with client_id == 0? From where?
> 
> If/when Android uses out-of-process gpu, doesn't this need to be
> BrowserGpuChannelHostFactory::GetGpuChannelId() ?

Double-checked: this should be
BrowserGpuChannelHostFactory::instance()->GetGpuChannelId() even for Android
without a GPU process.

https://codereview.chromium.org/685983005/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/685983005/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:288:
int surfaceTextureId, int clientId) {
On 2014/11/04 01:07:26, piman (Very slow to review) wrote:
> Does this need to change too? Can we really remove clientId from everywhere?

Ignore this.., I didn't read properly.

[reveman, 2014-11-04 06:20:47]

PTAL

https://codereview.chromium.org/685983005/diff/1/content/browser/android/brow...
File content/browser/android/browser_surface_texture_manager.cc (right):

https://codereview.chromium.org/685983005/diff/1/content/browser/android/brow...
content/browser/android/browser_surface_texture_manager.cc:99: const int
kBrowserClientId = 0;
On 2014/11/04 01:22:46, piman (Very slow to review) wrote:
> On 2014/11/04 01:07:26, piman (Very slow to review) wrote:
> > Why 0, where does it come from?
> > Can RegisterSurfaceTexture be called with client_id == 0? From where?
> > 
> > If/when Android uses out-of-process gpu, doesn't this need to be
> > BrowserGpuChannelHostFactory::GetGpuChannelId() ?
> 
> Double-checked: this should be
> BrowserGpuChannelHostFactory::instance()->GetGpuChannelId() even for Android
> without a GPU process.

Thanks. Fixed in latest patch.

https://codereview.chromium.org/685983005/diff/20001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_memory_buffer_factory_host_impl.cc (right):

https://codereview.chromium.org/685983005/diff/20001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_memory_buffer_factory_host_impl.cc:29:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
I'm changing this back to assume that it's only called on the IO thread as I
would otherwise have to add a struct for all parameters (more than 7). This
reduces the code a bit and keeps things simple. Downside is of course that it's
inconsistent with DestroyGpuMemoryBuffer.

https://codereview.chromium.org/685983005/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gpu_channel_host.cc (left):

https://codereview.chromium.org/685983005/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.cc:294: #if defined(USE_OZONE)
Note: I removed all these ifdefs as they are not needed as long as
gpu_memory_buffer.h doesn't have them.

[alexst (slow to review), 2014-11-04 14:24:39]

Nice! I like the explicit client id much better, secondary id was a bit magic.

https://codereview.chromium.org/685983005/diff/40001/content/common/child_pro...
File content/common/child_process_host_impl.cc (right):

https://codereview.chromium.org/685983005/diff/40001/content/common/child_pro...
content/common/child_process_host_impl.cc:340:
g_next_gpu_memory_buffer_id.GetNext(),
When do we get into this code? As far as I can tell, if the filter handles all
the messages.

https://codereview.chromium.org/685983005/diff/40001/ui/ozone/gpu/gpu_memory_...
File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right):

https://codereview.chromium.org/685983005/diff/40001/ui/ozone/gpu/gpu_memory_...
ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:128:
native_pixmap_map_.erase(GetIndex(id, client_id));
While it looks like it is safe to delete a non-existing key from a map, it feels
like we should still check that the id is valid. A compromised renderer could in
theory be generating bogus id's to be deleted and if that happened, maybe we
should log an error or at least DCHECK.

[reveman, 2014-11-04 18:19:39]

https://codereview.chromium.org/685983005/diff/40001/content/common/child_pro...
File content/common/child_process_host_impl.cc (right):

https://codereview.chromium.org/685983005/diff/40001/content/common/child_pro...
content/common/child_process_host_impl.cc:340:
g_next_gpu_memory_buffer_id.GetNext(),
On 2014/11/04 14:24:39, alexst wrote:
> When do we get into this code? As far as I can tell, if the filter handles all
> the messages.

This is for non-renderer child processes. The plan is to turn
BrowserGpuMemoryBufferManager into HostGpuMemoryBufferManager so it can be used
here to provide the same type of buffers to non-renderer child processes instead
of this shared memory only implementation that currently lives here.

https://codereview.chromium.org/685983005/diff/40001/ui/ozone/gpu/gpu_memory_...
File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right):

https://codereview.chromium.org/685983005/diff/40001/ui/ozone/gpu/gpu_memory_...
ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:128:
native_pixmap_map_.erase(GetIndex(id, client_id));
On 2014/11/04 14:24:39, alexst wrote:
> While it looks like it is safe to delete a non-existing key from a map, it
feels
> like we should still check that the id is valid. A compromised renderer could
in
> theory be generating bogus id's to be deleted and if that happened, maybe we
> should log an error or at least DCHECK.

This is not different from before this patch and I'd rather not change this now
as https://codereview.chromium.org/703463005 will add buffer Id validation in
the correct place. A DCHECK can be added here once that has landed but it would
be incorrect to add one now as a compromised renderer should never be able to
make use crash here, even in a debug build.

[alexst (slow to review), 2014-11-04 18:44:15]

> This is not different from before this patch and I'd rather not change this
now
> as https://codereview.chromium.org/703463005 will add buffer Id validation in
> the correct place. A DCHECK can be added here once that has landed but it
would
> be incorrect to add one now as a compromised renderer should never be able to
> make use crash here, even in a debug build.

Validation in the follow up sounds good. lgtm, but please run the patch through
ozone bots, they are not yet on CQ, we are working to get them there.

[reveman, 2014-11-04 18:55:05]

reveman@chromium.org changed reviewers:
+ kenrb@chromium.org, sky@chromium.org

[reveman, 2014-11-04 18:55:05]

+kenrb for content/common/*_messages.h
+sky for ui/

[piman, 2014-11-04 19:25:04]

lgtm

[sky, 2014-11-04 21:05:40]

LGTM

[kenrb, 2014-11-04 23:58:47]

ipc lgtm

[reveman, 2014-11-05 00:01:42]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-11-05 00:03:12]

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

[reveman, 2014-11-05 00:11:17]

The CQ bit was unchecked by reveman@chromium.org

[reveman, 2014-11-05 00:11:23]

The CQ bit was unchecked by reveman@chromium.org

[reveman, 2014-11-05 00:44:34]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-11-05 00:45:03]

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

[commit-bot: I haz the power, 2014-11-05 00:59:32]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-11-05 00:59:59]

Patchset 7 (id:??) landed as
https://crrev.com/510a6e058b4b3783dd7c9af18572d9a97fb257f6
Cr-Commit-Position: refs/heads/master@{#302726}