Fix crash caused by concurrent access to framebuffer_combo_complete_map_.

gpu/command_buffer/service/framebuffer_manager_unittest.cc

Index: gpu/command_buffer/service/framebuffer_manager_unittest.cc
diff --git a/gpu/command_buffer/service/framebuffer_manager_unittest.cc b/gpu/command_buffer/service/framebuffer_manager_unittest.cc
index c7133acd9c3c7e1e0003f52c82b0b143f591c0bb..a3dc984b0c4a6a7222529111f829c0ac8a7f9deb 100644
--- a/gpu/command_buffer/service/framebuffer_manager_unittest.cc
+++ b/gpu/command_buffer/service/framebuffer_manager_unittest.cc
@@ -34,7 +34,10 @@ const bool kUseDefaultTextures = false;
 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(),
@@ -111,7 +114,10 @@ class FramebufferInfoTestBase : public GpuServiceTest {
   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(),
@@ -832,59 +838,33 @@ TEST_F(FramebufferInfoTest, GetStatus) {
   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(),
@@ -899,35 +879,14 @@ TEST_F(FramebufferInfoTest, GetStatus) {
       .WillOnce(Return(GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT))
       .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE))
       .RetiresOnSaturation();
-  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);
+  ASSERT_EQ(
+      (GLenum)GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT,
+      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 {