Prerender: Avoid starting navigation for prerendering when it is duplicate

This CL avoids starting navigation for prerendering when it already
started for the same URL.

To do that, this moves PrerenderHost::StartNavigation() call into
PrerenderHostRegistry::RegisterHost(). This doesn't change behavior
around networking. Even before this change, the duplicate navigation
didn't issue a network request because PrerenderHost was destroyed right
after PrerenderHost::StartNavigation() was called. The browser tests
make sure this behavior.

In addition to that, this change renames RegisterHost() to
CreateAndStartHost() and then moves the PrerenderHost creation into the
function for code cleanup.

Bug: 1132746
Change-Id: Ibc1d51921d845c2746a1a0caca27e08e77256699
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2567514Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833583}

content/browser/prerender/prerender_host_registry.cc

@@ -18,16 +18,29 @@ PrerenderHostRegistry::PrerenderHostRegistry() {
 
 PrerenderHostRegistry::~PrerenderHostRegistry() = default;
 
-void PrerenderHostRegistry::RegisterHost(
-    const GURL& prerendering_url,
-    std::unique_ptr<PrerenderHost> prerender_host) {
+void PrerenderHostRegistry::CreateAndStartHost(
+    blink::mojom::PrerenderAttributesPtr attributes,
+    const GlobalFrameRoutingId& initiator_render_frame_host_id,
+    const url::Origin& initiator_origin) {
+  DCHECK(attributes);
+
   // Ignore prerendering requests for the same URL.
-  if (base::Contains(prerender_host_by_url_, prerendering_url)) {
-    // TODO(https://crbug.com/1132746): In addition to this check, we may have
-    // to avoid duplicate requests in the PrerenderHost layer so that the
-    // prerender host doesn't start navigation for the duplicate requests.
+  const GURL prerendering_url = attributes->url;
+  if (base::Contains(prerender_host_by_url_, prerendering_url))
     return;
-  }
+
+  auto prerender_host = std::make_unique<PrerenderHost>(
+      std::move(attributes), initiator_render_frame_host_id, initiator_origin);
+
+  // Start prerendering before adding the host to the map to make sure
+  // navigation for prerendering doesn't select itself.
+  // TODO(https://crbug.com/1132746): SelectForNavigation() should avoid select
+  // a prerender host when the current NavigationRequest is for prerendering
+  // regardless of the calling order of StartPrerendering(). This modification
+  // would require the proper `is_prerendering` state in NavigationRequest,
+  // RenderFrameHostImpl, or somewhere else.
+  prerender_host->StartPrerendering();
+
   prerender_host_by_url_[prerendering_url] = std::move(prerender_host);
 }
 

content/browser/prerender/prerender_host_registry.h

@@ -8,7 +8,10 @@
 #include <map>
 
 #include "content/common/content_export.h"
+#include "content/public/browser/global_routing_id.h"
+#include "third_party/blink/public/mojom/prerender/prerender.mojom.h"
 #include "url/gurl.h"
+#include "url/origin.h"
 
 namespace content {
 
@@ -19,7 +22,7 @@ class PrerenderHost;
 // to navigation code for activating prerendered contents. This is created and
 // owned by StoragePartitionImpl.
 //
-// TODO(https://crbug.com/1132746): Once the Prerender2 is migrated to the
+// TODO(https://crbug.com/1154501): Once the Prerender2 is migrated to the
 // MPArch, it would be more natural to make PrerenderHostRegistry scoped to
 // WebContents, that is, WebContents will own this.
 class CONTENT_EXPORT PrerenderHostRegistry {
@@ -32,9 +35,11 @@ class CONTENT_EXPORT PrerenderHostRegistry {
   PrerenderHostRegistry(PrerenderHostRegistry&&) = delete;
   PrerenderHostRegistry& operator=(PrerenderHostRegistry&&) = delete;
 
-  // Registers the host for `prerendering_url`.
-  void RegisterHost(const GURL& prerendering_url,
-                    std::unique_ptr<PrerenderHost> prerender_host);
+  // Creates and starts a host for `prerendering_url`.
+  void CreateAndStartHost(
+      blink::mojom::PrerenderAttributesPtr attributes,
+      const GlobalFrameRoutingId& initiator_render_frame_host_id,
+      const url::Origin& initiator_origin);
 
   // Destroys the host registered for `prerendering_url`.
   void AbandonHost(const GURL& prerendering_url);

content/browser/prerender/prerender_host_registry_unittest.cc

@@ -54,7 +54,7 @@ class PrerenderHostRegistryTest : public RenderViewHostImplTestHarness {
   std::unique_ptr<TestBrowserContext> browser_context_;
 };
 
-TEST_F(PrerenderHostRegistryTest, RegisterHost) {
+TEST_F(PrerenderHostRegistryTest, CreateAndStartHost) {
   std::unique_ptr<TestWebContents> web_contents =
       CreateWebContents(GURL("https://example.com/"));
   RenderFrameHostImpl* render_frame_host = web_contents->GetMainFrame();
@@ -63,26 +63,23 @@ TEST_F(PrerenderHostRegistryTest, RegisterHost) {
   const GURL kPrerenderingUrl("https://example.com/next");
   auto attributes = blink::mojom::PrerenderAttributes::New();
   attributes->url = kPrerenderingUrl;
-  auto prerender_host = std::make_unique<PrerenderHost>(
-      std::move(attributes), render_frame_host->GetGlobalFrameRoutingId(),
-      render_frame_host->GetLastCommittedOrigin());
-  PrerenderHost* prerender_host_rawptr = prerender_host.get();
 
   PrerenderHostRegistry* registry = GetPrerenderHostRegistry();
-
-  registry->RegisterHost(kPrerenderingUrl, std::move(prerender_host));
-  EXPECT_EQ(registry->FindHostByUrlForTesting(kPrerenderingUrl),
-            prerender_host_rawptr);
+  registry->CreateAndStartHost(std::move(attributes),
+                               render_frame_host->GetGlobalFrameRoutingId(),
+                               render_frame_host->GetLastCommittedOrigin());
+  PrerenderHost* prerender_host =
+      registry->FindHostByUrlForTesting(kPrerenderingUrl);
 
   // Artificially finish navigation to make the prerender host ready to activate
   // the prerendered page.
-  prerender_host_rawptr->DidFinishNavigation(nullptr);
+  prerender_host->DidFinishNavigation(nullptr);
 
   EXPECT_TRUE(registry->SelectForNavigation(kPrerenderingUrl));
   EXPECT_EQ(registry->FindHostByUrlForTesting(kPrerenderingUrl), nullptr);
 }
 
-TEST_F(PrerenderHostRegistryTest, RegisterHostForSameURL) {
+TEST_F(PrerenderHostRegistryTest, CreateAndStartHostForSameURL) {
   std::unique_ptr<TestWebContents> web_contents =
       CreateWebContents(GURL("https://example.com/"));
   RenderFrameHostImpl* render_frame_host = web_contents->GetMainFrame();
@@ -92,41 +89,34 @@ TEST_F(PrerenderHostRegistryTest, RegisterHostForSameURL) {
 
   auto attributes1 = blink::mojom::PrerenderAttributes::New();
   attributes1->url = kPrerenderingUrl;
-  auto prerender_host1 = std::make_unique<PrerenderHost>(
-      std::move(attributes1), render_frame_host->GetGlobalFrameRoutingId(),
-      render_frame_host->GetLastCommittedOrigin());
-  PrerenderHost* prerender_host1_rawptr = prerender_host1.get();
 
   auto attributes2 = blink::mojom::PrerenderAttributes::New();
   attributes2->url = kPrerenderingUrl;
-  auto prerender_host2 = std::make_unique<PrerenderHost>(
-      std::move(attributes2), render_frame_host->GetGlobalFrameRoutingId(),
-      render_frame_host->GetLastCommittedOrigin());
-  PrerenderHost* prerender_host2_rawptr = prerender_host2.get();
 
   PrerenderHostRegistry* registry = GetPrerenderHostRegistry();
+  registry->CreateAndStartHost(std::move(attributes1),
+                               render_frame_host->GetGlobalFrameRoutingId(),
+                               render_frame_host->GetLastCommittedOrigin());
+  PrerenderHost* prerender_host1 =
+      registry->FindHostByUrlForTesting(kPrerenderingUrl);
 
-  registry->RegisterHost(kPrerenderingUrl, std::move(prerender_host1));
-  EXPECT_EQ(registry->FindHostByUrlForTesting(kPrerenderingUrl),
-            prerender_host1_rawptr);
-
-  // Register the prerender host for the same URL. This second host should be
+  // Start the prerender host for the same URL. This second host should be
   // ignored, and the first host should still be findable.
-  registry->RegisterHost(kPrerenderingUrl, std::move(prerender_host2));
+  registry->CreateAndStartHost(std::move(attributes2),
+                               render_frame_host->GetGlobalFrameRoutingId(),
+                               render_frame_host->GetLastCommittedOrigin());
   EXPECT_EQ(registry->FindHostByUrlForTesting(kPrerenderingUrl),
-            prerender_host1_rawptr);
-  EXPECT_NE(registry->FindHostByUrlForTesting(kPrerenderingUrl),
-            prerender_host2_rawptr);
+            prerender_host1);
 
   // Artificially finish navigation to make the prerender host ready to activate
   // the prerendered page.
-  prerender_host1_rawptr->DidFinishNavigation(nullptr);
+  prerender_host1->DidFinishNavigation(nullptr);
 
   EXPECT_TRUE(registry->SelectForNavigation(kPrerenderingUrl));
   EXPECT_EQ(registry->FindHostByUrlForTesting(kPrerenderingUrl), nullptr);
 }
 
-TEST_F(PrerenderHostRegistryTest, RegisterHostForDifferentURLs) {
+TEST_F(PrerenderHostRegistryTest, CreateAndStartHostForDifferentURLs) {
   std::unique_ptr<TestWebContents> web_contents =
       CreateWebContents(GURL("https://example.com/"));
   RenderFrameHostImpl* render_frame_host = web_contents->GetMainFrame();
@@ -135,40 +125,34 @@ TEST_F(PrerenderHostRegistryTest, RegisterHostForDifferentURLs) {
   const GURL kPrerenderingUrl1("https://example.com/next1");
   auto attributes1 = blink::mojom::PrerenderAttributes::New();
   attributes1->url = kPrerenderingUrl1;
-  auto prerender_host1 = std::make_unique<PrerenderHost>(
-      std::move(attributes1), render_frame_host->GetGlobalFrameRoutingId(),
-      render_frame_host->GetLastCommittedOrigin());
-  PrerenderHost* prerender_host1_rawptr = prerender_host1.get();
 
   const GURL kPrerenderingUrl2("https://example.com/next2");
   auto attributes2 = blink::mojom::PrerenderAttributes::New();
   attributes2->url = kPrerenderingUrl2;
-  auto prerender_host2 = std::make_unique<PrerenderHost>(
-      std::move(attributes2), render_frame_host->GetGlobalFrameRoutingId(),
-      render_frame_host->GetLastCommittedOrigin());
-  PrerenderHost* prerender_host2_rawptr = prerender_host2.get();
 
   PrerenderHostRegistry* registry = GetPrerenderHostRegistry();
-
-  registry->RegisterHost(kPrerenderingUrl1, std::move(prerender_host1));
-  EXPECT_EQ(registry->FindHostByUrlForTesting(kPrerenderingUrl1),
-            prerender_host1_rawptr);
-  registry->RegisterHost(kPrerenderingUrl2, std::move(prerender_host2));
-  EXPECT_EQ(registry->FindHostByUrlForTesting(kPrerenderingUrl2),
-            prerender_host2_rawptr);
+  registry->CreateAndStartHost(std::move(attributes1),
+                               render_frame_host->GetGlobalFrameRoutingId(),
+                               render_frame_host->GetLastCommittedOrigin());
+  registry->CreateAndStartHost(std::move(attributes2),
+                               render_frame_host->GetGlobalFrameRoutingId(),
+                               render_frame_host->GetLastCommittedOrigin());
+  PrerenderHost* prerender_host1 =
+      registry->FindHostByUrlForTesting(kPrerenderingUrl1);
+  PrerenderHost* prerender_host2 =
+      registry->FindHostByUrlForTesting(kPrerenderingUrl2);
 
   // Artificially finish navigation to make the prerender hosts ready to
   // activate the prerendered pages.
-  prerender_host1_rawptr->DidFinishNavigation(nullptr);
-  prerender_host2_rawptr->DidFinishNavigation(nullptr);
+  prerender_host1->DidFinishNavigation(nullptr);
+  prerender_host2->DidFinishNavigation(nullptr);
 
   // Select the first host.
   EXPECT_TRUE(registry->SelectForNavigation(kPrerenderingUrl1));
   EXPECT_EQ(registry->FindHostByUrlForTesting(kPrerenderingUrl1), nullptr);
   // The second host should still be findable.
-  registry->RegisterHost(kPrerenderingUrl2, std::move(prerender_host2));
   EXPECT_EQ(registry->FindHostByUrlForTesting(kPrerenderingUrl2),
-            prerender_host2_rawptr);
+            prerender_host2);
 
   // Select the second host.
   EXPECT_TRUE(registry->SelectForNavigation(kPrerenderingUrl2));
@@ -184,20 +168,17 @@ TEST_F(PrerenderHostRegistryTest, SelectForNavigationBeforeReadyForActivation) {
   const GURL kPrerenderingUrl("https://example.com/next");
   auto attributes = blink::mojom::PrerenderAttributes::New();
   attributes->url = kPrerenderingUrl;
-  auto prerender_host = std::make_unique<PrerenderHost>(
-      std::move(attributes), render_frame_host->GetGlobalFrameRoutingId(),
-      render_frame_host->GetLastCommittedOrigin());
-  PrerenderHost* prerender_host_rawptr = prerender_host.get();
 
   PrerenderHostRegistry* registry = GetPrerenderHostRegistry();
-
-  registry->RegisterHost(kPrerenderingUrl, std::move(prerender_host));
-  EXPECT_EQ(registry->FindHostByUrlForTesting(kPrerenderingUrl),
-            prerender_host_rawptr);
+  registry->CreateAndStartHost(std::move(attributes),
+                               render_frame_host->GetGlobalFrameRoutingId(),
+                               render_frame_host->GetLastCommittedOrigin());
+  PrerenderHost* prerender_host =
+      registry->FindHostByUrlForTesting(kPrerenderingUrl);
 
   // The prerender host is not ready for activation yet, so the registry
   // shouldn't select the host and instead should abandon it.
-  ASSERT_FALSE(prerender_host_rawptr->is_ready_for_activation());
+  ASSERT_FALSE(prerender_host->is_ready_for_activation());
   EXPECT_FALSE(registry->SelectForNavigation(kPrerenderingUrl));
   EXPECT_EQ(registry->FindHostByUrlForTesting(kPrerenderingUrl), nullptr);
 }
@@ -211,16 +192,12 @@ TEST_F(PrerenderHostRegistryTest, AbandonHost) {
   const GURL kPrerenderingUrl("https://example.com/next");
   auto attributes = blink::mojom::PrerenderAttributes::New();
   attributes->url = kPrerenderingUrl;
-  auto prerender_host = std::make_unique<PrerenderHost>(
-      std::move(attributes), render_frame_host->GetGlobalFrameRoutingId(),
-      render_frame_host->GetLastCommittedOrigin());
-  PrerenderHost* prerender_host_rawptr = prerender_host.get();
 
   PrerenderHostRegistry* registry = GetPrerenderHostRegistry();
-
-  registry->RegisterHost(kPrerenderingUrl, std::move(prerender_host));
-  EXPECT_EQ(registry->FindHostByUrlForTesting(kPrerenderingUrl),
-            prerender_host_rawptr);
+  registry->CreateAndStartHost(std::move(attributes),
+                               render_frame_host->GetGlobalFrameRoutingId(),
+                               render_frame_host->GetLastCommittedOrigin());
+  EXPECT_NE(registry->FindHostByUrlForTesting(kPrerenderingUrl), nullptr);
 
   registry->AbandonHost(kPrerenderingUrl);
   EXPECT_EQ(registry->FindHostByUrlForTesting(kPrerenderingUrl), nullptr);

content/browser/prerender/prerender_processor.cc

@@ -49,17 +49,10 @@ void PrerenderProcessor::Start(
 
   prerendering_url_ = attributes->url;
 
-  // Start prerendering.
-  auto prerender_host = std::make_unique<PrerenderHost>(
+  GetPrerenderHostRegistry().CreateAndStartHost(
       std::move(attributes),
       initiator_render_frame_host_.GetGlobalFrameRoutingId(),
       initiator_origin_);
-  prerender_host->StartPrerendering();
-
-  // Register the prerender host to PrerenderHostRegistry so that navigation can
-  // find this prerendered contents.
-  GetPrerenderHostRegistry().RegisterHost(prerendering_url_,
-                                          std::move(prerender_host));
 }
 
 void PrerenderProcessor::Cancel() {

content/browser/prerender/prerender_processor.h

@@ -24,8 +24,8 @@ class RenderFrameHostImpl;
 // request (see comments on the mojom interface for details) and owned by the
 // initiator RenderFrameHostImpl's mojo::UniqueReceiverSet.
 //
-// When Start() is called from a renderer process, this instantiates a new
-// PrerenderHost, and forwards the request to the prerender host.
+// When Start() is called from a renderer process, this asks
+// PrerenderHostRegistry to create a PrerenderHost and start prerendering.
 class CONTENT_EXPORT PrerenderProcessor final
     : public blink::mojom::PrerenderProcessor {
  public: