content: Handle native GpuMemoryBuffer allocation failures properly.

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/strings/stringprintf.h"
 #include "base/synchronization/waitable_event.h"
@@ skipping 147 matching lines @@
   // Note: Handling of cases where the child process 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[new_id] = BufferInfo(size, format, gfx::EMPTY_BUFFER);
 
   gpu_memory_buffer_factory_host_->CreateGpuMemoryBuffer(
       new_id, size, format, usage, child_client_id, 0,
       base::Bind(&BrowserGpuMemoryBufferManager::
                      GpuMemoryBufferAllocatedForChildProcess,
-                 weak_ptr_factory_.GetWeakPtr(), child_client_id, callback));
+                 weak_ptr_factory_.GetWeakPtr(), new_id, child_client_id,
+                 callback));
 }
 
 gfx::GpuMemoryBuffer*
 BrowserGpuMemoryBufferManager::GpuMemoryBufferFromClientBuffer(
     ClientBuffer buffer) {
   return GpuMemoryBufferImpl::FromClientBuffer(buffer);
 }
 
 void BrowserGpuMemoryBufferManager::SetDestructionSyncPoint(
     gfx::GpuMemoryBuffer* buffer,
@@ skipping 128 matching lines @@
 void BrowserGpuMemoryBufferManager::GpuMemoryBufferDeleted(
     gfx::GpuMemoryBufferId id,
     int client_id,
     uint32 sync_point) {
   gpu_memory_buffer_factory_host_->DestroyGpuMemoryBuffer(id,
                                                           client_id,
                                                           sync_point);
 }
 
 void BrowserGpuMemoryBufferManager::GpuMemoryBufferAllocatedForChildProcess(
+    gfx::GpuMemoryBufferId id,
     int child_client_id,
     const AllocationCallback& callback,
     const gfx::GpuMemoryBufferHandle& handle) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   ClientMap::iterator client_it = clients_.find(child_client_id);
 
   // This can happen if the child process is removed while the buffer is being
   // allocated.
   if (client_it == clients_.end()) {
     if (!handle.is_null()) {
       gpu_memory_buffer_factory_host_->DestroyGpuMemoryBuffer(
           handle.id, child_client_id, 0);
     }
     callback.Run(gfx::GpuMemoryBufferHandle());
     return;
   }
 
   BufferMap& buffers = client_it->second;
 
-  BufferMap::iterator buffer_it = buffers.find(handle.id);
+  BufferMap::iterator buffer_it = buffers.find(id);
   DCHECK(buffer_it != buffers.end());
   DCHECK_EQ(buffer_it->second.type, gfx::EMPTY_BUFFER);
 
-  if (handle.is_null()) {
+  // The factory should never return a shared memory backed buffer.

[Avi (use Gerrit), 2015/06/10 17:01:47]

"Should never" implies DCHECK, that it shouldn't happen, and that it's a
programming error if it does. If this is a legitimate error condition that can
happen (as your CL description states) then change this comment.

Rather than "should never", explain the condition for which this can happen and
what it means (e.g. "If the handle isn't valid, that means that the GPU process
crashed or is misbehaving...")

[reveman, 2015/06/10 18:32:09]

Good point. Done.

+  bool valid_handle = !handle.is_null() && handle.id == id &&
+                      handle.type != gfx::SHARED_MEMORY_BUFFER;
+  if (!valid_handle) {
     buffers.erase(buffer_it);
     callback.Run(gfx::GpuMemoryBufferHandle());
     return;
   }
 
-  // The factory should never return a shared memory backed buffer.
-  DCHECK_NE(handle.type, gfx::SHARED_MEMORY_BUFFER);
-
   // Store the type of this buffer so it can be cleaned up if the child
   // process is removed.
   buffer_it->second.type = handle.type;
 
   callback.Run(handle);
 }
 
 }  // namespace content