Fix memory leaks when a renderer crashes and the user refreshes. Also fix memory leak every time a renderer process goes away.

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

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

6 files changed, 55 insertions, 10 deletions

diff --git a/chrome/browser/child_process_security_policy.cc b/chrome/browser/child_process_security_policy.cc
index f853c10..6a408bb1 100644
--- a/chrome/browser/child_process_security_policy.cc
+++ b/chrome/browser/child_process_security_policy.cc
@@ -118,10 +118,8 @@ void ChildProcessSecurityPolicy::Add(int renderer_id) {
 
 void ChildProcessSecurityPolicy::Remove(int renderer_id) {
   AutoLock lock(lock_);
-  if (security_state_.count(renderer_id) != 1) {
-    NOTREACHED() << "Remove renderers at most once.";
-    return;
-  }
+  if (!security_state_.count(renderer_id))
+    return;  // May be called multiple times.
 
   delete security_state_[renderer_id];
   security_state_.erase(renderer_id);
diff --git a/chrome/browser/child_process_security_policy.h b/chrome/browser/child_process_security_policy.h
index 9b675cf..ae1eb41 100644
--- a/chrome/browser/child_process_security_policy.h
+++ b/chrome/browser/child_process_security_policy.h
@@ -13,6 +13,7 @@
 #include "base/file_path.h"
 #include "base/lock.h"
 #include "base/singleton.h"
+#include "testing/gtest/include/gtest/gtest_prod.h"
 
 class FilePath;
 class GURL;
@@ -93,6 +94,9 @@ class ChildProcessSecurityPolicy {
   bool HasDOMUIBindings(int renderer_id);
 
  private:
+  friend class ChildProcessSecurityPolicyInProcessBrowserTest;
+  FRIEND_TEST(ChildProcessSecurityPolicyInProcessBrowserTest, NoLeak);
+
   class SecurityState;
 
   typedef std::set<std::string> SchemeSet;
diff --git a/chrome/browser/child_process_security_policy_unittest.cc b/chrome/browser/child_process_security_policy_unittest.cc
index 88f1e33..fed3a1f 100644
--- a/chrome/browser/child_process_security_policy_unittest.cc
+++ b/chrome/browser/child_process_security_policy_unittest.cc
@@ -6,8 +6,15 @@
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
+#include "chrome/browser/browser.h"
+#include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/browser/child_process_security_policy.h"
+#include "chrome/browser/renderer_host/render_process_host.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/test/in_process_browser_test.h"
+#include "chrome/test/ui_test_utils.h"
+#include "net/base/net_util.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_test_job.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -266,3 +273,41 @@ TEST_F(ChildProcessSecurityPolicyTest, RemoveRace) {
   EXPECT_FALSE(p->CanUploadFile(kRendererID, file));
   EXPECT_FALSE(p->HasDOMUIBindings(kRendererID));
 }
+
+class ChildProcessSecurityPolicyInProcessBrowserTest
+    : public InProcessBrowserTest {
+ public:
+  virtual void SetUp() {
+    EXPECT_EQ(
+      ChildProcessSecurityPolicy::GetInstance()->security_state_.size(), 0);
+    InProcessBrowserTest::SetUp();
+  }
+
+  virtual void TearDown() {
+    EXPECT_EQ(
+      ChildProcessSecurityPolicy::GetInstance()->security_state_.size(), 0);
+    InProcessBrowserTest::TearDown();
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(ChildProcessSecurityPolicyInProcessBrowserTest, NoLeak) {
+  FilePath path;
+  PathService::Get(chrome::DIR_TEST_DATA, &path);
+  path = path.Append(FilePath::FromWStringHack(L"google"));
+  path = path.Append(FilePath::FromWStringHack(L"google.html"));
+  GURL url = net::FilePathToFileURL(path);
+
+  ui_test_utils::NavigateToURL(browser(), url);
+  EXPECT_EQ(
+      ChildProcessSecurityPolicy::GetInstance()->security_state_.size(), 1);
+
+  TabContents* tab = browser()->GetTabContentsAt(0);
+  ASSERT_TRUE(tab != NULL);
+  base::KillProcess(
+      tab->process()->process().handle(), base::PROCESS_END_KILLED_BY_USER,
+      true);
+
+  tab->controller().Reload(true);
+  EXPECT_EQ(
+      ChildProcessSecurityPolicy::GetInstance()->security_state_.size(), 1);
+}
diff --git a/chrome/browser/renderer_host/browser_render_process_host.cc b/chrome/browser/renderer_host/browser_render_process_host.cc
index e1c9bd9..e8b608d 100644
--- a/chrome/browser/renderer_host/browser_render_process_host.cc
+++ b/chrome/browser/renderer_host/browser_render_process_host.cc
@@ -687,6 +687,9 @@ void BrowserRenderProcessHost::OnChannelError() {
   if (child_exited)
     process_.Close();
 
+  WebCacheManager::GetInstance()->Remove(pid());
+  ChildProcessSecurityPolicy::GetInstance()->Remove(pid());
+
   channel_.reset();
 
   // This process should detach all the listeners, causing the object to be
diff --git a/chrome/browser/renderer_host/render_process_host.cc b/chrome/browser/renderer_host/render_process_host.cc
index e70596743..349f8e2 100644
--- a/chrome/browser/renderer_host/render_process_host.cc
+++ b/chrome/browser/renderer_host/render_process_host.cc
@@ -90,10 +90,8 @@ void RenderProcessHost::Release(int listener_id) {
     NotificationService::current()->Notify(
         NotificationType::RENDERER_PROCESS_TERMINATED,
         Source<RenderProcessHost>(this), NotificationService::NoDetails());
-    if (pid_ >= 0) {
+    if (pid_ >= 0)
       all_hosts.Remove(pid_);
-      pid_ = -1;
-    }
     MessageLoop::current()->DeleteSoon(FROM_HERE, this);
   }
 }
diff --git a/chrome/browser/renderer_host/web_cache_manager.cc b/chrome/browser/renderer_host/web_cache_manager.cc
index 91b740e..4615e2b 100644
--- a/chrome/browser/renderer_host/web_cache_manager.cc
+++ b/chrome/browser/renderer_host/web_cache_manager.cc
@@ -85,9 +85,6 @@ void WebCacheManager::Add(int renderer_id) {
 }
 
 void WebCacheManager::Remove(int renderer_id) {
-  DCHECK(active_renderers_.count(renderer_id) > 0 ||
-         inactive_renderers_.count(renderer_id) > 0);
-
   // Erase all knowledge of this renderer
   active_renderers_.erase(renderer_id);
   inactive_renderers_.erase(renderer_id);