From b0b4bb3063d8b1d161167f183fdeb538acba48b7 Mon Sep 17 00:00:00 2001 From: Yuri Wiitala Date: Tue, 11 Feb 2020 17:09:31 -0800 Subject: Crash fix: Prevent a unit test from using a real UDP socket. The code in receiver_session_unittest.cc was creating an Environment instance that was binding a real UDP socket. Also, when the test was running in a Chrome embedder build of the openscreen_unittests, the result was a segfault because a full browser environment was not present. To fix this, a MockEnvironment class (subclass of Environment) is used to intercept socket calls and provide a fake local address/port for the tests to work with. Change-Id: I59a06ed12b58a1ba14d228cad7cd8dfffafc9bb2 Reviewed-on: https://chromium-review.googlesource.com/c/openscreen/+/2051319 Reviewed-by: Jordan Bayles Commit-Queue: Yuri Wiitala --- cast/streaming/BUILD.gn | 2 ++ cast/streaming/environment.h | 5 +++- cast/streaming/mock_environment.cc | 17 ++++++++++++ cast/streaming/mock_environment.h | 30 ++++++++++++++++++++++ cast/streaming/receiver_session.cc | 2 ++ cast/streaming/receiver_session.h | 4 +-- cast/streaming/receiver_session_unittest.cc | 40 +++++++++++++++++------------ cast/streaming/receiver_unittest.cc | 14 +--------- 8 files changed, 82 insertions(+), 32 deletions(-) create mode 100644 cast/streaming/mock_environment.cc create mode 100644 cast/streaming/mock_environment.h diff --git a/cast/streaming/BUILD.gn b/cast/streaming/BUILD.gn index 851f597d..ce7068af 100644 --- a/cast/streaming/BUILD.gn +++ b/cast/streaming/BUILD.gn @@ -111,6 +111,8 @@ source_set("unittests") { "frame_collector_unittest.cc", "frame_crypto_unittest.cc", "mock_compound_rtcp_parser_client.h", + "mock_environment.cc", + "mock_environment.h", "ntp_time_unittest.cc", "offer_messages_unittest.cc", "packet_receive_stats_tracker_unittest.cc", diff --git a/cast/streaming/environment.h b/cast/streaming/environment.h index 681715dd..278604b0 100644 --- a/cast/streaming/environment.h +++ b/cast/streaming/environment.h @@ -47,7 +47,10 @@ class Environment : public UdpSocket::Client { // Returns the local endpoint the socket is bound to, or the zero IPEndpoint // if socket creation/binding failed. - IPEndpoint GetBoundLocalEndpoint() const; + // + // Note: This method is virtual to allow unit tests to fake that there really + // is a bound socket. + virtual IPEndpoint GetBoundLocalEndpoint() const; // Set a handler function to run whenever non-recoverable socket errors occur. // If never set, the default is to emit log messages at error priority. diff --git a/cast/streaming/mock_environment.cc b/cast/streaming/mock_environment.cc new file mode 100644 index 00000000..9d712537 --- /dev/null +++ b/cast/streaming/mock_environment.cc @@ -0,0 +1,17 @@ +// 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 "cast/streaming/mock_environment.h" + +namespace openscreen { +namespace cast { + +MockEnvironment::MockEnvironment(ClockNowFunctionPtr now_function, + TaskRunner* task_runner) + : Environment(now_function, task_runner) {} + +MockEnvironment::~MockEnvironment() = default; + +} // namespace cast +} // namespace openscreen diff --git a/cast/streaming/mock_environment.h b/cast/streaming/mock_environment.h new file mode 100644 index 00000000..db66b141 --- /dev/null +++ b/cast/streaming/mock_environment.h @@ -0,0 +1,30 @@ +// 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. + +#ifndef CAST_STREAMING_MOCK_ENVIRONMENT_H_ +#define CAST_STREAMING_MOCK_ENVIRONMENT_H_ + +#include "cast/streaming/environment.h" +#include "gmock/gmock.h" + +namespace openscreen { +namespace cast { + +// An Environment that can intercept all packet sends, for unit testing. +class MockEnvironment : public Environment { + public: + MockEnvironment(ClockNowFunctionPtr now_function, TaskRunner* task_runner); + ~MockEnvironment() override; + + // Used to return fake values, to simulate a bound socket for testing. + MOCK_METHOD(IPEndpoint, GetBoundLocalEndpoint, (), (const, override)); + + // Used for intercepting packet sends from the implementation under test. + MOCK_METHOD(void, SendPacket, (absl::Span packet), (override)); +}; + +} // namespace cast +} // namespace openscreen + +#endif // CAST_STREAMING_MOCK_ENVIRONMENT_H_ diff --git a/cast/streaming/receiver_session.cc b/cast/streaming/receiver_session.cc index be9ae000..1c8d9d9f 100644 --- a/cast/streaming/receiver_session.cc +++ b/cast/streaming/receiver_session.cc @@ -10,9 +10,11 @@ #include "absl/strings/match.h" #include "absl/strings/numbers.h" +#include "cast/streaming/environment.h" #include "cast/streaming/message_port.h" #include "cast/streaming/message_util.h" #include "cast/streaming/offer_messages.h" +#include "cast/streaming/receiver.h" #include "util/logging.h" namespace openscreen { diff --git a/cast/streaming/receiver_session.h b/cast/streaming/receiver_session.h index 15a67de7..44954cac 100644 --- a/cast/streaming/receiver_session.h +++ b/cast/streaming/receiver_session.h @@ -10,10 +10,8 @@ #include #include "cast/streaming/answer_messages.h" -#include "cast/streaming/environment.h" #include "cast/streaming/message_port.h" #include "cast/streaming/offer_messages.h" -#include "cast/streaming/receiver.h" #include "cast/streaming/receiver_packet_router.h" #include "cast/streaming/session_config.h" #include "util/json/json_serialization.h" @@ -22,6 +20,8 @@ namespace openscreen { namespace cast { class CastSocket; +class Environment; +class Receiver; class VirtualConnectionRouter; class VirtualConnection; diff --git a/cast/streaming/receiver_session_unittest.cc b/cast/streaming/receiver_session_unittest.cc index 275047e9..f05f6e31 100644 --- a/cast/streaming/receiver_session_unittest.cc +++ b/cast/streaming/receiver_session_unittest.cc @@ -6,13 +6,17 @@ #include +#include "cast/streaming/mock_environment.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "platform/base/ip_address.h" #include "platform/test/fake_clock.h" #include "platform/test/fake_task_runner.h" using ::testing::_; using ::testing::Invoke; +using ::testing::NiceMock; +using ::testing::Return; using ::testing::StrictMock; namespace openscreen { @@ -221,16 +225,20 @@ void ExpectIsErrorAnswerMessage(const ErrorOr& message_or_error) { class ReceiverSessionTest : public ::testing::Test { public: - ReceiverSessionTest() - : clock_(Clock::time_point{}), - task_runner_(&clock_), - env_(std::make_unique(&FakeClock::now, - &task_runner_, - IPEndpoint{})) {} + ReceiverSessionTest() : clock_(Clock::time_point{}), task_runner_(&clock_) {} + + std::unique_ptr MakeEnvironment() { + auto environment = std::make_unique>( + &FakeClock::now, &task_runner_); + ON_CALL(*environment, GetBoundLocalEndpoint()) + .WillByDefault( + Return(IPEndpoint{IPAddress::Parse("127.0.0.1").value(), 12345})); + return environment; + } + private: FakeClock clock_; FakeTaskRunner task_runner_; - std::unique_ptr env_; }; TEST_F(ReceiverSessionTest, RegistersSelfOnMessagePump) { @@ -241,7 +249,7 @@ TEST_F(ReceiverSessionTest, RegistersSelfOnMessagePump) { StrictMock client; auto session = std::make_unique( - &client, std::move(env_), std::move(message_port), + &client, MakeEnvironment(), std::move(message_port), ReceiverSession::Preferences{}); EXPECT_EQ(raw_port->client(), session.get()); } @@ -250,7 +258,7 @@ TEST_F(ReceiverSessionTest, CanNegotiateWithDefaultPreferences) { auto message_port = std::make_unique(); SimpleMessagePort* raw_port = message_port.get(); StrictMock client; - ReceiverSession session(&client, std::move(env_), std::move(message_port), + ReceiverSession session(&client, MakeEnvironment(), std::move(message_port), ReceiverSession::Preferences{}); EXPECT_CALL(client, OnNegotiated(&session, _)) @@ -322,7 +330,7 @@ TEST_F(ReceiverSessionTest, CanNegotiateWithCustomCodecPreferences) { SimpleMessagePort* raw_port = message_port.get(); StrictMock client; ReceiverSession session( - &client, std::move(env_), std::move(message_port), + &client, MakeEnvironment(), std::move(message_port), ReceiverSession::Preferences{{ReceiverSession::VideoCodec::kVp9}, {ReceiverSession::AudioCodec::kOpus}}); @@ -362,7 +370,7 @@ TEST_F(ReceiverSessionTest, CanNegotiateWithCustomConstraints) { AspectRatioConstraint::kFixed}}; ReceiverSession session( - &client, std::move(env_), std::move(message_port), + &client, MakeEnvironment(), std::move(message_port), ReceiverSession::Preferences{{ReceiverSession::VideoCodec::kVp9}, {ReceiverSession::AudioCodec::kOpus}, std::move(constraints), @@ -422,7 +430,7 @@ TEST_F(ReceiverSessionTest, HandlesNoValidAudioStream) { auto message_port = std::make_unique(); SimpleMessagePort* raw_port = message_port.get(); StrictMock client; - ReceiverSession session(&client, std::move(env_), std::move(message_port), + ReceiverSession session(&client, MakeEnvironment(), std::move(message_port), ReceiverSession::Preferences{}); EXPECT_CALL(client, OnNegotiated(&session, _)).Times(1); @@ -448,7 +456,7 @@ TEST_F(ReceiverSessionTest, HandlesNoValidVideoStream) { auto message_port = std::make_unique(); SimpleMessagePort* raw_port = message_port.get(); StrictMock client; - ReceiverSession session(&client, std::move(env_), std::move(message_port), + ReceiverSession session(&client, MakeEnvironment(), std::move(message_port), ReceiverSession::Preferences{}); EXPECT_CALL(client, OnNegotiated(&session, _)).Times(1); @@ -474,7 +482,7 @@ TEST_F(ReceiverSessionTest, HandlesNoValidStreams) { auto message_port = std::make_unique(); SimpleMessagePort* raw_port = message_port.get(); StrictMock client; - ReceiverSession session(&client, std::move(env_), std::move(message_port), + ReceiverSession session(&client, MakeEnvironment(), std::move(message_port), ReceiverSession::Preferences{}); // We shouldn't call OnNegotiated if we failed to negotiate any streams. @@ -493,7 +501,7 @@ TEST_F(ReceiverSessionTest, HandlesMalformedOffer) { auto message_port = std::make_unique(); SimpleMessagePort* raw_port = message_port.get(); StrictMock client; - ReceiverSession session(&client, std::move(env_), std::move(message_port), + ReceiverSession session(&client, MakeEnvironment(), std::move(message_port), ReceiverSession::Preferences{}); // We shouldn't call OnNegotiated if we failed to negotiate any streams. @@ -511,7 +519,7 @@ TEST_F(ReceiverSessionTest, NotifiesReceiverDestruction) { auto message_port = std::make_unique(); SimpleMessagePort* raw_port = message_port.get(); StrictMock client; - ReceiverSession session(&client, std::move(env_), std::move(message_port), + ReceiverSession session(&client, MakeEnvironment(), std::move(message_port), ReceiverSession::Preferences{}); EXPECT_CALL(client, OnNegotiated(&session, _)).Times(2); diff --git a/cast/streaming/receiver_unittest.cc b/cast/streaming/receiver_unittest.cc index 31515279..5445d77e 100644 --- a/cast/streaming/receiver_unittest.cc +++ b/cast/streaming/receiver_unittest.cc @@ -15,6 +15,7 @@ #include "cast/streaming/constants.h" #include "cast/streaming/encoded_frame.h" #include "cast/streaming/frame_crypto.h" +#include "cast/streaming/mock_environment.h" #include "cast/streaming/receiver_packet_router.h" #include "cast/streaming/rtcp_common.h" #include "cast/streaming/rtcp_session.h" @@ -251,19 +252,6 @@ class MockSender : public CompoundRtcpParser::Client { EncryptedFrame frame_being_sent_; }; -// An Environment that can intercept all packet sends. ReceiverTest will connect -// the SendPacket() method calls to the MockSender. -class MockEnvironment : public Environment { - public: - MockEnvironment(ClockNowFunctionPtr now_function, TaskRunner* task_runner) - : Environment(now_function, task_runner) {} - - ~MockEnvironment() override = default; - - // Used for intercepting packet sends from the implementation under test. - MOCK_METHOD1(SendPacket, void(absl::Span packet)); -}; - class MockConsumer : public Receiver::Consumer { public: MOCK_METHOD1(OnFramesReady, void(int next_frame_buffer_size)); -- cgit v1.2.3