Fix bug in shader and program managers.

The bug was that the code would search by id instead of by pointer.
This meant it was possible to free the wrong thing (ie, pass in
an info with id=4 and free a different info that also happened
to have id=4). This was most likely to happen when calling
MarkAsDeleted since MarkAsDelete sets the id to 0.

TEST=unit tests
BUG=75629

R=apatrick@chromium.org

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

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

6 files changed, 112 insertions, 29 deletions

diff --git a/gpu/command_buffer/service/program_manager.cc b/gpu/command_buffer/service/program_manager.cc
index 683449a..b52c017 100644
--- a/gpu/command_buffer/service/program_manager.cc
+++ b/gpu/command_buffer/service/program_manager.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -454,15 +454,26 @@ bool ProgramManager::GetClientId(GLuint service_id, GLuint* client_id) const {
   return false;
 }
 
+bool ProgramManager::IsOwned(ProgramManager::ProgramInfo* info) {
+  for (ProgramInfoMap::iterator it = program_infos_.begin();
+       it != program_infos_.end(); ++it) {
+    if (it->second.get() == info) {
+      return true;
+    }
+  }
+  return false;
+}
+
 void ProgramManager::RemoveProgramInfoIfUnused(
     ShaderManager* shader_manager, ProgramInfo* info) {
   DCHECK(shader_manager);
   DCHECK(info);
+  DCHECK(IsOwned(info));
   if (info->IsDeleted() && !info->InUse()) {
     info->DetachShaders(shader_manager);
     for (ProgramInfoMap::iterator it = program_infos_.begin();
          it != program_infos_.end(); ++it) {
-      if (it->second->service_id() == info->service_id()) {
+      if (it->second.get() == info) {
         program_infos_.erase(it);
         return;
       }
@@ -476,12 +487,14 @@ void ProgramManager::MarkAsDeleted(
     ProgramManager::ProgramInfo* info) {
   DCHECK(shader_manager);
   DCHECK(info);
+  DCHECK(IsOwned(info));
   info->MarkAsDeleted();
   RemoveProgramInfoIfUnused(shader_manager, info);
 }
 
 void ProgramManager::UseProgram(ProgramManager::ProgramInfo* info) {
   DCHECK(info);
+  DCHECK(IsOwned(info));
   info->IncUseCount();
 }
 
@@ -490,6 +503,7 @@ void ProgramManager::UnuseProgram(
     ProgramManager::ProgramInfo* info) {
   DCHECK(shader_manager);
   DCHECK(info);
+  DCHECK(IsOwned(info));
   info->DecUseCount();
   RemoveProgramInfoIfUnused(shader_manager, info);
 }
diff --git a/gpu/command_buffer/service/program_manager.h b/gpu/command_buffer/service/program_manager.h
index 5c0070f..51a9bf8 100644
--- a/gpu/command_buffer/service/program_manager.h
+++ b/gpu/command_buffer/service/program_manager.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -247,6 +247,9 @@ class ProgramManager {
   // Returns true if prefix is invalid for gl.
   static bool IsInvalidPrefix(const char* name, size_t length);
 
+  // Check if a ProgramInfo is owned by this ProgramManager.
+  bool IsOwned(ProgramInfo* info);
+
  private:
   // Info for each "successfully linked" program by service side program Id.
   // TODO(gman): Choose a faster container.
diff --git a/gpu/command_buffer/service/program_manager_unittest.cc b/gpu/command_buffer/service/program_manager_unittest.cc
index 2f37cfd..6e10a38 100644
--- a/gpu/command_buffer/service/program_manager_unittest.cc
+++ b/gpu/command_buffer/service/program_manager_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -86,6 +86,27 @@ TEST_F(ProgramManagerTest, Destroy) {
   ASSERT_TRUE(info1 == NULL);
 }
 
+TEST_F(ProgramManagerTest, DeleteBug) {
+  ShaderManager shader_manager;
+  const GLuint kClient1Id = 1;
+  const GLuint kClient2Id = 2;
+  const GLuint kService1Id = 11;
+  const GLuint kService2Id = 12;
+  // Check we can create program.
+  manager_.CreateProgramInfo(kClient1Id, kService1Id);
+  manager_.CreateProgramInfo(kClient2Id, kService2Id);
+  // Check program got created.
+  ProgramManager::ProgramInfo::Ref info1(manager_.GetProgramInfo(kClient1Id));
+  ProgramManager::ProgramInfo::Ref info2(manager_.GetProgramInfo(kClient2Id));
+  ASSERT_TRUE(info1.get() != NULL);
+  ASSERT_TRUE(info2.get() != NULL);
+  manager_.UseProgram(info1);
+  manager_.MarkAsDeleted(&shader_manager, info1);
+  manager_.MarkAsDeleted(&shader_manager, info2);
+  EXPECT_TRUE(manager_.IsOwned(info1));
+  EXPECT_FALSE(manager_.IsOwned(info2));
+}
+
 TEST_F(ProgramManagerTest, ProgramInfo) {
   const GLuint kClient1Id = 1;
   const GLuint kService1Id = 11;
diff --git a/gpu/command_buffer/service/shader_manager.cc b/gpu/command_buffer/service/shader_manager.cc
index 61cb3f2..f85eb4d 100644
--- a/gpu/command_buffer/service/shader_manager.cc
+++ b/gpu/command_buffer/service/shader_manager.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -8,6 +8,30 @@
 namespace gpu {
 namespace gles2 {
 
+ShaderManager::ShaderInfo::ShaderInfo(GLuint service_id, GLenum shader_type)
+      : use_count_(0),
+        service_id_(service_id),
+        shader_type_(shader_type),
+        valid_(false) {
+}
+
+ShaderManager::ShaderInfo::~ShaderInfo() {
+}
+
+void ShaderManager::ShaderInfo::IncUseCount() {
+  ++use_count_;
+}
+
+void ShaderManager::ShaderInfo::DecUseCount() {
+  --use_count_;
+  DCHECK_GE(use_count_, 0);
+}
+
+void ShaderManager::ShaderInfo::MarkAsDeleted() {
+  DCHECK_NE(service_id_, 0u);
+  service_id_ = 0;
+}
+
 void ShaderManager::ShaderInfo::SetStatus(
     bool valid, const char* log, ShaderTranslatorInterface* translator) {
   valid_ = valid;
@@ -80,12 +104,23 @@ bool ShaderManager::GetClientId(GLuint service_id, GLuint* client_id) const {
   return false;
 }
 
+bool ShaderManager::IsOwned(ShaderManager::ShaderInfo* info) {
+  for (ShaderInfoMap::iterator it = shader_infos_.begin();
+       it != shader_infos_.end(); ++it) {
+    if (it->second.get() == info) {
+      return true;
+    }
+  }
+  return false;
+}
+
 void ShaderManager::RemoveShaderInfoIfUnused(ShaderManager::ShaderInfo* info) {
   DCHECK(info);
+  DCHECK(IsOwned(info));
   if (info->IsDeleted() && !info->InUse()) {
     for (ShaderInfoMap::iterator it = shader_infos_.begin();
          it != shader_infos_.end(); ++it) {
-      if (it->second->service_id() == info->service_id()) {
+      if (it->second.get() == info) {
         shader_infos_.erase(it);
         return;
       }
@@ -96,17 +131,20 @@ void ShaderManager::RemoveShaderInfoIfUnused(ShaderManager::ShaderInfo* info) {
 
 void ShaderManager::MarkAsDeleted(ShaderManager::ShaderInfo* info) {
   DCHECK(info);
+  DCHECK(IsOwned(info));
   info->MarkAsDeleted();
   RemoveShaderInfoIfUnused(info);
 }
 
 void ShaderManager::UseShader(ShaderManager::ShaderInfo* info) {
   DCHECK(info);
+  DCHECK(IsOwned(info));
   info->IncUseCount();
 }
 
 void ShaderManager::UnuseShader(ShaderManager::ShaderInfo* info) {
   DCHECK(info);
+  DCHECK(IsOwned(info));
   info->DecUseCount();
   RemoveShaderInfoIfUnused(info);
 }
diff --git a/gpu/command_buffer/service/shader_manager.h b/gpu/command_buffer/service/shader_manager.h
index 548a965..a39630c 100644
--- a/gpu/command_buffer/service/shader_manager.h
+++ b/gpu/command_buffer/service/shader_manager.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -78,28 +78,12 @@ class ShaderManager {
     friend class base::RefCounted<ShaderInfo>;
     friend class ShaderManager;
 
-    ShaderInfo(GLuint service_id, GLenum shader_type)
-        : use_count_(0),
-          service_id_(service_id),
-          shader_type_(shader_type),
-          valid_(false) {
-    }
-
-    ~ShaderInfo() { }
+    ShaderInfo(GLuint service_id, GLenum shader_type);
+    ~ShaderInfo();
 
-    void IncUseCount() {
-      ++use_count_;
-    }
-
-    void DecUseCount() {
-      --use_count_;
-      DCHECK_GE(use_count_, 0);
-    }
-
-    void MarkAsDeleted() {
-      DCHECK_NE(service_id_, 0u);
-      service_id_ = 0;
-    }
+    void IncUseCount();
+    void DecUseCount();
+    void MarkAsDeleted();
 
     int use_count_;
 
@@ -149,6 +133,9 @@ class ShaderManager {
   // then we free the info.
   void UnuseShader(ShaderInfo* info);
 
+  // Check if a ShaderInfo is owned by this ShaderManager.
+  bool IsOwned(ShaderInfo* info);
+
  private:
   // Info for each shader by service side shader Id.
   typedef std::map<GLuint, ShaderInfo::Ref> ShaderInfoMap;
diff --git a/gpu/command_buffer/service/shader_manager_unittest.cc b/gpu/command_buffer/service/shader_manager_unittest.cc
index c3c084d..da82cbc 100644
--- a/gpu/command_buffer/service/shader_manager_unittest.cc
+++ b/gpu/command_buffer/service/shader_manager_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -75,6 +75,26 @@ TEST_F(ShaderManagerTest, Destroy) {
   ASSERT_TRUE(info1 == NULL);
 }
 
+TEST_F(ShaderManagerTest, DeleteBug) {
+  const GLuint kClient1Id = 1;
+  const GLuint kClient2Id = 2;
+  const GLuint kService1Id = 11;
+  const GLuint kService2Id = 12;
+  const GLenum kShaderType = GL_VERTEX_SHADER;
+  // Check we can create shader.
+  manager_.CreateShaderInfo(kClient1Id, kService1Id, kShaderType);
+  manager_.CreateShaderInfo(kClient2Id, kService2Id, kShaderType);
+  ShaderManager::ShaderInfo::Ref info1(manager_.GetShaderInfo(kClient1Id));
+  ShaderManager::ShaderInfo::Ref info2(manager_.GetShaderInfo(kClient2Id));
+  ASSERT_TRUE(info1.get() != NULL);
+  ASSERT_TRUE(info2.get() != NULL);
+  manager_.UseShader(info1);
+  manager_.MarkAsDeleted(info1);
+  manager_.MarkAsDeleted(info2);
+  EXPECT_TRUE(manager_.IsOwned(info1));
+  EXPECT_FALSE(manager_.IsOwned(info2));
+}
+
 TEST_F(ShaderManagerTest, ShaderInfo) {
   const GLuint kClient1Id = 1;
   const GLuint kService1Id = 11;