PlzNavigate: Avoid duplicate SiteInstance creation during navigation.

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 18 matching lines @@
 #include "net/http/http_response_headers.h"
 #include "net/url_request/redirect_info.h"
 #include "ui/base/page_transition_types.h"
 #include "url/url_constants.h"
 
 namespace content {
 
 class NavigatorTestWithBrowserSideNavigation
     : public RenderViewHostImplTestHarness {
  public:
+  // Re-defines the private RenderFrameHostManager::SiteInstanceDescriptor here
+  // to allow access to it from tests.
+  typedef RenderFrameHostManager::SiteInstanceDescriptor SiteInstanceDescriptor;
+
   void SetUp() override {
     EnableBrowserSideNavigation();
     RenderViewHostImplTestHarness::SetUp();
   }
 
   TestNavigationURLLoader* GetLoaderForNavigationRequest(
       NavigationRequest* request) const {
     return static_cast<TestNavigationURLLoader*>(request->loader_for_testing());
   }
 
@@ skipping 34 matching lines @@
   bool DidRenderFrameHostRequestCommit(RenderFrameHostImpl* rfh) {
     MockRenderProcessHost* rph =
         static_cast<MockRenderProcessHost*>(rfh->GetProcess());
     const FrameMsg_CommitNavigation* commit_message =
         static_cast<const FrameMsg_CommitNavigation*>(
             rph->sink().GetUniqueMessageMatching(
                 FrameMsg_CommitNavigation::ID));
     return commit_message &&
            rfh->GetRoutingID() == commit_message->routing_id();
   }
+
+  SiteInstance* ConvertToSiteInstance(RenderFrameHostManager* rfhm,
+                                      const SiteInstanceDescriptor& descriptor,
+                                      SiteInstance* candidate_instance) {
+    return rfhm->ConvertToSiteInstance(descriptor, candidate_instance);
+  }
 };
 
 // PlzNavigate: Test a complete browser-initiated navigation starting with a
 // non-live renderer.
 TEST_F(NavigatorTestWithBrowserSideNavigation,
        SimpleBrowserInitiatedNavigationFromNonLiveRenderer) {
   const GURL kUrl("http://chromium.org/");
 
   EXPECT_FALSE(main_test_rfh()->IsRenderFrameLive());
 
@@ skipping 887 matching lines @@
   TestRenderFrameHost* main_rfh = main_test_rfh();
   main_rfh->SendRendererInitiatedNavigationRequest(kUrl2, true);
   navigation_request = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(navigation_request);
   EXPECT_EQ(NavigationRequest::RESPONSE_STARTED,
             navigation_request->state());
   EXPECT_FALSE(navigation_request->loader_for_testing());
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 }
 
+TEST_F(NavigatorTestWithBrowserSideNavigation,

[Charlie Reis, 2015/04/02 21:23:25]

Let's give a roadmap for the test in a comment before this line.  Something
like:

Tests several cases for converting SiteInstanceDescriptors into SiteInstances:
1) Pointer to current.
2) Pointer to unrelated.
3) Same-site URL, related.
4) Cross-site URL, related.
5) Same-site URL, unrelated (with and without candidate SiteInstances).
6) Cross-site URL, unrelated (with candidate SiteInstances).

(We can then number each of the cases below to make it easy to skim.)

[carlosk, 2015/04/03 16:11:19]

Done.

+       SiteInstanceDescriptionConversion) {
+  // Navigate to set a current SiteInstance on the RenderFrameHost.
+  GURL kUrl1("http://a.com");
+  contents()->NavigateAndCommit(kUrl1);
+  SiteInstance* current_instance = main_test_rfh()->GetSiteInstance();
+  ASSERT_TRUE(current_instance);
+
+  // Convert a descriptor pointing to the current instance.
+  RenderFrameHostManager* rfhm =
+      main_test_rfh()->frame_tree_node()->render_manager();
+  {
+    SiteInstanceDescriptor descriptor(current_instance);
+    SiteInstance* converted_instance =
+        ConvertToSiteInstance(rfhm, descriptor, nullptr);
+    EXPECT_EQ(current_instance, converted_instance);
+  }

[carlosk, 2015/04/02 17:15:05]

I used a lot of scoping here to avoid mixing up things from one step to the
next. The variables that I wanted to keep are all defined at the top test level.

[Charlie Reis, 2015/04/02 21:23:24]

Acknowledged.

+
+  // Convert a descriptor pointing an instance unrelated to the current one,
+  // with a different site.
+  GURL kUrl2("http://b.com");
+  scoped_refptr<SiteInstanceImpl> unrelated_instance(
+      static_cast<SiteInstanceImpl*>(
+          SiteInstanceImpl::Create(browser_context())));
+  unrelated_instance->SetSite(kUrl2);
+  EXPECT_FALSE(
+      current_instance->IsRelatedSiteInstance(unrelated_instance.get()));
+  {
+    SiteInstanceDescriptor descriptor(unrelated_instance.get());
+    SiteInstance* converted_instance =
+        ConvertToSiteInstance(rfhm, descriptor, nullptr);
+    EXPECT_EQ(unrelated_instance.get(), converted_instance);
+  }
+
+  // Convert a descriptor of a related instance with the same site as the
+  // current one.
+  GURL kUrlSameSiteAs1("http://www.a.com/foo");
+  {
+    SiteInstanceDescriptor descriptor(kUrlSameSiteAs1, true, browser_context());
+    SiteInstance* converted_instance =
+        ConvertToSiteInstance(rfhm, descriptor, nullptr);
+    EXPECT_EQ(current_instance, converted_instance);
+  }
+
+  // Convert a descriptor of a related instance with a site different from the
+  // current one.
+  GURL kUrlSameSiteAs2("http://www.b.com/foo");
+  scoped_refptr<SiteInstance> related_instance;
+  {
+    SiteInstanceDescriptor descriptor(kUrlSameSiteAs2, true, browser_context());
+    related_instance = ConvertToSiteInstance(rfhm, descriptor, nullptr);
+    // Should return a new instance, related to the current, set to the new site
+    // URL.
+    EXPECT_TRUE(
+        current_instance->IsRelatedSiteInstance(related_instance.get()));
+    EXPECT_NE(current_instance, related_instance.get());
+    EXPECT_NE(unrelated_instance.get(), related_instance.get());
+    EXPECT_EQ(
+        SiteInstanceImpl::GetSiteForURL(browser_context(), kUrlSameSiteAs2),

[Charlie Reis, 2015/04/02 21:23:25]

nit: s/SiteInstanceImpl/SiteInstance/

Also, I'm noticing that we tend to put BrowserContext as the first parameter on
methods like this, with the URL (etc) following.  Let's do the same on the
SiteInstanceDescriptor to be consistent.

[carlosk, 2015/04/03 16:11:19]

Done.

Also replaced SiteInstanceImpl::Create + SetSite with SiteInstance::CreateForURL
so I didn't need a SiteInstanceImpl pointer anymore.

And I held back my temptation to
s/SiteInstanceImpl::GetSiteForURL/SiteInstance::GetSiteForURL/ all over the
place. :)

+        related_instance->GetSiteURL());
+  }
+
+  // Convert a descriptor of an unrelated instance with the same site as the
+  // current one.
+  {
+    SiteInstanceDescriptor descriptor(kUrlSameSiteAs1, false,
+                                      browser_context());
+    scoped_refptr<SiteInstance> converted_instance_1 =
+        ConvertToSiteInstance(rfhm, descriptor, nullptr);
+    // Should return a new instance, unrelated to the current one, set to the
+    // provided site URL.
+    EXPECT_FALSE(
+        current_instance->IsRelatedSiteInstance(converted_instance_1.get()));
+    EXPECT_NE(current_instance, converted_instance_1.get());
+    EXPECT_NE(unrelated_instance.get(), converted_instance_1.get());
+    EXPECT_EQ(
+        SiteInstanceImpl::GetSiteForURL(browser_context(), kUrlSameSiteAs1),
+        converted_instance_1->GetSiteURL());
+
+    // Does the same but this time using unrelated_instance as a candidate,
+    // which has a different site.
+    scoped_refptr<SiteInstance> converted_instance_2 =
+        ConvertToSiteInstance(rfhm, descriptor, unrelated_instance.get());
+    // Should return yet another new instance, unrelated to the current one, set
+    // to the same site URL.
+    EXPECT_FALSE(
+        current_instance->IsRelatedSiteInstance(converted_instance_2.get()));
+    EXPECT_NE(current_instance, converted_instance_2.get());
+    EXPECT_NE(unrelated_instance.get(), converted_instance_2.get());
+    EXPECT_NE(converted_instance_1.get(), converted_instance_2.get());
+    EXPECT_EQ(
+        SiteInstanceImpl::GetSiteForURL(browser_context(), kUrlSameSiteAs1),
+        converted_instance_2->GetSiteURL());
+
+    // Converts once more but with converted_instance_1 as a candidate.
+    SiteInstance* converted_instance_3 =
+        ConvertToSiteInstance(rfhm, descriptor, converted_instance_1.get());
+    // Should return converted_instance_1

[Charlie Reis, 2015/04/02 21:23:25]

nit: End with period.
Also mention "because the site matches and it is unrelated to the current
SiteInstance."

[carlosk, 2015/04/03 16:11:19]

Done.

+    EXPECT_EQ(converted_instance_1.get(), converted_instance_3);
+  }
+
+  // Convert a descriptor of an unrelated instance with the same site of
+  // related_instance and using it as a candidate.
+  {
+    SiteInstanceDescriptor descriptor(kUrlSameSiteAs2, false,
+                                      browser_context());
+    scoped_refptr<SiteInstance> converted_instance =
+        ConvertToSiteInstance(rfhm, descriptor, related_instance.get());
+    // Should return a new instance, unrelated to the current, set to the
+    // provided site URL.
+    EXPECT_FALSE(
+        current_instance->IsRelatedSiteInstance(converted_instance.get()));
+    EXPECT_NE(related_instance.get(), converted_instance.get());
+    EXPECT_NE(unrelated_instance.get(), converted_instance.get());

[Charlie Reis, 2015/04/02 21:23:25]

As in case #5, can we add a line to ensure that ConvertToSiteInstance(rfhm,
descriptor, unrelated_instance.get()) returns unrelated_instance?

[carlosk, 2015/04/03 16:11:19]

Done.

+    EXPECT_EQ(
+        SiteInstanceImpl::GetSiteForURL(browser_context(), kUrlSameSiteAs2),
+        converted_instance->GetSiteURL());
+  }
+}
+
 }  // namespace content