Delete Programs when no longer referenced

TEST=unit tests
BUG=122007
R=apatrick@chromium.org


Review URL: http://codereview.chromium.org/10008019

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

9 files changed, 82 insertions, 32 deletions

diff --git a/gpu/command_buffer/build_gles2_cmd_buffer.py b/gpu/command_buffer/build_gles2_cmd_buffer.py
index c658053..e29b692 100755
--- a/gpu/command_buffer/build_gles2_cmd_buffer.py
+++ b/gpu/command_buffer/build_gles2_cmd_buffer.py
@@ -1105,6 +1105,7 @@ _FUNCTION_INFO = {
     'type': 'GETn',
     'decoder_func': 'DoGetProgramiv',
     'result': ['SizedResult<GLint>'],
+    'expectation': False,
   },
   'GetProgramInfoCHROMIUM': {
     'type': 'Custom',
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index a4f2f7b..3f23bb2 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -3851,7 +3851,6 @@ error::Error GLES2DecoderImpl::HandleDeleteProgram(
     ProgramManager::ProgramInfo* info = GetProgramInfo(client_id);
     if (info) {
       if (!info->IsDeleted()) {
-        glDeleteProgram(info->service_id());
         program_manager()->MarkAsDeleted(shader_manager(), info);
       }
     } else {
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc b/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
index 16246f5..6a56a2c 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
@@ -3306,8 +3306,12 @@ TEST_F(GLES2DecoderTest, IsFramebuffer) {
 TEST_F(GLES2DecoderTest, IsProgram) {
   // IsProgram is true as soon as the program is created.
   EXPECT_TRUE(DoIsProgram(client_program_id_));
+  EXPECT_CALL(*gl_, DeleteProgram(kServiceProgramId))
+      .Times(1)
+      .RetiresOnSaturation();
   DoDeleteProgram(client_program_id_, kServiceProgramId);
   EXPECT_FALSE(DoIsProgram(client_program_id_));
+
 }
 
 TEST_F(GLES2DecoderTest, IsRenderbuffer) {
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder_unittest_1.cc b/gpu/command_buffer/service/gles2_cmd_decoder_unittest_1.cc
index 19ea2bc..36cd8e2 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder_unittest_1.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder_unittest_1.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -157,6 +157,21 @@ void GLES2DecoderTestBase::SpecializedSetup<GetRenderbufferParameteriv, 0>(
 };
 
 template <>
+void GLES2DecoderTestBase::SpecializedSetup<GetProgramiv, 0>(
+    bool valid) {
+  if (valid) {
+    // GetProgramiv calls ClearGLError then GetError to make sure
+    // it actually got a value so it can report correctly to the client.
+    EXPECT_CALL(*gl_, GetError())
+        .WillOnce(Return(GL_NO_ERROR))
+        .RetiresOnSaturation();
+    EXPECT_CALL(*gl_, GetError())
+        .WillOnce(Return(GL_NO_ERROR))
+        .RetiresOnSaturation();
+  }
+}
+
+template <>
 void GLES2DecoderTestBase::SpecializedSetup<GetProgramInfoLog, 0>(
     bool /* valid */) {
   const GLuint kClientVertexShaderId = 5001;
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h b/gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h
index 2f3cf78..6d6749b 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h
+++ b/gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h
@@ -1408,16 +1408,9 @@ TEST_F(GLES2DecoderTest1, GetIntegervInvalidArgs1_1) {
 }
 
 TEST_F(GLES2DecoderTest1, GetProgramivValidArgs) {
-  EXPECT_CALL(*gl_, GetError())
-      .WillOnce(Return(GL_NO_ERROR))
-      .WillOnce(Return(GL_NO_ERROR))
-      .RetiresOnSaturation();
   SpecializedSetup<GetProgramiv, 0>(true);
   typedef GetProgramiv::Result Result;
   Result* result = static_cast<Result*>(shared_memory_address_);
-  EXPECT_CALL(
-      *gl_, GetProgramiv(
-          kServiceProgramId, GL_DELETE_STATUS, result->GetData()));
   result->size = 0;
   GetProgramiv cmd;
   cmd.Init(
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 7e44be7..5754406 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc
@@ -295,10 +295,7 @@ bool GLES2DecoderTestBase::DoIsProgram(GLuint client_id) {
 }
 
 void GLES2DecoderTestBase::DoDeleteProgram(
-    GLuint client_id, GLuint service_id) {
-  EXPECT_CALL(*gl_, DeleteProgram(service_id))
-      .Times(1)
-      .RetiresOnSaturation();
+    GLuint client_id, GLuint /* service_id */) {
   DeleteProgram cmd;
   cmd.Init(client_id);
   EXPECT_EQ(error::kNoError, ExecuteCmd(cmd));
diff --git a/gpu/command_buffer/service/program_manager.cc b/gpu/command_buffer/service/program_manager.cc
index 18593356..d34a181 100644
--- a/gpu/command_buffer/service/program_manager.cc
+++ b/gpu/command_buffer/service/program_manager.cc
@@ -49,13 +49,17 @@ bool ProgramManager::IsInvalidPrefix(const char* name, size_t length) {
       memcmp(name, kInvalidPrefix, sizeof(kInvalidPrefix)) == 0);
 }
 
-ProgramManager::ProgramInfo::ProgramInfo(GLuint service_id)
-    : use_count_(0),
+ProgramManager::ProgramInfo::ProgramInfo(
+    ProgramManager* manager, GLuint service_id)
+    : manager_(manager),
+      use_count_(0),
       max_attrib_name_length_(0),
       max_uniform_name_length_(0),
       service_id_(service_id),
+      deleted_(false),
       valid_(false),
       link_status_(false) {
+  manager_->StartTracking(this);
 }
 
 void ProgramManager::ProgramInfo::Reset() {
@@ -389,6 +393,9 @@ void ProgramManager::ProgramInfo::GetProgramiv(GLenum pname, GLint* params) {
       // Notice +1 to accomodate NULL terminator.
       *params = log_info_.get() ? (log_info_->size() + 1) : 0;
       break;
+    case GL_DELETE_STATUS:
+      *params = deleted_;
+      break;
     case GL_VALIDATE_STATUS:
       if (!IsValid()) {
         *params = GL_FALSE;
@@ -554,7 +561,15 @@ void ProgramManager::ProgramInfo::GetProgramInfo(
   DCHECK_EQ(ComputeOffset(header, strings), size);
 }
 
-ProgramManager::ProgramInfo::~ProgramInfo() {}
+ProgramManager::ProgramInfo::~ProgramInfo() {
+  if (manager_) {
+    if (manager_->have_context_) {
+      glDeleteProgram(service_id());
+    }
+    manager_->StopTracking(this);
+    manager_ = NULL;
+  }
+}
 
 // TODO(gman): make this some kind of random number. Base::RandInt is not
 // callable because of the sandbox. What matters is that it's possibly different
@@ -562,7 +577,9 @@ ProgramManager::ProgramInfo::~ProgramInfo() {}
 static int uniform_random_offset_ = 3;
 
 ProgramManager::ProgramManager()
-    : uniform_swizzle_(uniform_random_offset_++ % 15) {
+    : uniform_swizzle_(uniform_random_offset_++ % 15),
+      program_info_count_(0),
+      have_context_(true) {
 }
 
 ProgramManager::~ProgramManager() {
@@ -570,16 +587,16 @@ ProgramManager::~ProgramManager() {
 }
 
 void ProgramManager::Destroy(bool have_context) {
-  while (!program_infos_.empty()) {
-    if (have_context) {
-      ProgramInfo* info = program_infos_.begin()->second;
-      if (!info->IsDeleted()) {
-        glDeleteProgram(info->service_id());
-        info->MarkAsDeleted();
-      }
-    }
-    program_infos_.erase(program_infos_.begin());
-  }
+  have_context_ = have_context;
+  program_infos_.clear();
+}
+
+void ProgramManager::StartTracking(ProgramManager::ProgramInfo* /* program */) {
+  ++program_info_count_;
+}
+
+void ProgramManager::StopTracking(ProgramManager::ProgramInfo* /* program */) {
+  --program_info_count_;
 }
 
 ProgramManager::ProgramInfo* ProgramManager::CreateProgramInfo(
@@ -587,7 +604,7 @@ ProgramManager::ProgramInfo* ProgramManager::CreateProgramInfo(
   std::pair<ProgramInfoMap::iterator, bool> result =
       program_infos_.insert(
           std::make_pair(client_id,
-                         ProgramInfo::Ref(new ProgramInfo(service_id))));
+                         ProgramInfo::Ref(new ProgramInfo(this, service_id))));
   DCHECK(result.second);
   return result.first->second;
 }
diff --git a/gpu/command_buffer/service/program_manager.h b/gpu/command_buffer/service/program_manager.h
index c627957..7315b68 100644
--- a/gpu/command_buffer/service/program_manager.h
+++ b/gpu/command_buffer/service/program_manager.h
@@ -72,7 +72,7 @@ class GPU_EXPORT ProgramManager {
     typedef std::vector<VertexAttribInfo> AttribInfoVector;
     typedef std::vector<int> SamplerIndices;
 
-    explicit ProgramInfo(GLuint service_id);
+    ProgramInfo(ProgramManager* manager, GLuint service_id);
 
     GLuint service_id() const {
       return service_id_;
@@ -125,7 +125,7 @@ class GPU_EXPORT ProgramManager {
     bool SetSamplers(GLint fake_location, GLsizei count, const GLint* value);
 
     bool IsDeleted() const {
-      return service_id_ == 0;
+      return deleted_;
     }
 
     void GetProgramiv(GLenum pname, GLint* params);
@@ -194,8 +194,8 @@ class GPU_EXPORT ProgramManager {
     }
 
     void MarkAsDeleted() {
-      DCHECK_NE(service_id_, 0u);
-      service_id_ = 0;
+      DCHECK(!deleted_);
+      deleted_ =  true;
     }
 
     // Resets the program.
@@ -227,6 +227,8 @@ class GPU_EXPORT ProgramManager {
       return (fake_location >> 16) & 0xFFFF;
     }
 
+    ProgramManager* manager_;
+
     int use_count_;
 
     GLsizei max_attrib_name_length_;
@@ -251,6 +253,9 @@ class GPU_EXPORT ProgramManager {
     // Shaders by type of shader.
     ShaderManager::ShaderInfo::Ref attached_shaders_[kMaxAttachedShaders];
 
+    // True if this program is marked as deleted.
+    bool deleted_;
+
     // This is true if glLinkProgram was successful at least once.
     bool valid_;
 
@@ -298,6 +303,9 @@ class GPU_EXPORT ProgramManager {
   GLint UnswizzleLocation(GLint swizzled_location) const;
 
  private:
+  void StartTracking(ProgramInfo* info);
+  void StopTracking(ProgramInfo* info);
+
   // Info for each "successfully linked" program by service side program Id.
   // TODO(gman): Choose a faster container.
   typedef std::map<GLuint, ProgramInfo::Ref> ProgramInfoMap;
@@ -305,6 +313,12 @@ class GPU_EXPORT ProgramManager {
 
   int uniform_swizzle_;
 
+  // Counts the number of ProgramInfo allocated with 'this' as its manager.
+  // Allows to check no ProgramInfo will outlive this.
+  unsigned int program_info_count_;
+
+  bool have_context_;
+
   void RemoveProgramInfoIfUnused(
       ShaderManager* shader_manager, ProgramInfo* info);
 
diff --git a/gpu/command_buffer/service/program_manager_unittest.cc b/gpu/command_buffer/service/program_manager_unittest.cc
index ba7288ae..3482081 100644
--- a/gpu/command_buffer/service/program_manager_unittest.cc
+++ b/gpu/command_buffer/service/program_manager_unittest.cc
@@ -105,6 +105,10 @@ TEST_F(ProgramManagerTest, DeleteBug) {
   ASSERT_TRUE(info2);
   manager_.UseProgram(info1);
   manager_.MarkAsDeleted(&shader_manager, info1);
+  //  Program will be deleted when last ref is released.
+  EXPECT_CALL(*gl_, DeleteProgram(kService2Id))
+      .Times(1)
+      .RetiresOnSaturation();
   manager_.MarkAsDeleted(&shader_manager, info2);
   EXPECT_TRUE(manager_.IsOwned(info1));
   EXPECT_FALSE(manager_.IsOwned(info2));
@@ -839,6 +843,9 @@ TEST_F(ProgramManagerWithShaderTest, ProgramInfoUseCount) {
   manager_.UnuseProgram(&shader_manager_, program_info);
   EXPECT_TRUE(program_info->InUse());
   // this should delete the info.
+  EXPECT_CALL(*gl_, DeleteProgram(kServiceProgramId))
+      .Times(1)
+      .RetiresOnSaturation();
   manager_.UnuseProgram(&shader_manager_, program_info);
   info2 = manager_.GetProgramInfo(kClientProgramId);
   EXPECT_TRUE(info2 == NULL);
@@ -886,6 +893,9 @@ TEST_F(ProgramManagerWithShaderTest, ProgramInfoUseCount2) {
       manager_.GetProgramInfo(kClientProgramId);
   EXPECT_EQ(program_info, info2);
   // this should delete the program.
+  EXPECT_CALL(*gl_, DeleteProgram(kServiceProgramId))
+      .Times(1)
+      .RetiresOnSaturation();
   manager_.MarkAsDeleted(&shader_manager_, program_info);
   info2 = manager_.GetProgramInfo(kClientProgramId);
   EXPECT_TRUE(info2 == NULL);