Ensure that ContextGroup is destroyed when an appropriate GL context is current.

Before it relied on the destructor being invoked and this was not always true. I think the subsequent invalid GL commands might have caused memory corruption that lead to the bug referenced below.

Now the ContextGroup is destroyed explicitly when the context is known to be correct.

BUG=97775
Review URL: http://codereview.chromium.org/8135014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@104157 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 66 insertions, 35 deletions

diff --git a/gpu/command_buffer/service/context_group.cc b/gpu/command_buffer/service/context_group.cc
index d04d1d8..ec22342 100644
--- a/gpu/command_buffer/service/context_group.cc
+++ b/gpu/command_buffer/service/context_group.cc
@@ -17,8 +17,7 @@ namespace gpu {
 namespace gles2 {
 
 ContextGroup::ContextGroup(bool bind_generates_resource)
-    : initialized_(false),
-      have_context_(true),
+    : num_contexts_(0),
       bind_generates_resource_(bind_generates_resource),
       max_vertex_attribs_(0u),
       max_texture_units_(0u),
@@ -36,7 +35,7 @@ ContextGroup::ContextGroup(bool bind_generates_resource)
 }
 
 ContextGroup::~ContextGroup() {
-  Destroy();
+  CHECK(num_contexts_ == 0);
 }
 
 static void GetIntegerv(GLenum pname, uint32* var) {
@@ -47,7 +46,8 @@ static void GetIntegerv(GLenum pname, uint32* var) {
 
 bool ContextGroup::Initialize(const DisallowedFeatures& disallowed_features,
                               const char* allowed_features) {
-  if (initialized_) {
+  if (num_contexts_ > 0) {
+    ++num_contexts_;
     return true;
   }
 
@@ -115,38 +115,42 @@ bool ContextGroup::Initialize(const DisallowedFeatures& disallowed_features,
     return false;
   }
 
-  initialized_ = true;
+  ++num_contexts_;
   return true;
 }
 
-void ContextGroup::Destroy() {
+void ContextGroup::Destroy(bool have_context) {
+  DCHECK(num_contexts_ > 0);
+  if (--num_contexts_ > 0)
+    return;
+
   if (buffer_manager_ != NULL) {
-    buffer_manager_->Destroy(have_context_);
+    buffer_manager_->Destroy(have_context);
     buffer_manager_.reset();
   }
 
   if (framebuffer_manager_ != NULL) {
-    framebuffer_manager_->Destroy(have_context_);
+    framebuffer_manager_->Destroy(have_context);
     framebuffer_manager_.reset();
   }
 
   if (renderbuffer_manager_ != NULL) {
-    renderbuffer_manager_->Destroy(have_context_);
+    renderbuffer_manager_->Destroy(have_context);
     renderbuffer_manager_.reset();
   }
 
   if (texture_manager_ != NULL) {
-    texture_manager_->Destroy(have_context_);
+    texture_manager_->Destroy(have_context);
     texture_manager_.reset();
   }
 
   if (program_manager_ != NULL) {
-    program_manager_->Destroy(have_context_);
+    program_manager_->Destroy(have_context);
     program_manager_.reset();
   }
 
   if (shader_manager_ != NULL) {
-    shader_manager_->Destroy(have_context_);
+    shader_manager_->Destroy(have_context);
     shader_manager_.reset();
   }
 }
diff --git a/gpu/command_buffer/service/context_group.h b/gpu/command_buffer/service/context_group.h
index 1700423..48fac44 100644
--- a/gpu/command_buffer/service/context_group.h
+++ b/gpu/command_buffer/service/context_group.h
@@ -39,14 +39,14 @@ class ContextGroup : public base::RefCounted<ContextGroup> {
   explicit ContextGroup(bool bind_generates_resource);
   ~ContextGroup();
 
-  // This should only be called by GLES2Decoder.
+  // This should only be called by GLES2Decoder. This must be paired with a
+  // call to destroy if it succeeds.
   bool Initialize(const DisallowedFeatures& disallowed_features,
                   const char* allowed_features);
 
-  // Sets the ContextGroup has having a lost context.
-  void set_have_context(bool have_context) {
-    have_context_ = have_context;
-  }
+  // Destroys all the resources when called for the last context in the group.
+  // It should only be called by GLES2Decoder.
+  void Destroy(bool have_context);
 
   bool bind_generates_resource() {
     return bind_generates_resource_;
@@ -111,12 +111,8 @@ class ContextGroup : public base::RefCounted<ContextGroup> {
   IdAllocatorInterface* GetIdAllocator(unsigned namespace_id);
 
  private:
-  // Destroys all the resources.
-  void Destroy();
-
   // Whether or not this context is initialized.
-  bool initialized_;
-  bool have_context_;
+  int num_contexts_;
   bool bind_generates_resource_;
 
   uint32 max_vertex_attribs_;
diff --git a/gpu/command_buffer/service/context_group_unittest.cc b/gpu/command_buffer/service/context_group_unittest.cc
index bf8809e..bb60af9 100644
--- a/gpu/command_buffer/service/context_group_unittest.cc
+++ b/gpu/command_buffer/service/context_group_unittest.cc
@@ -40,10 +40,6 @@ class ContextGroupTest : public testing::Test {
   }
 
   virtual void TearDown() {
-    // we must release the ContextGroup before we clear out the GL interface.
-    // since its destructor uses GL.
-    group_->set_have_context(false);
-    group_ = NULL;
     ::gfx::GLInterface::SetGLInterface(NULL);
     gl_.reset();
   }
@@ -93,6 +89,46 @@ TEST_F(ContextGroupTest, InitializeNoExtensions) {
   EXPECT_TRUE(group_->texture_manager() != NULL);
   EXPECT_TRUE(group_->program_manager() != NULL);
   EXPECT_TRUE(group_->shader_manager() != NULL);
+
+  group_->Destroy(false);
+  EXPECT_TRUE(group_->buffer_manager() == NULL);
+  EXPECT_TRUE(group_->framebuffer_manager() == NULL);
+  EXPECT_TRUE(group_->renderbuffer_manager() == NULL);
+  EXPECT_TRUE(group_->texture_manager() == NULL);
+  EXPECT_TRUE(group_->program_manager() == NULL);
+  EXPECT_TRUE(group_->shader_manager() == NULL);
+}
+
+TEST_F(ContextGroupTest, MultipleContexts) {
+  TestHelper::SetupContextGroupInitExpectations(gl_.get(),
+      DisallowedFeatures(), "");
+  group_->Initialize(DisallowedFeatures(), "");
+  group_->Initialize(DisallowedFeatures(), "");
+
+  EXPECT_TRUE(group_->buffer_manager() != NULL);
+  EXPECT_TRUE(group_->framebuffer_manager() != NULL);
+  EXPECT_TRUE(group_->renderbuffer_manager() != NULL);
+  EXPECT_TRUE(group_->texture_manager() != NULL);
+  EXPECT_TRUE(group_->program_manager() != NULL);
+  EXPECT_TRUE(group_->shader_manager() != NULL);
+
+  group_->Destroy(false);
+
+  EXPECT_TRUE(group_->buffer_manager() != NULL);
+  EXPECT_TRUE(group_->framebuffer_manager() != NULL);
+  EXPECT_TRUE(group_->renderbuffer_manager() != NULL);
+  EXPECT_TRUE(group_->texture_manager() != NULL);
+  EXPECT_TRUE(group_->program_manager() != NULL);
+  EXPECT_TRUE(group_->shader_manager() != NULL);
+
+  group_->Destroy(false);
+
+  EXPECT_TRUE(group_->buffer_manager() == NULL);
+  EXPECT_TRUE(group_->framebuffer_manager() == NULL);
+  EXPECT_TRUE(group_->renderbuffer_manager() == NULL);
+  EXPECT_TRUE(group_->texture_manager() == NULL);
+  EXPECT_TRUE(group_->program_manager() == NULL);
+  EXPECT_TRUE(group_->shader_manager() == NULL);
 }
 
 }  // namespace gles2
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index 35ae5b9..c3402d9 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -2360,9 +2360,6 @@ bool GLES2DecoderImpl::GetServiceTextureId(uint32 client_texture_id,
 void GLES2DecoderImpl::Destroy() {
   bool have_context = context_.get() && MakeCurrent();
 
-  if (group_.get())
-    group_->set_have_context(have_context);
-
   SetParent(NULL, 0);
 
   if (have_context) {
@@ -2396,10 +2393,6 @@ void GLES2DecoderImpl::Destroy() {
       offscreen_resolved_frame_buffer_->Destroy();
     if (offscreen_resolved_color_texture_.get())
       offscreen_resolved_color_texture_->Destroy();
-
-    // must release the ContextGroup before destroying the context as its
-    // destructor uses GL.
-    group_ = NULL;
   } else {
     if (offscreen_target_frame_buffer_.get())
       offscreen_target_frame_buffer_->Invalidate();
@@ -2421,6 +2414,9 @@ void GLES2DecoderImpl::Destroy() {
       offscreen_resolved_color_texture_->Invalidate();
   }
 
+  group_->Destroy(have_context);
+  group_ = NULL;
+
   if (context_.get()) {
     context_->ReleaseCurrent(NULL);
     context_ = NULL;
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc b/gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc
index 7dace75..1a72012 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc
@@ -221,8 +221,7 @@ void GLES2DecoderTestBase::TearDown() {
       .RetiresOnSaturation();
   decoder_->Destroy();
   decoder_.reset();
-  group_->set_have_context(false);
-  group_ = NULL;
+  group_->Destroy(false);
   engine_.reset();
   ::gfx::GLInterface::SetGLInterface(NULL);
   gl_.reset();