Fix crash caused by concurrent access to framebuffer_combo_complete_map_.

gpu/command_buffer/service/framebuffer_manager.cc

Index: gpu/command_buffer/service/framebuffer_manager.cc
diff --git a/gpu/command_buffer/service/framebuffer_manager.cc b/gpu/command_buffer/service/framebuffer_manager.cc
index 9a1b77cdfce90133e57274917bf9f482962c87d8..c5b5a13d3cd817e5a42b61fbfe4cd1719faaa613 100644
--- a/gpu/command_buffer/service/framebuffer_manager.cc
+++ b/gpu/command_buffer/service/framebuffer_manager.cc
@@ -6,6 +6,7 @@
 #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"
@@ -22,24 +23,6 @@ DecoderFramebufferState::DecoderFramebufferState()
 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:
@@ -260,14 +243,24 @@ 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);
 }
@@ -503,46 +496,40 @@ GLenum Framebuffer::IsPossiblyComplete() const {
 
 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;
@@ -778,5 +765,3 @@ void FramebufferManager::OnTextureRefDetached(TextureRef* texture) {
 
 }  // namespace gles2
 }  // namespace gpu
-
-