Add GenericSharedMemoryId and use w/ GpuMemoryBuffer

content/browser/gpu/browser_gpu_memory_buffer_manager.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/gpu/browser_gpu_memory_buffer_manager.h"
 
 #include "base/atomic_sequence_num.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/trace_event/process_memory_dump.h"
 #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"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_switches.h"
 #include "gpu/GLES2/gl2extchromium.h"
 #include "ui/gl/gl_switches.h"
 
 #if defined(OS_MACOSX)
 #include "content/common/gpu/gpu_memory_buffer_factory_io_surface.h"
@@ skipping 102 matching lines @@
     if (IsGpuMemoryBufferFactoryConfigurationSupported(type, configuration))
       configurations.push_back(configuration);
   }
 #endif
 
   return configurations;
 }
 
 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 {
   AllocateGpuMemoryBufferRequest(const gfx::Size& size,
                                  gfx::BufferFormat format,
                                  gfx::BufferUsage usage,
                                  int client_id,
                                  int surface_id)
       : event(true, false),
         size(size),
@@ skipping 70 matching lines @@
 BrowserGpuMemoryBufferManager::AllocateGpuMemoryBufferForScanout(
     const gfx::Size& size,
     gfx::BufferFormat format,
     int32 surface_id) {
   DCHECK_GT(surface_id, 0);
   return AllocateGpuMemoryBufferForSurface(
       size, format, gfx::BufferUsage::SCANOUT, surface_id);
 }
 
 void BrowserGpuMemoryBufferManager::AllocateGpuMemoryBufferForChildProcess(
+    gfx::GpuMemoryBufferId id,
     const gfx::Size& size,
     gfx::BufferFormat format,
     gfx::BufferUsage usage,
     base::ProcessHandle child_process_handle,
     int child_client_id,
     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;
   }
 
   // Early out if we cannot fallback to shared memory buffer.
   if (!GpuMemoryBufferImplSharedMemory::IsFormatSupported(format) ||
       !GpuMemoryBufferImplSharedMemory::IsUsageSupported(usage) ||
       !GpuMemoryBufferImplSharedMemory::IsSizeValidForFormat(size, format)) {
     callback.Run(gfx::GpuMemoryBufferHandle());
     return;
   }
 
   BufferMap& buffers = clients_[child_client_id];
-  DCHECK(buffers.find(new_id) == buffers.end());
+
+  if (!TryAddNewBufferToMap(
+          buffers, id,
+          BufferInfo(size, gfx::SHARED_MEMORY_BUFFER, format, usage, 0))) {
+    LOG(ERROR) << "Client process attempted to allocate a GpuMemoryBuffer with "

[reveman, 2015/08/10 22:16:14]

nit: "Child process..." to be consistent with function argument names

[ericrk, 2015/08/11 01:40:07]

Done.

+                  "an existing ID. Failing allocation.";

[reveman, 2015/08/10 22:16:14]

nit: I think "Failing allocation." is implicit from LOG(ERROR)

[ericrk, 2015/08/11 01:40:07]

Done.

+    callback.Run(gfx::GpuMemoryBufferHandle());
+    return;
+  }
 
   // Allocate shared memory buffer as fallback.
-  buffers[new_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*
 BrowserGpuMemoryBufferManager::GpuMemoryBufferFromClientBuffer(
     ClientBuffer buffer) {
   return GpuMemoryBufferImpl::FromClientBuffer(buffer);
 }
 
 void BrowserGpuMemoryBufferManager::SetDestructionSyncPoint(
     gfx::GpuMemoryBuffer* buffer,
@@ skipping 33 matching lines @@
                       base::trace_event::MemoryAllocatorDump::kUnitsBytes,
                       buffer_size_in_bytes);
 
       // Create the cross-process ownership edge. If the client creates a
       // corresponding dump for the same buffer, this will avoid to
       // double-count them in tracing. If, instead, no other process will emit a
       // dump with the same guid, the segment will be accounted to the browser.
       uint64 client_tracing_process_id = ClientIdToTracingProcessId(client_id);
 
       base::trace_event::MemoryAllocatorDumpGuid shared_buffer_guid =
-          gfx::GetGpuMemoryBufferGUIDForTracing(client_tracing_process_id,
-                                                buffer_id);
+          base::GetGenericSharedMemoryGUIDForTracing(client_tracing_process_id,
+                                                     buffer_id);
       pmd->CreateSharedGlobalAllocatorDump(shared_buffer_guid);
       pmd->AddOwnershipEdge(dump->guid(), shared_buffer_guid);
     }
   }
 
   return true;
 }
 
 void BrowserGpuMemoryBufferManager::ChildProcessDeletedGpuMemoryBuffer(
     gfx::GpuMemoryBufferId id,
@@ skipping 62 matching lines @@
     if (configuration.format == format && configuration.usage == usage)
       return true;
   }
   return false;
 }
 
 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,
                                               request->usage)) {
     // Note: Unretained is safe as this is only used for synchronous allocation
     // from a non-IO thread.
     AllocateGpuMemoryBufferOnIO(
         new_id, request->size, request->format, request->usage,
         request->client_id, request->surface_id, false,
         base::Bind(&BrowserGpuMemoryBufferManager::
                        GpuMemoryBufferAllocatedForSurfaceOnIO,
                    base::Unretained(this), base::Unretained(request)));
     return;
   }
 
   DCHECK(GpuMemoryBufferImplSharedMemory::IsFormatSupported(request->format))
       << static_cast<int>(request->format);
   DCHECK(GpuMemoryBufferImplSharedMemory::IsUsageSupported(request->usage))
       << static_cast<int>(request->usage);
 
   BufferMap& buffers = clients_[request->client_id];
-  DCHECK(buffers.find(new_id) == buffers.end());
 
   // Allocate shared memory buffer as fallback.
-  buffers[new_id] = BufferInfo(request->size, gfx::SHARED_MEMORY_BUFFER,
-                               request->format, request->usage, 0);
+  DCHECK(TryAddNewBufferToMap(
+      buffers, new_id, BufferInfo(request->size, gfx::SHARED_MEMORY_BUFFER,
+                                  request->format, request->usage, 0)));

[reveman, 2015/08/10 22:16:14]

Is this DCHECK safe? Is it not possible for a child process to control this ID? 

Also what happens here in a release build without DCHECKs enabled? Please avoid
code that is critical for correct execution in DCHECKs.

[ericrk, 2015/08/11 01:40:07]

The ID here is safe, as it's allocated and controlled by this function (see line
405).. that said, yup, this totally shouldn't be in a DCHECK.

   // Note: Unretained is safe as IO thread is stopped before manager is
   // destroyed.
   request->result = GpuMemoryBufferImplSharedMemory::Create(
       new_id, request->size, request->format,
       base::Bind(
           &GpuMemoryBufferDeleted,
           BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO),
           base::Bind(&BrowserGpuMemoryBufferManager::DestroyGpuMemoryBufferOnIO,
                      base::Unretained(this), new_id, request->client_id)));
   request->event.Signal();
@@ skipping 26 matching lines @@
     gfx::GpuMemoryBufferId id,
     const gfx::Size& size,
     gfx::BufferFormat format,
     gfx::BufferUsage usage,
     int client_id,
     int surface_id,
     bool reused_gpu_process,
     const AllocationCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  BufferMap& buffers = clients_[client_id];
-  DCHECK(buffers.find(id) == buffers.end());
-
   GpuProcessHost* host = GpuProcessHost::FromID(gpu_host_id_);
   if (!host) {
     host = GpuProcessHost::Get(GpuProcessHost::GPU_PROCESS_KIND_SANDBOXED,
                                CAUSE_FOR_GPU_LAUNCH_GPU_MEMORY_BUFFER_ALLOCATE);
     if (!host) {
       LOG(ERROR) << "Failed to launch GPU process.";
       callback.Run(gfx::GpuMemoryBufferHandle());
       return;
     }
     gpu_host_id_ = host->host_id();
     reused_gpu_process = false;
   } else {
     if (reused_gpu_process) {
       // We come here if we retried to allocate the buffer because of a
       // failure in GpuMemoryBufferAllocatedOnIO, but we ended up with the
       // same process ID, meaning the failure was not because of a channel
       // error, but another reason. So fail now.
       LOG(ERROR) << "Failed to allocate GpuMemoryBuffer.";
       callback.Run(gfx::GpuMemoryBufferHandle());
       return;
     }
     reused_gpu_process = true;
   }
 
+  BufferMap& buffers = clients_[client_id];
+
   // Note: Handling of cases where the client is removed before the allocation
   // completes is less subtle if we set the buffer type to EMPTY_BUFFER here
   // and verify that this has not changed when allocation completes.
-  buffers[id] = BufferInfo(size, gfx::EMPTY_BUFFER, format, usage, 0);
+  if (!TryAddNewBufferToMap(
+          buffers, id, BufferInfo(size, gfx::EMPTY_BUFFER, format, usage, 0))) {
+    LOG(ERROR) << "Client process attempted to allocate a GpuMemoryBuffer with "
+                  "an existing ID. Failing allocation.";
+    callback.Run(gfx::GpuMemoryBufferHandle());
+    return;
+  }
 
   // Note: Unretained is safe as IO thread is stopped before manager is
   // destroyed.
   host->CreateGpuMemoryBuffer(
       id, size, format, usage, client_id, surface_id,
       base::Bind(&BrowserGpuMemoryBufferManager::GpuMemoryBufferAllocatedOnIO,
                  base::Unretained(this), id, client_id, surface_id,
                  gpu_host_id_, reused_gpu_process, callback));
 }
 
@@ skipping 98 matching lines @@
     // The gpu_client uses a fixed tracing ID.
     return gpu_client_tracing_id_;
   }
 
   // In normal cases, |client_id| is a child process id, so we can perform
   // the standard conversion.
   return ChildProcessHostImpl::ChildProcessUniqueIdToTracingProcessId(
       client_id);
 }
 
+bool BrowserGpuMemoryBufferManager::TryAddNewBufferToMap(
+    BufferMap& buffers,
+    int id,
+    const BufferInfo& buffer_info) {
+  auto insert_result = buffers.insert(std::make_pair(
+      id, buffer_info));
+
+  // Pair is returned. Second value is true if a new element was inserted.
+  return insert_result.second;
+}

[reveman, 2015/08/10 22:16:14]

I think "if (!buffers_.insert(make_pair(key, value)).second) { ... }" above
instead of this helper function is fine. If you prefer to have this helper
function then can you move it to anonymous namespace or make it static?

[ericrk, 2015/08/11 01:40:07]

removed this... probably not worth the function... was on the fence on this one.

+
 }  // namespace content