From 7d63c5e0fd535f85b94c1918963604e5aeb8a394 Mon Sep 17 00:00:00 2001 From: Yuwei Huang Date: Thu, 2 Sep 2021 20:06:22 +0000 Subject: [PATCH] [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 Reviewed-by: Joe Downing Cr-Commit-Position: refs/heads/main@{#917805} --- remoting/host/BUILD.gn | 4 + remoting/host/fake_ipc_server.cc | 37 ++++ remoting/host/fake_ipc_server.h | 42 ++++ remoting/host/ipc_server.h | 42 ++++ remoting/host/mojo_ipc_server.h | 28 +-- .../host/remote_open_url_message_handler.cc | 26 ++- .../host/remote_open_url_message_handler.h | 15 +- ...emote_open_url_message_handler_unittest.cc | 199 ++++++++++++++++++ remoting/protocol/fake_message_pipe.h | 2 + 9 files changed, 365 insertions(+), 30 deletions(-) create mode 100644 remoting/host/fake_ipc_server.cc create mode 100644 remoting/host/fake_ipc_server.h create mode 100644 remoting/host/ipc_server.h create mode 100644 remoting/host/remote_open_url_message_handler_unittest.cc diff --git a/remoting/host/BUILD.gn b/remoting/host/BUILD.gn index 4f617a29cde970..5f2c40ac734e3b 100644 --- a/remoting/host/BUILD.gn +++ b/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", diff --git a/remoting/host/fake_ipc_server.cc b/remoting/host/fake_ipc_server.cc new file mode 100644 index 00000000000000..31bd74a14ae338 --- /dev/null +++ b/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 diff --git a/remoting/host/fake_ipc_server.h b/remoting/host/fake_ipc_server.h new file mode 100644 index 00000000000000..598102fa94bb43 --- /dev/null +++ b/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_ diff --git a/remoting/host/ipc_server.h b/remoting/host/ipc_server.h new file mode 100644 index 00000000000000..a66e80b51eb093 --- /dev/null +++ b/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_ diff --git a/remoting/host/mojo_ipc_server.h b/remoting/host/mojo_ipc_server.h index f00a716c3483dd..102ea07c9c6ac8 100644 --- a/remoting/host/mojo_ipc_server.h +++ b/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(); } diff --git a/remoting/host/remote_open_url_message_handler.cc b/remoting/host/remote_open_url_message_handler.cc index 39c3f9b69baf95..812ed74aeb94ea 100644 --- a/remoting/host/remote_open_url_message_handler.cc +++ b/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 pipe) + : RemoteOpenUrlMessageHandler( + name, + std::move(pipe), + std::make_unique>( + GetRemoteOpenUrlIpcChannelName(), + this)) {} + +RemoteOpenUrlMessageHandler::RemoteOpenUrlMessageHandler( + const std::string& name, + std::unique_ptr pipe, + std::unique_ptr 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()."; diff --git a/remoting/host/remote_open_url_message_handler.h b/remoting/host/remote_open_url_message_handler.h index 6148a0e7281083..47e8421b90bfcb 100644 --- a/remoting/host/remote_open_url_message_handler.h +++ b/remoting/host/remote_open_url_message_handler.h @@ -9,9 +9,8 @@ #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" @@ -19,6 +18,8 @@ class GURL; namespace remoting { +class IpcServer; + class RemoteOpenUrlMessageHandler final : public mojom::RemoteUrlOpener, public protocol::NamedMessagePipeHandler { @@ -37,6 +38,13 @@ class RemoteOpenUrlMessageHandler final delete; private: + friend class RemoteOpenUrlMessageHandlerTest; + + // Used by unittests. + RemoteOpenUrlMessageHandler(const std::string& name, + std::unique_ptr pipe, + std::unique_ptr ipc_server); + void OnIpcDisconnected(); // RemoteUrlOpener implementation. @@ -44,8 +52,7 @@ class RemoteOpenUrlMessageHandler final SEQUENCE_CHECKER(sequence_checker_); - MojoIpcServer ipc_server_{ - GetRemoteOpenUrlIpcChannelName(), this}; + std::unique_ptr ipc_server_; static_assert( std::is_same< diff --git a/remoting/host/remote_open_url_message_handler_unittest.cc b/remoting/host/remote_open_url_message_handler_unittest.cc new file mode 100644 index 00000000000000..421564271cc9be --- /dev/null +++ b/remoting/host/remote_open_url_message_handler_unittest.cc @@ -0,0 +1,199 @@ +// 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/remote_open_url_message_handler.h" + +#include +#include + +#include "base/bind.h" +#include "base/memory/scoped_refptr.h" +#include "base/test/mock_callback.h" +#include "mojo/public/cpp/bindings/receiver_set.h" +#include "net/base/io_buffer.h" +#include "remoting/base/compound_buffer.h" +#include "remoting/host/fake_ipc_server.h" +#include "remoting/host/mojom/remote_url_opener.mojom.h" +#include "remoting/proto/remote_open_url.pb.h" +#include "remoting/protocol/fake_message_pipe.h" +#include "remoting/protocol/fake_message_pipe_wrapper.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "url/gurl.h" + +namespace remoting { + +namespace { + +protocol::RemoteOpenUrl ParseMessage(const std::string& data) { + protocol::RemoteOpenUrl message; + message.ParseFromString(data); + return message; +} + +std::unique_ptr MessageToBuffer( + const protocol::RemoteOpenUrl& message) { + auto buffer = std::make_unique(); + std::string data = message.SerializeAsString(); + buffer->Append(base::MakeRefCounted(data.data()), + data.size()); + return buffer; +} + +} // namespace + +class RemoteOpenUrlMessageHandlerTest : public testing::Test { + public: + RemoteOpenUrlMessageHandlerTest(); + ~RemoteOpenUrlMessageHandlerTest() override; + + protected: + void OpenUrl(mojo::ReceiverId receiver_id, + const GURL& url, + mojom::RemoteUrlOpener::OpenUrlCallback callback); + + protocol::FakeMessagePipe fake_pipe_{/* asynchronous= */ false}; + FakeIpcServer::TestState ipc_server_state_; + RemoteOpenUrlMessageHandler* message_handler_; +}; + +RemoteOpenUrlMessageHandlerTest::RemoteOpenUrlMessageHandlerTest() { + // Lifetime of |message_handler_| is controlled by the |fake_pipe_|. + message_handler_ = new RemoteOpenUrlMessageHandler( + "fake name", fake_pipe_.Wrap(), + std::make_unique(&ipc_server_state_)); + fake_pipe_.OpenPipe(); + EXPECT_TRUE(ipc_server_state_.is_server_started); +} + +RemoteOpenUrlMessageHandlerTest::~RemoteOpenUrlMessageHandlerTest() { + if (fake_pipe_.pipe_opened()) { + fake_pipe_.ClosePipe(); + EXPECT_FALSE(ipc_server_state_.is_server_started); + } +} + +void RemoteOpenUrlMessageHandlerTest::OpenUrl( + mojo::ReceiverId receiver_id, + const GURL& url, + mojom::RemoteUrlOpener::OpenUrlCallback callback) { + ipc_server_state_.current_receiver = receiver_id; + message_handler_->OpenUrl(url, std::move(callback)); +} + +TEST_F(RemoteOpenUrlMessageHandlerTest, OpenUrl) { + base::MockCallback callback; + EXPECT_CALL(callback, Run(mojom::OpenUrlResult::SUCCESS)).Times(1); + + const uint64_t receiver_id = 1u; + OpenUrl(receiver_id, GURL("http://google.com/"), callback.Get()); + protocol::RemoteOpenUrl response; + response.mutable_open_url_response()->set_id(receiver_id); + response.mutable_open_url_response()->set_result( + protocol::RemoteOpenUrl::OpenUrlResponse::SUCCESS); + fake_pipe_.Receive(MessageToBuffer(response)); + + protocol::RemoteOpenUrl request_message = + ParseMessage(fake_pipe_.sent_messages().front()); + ASSERT_TRUE(request_message.has_open_url_request()); + ASSERT_EQ(receiver_id, request_message.open_url_request().id()); + ASSERT_EQ("http://google.com/", request_message.open_url_request().url()); + ASSERT_EQ(receiver_id, ipc_server_state_.last_closed_receiver); +} + +TEST_F(RemoteOpenUrlMessageHandlerTest, OpenInvalidUrl_Failure) { + base::MockCallback callback; + EXPECT_CALL(callback, Run(mojom::OpenUrlResult::FAILURE)).Times(1); + + OpenUrl(1u, GURL("invalid_url"), callback.Get()); + + ASSERT_EQ(0u, fake_pipe_.sent_messages().size()); +} + +TEST_F(RemoteOpenUrlMessageHandlerTest, OpenMultipleUrls) { + base::MockCallback url_1_callback; + EXPECT_CALL(url_1_callback, Run(mojom::OpenUrlResult::SUCCESS)).Times(1); + + base::MockCallback url_2_callback; + EXPECT_CALL(url_2_callback, Run(mojom::OpenUrlResult::FAILURE)).Times(1); + + OpenUrl(1u, GURL("http://google.com/url1"), url_1_callback.Get()); + OpenUrl(2u, GURL("http://google.com/url2"), url_2_callback.Get()); + + protocol::RemoteOpenUrl response; + response.mutable_open_url_response()->set_id(1u); + response.mutable_open_url_response()->set_result( + protocol::RemoteOpenUrl::OpenUrlResponse::SUCCESS); + fake_pipe_.Receive(MessageToBuffer(response)); + ASSERT_EQ(1u, ipc_server_state_.last_closed_receiver); + + response.mutable_open_url_response()->set_id(2u); + response.mutable_open_url_response()->set_result( + protocol::RemoteOpenUrl::OpenUrlResponse::FAILURE); + fake_pipe_.Receive(MessageToBuffer(response)); + ASSERT_EQ(2u, ipc_server_state_.last_closed_receiver); + + base::queue sent_messages = fake_pipe_.sent_messages(); + ASSERT_EQ(2u, sent_messages.size()); + + protocol::RemoteOpenUrl request_message_1 = + ParseMessage(sent_messages.front()); + ASSERT_TRUE(request_message_1.has_open_url_request()); + ASSERT_EQ(1u, request_message_1.open_url_request().id()); + ASSERT_EQ("http://google.com/url1", + request_message_1.open_url_request().url()); + sent_messages.pop(); + + protocol::RemoteOpenUrl request_message_2 = + ParseMessage(sent_messages.front()); + ASSERT_TRUE(request_message_2.has_open_url_request()); + ASSERT_EQ(2u, request_message_2.open_url_request().id()); + ASSERT_EQ("http://google.com/url2", + request_message_2.open_url_request().url()); +} + +TEST_F(RemoteOpenUrlMessageHandlerTest, + DisconnectReceiver_CallbackSilentlyDropped) { + // This test is to make sure that callbacks will be dropped immediately after + // the the corresponding IPC is disconnected, which is to prevent holding the + // callback until the message handler is destroyed in cases like the server + // never responds and the client quits due to timeout. + class DestructionDetector { + public: + explicit DestructionDetector(bool* out_is_destructor_called) + : out_is_destructor_called_(out_is_destructor_called) {} + + ~DestructionDetector() { *out_is_destructor_called_ = true; } + + private: + bool* out_is_destructor_called_; + }; + + bool is_callback_destroyed = false; + // The ownership of |DestructionDetector| is passed to the callback, so it is + // used here to detect whether the callback itself has been destroyed. + auto destruction_detecting_callback = base::BindOnce( + [](std::unique_ptr destruction_detector, + mojom::OpenUrlResult unused) { + FAIL() << "Callback should be silently dropped."; + }, + std::make_unique(&is_callback_destroyed)); + + OpenUrl(1u, GURL("http://google.com/url1"), + std::move(destruction_detecting_callback)); + ipc_server_state_.current_receiver = 1u; + ipc_server_state_.disconnect_handler.Run(); + + ASSERT_TRUE(is_callback_destroyed); +} + +TEST_F(RemoteOpenUrlMessageHandlerTest, + DisconnectMessagePipe_PendingCallbacksRunWithLocalFallback) { + base::MockCallback callback; + EXPECT_CALL(callback, Run(mojom::OpenUrlResult::LOCAL_FALLBACK)).Times(1); + + OpenUrl(1u, GURL("http://google.com/url1"), callback.Get()); + fake_pipe_.ClosePipe(); +} + +} // namespace remoting diff --git a/remoting/protocol/fake_message_pipe.h b/remoting/protocol/fake_message_pipe.h index 829baf679ba494..dfa148a7839ade 100644 --- a/remoting/protocol/fake_message_pipe.h +++ b/remoting/protocol/fake_message_pipe.h @@ -53,6 +53,8 @@ class FakeMessagePipe final : public MessagePipe { // Returns all messages sent using Send(). const base::queue& sent_messages() { return sent_messages_; } + bool pipe_opened() const { return pipe_opened_; } + private: void SendImpl(google::protobuf::MessageLite* message, base::OnceClosure done); void ReceiveImpl(std::unique_ptr message);