PlzNavigate: improve tracking of improper subframe swap-out.

content/browser/frame_host/navigator_impl_unittest.cc

Index: content/browser/frame_host/navigator_impl_unittest.cc
diff --git a/content/browser/frame_host/navigator_impl_unittest.cc b/content/browser/frame_host/navigator_impl_unittest.cc
index f8069f526fb74391a0d02ab6efb8e9ed30a98633..e9648fe61376a2b484d4483c1cbedebb290bf518 100644
--- a/content/browser/frame_host/navigator_impl_unittest.cc
+++ b/content/browser/frame_host/navigator_impl_unittest.cc
@@ -71,7 +71,8 @@ class NavigatorTestWithBrowserSideNavigation
   NavigationRequest* GetNavigationRequestForFrameTreeNode(
       FrameTreeNode* frame_tree_node) {
     return static_cast<NavigatorImpl*>(frame_tree_node->navigator())
-        ->GetNavigationRequestForNodeForTesting(frame_tree_node);
+        ->GetNavigationRequestForNodeForTesting(
+            frame_tree_node->frame_tree_node_id());
   }
 
   TestRenderFrameHost* GetSpeculativeRenderFrameHost(FrameTreeNode* node) {
@@ -274,9 +275,12 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, BeginNavigation) {
   const GURL kUrl3("http://www.gmail.com/");
 
   contents()->NavigateAndCommit(kUrl1);
+  process()->sink().ClearMessages();

[clamy, 2015/04/14 15:32:03]

Why this line?

[carlosk, 2015/04/23 16:10:12]

As there's more than one commit happening and we check for a single commit
messages later on to determine which RFH committed
(DidRenderFrameHostRequestCommit) this cleanup is necessary at this point.

 
   // Add a subframe.
   FrameTreeNode* root_node = contents()->GetFrameTree()->root();
+  NavigatorImpl* navigator =
+      static_cast<NavigatorImpl*>(root_node->navigator());
   TestRenderFrameHost* subframe_rfh = main_test_rfh()->AppendChild("Child");
   ASSERT_TRUE(subframe_rfh);
 
@@ -284,7 +288,8 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, BeginNavigation) {
   FrameTreeNode* subframe_node = subframe_rfh->frame_tree_node();
   RequestNavigation(subframe_node, kUrl2);
   NavigationRequest* subframe_request =
-      GetNavigationRequestForFrameTreeNode(subframe_node);
+      navigator->GetNavigationRequestForNodeForTesting(
+          subframe_node->frame_tree_node_id());
   TestNavigationURLLoader* subframe_loader =
       GetLoaderForNavigationRequest(subframe_request);
 
@@ -315,7 +320,8 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, BeginNavigation) {
   // Now start a navigation at the root node.
   RequestNavigation(root_node, kUrl3);
   NavigationRequest* main_request =
-      GetNavigationRequestForFrameTreeNode(root_node);
+      navigator->GetNavigationRequestForNodeForTesting(
+          root_node->frame_tree_node_id());
   ASSERT_TRUE(main_request);
   EXPECT_EQ(NavigationRequest::WAITING_FOR_RENDERER_RESPONSE,
             main_request->state());
@@ -341,12 +347,36 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, BeginNavigation) {
 
   // As the main frame hasn't yet committed the subframe still exists. Thus, the
   // above situation regarding subframe navigations is valid here.
+  ASSERT_TRUE(navigator->GetNavigationRequestForNodeForTesting(
+      subframe_node->frame_tree_node_id()));
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kSitePerProcess)) {
     EXPECT_TRUE(GetSpeculativeRenderFrameHost(subframe_node));
   } else {
     EXPECT_FALSE(GetSpeculativeRenderFrameHost(subframe_node));
   }
+
+  // Have the root node's speculative RenderFrameHost commit the navigation.
+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+  main_loader->CallOnResponseStarted(response, MakeEmptyStream());
+  TestRenderFrameHost* speculative_root_rfh =
+      GetSpeculativeRenderFrameHost(root_node);
+  ASSERT_TRUE(speculative_root_rfh);
+  EXPECT_TRUE(DidRenderFrameHostRequestCommit(speculative_root_rfh));
+  EXPECT_FALSE(DidRenderFrameHostRequestCommit(main_test_rfh()));
+
+  // The subframe request is still there as the subframe still exists.

[nasko, 2015/04/14 16:06:29]

This doesn't match reality and I'm not sure it is a good idea to include. When a
parent frame commits a new document, all of the existing subframes will
disappear, as the old document is being destroyed.

Is it possible to split in a different test case what you are trying to test for
subframes?

[carlosk, 2015/04/23 16:10:13]

The request still exists because at this stage we're still at the old document.
The subframe is only eliminated when the navigated RFH finally navigates (in the
SendNavigate call below).

Instead of creating a new test I think this one should be renamed to better
match what it's actually testing: a complex use-case of simultaneous navigation
at the root and subframe levels. I made this change in the latest patch set so
WDYT?

+  ASSERT_TRUE(navigator->GetNavigationRequestForNodeForTesting(
+      subframe_node->frame_tree_node_id()));
+
+  // Commit the navigation.

[clamy, 2015/04/14 15:32:03]

Add that the subframe will be destroyed as a result.

[carlosk, 2015/04/23 16:10:13]

This code was removed.

+  int64 subframe_node_id = subframe_node->frame_tree_node_id();
+  speculative_root_rfh->SendNavigate(1, kUrl3);
+  subframe_node = nullptr; // Was just destroyed.

[clamy, 2015/04/14 15:32:03]

I don't think this line is needed.

[carlosk, 2015/04/23 16:10:12]

I was copying a pattern I saw in RenderFrameHostManagerTest::DetachPendingChild
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...)
where the subframe pointer was set to NULL. It makes it clear that the pointer
is now invalid and will SEGFAULT if one should inadvertently try to use it
(faster feedback than waiting for ASAN trybot results).

But this code was removed anyway.

+
+  // The subframe request should have been cleaned up with the subframe.
+  ASSERT_FALSE(navigator->GetNavigationRequestForNodeForTesting(
+      subframe_node_id));
 }
 
 // PlzNavigate: Test that committing an HTTP 204 or HTTP 205 response cancels