Fix for a memory corruption.

As identified by http://crbug.com/90867, we sometimes leaked render view hosts by overwriting them in the swapped out map of the render view host manager.
 
This should not happen (two different hosts with the same instance id) and will eventually be fixed, but in the mean time this CL recovers from that problem and prevent the leak, and also the memory corruptions that were caused by it.

The memory corruption were caused by the fact that the leaked host would not be told when their delegate_ would die and might try to call them post-mortem.

BUG=90867
TEST=RenderViewHostManagerTest.LeakingRenderViewHosts

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

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

content/browser/renderer_host/render_view_host_manager_browsertest.cc

@@ -8,12 +8,15 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/browser/renderer_host/render_view_host.h"
+#include "content/browser/renderer_host/render_view_host_observer.h"
 #include "content/browser/site_instance.h"
 #include "content/browser/tab_contents/tab_contents.h"
 #include "content/common/content_notification_types.h"
 #include "content/common/notification_details.h"
 #include "content/common/notification_observer.h"
 #include "content/common/notification_registrar.h"
+#include "content/common/url_constants.h"
 #include "net/base/net_util.h"
 #include "net/test/test_server.h"
 
@@ -246,3 +249,89 @@ IN_PROC_BROWSER_TEST_F(RenderViewHostManagerTest, ClickLinkAfter204Error) {
       browser()->GetSelectedTabContents()->GetSiteInstance());
   EXPECT_EQ(orig_site_instance, noref_site_instance);
 }
+
+// This class holds onto RenderViewHostObservers for as long as their observed
+// RenderViewHosts are alive. This allows us to confirm that all hosts have
+// properly been shutdown.
+class RenderViewHostObserverArray {
+ public:
+  ~RenderViewHostObserverArray() {
+    // In case some would be left in there with a dead pointer to us.
+    for (std::list<RVHObserver*>::iterator iter = observers_.begin();
+         iter != observers_.end(); ++iter) {
+      (*iter)->ClearParent();
+    }
+  }
+  void AddObserverToRVH(RenderViewHost* rvh) {
+    observers_.push_back(new RVHObserver(this, rvh));
+  }
+  size_t GetNumObservers() const {
+    return observers_.size();
+  }
+ private:
+  friend class RVHObserver;
+  class RVHObserver : public RenderViewHostObserver {
+   public:
+    RVHObserver(RenderViewHostObserverArray* parent, RenderViewHost* rvh)
+        : RenderViewHostObserver(rvh),
+          parent_(parent) {
+    }
+    virtual void RenderViewHostDestroyed() OVERRIDE {
+      if (parent_)
+        parent_->RemoveObserver(this);
+      RenderViewHostObserver::RenderViewHostDestroyed();
+    };
+    void ClearParent() {
+      parent_ = NULL;
+    }
+   private:
+    RenderViewHostObserverArray* parent_;
+  };
+
+  void RemoveObserver(RVHObserver* observer) {
+    observers_.remove(observer);
+  }
+
+  std::list<RVHObserver*> observers_;
+};
+
+// Test for crbug.com/90867. Make sure we don't leak render view hosts since
+// they may cause crashes or memory corruptions when trying to call dead
+// delegate_.
+IN_PROC_BROWSER_TEST_F(RenderViewHostManagerTest, LeakingRenderViewHosts) {
+  // Start two servers with different sites.
+  ASSERT_TRUE(test_server()->Start());
+  net::TestServer https_server(net::TestServer::TYPE_HTTPS,
+                               FilePath(FILE_PATH_LITERAL("chrome/test/data")));
+  ASSERT_TRUE(https_server.Start());
+
+  // Create a new tab so that we can close the one we navigate and still have
+  // a running browser.
+  AddBlankTabAndShow(browser());
+
+  // Load a random page and then navigate to view-source: of it.
+  // This is one way to cause two rvh instances for the same instance id.
+  GURL navigated_url(test_server()->GetURL("files/title2.html"));
+  ui_test_utils::NavigateToURL(browser(), navigated_url);
+
+  // Observe the newly created render_view_host to make sure it will not leak.
+  RenderViewHostObserverArray rvh_observers;
+  rvh_observers.AddObserverToRVH(browser()->GetSelectedTabContents()->
+      render_view_host());
+
+  GURL view_source_url(chrome::kViewSourceScheme + std::string(":") +
+      navigated_url.spec());
+  ui_test_utils::NavigateToURL(browser(), view_source_url);
+  rvh_observers.AddObserverToRVH(browser()->GetSelectedTabContents()->
+      render_view_host());
+
+  // Now navigate to a different instance so that we swap out again.
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server.GetURL("files/title2.html"));
+  rvh_observers.AddObserverToRVH(browser()->GetSelectedTabContents()->
+      render_view_host());
+
+  // This used to leak a render view host.
+  browser()->CloseTabContents(browser()->GetSelectedTabContents());
+  EXPECT_EQ(0U, rvh_observers.GetNumObservers());
+}

content/browser/tab_contents/render_view_host_manager.cc

@@ -635,8 +635,19 @@ void RenderViewHostManager::CommitPending() {
   // in case we navigate back to it.
   if (old_render_view_host->IsRenderViewLive()) {
     DCHECK(old_render_view_host->is_swapped_out());
-    swapped_out_hosts_[old_render_view_host->site_instance()->id()] =
-        old_render_view_host;
+    // Temp fix for http://crbug.com/90867 until we do a better cleanup to make
+    // sure we don't get different rvh instances for the same site instance
+    // in the same rvhmgr.
+    // TODO(creis): Clean this up, and duplication in SwapInRenderViewHost.
+    int32 old_site_instance_id = old_render_view_host->site_instance()->id();
+    RenderViewHostMap::iterator iter =
+        swapped_out_hosts_.find(old_site_instance_id);
+    if (iter != swapped_out_hosts_.end() &&
+        iter->second != old_render_view_host) {
+      // Shutdown the RVH that will be replaced in the map to avoid a leak.
+      iter->second->Shutdown();
+    }
+    swapped_out_hosts_[old_site_instance_id] = old_render_view_host;
   } else {
     old_render_view_host->Shutdown();
   }
@@ -855,8 +866,19 @@ void RenderViewHostManager::SwapInRenderViewHost(RenderViewHost* rvh) {
   // in case we navigate back to it.
   if (old_render_view_host->IsRenderViewLive()) {
     DCHECK(old_render_view_host->is_swapped_out());
-    swapped_out_hosts_[old_render_view_host->site_instance()->id()] =
-        old_render_view_host;
+    // Temp fix for http://crbug.com/90867 until we do a better cleanup to make
+    // sure we don't get different rvh instances for the same site instance
+    // in the same rvhmgr.
+    // TODO(creis): Clean this up as well as duplication with CommitPending.
+    int32 old_site_instance_id = old_render_view_host->site_instance()->id();
+    RenderViewHostMap::iterator iter =
+        swapped_out_hosts_.find(old_site_instance_id);
+    if (iter != swapped_out_hosts_.end() &&
+        iter->second != old_render_view_host) {
+      // Shutdown the RVH that will be replaced in the map to avoid a leak.
+      iter->second->Shutdown();
+    }
+    swapped_out_hosts_[old_site_instance_id] = old_render_view_host;
   } else {
     old_render_view_host->Shutdown();
   }