Fix crash caused by concurrent access to framebuffer_combo_complete_map_.

gpu/command_buffer/service/framebuffer_manager.cc

 // Copyright (c) 2012 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 "gpu/command_buffer/service/framebuffer_manager.h"
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "gpu/command_buffer/common/gles2_cmd_utils.h"
+#include "gpu/command_buffer/service/framebuffer_completeness_cache.h"
 #include "gpu/command_buffer/service/renderbuffer_manager.h"
 #include "gpu/command_buffer/service/texture_manager.h"
 #include "ui/gl/gl_bindings.h"
 
 namespace gpu {
 namespace gles2 {
 
 DecoderFramebufferState::DecoderFramebufferState()
     : clear_state_dirty(false),
       bound_read_framebuffer(NULL),
       bound_draw_framebuffer(NULL) {
 }
 
 DecoderFramebufferState::~DecoderFramebufferState() {
 }
 
-Framebuffer::FramebufferComboCompleteMap*
-    Framebuffer::framebuffer_combo_complete_map_;
-
-// Framebuffer completeness is not cacheable on OS X because of dynamic
-// graphics switching.
-// http://crbug.com/180876
-#if defined(OS_MACOSX)
-bool Framebuffer::allow_framebuffer_combo_complete_map_ = false;
-#else
-bool Framebuffer::allow_framebuffer_combo_complete_map_ = true;
-#endif
-
-void Framebuffer::ClearFramebufferCompleteComboMap() {
-  if (framebuffer_combo_complete_map_) {
-    framebuffer_combo_complete_map_->clear();
-  }
-}
-
 class RenderbufferAttachment
     : public Framebuffer::Attachment {
  public:
   explicit RenderbufferAttachment(
       Renderbuffer* renderbuffer)
       : renderbuffer_(renderbuffer) {
   }
 
   GLsizei width() const override { return renderbuffer_->width(); }
 
@@ skipping 200 matching lines @@
   GLsizei samples_;
 
   DISALLOW_COPY_AND_ASSIGN(TextureAttachment);
 };
 
 FramebufferManager::TextureDetachObserver::TextureDetachObserver() {}
 
 FramebufferManager::TextureDetachObserver::~TextureDetachObserver() {}
 
 FramebufferManager::FramebufferManager(
-    uint32 max_draw_buffers, uint32 max_color_attachments,
-    ContextGroup::ContextType context_type)
+    uint32 max_draw_buffers,
+    uint32 max_color_attachments,
+    ContextGroup::ContextType context_type,
+    const scoped_refptr<FramebufferCompletenessCache>&
+        framebuffer_combo_complete_cache)
     : framebuffer_state_change_count_(1),
       framebuffer_count_(0),
       have_context_(true),
       max_draw_buffers_(max_draw_buffers),
       max_color_attachments_(max_color_attachments),
-      context_type_(context_type) {
+      context_type_(context_type),
+      framebuffer_combo_complete_cache_(framebuffer_combo_complete_cache) {
+#if defined(OS_MACOSX)
+  // Framebuffer completeness is not cacheable on OS X because of dynamic
+  // graphics switching.
+  // http://crbug.com/180876
+  framebuffer_combo_complete_cache_ = NULL;

[no sievers, 2015/08/19 23:39:47]

Isn't it easier to pass in NULL on Mac from GpuCommandBufferStub? (I don't think
we have to worry about GPU switching from other call sites.) THen you also don't
need the setter method for testing.

[no sievers, 2015/08/19 23:47:00]

And you can add a TODO, because I think we can do better and figure out from the
call site if the GPU is actually switchable. I'm not sure if we are even also
missing Optimus switching here. +kbr@

[Ken Russell (switch to Gerrit), 2015/08/20 00:04:10]

To the best of my knowledge, Optimus switching doesn't migrate the process from
one GPU to the other -- only dynamically decides at program startup which GPU it
will run on. Therefore I think this general approach is still OK.

[Tobias Sargeant, 2015/08/20 11:05:13]

Moving it to the ContextGroup constructor instead. That should give the same
code cleanup effect, but also intercepts other call paths in
components/view_manager/gles2/command_buffer_driver.cc and
components/view_manager/gles2/command_buffer_local.cc. I'm unsure whether these
are used on OSX.

Also, the latter of those two files was missed initially, so it looks like no
trybots compile it.

[Tobias Sargeant, 2015/08/20 11:05:13]

Done.

+#endif
   DCHECK_GT(max_draw_buffers_, 0u);
   DCHECK_GT(max_color_attachments_, 0u);
 }
 
 FramebufferManager::~FramebufferManager() {
   DCHECK(framebuffers_.empty());
   // If this triggers, that means something is keeping a reference to a
   // Framebuffer belonging to this.
   CHECK_EQ(framebuffer_count_, 0u);
 }
@@ skipping 215 matching lines @@
     }
   }
 
   // This does not mean the framebuffer is actually complete. It just means our
   // checks passed.
   return GL_FRAMEBUFFER_COMPLETE;
 }
 
 GLenum Framebuffer::GetStatus(
     TextureManager* texture_manager, GLenum target) const {
+  if (!manager_->GetFramebufferComboCompleteCache()) {
+    return glCheckFramebufferStatusEXT(target);
+  }
   // Check if we have this combo already.
   std::string signature;
-  if (allow_framebuffer_combo_complete_map_) {
-    size_t signature_size = sizeof(target);
-    for (AttachmentMap::const_iterator it = attachments_.begin();
-         it != attachments_.end(); ++it) {
-      Attachment* attachment = it->second.get();
-      signature_size += sizeof(it->first) +
-                        attachment->GetSignatureSize(texture_manager);
-    }
 
-    signature.reserve(signature_size);
-    signature.append(reinterpret_cast<const char*>(&target), sizeof(target));
+  size_t signature_size = sizeof(target);
+  for (AttachmentMap::const_iterator it = attachments_.begin();
+       it != attachments_.end(); ++it) {
+    Attachment* attachment = it->second.get();
+    signature_size +=
+        sizeof(it->first) + attachment->GetSignatureSize(texture_manager);
+  }
 
-    for (AttachmentMap::const_iterator it = attachments_.begin();
-         it != attachments_.end(); ++it) {
-      Attachment* attachment = it->second.get();
-      signature.append(reinterpret_cast<const char*>(&it->first),
-                       sizeof(it->first));
-      attachment->AddToSignature(texture_manager, &signature);
-    }
-    DCHECK(signature.size() == signature_size);
+  signature.reserve(signature_size);
+  signature.append(reinterpret_cast<const char*>(&target), sizeof(target));
 
-    if (!framebuffer_combo_complete_map_) {
-      framebuffer_combo_complete_map_ = new FramebufferComboCompleteMap();
-    }
+  for (AttachmentMap::const_iterator it = attachments_.begin();
+       it != attachments_.end(); ++it) {
+    Attachment* attachment = it->second.get();
+    signature.append(reinterpret_cast<const char*>(&it->first),
+                     sizeof(it->first));
+    attachment->AddToSignature(texture_manager, &signature);
+  }
+  DCHECK(signature.size() == signature_size);
 
-    FramebufferComboCompleteMap::const_iterator it =
-        framebuffer_combo_complete_map_->find(signature);
-    if (it != framebuffer_combo_complete_map_->end()) {
-      return GL_FRAMEBUFFER_COMPLETE;
-    }
+  if (manager_->GetFramebufferComboCompleteCache()->IsComplete(signature)) {
+    return GL_FRAMEBUFFER_COMPLETE;
   }
 
   GLenum result = glCheckFramebufferStatusEXT(target);
 
-  // Insert the new result into the combo map.
-  if (allow_framebuffer_combo_complete_map_ &&
-      result == GL_FRAMEBUFFER_COMPLETE) {
-    framebuffer_combo_complete_map_->insert(std::make_pair(signature, true));
+  if (result == GL_FRAMEBUFFER_COMPLETE) {
+    manager_->GetFramebufferComboCompleteCache()->SetComplete(signature);
   }
 
   return result;
 }
 
 bool Framebuffer::IsCleared() const {
   // are all the attachments cleaared?
   for (AttachmentMap::const_iterator it = attachments_.begin();
        it != attachments_.end(); ++it) {
     Attachment* attachment = it->second.get();
@@ skipping 215 matching lines @@
            texture_detach_observers_.begin();
        it != texture_detach_observers_.end();
        ++it) {
     TextureDetachObserver* observer = *it;
     observer->OnTextureRefDetachedFromFramebuffer(texture);
   }
 }
 
 }  // namespace gles2
 }  // namespace gpu
-
-