Add ref-count on RenderViewHost for each new RenderFrameProxyHost.

content/browser/frame_host/render_frame_host_manager_browsertest.cc

 // Copyright 2013 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.
 
 #include <set>
 
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
 #include "base/memory/ref_counted.h"
 #include "base/path_service.h"
@@ skipping 17 matching lines @@
 #include "content/public/common/page_state.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "net/base/net_util.h"
 #include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
 
 using base::ASCIIToUTF16;
 
 namespace content {
 
 namespace {
 
 const char kOpenUrlViaClickTargetFunc[] =
     "(function(url) {\n"
@@ skipping 32 matching lines @@
 
   void StartServer() {
     // Support multiple sites on the test server.
     host_resolver()->AddRule("*", "127.0.0.1");
     ASSERT_TRUE(test_server()->Start());
 
     foo_host_port_ = test_server()->host_port_pair();
     foo_host_port_.set_host(foo_com_);
   }
 
+  void StartEmbeddedServer() {
+    // Support multiple sites on the embedded test server.
+    host_resolver()->AddRule("*", "127.0.0.1");
+    ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+    SetupCrossSiteRedirector(embedded_test_server());
+  }
+
   // Returns a URL on foo.com with the given path.
   GURL GetCrossSiteURL(const std::string& path) {
     GURL cross_site_url(test_server()->GetURL(path));
     return cross_site_url.ReplaceComponents(replace_host_);
   }
 
  protected:
   std::string foo_com_;
   GURL::Replacements replace_host_;
   net::HostPortPair foo_host_port_;
@@ skipping 1615 matching lines @@
   shell()->web_contents()->GetController().GoBack();
   back_nav_load_observer.Wait();
   EXPECT_NE(process_id,
             shell()->web_contents()->GetRenderProcessHost()->GetID());
 
   // Ensure that the file access still exists in the new process ID.
   EXPECT_TRUE(ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(
       shell()->web_contents()->GetRenderProcessHost()->GetID(), file));
 }
 
+// This class implements waiting for RenderFrameHost destruction. It relies on
+// the fact that RenderFrameDeleted event is fired when RenderFrameHost is
+// destroyed.
+// Note: RenderFrameDeleted is also fired when the process associated with the
+// RenderFrameHost crashes, so this cannot be used in cases where process dying
+// is expected.

[ncarter (slow), 2015/05/22 23:03:14]

If you wanted to make it robust against this, you could probably do it by making
sure that either FrameDeleted(|rfh|) or RenderFrameHostChanged(|rfh|, _) had
occurred before RenderFrameDeleted(|rfh|).

[ncarter (slow), 2015/05/22 23:04:29]

[but I'm okay with just leaving the "Note:" too]

+class RenderFrameHostDestructionObserver : public WebContentsObserver {

[ncarter (slow), 2015/05/22 23:03:14]

This is pretty similar to
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

If you want to pull it out into a test util, I'd support that.

[nasko, 2015/05/22 23:46:49]

Might be a good idea. I'd leave that for another CL though. I even thought of
pulling it into a common test file, as I imagine it might be used in other
places, but wanted to see this being stable for a bit before doing that.

Pulling it into common code and combining it with the other observer will be
also good to incorporate your note (above) about making it more robust.

[ncarter (slow), 2015/05/23 00:33:36]

Acknowledged.

+ public:
+  explicit RenderFrameHostDestructionObserver(WebContents* web_contents)
+      : WebContentsObserver(web_contents),
+        message_loop_runner_(new MessageLoopRunner),
+        started_watching(false) {}
+  ~RenderFrameHostDestructionObserver() override {}
+
+  void Observe(RenderFrameHost* rfh) {

[ncarter (slow), 2015/05/22 23:03:14]

This shadows WebContentsObserver::Observe, which is kind of weird.

Consider dropping this method and just passing the rfh in the ctor? You can get
the WebContents via WebContents::FromRenderFrameHost(rfh).

[nasko, 2015/05/22 23:46:50]

Done.

+    started_watching = true;
+    watched_render_frame_hosts_.insert(rfh);
+  }
+
+  void Wait() {
+    if (!started_watching)
+      return;
+    message_loop_runner_->Run();
+  }
+
+ private:
+  // WebContentsObserver implementation:
+  void RenderFrameDeleted(RenderFrameHost* rfh) override {

[ncarter (slow), 2015/05/22 23:03:14]

This should be public.

[nasko, 2015/05/22 23:46:49]

Why should it? I've changed it, but curious about the rationale.

[ncarter (slow), 2015/05/23 00:33:36]

1. When you have something that's private in a derived class but public in the
base class (and you're using public inheritance), callers can still access it by
casting to the base class, so it's not really hidden.

2. Marking one method private does nothing to prevent users of
RenderFrameHostDestructionObserver from calling the other two dozen public
methods of WebContentsObserver.

3. A private method ought to be a signal that "it's an implementation detail and
all callers should be found right here in this class", but that's not the case
here.

https://isocpp.org/wiki/faq/proper-inheritance#hiding-inherited-public

If you really wanted to hide the WCO guts from end-users of
RenderFrameHostDestructionObserver, the options are private inheritance (which
google style doesn't allow) or composition (move the WCO implementation to a
nested class and have a member variable of that type).

[nasko, 2015/05/23 02:23:08]

Acknowledged.

+    watched_render_frame_hosts_.erase(rfh);
+    if (!started_watching)
+      return;
+    if (watched_render_frame_hosts_.size() == 0) {
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE, message_loop_runner_->QuitClosure());
+    }
+  }
+
+  scoped_refptr<MessageLoopRunner> message_loop_runner_;
+  bool started_watching;
+  std::set<RenderFrameHost*> watched_render_frame_hosts_;
+};
+
+// Ensures that no RenderFrameHost/RenderViewHost objects are leaked when
+// doing a simple cross-process navigation.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
+                       CleanupOnCrossProcessNavigation) {
+  StartEmbeddedServer();
+
+  // Do an initial navigation and capture objects we expect to be cleaned up
+  // on cross-process navigation.
+  GURL start_url = embedded_test_server()->GetURL("/title1.html");
+  NavigateToURL(shell(), start_url);
+
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+  int32 orig_site_instance_id =
+      root->current_frame_host()->GetSiteInstance()->GetId();
+  int initial_process_id =
+      root->current_frame_host()->GetSiteInstance()->GetProcess()->GetID();
+  int initial_rfh_id = root->current_frame_host()->GetRoutingID();
+  int initial_rvh_id =
+      root->current_frame_host()->render_view_host()->GetRoutingID();
+
+  // Navigate cross-process and ensure that cleanup is performed as expected.
+  GURL cross_site_url =
+      embedded_test_server()->GetURL("foo.com", "/title2.html");
+  RenderFrameHostDestructionObserver rfh_observer(shell()->web_contents());
+  rfh_observer.Observe(root->current_frame_host());
+  NavigateToURL(shell(), cross_site_url);
+  rfh_observer.Wait();
+
+  EXPECT_NE(orig_site_instance_id,
+            root->current_frame_host()->GetSiteInstance()->GetId());
+  EXPECT_FALSE(RenderFrameHost::FromID(initial_process_id, initial_rfh_id));
+  EXPECT_FALSE(RenderViewHost::FromID(initial_process_id, initial_rvh_id));
+}
+
 }  // namespace content