PlzNavigate: improve tracking of improper subframe swap-out.

content/browser/frame_host/navigator_impl_unittest.cc

 // Copyright 2014 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 "base/command_line.h"
 #include "base/macros.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/frame_host/navigation_request.h"
@@ skipping 53 matching lines @@
     load_params.frame_tree_node_id = node->frame_tree_node_id();
     load_params.referrer = referrer;
     load_params.transition_type = transition_type;
 
     controller().LoadURLWithParams(load_params);
   }
 
   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) {
     return static_cast<TestRenderFrameHost*>(
         node->render_manager()->speculative_render_frame_host_.get());
   }
 
   // Checks if this RenderFrameHost sent a single FrameMsg_CommitNavigation
   // since the last clearing of the sink.
   // Note: caller must invoke ClearMessages on the sink at some point before
@@ skipping 182 matching lines @@
 }
 
 // PlzNavigate: Test that a proper NavigationRequest is created by
 // RequestNavigation.
 TEST_F(NavigatorTestWithBrowserSideNavigation, BeginNavigation) {
   const GURL kUrl1("http://www.google.com/");
   const GURL kUrl2("http://www.chromium.org/");
   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);
 
   // Start a navigation at the subframe.
   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);
 
   // Subframe navigations should start right away as they don't have to request
   // beforeUnload to run at the renderer.
   ASSERT_TRUE(subframe_request);
   ASSERT_TRUE(subframe_loader);
   EXPECT_EQ(NavigationRequest::STARTED, subframe_request->state());
   EXPECT_EQ(kUrl2, subframe_request->common_params().url);
   EXPECT_EQ(kUrl2, subframe_loader->request_info()->common_params.url);
@@ skipping 10 matching lines @@
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kSitePerProcess)) {
     EXPECT_TRUE(GetSpeculativeRenderFrameHost(subframe_node));
   } else {
     EXPECT_FALSE(GetSpeculativeRenderFrameHost(subframe_node));
   }
 
   // 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());
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(root_node));
 
   // Simulate a BeforeUnloadACK IPC on the main frame.
   main_test_rfh()->SendBeforeUnloadACK(true);
   TestNavigationURLLoader* main_loader =
       GetLoaderForNavigationRequest(main_request);
   EXPECT_EQ(kUrl3, main_request->common_params().url);
   EXPECT_EQ(kUrl3, main_loader->request_info()->common_params.url);
   EXPECT_EQ(kUrl3, main_loader->request_info()->first_party_for_cookies);
   EXPECT_TRUE(main_loader->request_info()->is_main_frame);
   EXPECT_FALSE(main_loader->request_info()->parent_is_main_frame);
   EXPECT_TRUE(main_request->browser_initiated());
   // BeforeUnloadACK was received from the renderer so the navigation should
   // have started.
   EXPECT_EQ(NavigationRequest::STARTED, main_request->state());
 
   // Main frame navigation to a different site should use a speculative
   // RenderFrameHost.
   EXPECT_TRUE(GetSpeculativeRenderFrameHost(root_node));
 
   // 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
 // the navigation.
 TEST_F(NavigatorTestWithBrowserSideNavigation, NoContent) {
   const GURL kUrl1("http://www.chromium.org/");
   const GURL kUrl2("http://www.google.com/");
 
   // Load a URL.
   contents()->NavigateAndCommit(kUrl1);
@@ skipping 770 matching lines @@
 
     SiteInstance* converted_instance_2 =
         ConvertToSiteInstance(rfhm, descriptor, unrelated_instance.get());
     // Should return |unrelated_instance| because its site matches and it is
     // unrelated to the current SiteInstance.
     EXPECT_EQ(unrelated_instance.get(), converted_instance_2);
   }
 }
 
 }  // namespace content