NoStatePrefetch: Move mojo disconnect handler from PrerenderProcessorClient to PrerenderProcessor

This is a preparation for removing webkit-prefixed prerendering events
(see https://crbug.com/839030). mojom::PrerepderProcessorClient is used
for dispatching the events and will be removed along with the events, so
it won't be available for monitoring the connection.

After this CL, mojom::PrerenderProcessor is used for monitoring the
connection instead of mojom::PrerenderProcessorClient so that follow-up
CLs can remove it. The renderer-side endpoints of
mojom::PrerenderProcessor and mojom::PrerenderProcessorClient have the
same lifetime, so the timing of disconnection is not changed
before/after this CL.

Another notable point in this CL is to change the ownership of
PrerenderProcessorImpl in the browser process. Before this CL,
PrerenderProcessorImpl is owned by mojo::MakeSelfOwnedReceiver. After
this CL, the processor destroys itself on the disconnect handler. This
change is necessary for monitoring the disconnection because
mojo::MakeSelfOwnedReceiver doesn't provide any hook point to register
the discconnect handler.

Bug: 839030
Change-Id: I36a094466690d352e1c149792a345ebdaf8a32d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2522725Reviewed-by: Robert Ogden <robertogden@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827590}

chrome/browser/prefetch/no_state_prefetch/prerender_unittest.cc

@@ -1503,31 +1503,6 @@ TEST_F(PrerenderTest, DISABLED_LinkManagerCancelThenAddAgain) {
   EXPECT_FALSE(prerender_manager()->FindEntry(url));
 }
 
-TEST_F(PrerenderTest, LinkManagerRendererDisconnect) {
-  prerender_manager()->SetTickClockForTesting(tick_clock());
-  EXPECT_TRUE(IsEmptyPrerenderLinkManager());
-  GURL url("http://www.myexample.com");
-  DummyPrerenderContents* prerender_contents =
-      prerender_manager()->CreateNextPrerenderContents(url,
-                                                       FINAL_STATUS_TIMED_OUT);
-
-  EXPECT_TRUE(AddSimplePrerender(url));
-  EXPECT_TRUE(prerender_contents->prerendering_has_started());
-  EXPECT_FALSE(prerender_contents->prerendering_has_been_cancelled());
-  ASSERT_EQ(prerender_contents, prerender_manager()->FindEntry(url));
-
-  // Disconnect all clients. Spin the run loop to give the link manager
-  // opportunity to detect disconnection.
-  DisconnectAllPrerenderProcessorClients();
-  base::RunLoop().RunUntilIdle();
-
-  tick_clock()->Advance(prerender_manager()->config().abandon_time_to_live +
-                        TimeDelta::FromSeconds(1));
-
-  EXPECT_FALSE(prerender_manager()->FindEntry(url));
-  EXPECT_TRUE(IsEmptyPrerenderLinkManager());
-}
-
 // Creates two prerenders, one of which should be blocked by the
 // max_link_concurrency; abandons both of them and waits to make sure both
 // are cleared from the PrerenderLinkManager.

components/no_state_prefetch/browser/BUILD.gn

@@ -54,13 +54,15 @@ source_set("unit_tests") {
   testonly = true
   sources = [
     "prerender_history_unittest.cc",
+    "prerender_processor_impl_unittest.cc",
     "prerender_util_unittest.cc",
   ]
 
   deps = [
-    ":browser",
     "//base",
     "//base/test:test_support",
+    "//components/no_state_prefetch/browser",
+    "//content/test:test_support",
     "//google_apis:google_apis",
     "//testing/gtest",
     "//url:url",

components/no_state_prefetch/browser/DEPS

@@ -5,6 +5,7 @@ include_rules = [
   "+components/keyed_service/core",
   "+content/public/browser",
   "+content/public/common",
+  "+content/public/test",
   "+mojo/public",
   "+net/http",
   "+services/metrics/public/cpp",

components/no_state_prefetch/browser/prerender_link_manager.cc

@@ -125,12 +125,6 @@ base::Optional<int> PrerenderLinkManager::OnStartPrerender(
       std::move(attributes), initiator_origin, std::move(processor_client),
       manager_->GetCurrentTimeTicks(), prerender_contents);
 
-  // Observe disconnect of the client to abandon the running prerender. The raw
-  // pointer to |this| is safe because |prerender| is owned by |this|.
-  prerender->remote_processor_client.set_disconnect_handler(
-      base::BindOnce(&PrerenderLinkManager::OnAbandonPrerender,
-                     base::Unretained(this), prerender->prerender_id));
-
   // Stash pointer used only for comparison later.
   const LinkPrerender* prerender_ptr = prerender.get();
 

components/no_state_prefetch/browser/prerender_link_manager.h

@@ -40,7 +40,7 @@ class PrerenderLinkManager : public KeyedService,
   // Called when a <link rel=prerender ...> element has been inserted into the
   // document. Returns the prerender id that is used for canceling or abandoning
   // prerendering. Returns base::nullopt if the prerender was not started.
-  base::Optional<int> OnStartPrerender(
+  virtual base::Optional<int> OnStartPrerender(
       int launcher_render_process_id,
       int launcher_render_view_id,
       blink::mojom::PrerenderAttributesPtr attributes,
@@ -50,13 +50,13 @@ class PrerenderLinkManager : public KeyedService,
 
   // Called when a <link rel=prerender ...> element has been explicitly removed
   // from a document.
-  void OnCancelPrerender(int prerender_id);
+  virtual void OnCancelPrerender(int prerender_id);
 
   // Called when a renderer launching <link rel=prerender ...> has navigated
   // away from the launching page, the launching renderer process has crashed,
   // or perhaps the renderer process was fast-closed when the last render view
   // in it was closed.
-  void OnAbandonPrerender(int prerender_id);
+  virtual void OnAbandonPrerender(int prerender_id);
 
  private:
   friend class PrerenderBrowserTest;

components/no_state_prefetch/browser/prerender_processor_impl.cc

@@ -10,7 +10,6 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/common/referrer.h"
-#include "mojo/public/cpp/bindings/self_owned_receiver.h"
 
 namespace prerender {
 
@@ -18,11 +17,16 @@ PrerenderProcessorImpl::PrerenderProcessorImpl(
     int render_process_id,
     int render_frame_id,
     const url::Origin& initiator_origin,
+    mojo::PendingReceiver<blink::mojom::PrerenderProcessor> receiver,
     std::unique_ptr<PrerenderProcessorImplDelegate> delegate)
     : render_process_id_(render_process_id),
       render_frame_id_(render_frame_id),
       initiator_origin_(initiator_origin),
-      delegate_(std::move(delegate)) {}
+      delegate_(std::move(delegate)) {
+  receiver_.Bind(std::move(receiver));
+  receiver_.set_disconnect_handler(
+      base::BindOnce(&PrerenderProcessorImpl::Abandon, base::Unretained(this)));
+}
 
 PrerenderProcessorImpl::~PrerenderProcessorImpl() = default;
 
@@ -31,11 +35,12 @@ void PrerenderProcessorImpl::Create(
     content::RenderFrameHost* frame_host,
     mojo::PendingReceiver<blink::mojom::PrerenderProcessor> receiver,
     std::unique_ptr<PrerenderProcessorImplDelegate> delegate) {
-  mojo::MakeSelfOwnedReceiver(
-      std::make_unique<PrerenderProcessorImpl>(
-          frame_host->GetProcess()->GetID(), frame_host->GetRoutingID(),
-          frame_host->GetLastCommittedOrigin(), std::move(delegate)),
-      std::move(receiver));
+  // PrerenderProcessorImpl is a self-owned object. This deletes itself on the
+  // mojo disconnect handler.
+  new PrerenderProcessorImpl(frame_host->GetProcess()->GetID(),
+                             frame_host->GetRoutingID(),
+                             frame_host->GetLastCommittedOrigin(),
+                             std::move(receiver), std::move(delegate));
 }
 
 void PrerenderProcessorImpl::Start(
@@ -79,6 +84,15 @@ void PrerenderProcessorImpl::Cancel() {
     link_manager->OnCancelPrerender(*prerender_id_);
 }
 
+void PrerenderProcessorImpl::Abandon() {
+  if (prerender_id_) {
+    auto* link_manager = GetPrerenderLinkManager();
+    if (link_manager)
+      link_manager->OnAbandonPrerender(*prerender_id_);
+  }
+  delete this;
+}
+
 PrerenderLinkManager* PrerenderProcessorImpl::GetPrerenderLinkManager() {
   auto* render_frame_host =
       content::RenderFrameHost::FromID(render_process_id_, render_frame_id_);

components/no_state_prefetch/browser/prerender_processor_impl.h

@@ -6,6 +6,7 @@
 #define COMPONENTS_NO_STATE_PREFETCH_BROWSER_PRERENDER_PROCESSOR_IMPL_H_
 
 #include "components/no_state_prefetch/browser/prerender_processor_impl_delegate.h"
+#include "mojo/public/cpp/bindings/receiver.h"
 #include "third_party/blink/public/mojom/prerender/prerender.mojom.h"
 #include "url/origin.h"
 
@@ -15,12 +16,17 @@ class RenderFrameHost;
 
 namespace prerender {
 
+// PrerenderProcessorImpl implements blink::mojom::PrerenderProcessor and works
+// as the browser-side entry point of NoStatePrefetch for <link rel=prerender>.
+// This is a self-owned object and deletes itself when the mojo connection is
+// lost.
 class PrerenderProcessorImpl : public blink::mojom::PrerenderProcessor {
  public:
   PrerenderProcessorImpl(
       int render_process_id,
       int render_frame_id,
       const url::Origin& initiator_origin,
+      mojo::PendingReceiver<blink::mojom::PrerenderProcessor> receiver,
       std::unique_ptr<PrerenderProcessorImplDelegate> delegate);
   ~PrerenderProcessorImpl() override;
 
@@ -36,6 +42,10 @@ class PrerenderProcessorImpl : public blink::mojom::PrerenderProcessor {
   void Cancel() override;
 
  private:
+  // Abandons prerendering and deletes `this`. Called from the mojo disconnect
+  // handler.
+  void Abandon();
+
   PrerenderLinkManager* GetPrerenderLinkManager();
 
   const int render_process_id_;
@@ -46,6 +56,8 @@ class PrerenderProcessorImpl : public blink::mojom::PrerenderProcessor {
   // The ID of PrerenderLinkManager::LinkPrerender. Used for canceling or
   // abandoning prerendering.
   base::Optional<int> prerender_id_;
+
+  mojo::Receiver<blink::mojom::PrerenderProcessor> receiver_{this};
 };
 
 }  // namespace prerender

components/no_state_prefetch/browser/prerender_processor_impl_unittest.cc

+// Copyright 2020 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 "components/no_state_prefetch/browser/prerender_processor_impl.h"
+
+#include "base/run_loop.h"
+#include "components/no_state_prefetch/browser/prerender_link_manager.h"
+#include "components/no_state_prefetch/browser/prerender_processor_impl_delegate.h"
+#include "content/public/test/test_browser_context.h"
+#include "content/public/test/test_renderer_host.h"
+#include "third_party/blink/public/common/features.h"
+
+namespace prerender {
+
+class MockPrerenderLinkManager final : public PrerenderLinkManager {
+ public:
+  MockPrerenderLinkManager() : PrerenderLinkManager(/*manager=*/nullptr) {}
+
+  base::Optional<int> OnStartPrerender(
+      int launcher_render_process_id,
+      int launcher_render_view_id,
+      blink::mojom::PrerenderAttributesPtr attributes,
+      const url::Origin& initiator_origin,
+      mojo::PendingRemote<blink::mojom::PrerenderProcessorClient>
+          processor_client) override {
+    DCHECK(!is_start_called_);
+    is_start_called_ = true;
+    return prerender_id_;
+  }
+
+  void OnCancelPrerender(int prerender_id) override {
+    DCHECK_EQ(prerender_id_, prerender_id);
+    DCHECK(!is_cancel_called_);
+    is_cancel_called_ = true;
+  }
+
+  void OnAbandonPrerender(int prerender_id) override {
+    DCHECK_EQ(prerender_id_, prerender_id);
+    DCHECK(!is_abandon_called_);
+    is_abandon_called_ = true;
+  }
+
+  bool is_start_called() const { return is_start_called_; }
+  bool is_cancel_called() const { return is_cancel_called_; }
+  bool is_abandon_called() const { return is_abandon_called_; }
+
+ private:
+  const int prerender_id_ = 100;
+  bool is_start_called_ = false;
+  bool is_cancel_called_ = false;
+  bool is_abandon_called_ = false;
+};
+
+class MockPrerenderProcessorImplDelegate final
+    : public PrerenderProcessorImplDelegate {
+ public:
+  explicit MockPrerenderProcessorImplDelegate(
+      MockPrerenderLinkManager* link_manager)
+      : link_manager_(link_manager) {}
+
+  PrerenderLinkManager* GetPrerenderLinkManager(
+      content::BrowserContext* browser_context) override {
+    return link_manager_;
+  }
+
+ private:
+  MockPrerenderLinkManager* link_manager_;
+};
+
+class PrerenderProcessorImplTest
+    : public content::RenderViewHostTestHarness,
+      public blink::mojom::PrerenderProcessorClient {
+ public:
+  // blink::mojom::PrerenderProcessorClient:
+  void OnPrerenderStart() override {}
+  void OnPrerenderStopLoading() override {}
+  void OnPrerenderDomContentLoaded() override {}
+  void OnPrerenderStop() override {}
+
+ protected:
+  mojo::Receiver<blink::mojom::PrerenderProcessorClient> receiver_{this};
+};
+
+TEST_F(PrerenderProcessorImplTest, StartCancelAbandon) {
+  auto link_manager = std::make_unique<MockPrerenderLinkManager>();
+
+  mojo::Remote<blink::mojom::PrerenderProcessor> remote;
+  PrerenderProcessorImpl::Create(
+      main_rfh(), remote.BindNewPipeAndPassReceiver(),
+      std::make_unique<MockPrerenderProcessorImplDelegate>(link_manager.get()));
+
+  auto attributes = blink::mojom::PrerenderAttributes::New();
+  attributes->url = GURL("https://example.com/prefetch");
+  attributes->referrer = blink::mojom::Referrer::New();
+
+  // Start() call should be propagated to the link manager.
+  EXPECT_FALSE(link_manager->is_start_called());
+  remote->Start(std::move(attributes), receiver_.BindNewPipeAndPassRemote());
+  remote.FlushForTesting();
+  EXPECT_TRUE(link_manager->is_start_called());
+
+  // Cancel() call should be propagated to the link manager.
+  EXPECT_FALSE(link_manager->is_cancel_called());
+  remote->Cancel();
+  remote.FlushForTesting();
+  EXPECT_TRUE(link_manager->is_cancel_called());
+
+  // Connection lost should abandon the link manager.
+  EXPECT_FALSE(link_manager->is_abandon_called());
+  remote.reset();
+  // FlushForTesting() is no longer available. Instead, use base::RunLoop.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(link_manager->is_abandon_called());
+}
+
+TEST_F(PrerenderProcessorImplTest, StartAbandon) {
+  auto link_manager = std::make_unique<MockPrerenderLinkManager>();
+
+  mojo::Remote<blink::mojom::PrerenderProcessor> remote;
+  PrerenderProcessorImpl::Create(
+      main_rfh(), remote.BindNewPipeAndPassReceiver(),
+      std::make_unique<MockPrerenderProcessorImplDelegate>(link_manager.get()));
+
+  auto attributes = blink::mojom::PrerenderAttributes::New();
+  attributes->url = GURL("https://example.com/prefetch");
+  attributes->referrer = blink::mojom::Referrer::New();
+
+  // Start() call should be propagated to the link manager.
+  EXPECT_FALSE(link_manager->is_start_called());
+  remote->Start(std::move(attributes), receiver_.BindNewPipeAndPassRemote());
+  remote.FlushForTesting();
+  EXPECT_TRUE(link_manager->is_start_called());
+
+  // Connection lost should abandon the link manager.
+  EXPECT_FALSE(link_manager->is_abandon_called());
+  remote.reset();
+  // FlushForTesting() is no longer available. Instead, use base::RunLoop.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(link_manager->is_abandon_called());
+}
+
+TEST_F(PrerenderProcessorImplTest, Cancel) {
+  auto link_manager = std::make_unique<MockPrerenderLinkManager>();
+
+  mojo::Remote<blink::mojom::PrerenderProcessor> remote;
+  PrerenderProcessorImpl::Create(
+      main_rfh(), remote.BindNewPipeAndPassReceiver(),
+      std::make_unique<MockPrerenderProcessorImplDelegate>(link_manager.get()));
+
+  // Call Cancel() before Start().
+  EXPECT_FALSE(link_manager->is_cancel_called());
+  remote->Cancel();
+  remote.FlushForTesting();
+  // The cancellation should not be propagated to the link manager.
+  EXPECT_FALSE(link_manager->is_cancel_called());
+}
+
+TEST_F(PrerenderProcessorImplTest, Abandon) {
+  auto link_manager = std::make_unique<MockPrerenderLinkManager>();
+
+  mojo::Remote<blink::mojom::PrerenderProcessor> remote;
+  PrerenderProcessorImpl::Create(
+      main_rfh(), remote.BindNewPipeAndPassReceiver(),
+      std::make_unique<MockPrerenderProcessorImplDelegate>(link_manager.get()));
+
+  // Disconnect before Start().
+  EXPECT_FALSE(link_manager->is_abandon_called());
+  remote.reset();
+  // FlushForTesting() is no longer available. Instead, use base::RunLoop.
+  base::RunLoop().RunUntilIdle();
+  // The disconnection should not be propagated to the link manager.
+  EXPECT_FALSE(link_manager->is_abandon_called());
+}
+
+}  // namespace prerender