Fix crash caused by concurrent access to framebuffer_combo_complete_map_.

gpu/command_buffer/service/framebuffer_manager_unittest.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/error_state_mock.h"
 #include "gpu/command_buffer/service/framebuffer_manager.h"
 #include "gpu/command_buffer/service/feature_info.h"
 #include "gpu/command_buffer/service/gpu_service_test.h"
 #include "gpu/command_buffer/service/renderbuffer_manager.h"
 #include "gpu/command_buffer/service/test_helper.h"
@@ skipping 16 matching lines @@
 const GLint kMaxSamples = 4;
 const uint32 kMaxDrawBuffers = 16;
 const uint32 kMaxColorAttachments = 16;
 const bool kUseDefaultTextures = false;
 
 }  // namespace
 
 class FramebufferManagerTest : public GpuServiceTest {
  public:
   FramebufferManagerTest()
-      : manager_(1, 1, ContextGroup::CONTEXT_TYPE_UNDEFINED),
+      : manager_(1,
+                 1,
+                 ContextGroup::CONTEXT_TYPE_UNDEFINED,
+                 new FramebufferCompletenessCache),
         feature_info_(new FeatureInfo()) {
     texture_manager_.reset(new TextureManager(NULL,
                                               feature_info_.get(),
                                               kMaxTextureSize,
                                               kMaxCubemapSize,
                                               kMaxRectangleTextureSize,
                                               kMax3DTextureSize,
                                               kUseDefaultTextures));
     renderbuffer_manager_.reset(new RenderbufferManager(NULL,
                                                         kMaxRenderbufferSize,
@@ skipping 56 matching lines @@
   framebuffer1 = manager_.GetFramebuffer(kClient1Id);
   ASSERT_TRUE(framebuffer1 == NULL);
 }
 
 class FramebufferInfoTestBase : public GpuServiceTest {
  public:
   static const GLuint kClient1Id = 1;
   static const GLuint kService1Id = 11;
 
   explicit FramebufferInfoTestBase(ContextGroup::ContextType context_type)
-      : manager_(kMaxDrawBuffers,  kMaxColorAttachments, context_type),
+      : manager_(kMaxDrawBuffers,
+                 kMaxColorAttachments,
+                 context_type,
+                 new FramebufferCompletenessCache),
         feature_info_(new FeatureInfo()) {
     texture_manager_.reset(new TextureManager(NULL,
                                               feature_info_.get(),
                                               kMaxTextureSize,
                                               kMaxCubemapSize,
                                               kMaxRectangleTextureSize,
                                               kMax3DTextureSize,
                                               kUseDefaultTextures));
     renderbuffer_manager_.reset(new RenderbufferManager(NULL,
                                                         kMaxRenderbufferSize,
@@ skipping 700 matching lines @@
   ASSERT_TRUE(renderbuffer1 != NULL);
   texture_manager_->CreateTexture(kTextureClient2Id, kTextureService2Id);
   scoped_refptr<TextureRef> texture2(
       texture_manager_->GetTexture(kTextureClient2Id));
   ASSERT_TRUE(texture2.get() != NULL);
   texture_manager_->SetTarget(texture2.get(), GL_TEXTURE_2D);
 
   EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_FRAMEBUFFER))
       .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE))
       .RetiresOnSaturation();
-  framebuffer_->GetStatus(texture_manager_.get(), GL_FRAMEBUFFER);
-
-  // Check a second call for the same type does not call anything
-  if (!framebuffer_->AllowFramebufferComboCompleteMapForTesting()) {
-    EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_FRAMEBUFFER))
-        .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE))
-        .RetiresOnSaturation();
-  }
-  framebuffer_->GetStatus(texture_manager_.get(), GL_FRAMEBUFFER);

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

I think all of these GetStatus() calls are important, because they test if the
cache actually works and we end up *not* calling
gl_->CheckFramebufferStatusEXT() again.

But you should be able to change it so that we don't need the if-conditions and
the tests works the same way - with the cache - on all platforms.

Can you double check that the test fails if we don't enable the cache and with
the extra GetStatus calls? It probably comes down to making sure we created the
right StrictMock for gl_.

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

Done, and tested that it fails if the cache is unset.

+  ASSERT_EQ((GLenum)GL_FRAMEBUFFER_COMPLETE,
+            framebuffer_->GetStatus(texture_manager_.get(), GL_FRAMEBUFFER));
 
   // Check changing the attachments calls CheckFramebufferStatus.
   framebuffer_->AttachTexture(
       GL_COLOR_ATTACHMENT0, texture2.get(), kTarget1, kLevel1, kSamples1);
   EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_FRAMEBUFFER))
       .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE)).RetiresOnSaturation();
-  framebuffer_->GetStatus(texture_manager_.get(), GL_FRAMEBUFFER);
-
-  // Check a second call for the same type does not call anything.
-  if (!framebuffer_->AllowFramebufferComboCompleteMapForTesting()) {
-    EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_FRAMEBUFFER))
-        .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE))
-        .RetiresOnSaturation();
-  }
-  framebuffer_->GetStatus(texture_manager_.get(), GL_FRAMEBUFFER);
+  ASSERT_EQ((GLenum)GL_FRAMEBUFFER_COMPLETE,
+            framebuffer_->GetStatus(texture_manager_.get(), GL_FRAMEBUFFER));
 
   // Check a second call with a different target calls CheckFramebufferStatus.
   EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_READ_FRAMEBUFFER))
       .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE))
       .RetiresOnSaturation();
-  framebuffer_->GetStatus(texture_manager_.get(), GL_READ_FRAMEBUFFER);
-
-  // Check a second call for the same type does not call anything.
-  if (!framebuffer_->AllowFramebufferComboCompleteMapForTesting()) {
-    EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_READ_FRAMEBUFFER))
-        .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE))
-        .RetiresOnSaturation();
-  }
-  framebuffer_->GetStatus(texture_manager_.get(), GL_READ_FRAMEBUFFER);
+  ASSERT_EQ(
+      (GLenum)GL_FRAMEBUFFER_COMPLETE,
+      framebuffer_->GetStatus(texture_manager_.get(), GL_READ_FRAMEBUFFER));
 
   // Check adding another attachment calls CheckFramebufferStatus.
   framebuffer_->AttachRenderbuffer(GL_DEPTH_ATTACHMENT, renderbuffer1);
   EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_READ_FRAMEBUFFER))
       .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE))
       .RetiresOnSaturation();
-  framebuffer_->GetStatus(texture_manager_.get(), GL_READ_FRAMEBUFFER);
-
-  // Check a second call for the same type does not call anything.
-  if (!framebuffer_->AllowFramebufferComboCompleteMapForTesting()) {
-    EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_READ_FRAMEBUFFER))
-        .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE))
-        .RetiresOnSaturation();
-  }
-  framebuffer_->GetStatus(texture_manager_.get(), GL_READ_FRAMEBUFFER);
+  ASSERT_EQ(
+      (GLenum)GL_FRAMEBUFFER_COMPLETE,
+      framebuffer_->GetStatus(texture_manager_.get(), GL_READ_FRAMEBUFFER));
 
   // Check changing the format calls CheckFramebuffferStatus.
   TestHelper::SetTexParameteriWithExpectations(gl_.get(),
                                                error_state_.get(),
                                                texture_manager_.get(),
                                                texture2.get(),
                                                GL_TEXTURE_WRAP_S,
                                                GL_CLAMP_TO_EDGE,
                                                GL_NO_ERROR);
 
   EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_READ_FRAMEBUFFER))
       .WillOnce(Return(GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT))
       .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE))
       .RetiresOnSaturation();
-  framebuffer_->GetStatus(texture_manager_.get(), GL_READ_FRAMEBUFFER);
+  ASSERT_EQ(
+      (GLenum)GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT,
+      framebuffer_->GetStatus(texture_manager_.get(), GL_READ_FRAMEBUFFER));
 
-  // Check since it did not return FRAMEBUFFER_COMPLETE that it calls
-  // CheckFramebufferStatus
-  framebuffer_->GetStatus(texture_manager_.get(), GL_READ_FRAMEBUFFER);
-
-  // Check putting it back does not call CheckFramebufferStatus.
-  if (!framebuffer_->AllowFramebufferComboCompleteMapForTesting()) {
-    EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_READ_FRAMEBUFFER))
-        .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE))
-        .RetiresOnSaturation();
-  }
-  TestHelper::SetTexParameteriWithExpectations(gl_.get(),
-                                               error_state_.get(),
-                                               texture_manager_.get(),
-                                               texture2.get(),
-                                               GL_TEXTURE_WRAP_S,
-                                               GL_REPEAT,
-                                               GL_NO_ERROR);
-  framebuffer_->GetStatus(texture_manager_.get(), GL_READ_FRAMEBUFFER);
-
-  // Check Unbinding does not call CheckFramebufferStatus
-  framebuffer_->UnbindRenderbuffer(GL_RENDERBUFFER, renderbuffer1);
-  if (!framebuffer_->AllowFramebufferComboCompleteMapForTesting()) {
-    EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_READ_FRAMEBUFFER))
-        .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE))
-        .RetiresOnSaturation();
-  }
-  framebuffer_->GetStatus(texture_manager_.get(), GL_READ_FRAMEBUFFER);
+  // Check that incomplete framebuffer statuses are not cached.
+  ASSERT_EQ(
+      (GLenum)GL_FRAMEBUFFER_COMPLETE,
+      framebuffer_->GetStatus(texture_manager_.get(), GL_READ_FRAMEBUFFER));
 }
 
 class FramebufferInfoES3Test : public FramebufferInfoTestBase {
  public:
   FramebufferInfoES3Test()
       : FramebufferInfoTestBase(ContextGroup::CONTEXT_TYPE_WEBGL2) {
   }
 };
 
 TEST_F(FramebufferInfoES3Test, DifferentDimensions) {
@@ skipping 57 matching lines @@
       renderbuffer_manager_->GetRenderbuffer(kRenderbufferClientId);
   ASSERT_TRUE(renderbuffer != NULL);
   framebuffer_->AttachRenderbuffer(GL_COLOR_ATTACHMENT1, renderbuffer);
   EXPECT_TRUE(framebuffer_->GetReadBufferAttachment());
 }
 
 }  // namespace gles2
 }  // namespace gpu