Add GenericSharedMemoryId and use w/ GpuMemoryBuffer

content/browser/gpu/browser_gpu_memory_buffer_manager.cc

Index: content/browser/gpu/browser_gpu_memory_buffer_manager.cc
diff --git a/content/browser/gpu/browser_gpu_memory_buffer_manager.cc b/content/browser/gpu/browser_gpu_memory_buffer_manager.cc
index a1d54b7b365c2f4485e71dfe0bd38a63402556d5..2ed15a0057e260f235aa9c2ceb2e666f0c89275e 100644
--- a/content/browser/gpu/browser_gpu_memory_buffer_manager.cc
+++ b/content/browser/gpu/browser_gpu_memory_buffer_manager.cc
@@ -14,6 +14,7 @@
 #include "base/trace_event/trace_event.h"
 #include "content/browser/gpu/gpu_process_host.h"
 #include "content/common/child_process_host_impl.h"
+#include "content/common/generic_shared_memory_id_generator.h"
 #include "content/common/gpu/client/gpu_memory_buffer_impl.h"
 #include "content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h"
 #include "content/common/gpu/gpu_memory_buffer_factory_shared_memory.h"
@@ -120,9 +121,6 @@ GetSupportedGpuMemoryBufferConfigurations(gfx::GpuMemoryBufferType type) {
 
 BrowserGpuMemoryBufferManager* g_gpu_memory_buffer_manager = nullptr;
 
-// Global atomic to generate gpu memory buffer unique IDs.
-base::StaticAtomicSequenceNumber g_next_gpu_memory_buffer_id;
-
 }  // namespace
 
 struct BrowserGpuMemoryBufferManager::AllocateGpuMemoryBufferRequest {
@@ -213,6 +211,7 @@ BrowserGpuMemoryBufferManager::AllocateGpuMemoryBufferForScanout(
 }
 
 void BrowserGpuMemoryBufferManager::AllocateGpuMemoryBufferForChildProcess(
+    gfx::GpuMemoryBufferId id,
     const gfx::Size& size,
     gfx::BufferFormat format,
     gfx::BufferUsage usage,
@@ -221,11 +220,9 @@ void BrowserGpuMemoryBufferManager::AllocateGpuMemoryBufferForChildProcess(
     const AllocationCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  gfx::GpuMemoryBufferId new_id = g_next_gpu_memory_buffer_id.GetNext();
-
   // Use service side allocation if this is a supported configuration.
   if (IsGpuMemoryBufferConfigurationSupported(format, usage)) {
-    AllocateGpuMemoryBufferOnIO(new_id, size, format, usage, child_client_id, 0,
+    AllocateGpuMemoryBufferOnIO(id, size, format, usage, child_client_id, 0,
                                 false, callback);
     return;
   }
@@ -239,13 +236,17 @@ void BrowserGpuMemoryBufferManager::AllocateGpuMemoryBufferForChildProcess(
   }
 
   BufferMap& buffers = clients_[child_client_id];
-  DCHECK(buffers.find(new_id) == buffers.end());
+  if (buffers.find(id) != buffers.end()) {

[reveman, 2015/08/06 19:19:36]

nit: please save the iterator and use it to set BufferInfo below to avoid an
unnecessary map lookup in release code.

[ericrk, 2015/08/06 22:05:19]

At this point iterator shouldn't exist (we expect it to be buffers.end()). That
doesn't really have any re-use value?

+    LOG(ERROR) << "Client process attempted to allocate a GpuMemoryBuffer with "
+                  "an existing ID. Failing allocation.";
+    callback.Run(gfx::GpuMemoryBufferHandle());
+    return;
+  }
 
   // Allocate shared memory buffer as fallback.
-  buffers[new_id] =
-      BufferInfo(size, gfx::SHARED_MEMORY_BUFFER, format, usage, 0);
+  buffers[id] = BufferInfo(size, gfx::SHARED_MEMORY_BUFFER, format, usage, 0);
   callback.Run(GpuMemoryBufferImplSharedMemory::AllocateForChildProcess(
-      new_id, size, format, child_process_handle));
+      id, size, format, child_process_handle));
 }
 
 gfx::GpuMemoryBuffer*
@@ -302,6 +303,16 @@ bool BrowserGpuMemoryBufferManager::OnMemoryDump(
                                                 buffer_id);
       pmd->CreateSharedGlobalAllocatorDump(shared_buffer_guid);
       pmd->AddOwnershipEdge(dump->guid(), shared_buffer_guid);
+
+      if (buffer.second.type ==
+          gfx::GpuMemoryBufferType::SHARED_MEMORY_BUFFER) {
+        // Normally, the GpuMemoryBufferFactory handles establishing the link
+        // between a GpuMemoryBufferId and any GenericSharedMemoryId. However,
+        // there is no manager for SHARED_MEMORY_BUFFER buffers, so handle
+        // this here.

[reveman, 2015/08/06 19:19:36]

I was thinking that establishing the link is still up to the GMB manager code
here and all the factory does is make sure there's a guid for whatever memory is
really backing the buffer. Does that make sense?

[ericrk, 2015/08/06 22:05:19]

Using same GUID for both GpuMemoryBuffers and GenericSharedMemory, so edge is no
longer necessary. pulling this.

+        GpuMemoryBufferImplSharedMemory::DumpGenericSharedMemory(
+            pmd, client_tracing_process_id, buffer_id);

[reveman, 2015/08/06 19:19:36]

Can we move this into a static function as part
GpuMemoryBufferFactorySharedMemory? that way we can easily have the shared
memory factory that is currently only used for testing purposes do the right
thing if ever use it on the gpu process side.

[ericrk, 2015/08/06 22:05:19]

Pulled this for now, will re-add generic memory dumping in next CL.

+      }
     }
   }
 
@@ -382,7 +393,7 @@ void BrowserGpuMemoryBufferManager::AllocateGpuMemoryBufferForSurfaceOnIO(
     AllocateGpuMemoryBufferRequest* request) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  gfx::GpuMemoryBufferId new_id = g_next_gpu_memory_buffer_id.GetNext();
+  gfx::GpuMemoryBufferId new_id = content::GetNextGenericSharedMemoryId();
 
   // Use service side allocation if this is a supported configuration.
   if (IsGpuMemoryBufferConfigurationSupported(request->format,
@@ -456,7 +467,12 @@ void BrowserGpuMemoryBufferManager::AllocateGpuMemoryBufferOnIO(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   BufferMap& buffers = clients_[client_id];
-  DCHECK(buffers.find(id) == buffers.end());
+  if (buffers.find(id) != buffers.end()) {

[reveman, 2015/08/06 19:19:36]

same here. can we avoid two lookups?

[ericrk, 2015/08/06 22:05:19]

Same comment as above.

+    LOG(ERROR) << "Client process attempted to allocate a GpuMemoryBuffer with "
+                  "an existing ID. Failing allocation.";
+    callback.Run(gfx::GpuMemoryBufferHandle());
+    return;
+  }
 
   GpuProcessHost* host = GpuProcessHost::FromID(gpu_host_id_);
   if (!host) {