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) {
   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 {
+// Framebuffer completeness is not cacheable on OS X because of dynamic
+// graphics switching.
+// http://crbug.com/180876
+#if defined(OS_MACOSX)

[no sievers, 2015/08/19 00:43:13]

Doesn't this break unittesting on Mac?
You could move the #ifdef to the construction call site for production code
instead.

[Tobias Sargeant, 2015/08/19 11:33:13]

It would've, if not for the removal of the unittests that asserted the caching
behaviour. As caching wasn't required for correctness (in fact not caching is
required for correctness on OSX - we don't have a test for that) I felt like it
was right to remove the tests.

I've put them back now, and rejigged this code.

+  return glCheckFramebufferStatusEXT(target);
+#else
+  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;
+#endif
 }
 
 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();
     if (!attachment->cleared()) {
       return false;
     }
@@ skipping 212 matching lines @@
            texture_detach_observers_.begin();
        it != texture_detach_observers_.end();
        ++it) {
     TextureDetachObserver* observer = *it;
     observer->OnTextureRefDetachedFromFramebuffer(texture);
   }
 }
 
 }  // namespace gles2
 }  // namespace gpu
-
-