[remoting host] Add unittest for RemoteOpenUrlMessageHandler

This CL adds a unittest for RemoteOpenUrlMessageHandler.

Bug: b:183135000
Change-Id: I78757a438b9e80c74c4420b15ac03fe55bfbf128
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3126902
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#917805}

remoting/host/BUILD.gn

@@ -357,6 +357,7 @@ static_library("common") {
     "ipc_mouse_cursor_monitor.h",
     "ipc_screen_controls.cc",
     "ipc_screen_controls.h",
+    "ipc_server.h",
     "ipc_video_frame_capturer.cc",
     "ipc_video_frame_capturer.h",
     "it2me_desktop_environment.cc",
@@ -664,6 +665,8 @@ static_library("test_support") {
     "fake_desktop_environment.h",
     "fake_host_extension.cc",
     "fake_host_extension.h",
+    "fake_ipc_server.cc",
+    "fake_ipc_server.h",
     "fake_keyboard_layout_monitor.cc",
     "fake_keyboard_layout_monitor.h",
     "fake_mouse_cursor_monitor.cc",
@@ -737,6 +740,7 @@ source_set("unit_tests") {
     "policy_watcher_unittest.cc",
     "process_stats_sender_unittest.cc",
     "remote_input_filter_unittest.cc",
+    "remote_open_url_message_handler_unittest.cc",
     "remoting_register_support_host_request_unittest.cc",
     "resizing_host_observer_unittest.cc",
     "resources_unittest.cc",

remoting/host/fake_ipc_server.cc

@@ -0,0 +1,37 @@
+// Copyright 2021 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 "remoting/host/fake_ipc_server.h"
+
+namespace remoting {
+
+FakeIpcServer::TestState::TestState() = default;
+
+FakeIpcServer::TestState::~TestState() = default;
+
+FakeIpcServer::FakeIpcServer(TestState* test_state) : test_state_(test_state) {}
+
+FakeIpcServer::~FakeIpcServer() = default;
+
+void FakeIpcServer::StartServer() {
+  test_state_->is_server_started = true;
+}
+
+void FakeIpcServer::StopServer() {
+  test_state_->is_server_started = false;
+}
+
+void FakeIpcServer::Close(mojo::ReceiverId id) {
+  test_state_->last_closed_receiver = id;
+}
+
+void FakeIpcServer::set_disconnect_handler(base::RepeatingClosure handler) {
+  test_state_->disconnect_handler = handler;
+}
+
+mojo::ReceiverId FakeIpcServer::current_receiver() const {
+  return test_state_->current_receiver;
+}
+
+}  // namespace remoting

remoting/host/fake_ipc_server.h

@@ -0,0 +1,42 @@
+// Copyright 2021 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.
+
+#ifndef REMOTING_HOST_FAKE_IPC_SERVER_H_
+#define REMOTING_HOST_FAKE_IPC_SERVER_H_
+
+#include "remoting/host/ipc_server.h"
+
+namespace remoting {
+
+class FakeIpcServer final : public IpcServer {
+ public:
+  // Used to interact with FakeIpcServer after ownership is passed elsewhere.
+  struct TestState {
+    TestState();
+    ~TestState();
+
+    bool is_server_started = false;
+    base::RepeatingClosure disconnect_handler;
+    mojo::ReceiverId current_receiver = 0u;
+    mojo::ReceiverId last_closed_receiver = 0u;
+  };
+
+  explicit FakeIpcServer(TestState* test_state);
+  FakeIpcServer(const FakeIpcServer&) = delete;
+  FakeIpcServer& operator=(const FakeIpcServer&) = delete;
+  ~FakeIpcServer() override;
+
+  void StartServer() override;
+  void StopServer() override;
+  void Close(mojo::ReceiverId id) override;
+  void set_disconnect_handler(base::RepeatingClosure handler) override;
+  mojo::ReceiverId current_receiver() const override;
+
+ private:
+  TestState* test_state_;
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_FAKE_IPC_SERVER_H_

remoting/host/ipc_server.h

@@ -0,0 +1,42 @@
+// Copyright 2021 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.
+
+#ifndef REMOTING_HOST_IPC_SERVER_H_
+#define REMOTING_HOST_IPC_SERVER_H_
+
+#include "base/callback.h"
+#include "mojo/public/cpp/bindings/receiver_set.h"
+
+namespace remoting {
+
+// An interface for MojoIpcServer to allow mocking in unittests.
+class IpcServer {
+ public:
+  virtual ~IpcServer() = default;
+
+  // Starts sending out mojo invitations and accepting IPCs. No-op if the server
+  // is already started.
+  virtual void StartServer() = 0;
+
+  // Stops sending out mojo invitations and accepting IPCs. No-op if the server
+  // is already stopped.
+  virtual void StopServer() = 0;
+
+  // Close the receiver identified by |id| and disconnect the remote. No-op if
+  // |id| doesn't exist or the receiver is already closed.
+  virtual void Close(mojo::ReceiverId id) = 0;
+
+  // Sets a callback to be invoked any time a receiver is disconnected. You may
+  // find out which receiver is being disconnected by calling
+  // |current_receiver()|.
+  virtual void set_disconnect_handler(base::RepeatingClosure handler) = 0;
+
+  // Call this method to learn which receiver has received the incoming IPC or
+  // which receiver is being disconnected.
+  virtual mojo::ReceiverId current_receiver() const = 0;
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_IPC_SERVER_H_

remoting/host/mojo_ipc_server.h

@@ -18,6 +18,7 @@
 #include "mojo/public/cpp/platform/named_platform_channel.h"
 #include "mojo/public/cpp/system/message_pipe.h"
 #include "mojo/public/cpp/system/simple_watcher.h"
+#include "remoting/host/ipc_server.h"
 
 namespace mojo {
 class IsolatedConnection;
@@ -27,22 +28,14 @@ namespace remoting {
 
 // Template-less base class to keep implementations in the .cc file. For usage,
 // see MojoIpcServer.
-class MojoIpcServerBase {
+class MojoIpcServerBase : public IpcServer {
  public:
   // Internal use only.
   struct PendingConnection;
 
-  // Starts sending out mojo invitations and accepting IPCs. No-op if the server
-  // is already started.
-  void StartServer();
-
-  // Stops sending out mojo invitations and accepting IPCs. No-op if the server
-  // is already stopped.
-  void StopServer();
-
-  // Close the receiver identified by |id| and disconnects the remote. No-op if
-  // |id| doesn't exist or the receiver is already closed.
-  void Close(mojo::ReceiverId id);
+  void StartServer() override;
+  void StopServer() override;
+  void Close(mojo::ReceiverId id) override;
 
   // Sets a callback to be run when an invitation is sent. Used by unit tests
   // only.
@@ -54,7 +47,7 @@ class MojoIpcServerBase {
  protected:
   explicit MojoIpcServerBase(
       const mojo::NamedPlatformChannel::ServerName& server_name);
-  virtual ~MojoIpcServerBase();
+  ~MojoIpcServerBase() override;
 
   void SendInvitation();
 
@@ -137,16 +130,11 @@ class MojoIpcServer final : public MojoIpcServerBase {
   MojoIpcServer(const MojoIpcServer&) = delete;
   MojoIpcServer& operator=(const MojoIpcServer&) = delete;
 
-  // Sets a callback to be invoked any time a receiver is disconnected. You may
-  // find out which receiver is being disconnected by calling
-  // |current_receiver()|.
-  void set_disconnect_handler(base::RepeatingClosure handler) {
+  void set_disconnect_handler(base::RepeatingClosure handler) override {
     receiver_set_.set_disconnect_handler(handler);
   }
 
-  // Call this method to learn which receiver has received the incoming IPC or
-  // which receiver is being disconnected.
-  mojo::ReceiverId current_receiver() const {
+  mojo::ReceiverId current_receiver() const override {
     return receiver_set_.current_receiver();
   }
 

remoting/host/remote_open_url_message_handler.cc

@@ -8,6 +8,8 @@
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "remoting/base/compound_buffer.h"
+#include "remoting/host/mojo_ipc_server.h"
+#include "remoting/host/remote_open_url_constants.h"
 #include "remoting/protocol/message_serialization.h"
 #include "url/gurl.h"
 
@@ -36,8 +38,20 @@ mojom::OpenUrlResult ToMojomOpenUrlResult(
 RemoteOpenUrlMessageHandler::RemoteOpenUrlMessageHandler(
     const std::string& name,
     std::unique_ptr<protocol::MessagePipe> pipe)
+    : RemoteOpenUrlMessageHandler(
+          name,
+          std::move(pipe),
+          std::make_unique<MojoIpcServer<mojom::RemoteUrlOpener>>(
+              GetRemoteOpenUrlIpcChannelName(),
+              this)) {}
+
+RemoteOpenUrlMessageHandler::RemoteOpenUrlMessageHandler(
+    const std::string& name,
+    std::unique_ptr<protocol::MessagePipe> pipe,
+    std::unique_ptr<IpcServer> ipc_server)
     : protocol::NamedMessagePipeHandler(name, std::move(pipe)) {
-  ipc_server_.set_disconnect_handler(base::BindRepeating(
+  ipc_server_ = std::move(ipc_server);
+  ipc_server_->set_disconnect_handler(base::BindRepeating(
       &RemoteOpenUrlMessageHandler::OnIpcDisconnected, base::Unretained(this)));
 }
 
@@ -50,7 +64,7 @@ RemoteOpenUrlMessageHandler::~RemoteOpenUrlMessageHandler() {
 void RemoteOpenUrlMessageHandler::OnConnected() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  ipc_server_.StartServer();
+  ipc_server_->StartServer();
 }
 
 void RemoteOpenUrlMessageHandler::OnIncomingMessage(
@@ -72,13 +86,13 @@ void RemoteOpenUrlMessageHandler::OnIncomingMessage(
   }
   std::move(it->second).Run(ToMojomOpenUrlResult(response.result()));
   callbacks_.erase(it);
-  ipc_server_.Close(response.id());
+  ipc_server_->Close(response.id());
 }
 
 void RemoteOpenUrlMessageHandler::OnDisconnecting() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  ipc_server_.StopServer();
+  ipc_server_->StopServer();
 
   // The remote connection is going away, so inform all IPC clients to open the
   // URL locally.
@@ -91,7 +105,7 @@ void RemoteOpenUrlMessageHandler::OnDisconnecting() {
 void RemoteOpenUrlMessageHandler::OnIpcDisconnected() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  auto it = callbacks_.find(ipc_server_.current_receiver());
+  auto it = callbacks_.find(ipc_server_->current_receiver());
   if (it != callbacks_.end()) {
     LOG(WARNING)
         << "The client has disconnected before the response is received.";
@@ -108,7 +122,7 @@ void RemoteOpenUrlMessageHandler::OpenUrl(const GURL& url,
     return;
   }
 
-  mojo::ReceiverId id = ipc_server_.current_receiver();
+  mojo::ReceiverId id = ipc_server_->current_receiver();
   DCHECK(callbacks_.find(id) == callbacks_.end())
       << "The client has made more than one call to OpenUrl().";
 

remoting/host/remote_open_url_message_handler.h

@@ -9,16 +9,17 @@
 
 #include "base/containers/flat_map.h"
 #include "base/sequence_checker.h"
-#include "remoting/host/mojo_ipc_server.h"
+#include "mojo/public/cpp/bindings/receiver_set.h"
 #include "remoting/host/mojom/remote_url_opener.mojom.h"
-#include "remoting/host/remote_open_url_constants.h"
 #include "remoting/proto/remote_open_url.pb.h"
 #include "remoting/protocol/named_message_pipe_handler.h"
 
 class GURL;
 
 namespace remoting {
 
+class IpcServer;
+
 class RemoteOpenUrlMessageHandler final
     : public mojom::RemoteUrlOpener,
       public protocol::NamedMessagePipeHandler {
@@ -37,15 +38,21 @@ class RemoteOpenUrlMessageHandler final
       delete;
 
  private:
+  friend class RemoteOpenUrlMessageHandlerTest;
+
+  // Used by unittests.
+  RemoteOpenUrlMessageHandler(const std::string& name,
+                              std::unique_ptr<protocol::MessagePipe> pipe,
+                              std::unique_ptr<IpcServer> ipc_server);
+
   void OnIpcDisconnected();
 
   // RemoteUrlOpener implementation.
   void OpenUrl(const GURL& url, OpenUrlCallback callback) override;
 
   SEQUENCE_CHECKER(sequence_checker_);
 
-  MojoIpcServer<mojom::RemoteUrlOpener> ipc_server_{
-      GetRemoteOpenUrlIpcChannelName(), this};
+  std::unique_ptr<IpcServer> ipc_server_;
 
   static_assert(
       std::is_same<