From 8e9ba76ba55c1624567dd81599cc526528e8e5e6 Mon Sep 17 00:00:00 2001 From: "jam@chromium.org" Date: Wed, 21 May 2014 16:39:18 +0000 Subject: [PATCH] Add regression tests to ensure that IPC::Listener::OnBadMessageReceived is always called for all cases, i.e. 1) using IPC::Channel directly 2) using IPC::ChannelProxy and the IPC is dispatched on the IO thread 2) using IPC::ChannelProxy and the IPC is dispatched on the listener thread R=tsepez@chromium.org Review URL: https://codereview.chromium.org/282303014 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@271915 0039d316-1c4b-4281-b951-d872f2087c98 --- .../render_view_host_unittest.cc | 42 --- ipc/ipc_channel_proxy_unittest.cc | 245 +++++++++++++----- ipc/ipc_channel_proxy_unittest_messages.h | 44 ++++ 3 files changed, 228 insertions(+), 103 deletions(-) create mode 100644 ipc/ipc_channel_proxy_unittest_messages.h diff --git a/content/browser/renderer_host/render_view_host_unittest.cc b/content/browser/renderer_host/render_view_host_unittest.cc index c4b922a9a5b1ee..600ef7e07b7452 100644 --- a/content/browser/renderer_host/render_view_host_unittest.cc +++ b/content/browser/renderer_host/render_view_host_unittest.cc @@ -207,48 +207,6 @@ TEST_F(RenderViewHostTest, DragEnteredFileURLsStillBlocked) { EXPECT_FALSE(policy->CanRequestURL(id, sensitive_file_url)); EXPECT_FALSE(policy->CanReadFile(id, sensitive_file_path)); } -/* TODO(jam) -// The test that follow trigger DCHECKS in debug build. -#if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON) - -// Test that when we fail to de-serialize a message, RenderViewHost calls the -// ReceivedBadMessage() handler. -TEST_F(RenderViewHostTest, BadMessageHandlerRenderViewHost) { - EXPECT_EQ(0, process()->bad_msg_count()); - // craft an incorrect ViewHostMsg_UpdateTargetURL message. The real one has - // two payload items but the one we construct has none. - IPC::Message message(0, ViewHostMsg_UpdateTargetURL::ID, - IPC::Message::PRIORITY_NORMAL); - test_rvh()->OnMessageReceived(message); - EXPECT_EQ(1, process()->bad_msg_count()); -} - -// Test that when we fail to de-serialize a message, RenderWidgetHost calls the -// ReceivedBadMessage() handler. -TEST_F(RenderViewHostTest, BadMessageHandlerRenderWidgetHost) { - EXPECT_EQ(0, process()->bad_msg_count()); - // craft an incorrect ViewHostMsg_UpdateRect message. The real one has - // one payload item but the one we construct has none. - IPC::Message message(0, ViewHostMsg_UpdateRect::ID, - IPC::Message::PRIORITY_NORMAL); - test_rvh()->OnMessageReceived(message); - EXPECT_EQ(1, process()->bad_msg_count()); -} - -// Test that OnInputEventAck() detects bad messages. -TEST_F(RenderViewHostTest, BadMessageHandlerInputEventAck) { - EXPECT_EQ(0, process()->bad_msg_count()); - // InputHostMsg_HandleInputEvent_ACK is defined taking 0 params but - // the code actually expects it to have at least one int para, this this - // bogus message will not fail at de-serialization but should fail in - // OnInputEventAck() processing. - IPC::Message message(0, InputHostMsg_HandleInputEvent_ACK::ID, - IPC::Message::PRIORITY_NORMAL); - test_rvh()->OnMessageReceived(message); - EXPECT_EQ(1, process()->bad_msg_count()); -} -#endif -*/ TEST_F(RenderViewHostTest, MessageWithBadHistoryItemFiles) { base::FilePath file_path; diff --git a/ipc/ipc_channel_proxy_unittest.cc b/ipc/ipc_channel_proxy_unittest.cc index c5e078a912ee20..ee50caa868a81c 100644 --- a/ipc/ipc_channel_proxy_unittest.cc +++ b/ipc/ipc_channel_proxy_unittest.cc @@ -8,47 +8,69 @@ #include "base/pickle.h" #include "base/threading/thread.h" #include "ipc/ipc_message.h" -#include "ipc/ipc_message_macros.h" #include "ipc/ipc_test_base.h" #include "ipc/message_filter.h" -namespace { +// Get basic type definitions. +#define IPC_MESSAGE_IMPL +#include "ipc/ipc_channel_proxy_unittest_messages.h" -#if defined(IPC_MESSAGE_START) -#undef IPC_MESSAGE_START -#endif +// Generate constructors. +#include "ipc/struct_constructor_macros.h" +#include "ipc/ipc_channel_proxy_unittest_messages.h" -enum Command { - SEND, - QUIT -}; +// Generate destructors. +#include "ipc/struct_destructor_macros.h" +#include "ipc/ipc_channel_proxy_unittest_messages.h" -static void Send(IPC::Sender* sender, - int message_class, - Command command) { - const int IPC_MESSAGE_START = message_class; - IPC::Message* message = new IPC::Message(0, - IPC_MESSAGE_ID(), - IPC::Message::PRIORITY_NORMAL); - message->WriteInt(command); - sender->Send(message); -} +// Generate param traits write methods. +#include "ipc/param_traits_write_macros.h" +namespace IPC { +#include "ipc/ipc_channel_proxy_unittest_messages.h" +} // namespace IPC + +// Generate param traits read methods. +#include "ipc/param_traits_read_macros.h" +namespace IPC { +#include "ipc/ipc_channel_proxy_unittest_messages.h" +} // namespace IPC + +// Generate param traits log methods. +#include "ipc/param_traits_log_macros.h" +namespace IPC { +#include "ipc/ipc_channel_proxy_unittest_messages.h" +} // namespace IPC + + +namespace { class QuitListener : public IPC::Listener { public: - QuitListener() {} + QuitListener() : bad_message_received_(false) {} virtual ~QuitListener() {} virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE { - PickleIterator iter(message); + IPC_BEGIN_MESSAGE_MAP(QuitListener, message) + IPC_MESSAGE_HANDLER(WorkerMsg_Quit, OnQuit) + IPC_MESSAGE_HANDLER(TestMsg_BadMessage, OnBadMessage) + IPC_END_MESSAGE_MAP() + return true; + } - int command = SEND; - EXPECT_TRUE(iter.ReadInt(&command)); - if (command == QUIT) - base::MessageLoop::current()->QuitWhenIdle(); + virtual void OnBadMessageReceived(const IPC::Message& message) OVERRIDE { + bad_message_received_ = true; + } - return true; + void OnQuit() { + base::MessageLoop::current()->QuitWhenIdle(); } + + void OnBadMessage(const BadType& bad_type) { + // Should never be called since IPC wouldn't be deserialized correctly. + CHECK(false); + } + + bool bad_message_received_; }; class ChannelReflectorListener : public IPC::Listener { @@ -62,20 +84,35 @@ class ChannelReflectorListener : public IPC::Listener { } virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE { - CHECK(channel_); + IPC_BEGIN_MESSAGE_MAP(ChannelReflectorListener, message) + IPC_MESSAGE_HANDLER(TestMsg_Bounce, OnTestBounce) + IPC_MESSAGE_HANDLER(TestMsg_SendBadMessage, OnSendBadMessage) + IPC_MESSAGE_HANDLER(UtilityMsg_Bounce, OnUtilityBounce) + IPC_MESSAGE_HANDLER(WorkerMsg_Bounce, OnBounce) + IPC_MESSAGE_HANDLER(WorkerMsg_Quit, OnQuit) + IPC_END_MESSAGE_MAP() + return true; + } - PickleIterator iter(message); + void OnTestBounce() { + channel_->Send(new TestMsg_Bounce()); + } - int command = SEND; - EXPECT_TRUE(iter.ReadInt(&command)); - if (command == QUIT) { - channel_->Send(new IPC::Message(message)); - base::MessageLoop::current()->QuitWhenIdle(); - return true; - } + void OnSendBadMessage() { + channel_->Send(new TestMsg_BadMessage(BadType())); + } - channel_->Send(new IPC::Message(message)); - return true; + void OnUtilityBounce() { + channel_->Send(new UtilityMsg_Bounce()); + } + + void OnBounce() { + channel_->Send(new WorkerMsg_Bounce()); + } + + void OnQuit() { + channel_->Send(new WorkerMsg_Quit()); + base::MessageLoop::current()->QuitWhenIdle(); } private: @@ -149,7 +186,21 @@ class MessageCountFilter : public IPC::MessageFilter { EXPECT_EQ(supported_message_class_, IPC_MESSAGE_CLASS(message)); } ++messages_received_; - return message_filtering_enabled_; + + if (!message_filtering_enabled_) + return false; + + bool handled = true; + IPC_BEGIN_MESSAGE_MAP(MessageCountFilter, message) + IPC_MESSAGE_HANDLER(TestMsg_BadMessage, OnBadMessage) + IPC_MESSAGE_UNHANDLED(handled = false) + IPC_END_MESSAGE_MAP() + return handled; + } + + void OnBadMessage(const BadType& bad_type) { + // Should never be called since IPC wouldn't be deserialized correctly. + CHECK(false); } virtual bool GetSupportedMessageClasses( @@ -207,11 +258,15 @@ class IPCChannelProxyTest : public IPCTestBase { } void SendQuitMessageAndWaitForIdle() { - Send(sender(), -1, QUIT); + sender()->Send(new WorkerMsg_Quit); base::MessageLoop::current()->Run(); EXPECT_TRUE(WaitForClientShutdown()); } + bool DidListenerGetBadMessage() { + return listener_->bad_message_received_; + } + private: scoped_ptr thread_; scoped_ptr listener_; @@ -220,20 +275,19 @@ class IPCChannelProxyTest : public IPCTestBase { TEST_F(IPCChannelProxyTest, MessageClassFilters) { // Construct a filter per message class. std::vector > class_filters; - for (uint32 i = 0; i < LastIPCMsgStart; ++i) { - class_filters.push_back(make_scoped_refptr( - new MessageCountFilter(i))); - channel_proxy()->AddFilter(class_filters.back().get()); - } + class_filters.push_back(make_scoped_refptr( + new MessageCountFilter(TestMsgStart))); + class_filters.push_back(make_scoped_refptr( + new MessageCountFilter(UtilityMsgStart))); + for (size_t i = 0; i < class_filters.size(); ++i) + channel_proxy()->AddFilter(class_filters[i].get()); // Send a message for each class; each filter should receive just one message. - for (uint32 i = 0; i < LastIPCMsgStart; ++i) - Send(sender(), i, SEND); + sender()->Send(new TestMsg_Bounce()); + sender()->Send(new UtilityMsg_Bounce()); // Send some messages not assigned to a specific or valid message class. - Send(sender(), -1, SEND); - Send(sender(), LastIPCMsgStart, SEND); - Send(sender(), LastIPCMsgStart + 1, SEND); + sender()->Send(new WorkerMsg_Bounce); // Each filter should have received just the one sent message of the // corresponding class. @@ -244,9 +298,8 @@ TEST_F(IPCChannelProxyTest, MessageClassFilters) { TEST_F(IPCChannelProxyTest, GlobalAndMessageClassFilters) { // Add a class and global filter. - const int kMessageClass = 7; scoped_refptr class_filter( - new MessageCountFilter(kMessageClass)); + new MessageCountFilter(TestMsgStart)); class_filter->set_message_filtering_enabled(false); channel_proxy()->AddFilter(class_filter.get()); @@ -254,12 +307,12 @@ TEST_F(IPCChannelProxyTest, GlobalAndMessageClassFilters) { global_filter->set_message_filtering_enabled(false); channel_proxy()->AddFilter(global_filter.get()); - // A message of class |kMessageClass| should be seen by both the global - // filter and |kMessageClass|-specific filter. - Send(sender(), kMessageClass, SEND); + // A message of class Test should be seen by both the global filter and + // Test-specific filter. + sender()->Send(new TestMsg_Bounce); // A message of a different class should be seen only by the global filter. - Send(sender(), kMessageClass + 1, SEND); + sender()->Send(new UtilityMsg_Bounce); // Flush all messages. SendQuitMessageAndWaitForIdle(); @@ -267,16 +320,15 @@ TEST_F(IPCChannelProxyTest, GlobalAndMessageClassFilters) { // The class filter should have received only the class-specific message. EXPECT_EQ(1U, class_filter->messages_received()); - // The global filter should have received both SEND messages, as well as the - // final QUIT message. + // The global filter should have received both messages, as well as the final + // QUIT message. EXPECT_EQ(3U, global_filter->messages_received()); } TEST_F(IPCChannelProxyTest, FilterRemoval) { // Add a class and global filter. - const int kMessageClass = 7; scoped_refptr class_filter( - new MessageCountFilter(kMessageClass)); + new MessageCountFilter(TestMsgStart)); scoped_refptr global_filter(new MessageCountFilter()); // Add and remove both types of filters. @@ -286,8 +338,8 @@ TEST_F(IPCChannelProxyTest, FilterRemoval) { channel_proxy()->RemoveFilter(class_filter.get()); // Send some messages; they should not be seen by either filter. - Send(sender(), 0, SEND); - Send(sender(), kMessageClass, SEND); + sender()->Send(new TestMsg_Bounce); + sender()->Send(new UtilityMsg_Bounce); // Ensure that the filters were removed and did not receive any messages. SendQuitMessageAndWaitForIdle(); @@ -299,6 +351,77 @@ TEST_F(IPCChannelProxyTest, FilterRemoval) { EXPECT_EQ(0U, global_filter->messages_received()); } +// The test that follow trigger DCHECKS in debug build. +#if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON) + +TEST_F(IPCChannelProxyTest, BadMessageOnListenerThread) { + scoped_refptr class_filter( + new MessageCountFilter(TestMsgStart)); + class_filter->set_message_filtering_enabled(false); + channel_proxy()->AddFilter(class_filter.get()); + + sender()->Send(new TestMsg_SendBadMessage()); + + SendQuitMessageAndWaitForIdle(); + EXPECT_TRUE(DidListenerGetBadMessage()); +} + +TEST_F(IPCChannelProxyTest, BadMessageOnIPCThread) { + scoped_refptr class_filter( + new MessageCountFilter(TestMsgStart)); + class_filter->set_message_filtering_enabled(true); + channel_proxy()->AddFilter(class_filter.get()); + + sender()->Send(new TestMsg_SendBadMessage()); + + SendQuitMessageAndWaitForIdle(); + EXPECT_TRUE(DidListenerGetBadMessage()); +} + +class IPCChannelBadMessageTest : public IPCTestBase { + public: + IPCChannelBadMessageTest() {} + virtual ~IPCChannelBadMessageTest() {} + + virtual void SetUp() OVERRIDE { + IPCTestBase::SetUp(); + + Init("ChannelProxyClient"); + + listener_.reset(new QuitListener()); + CreateChannel(listener_.get()); + ASSERT_TRUE(ConnectChannel()); + + ASSERT_TRUE(StartClient()); + } + + virtual void TearDown() { + listener_.reset(); + IPCTestBase::TearDown(); + } + + void SendQuitMessageAndWaitForIdle() { + sender()->Send(new WorkerMsg_Quit); + base::MessageLoop::current()->Run(); + EXPECT_TRUE(WaitForClientShutdown()); + } + + bool DidListenerGetBadMessage() { + return listener_->bad_message_received_; + } + + private: + scoped_ptr listener_; +}; + +TEST_F(IPCChannelBadMessageTest, BadMessage) { + sender()->Send(new TestMsg_SendBadMessage()); + SendQuitMessageAndWaitForIdle(); + EXPECT_TRUE(DidListenerGetBadMessage()); +} + +#endif + MULTIPROCESS_IPC_TEST_CLIENT_MAIN(ChannelProxyClient) { base::MessageLoopForIO main_message_loop; ChannelReflectorListener listener; diff --git a/ipc/ipc_channel_proxy_unittest_messages.h b/ipc/ipc_channel_proxy_unittest_messages.h new file mode 100644 index 00000000000000..0108f1b7c78750 --- /dev/null +++ b/ipc/ipc_channel_proxy_unittest_messages.h @@ -0,0 +1,44 @@ +// 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 "ipc/ipc_message_macros.h" + +// Singly-included section for enums and custom IPC traits. +#ifndef IPC_CHANNEL_PROXY_UNITTEST_MESSAGES_H_ +#define IPC_CHANNEL_PROXY_UNITTEST_MESSAGES_H_ + +class BadType { + public: + BadType() {} +}; + +namespace IPC { + +template <> +struct ParamTraits { + static void Write(Message* m, const BadType& p) {} + static bool Read(const Message* m, PickleIterator* iter, BadType* r) { + return false; + } + static void Log(const BadType& p, std::string* l) {} +}; + +} + +#endif // IPC_CHANNEL_PROXY_UNITTEST_MESSAGES_H_ + + +#define IPC_MESSAGE_START TestMsgStart +IPC_MESSAGE_CONTROL0(TestMsg_Bounce) +IPC_MESSAGE_CONTROL0(TestMsg_SendBadMessage) +IPC_MESSAGE_CONTROL1(TestMsg_BadMessage, BadType) + +#undef IPC_MESSAGE_START +#define IPC_MESSAGE_START UtilityMsgStart +IPC_MESSAGE_CONTROL0(UtilityMsg_Bounce) + +#undef IPC_MESSAGE_START +#define IPC_MESSAGE_START WorkerMsgStart +IPC_MESSAGE_CONTROL0(WorkerMsg_Bounce) +IPC_MESSAGE_CONTROL0(WorkerMsg_Quit)