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 <jophba@chromium.org>
Commit-Queue: Yuri Wiitala <miu@chromium.org>

8 files changed, 82 insertions, 32 deletions

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<const uint8_t> 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 <vector>
 
 #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 <utility>
 
+#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<Json::Value>& message_or_error) {
 
 class ReceiverSessionTest : public ::testing::Test {
  public:
-  ReceiverSessionTest()
-      : clock_(Clock::time_point{}),
-        task_runner_(&clock_),
-        env_(std::make_unique<Environment>(&FakeClock::now,
-                                           &task_runner_,
-                                           IPEndpoint{})) {}
+  ReceiverSessionTest() : clock_(Clock::time_point{}), task_runner_(&clock_) {}
+
+  std::unique_ptr<MockEnvironment> MakeEnvironment() {
+    auto environment = std::make_unique<NiceMock<MockEnvironment>>(
+        &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<Environment> env_;
 };
 
 TEST_F(ReceiverSessionTest, RegistersSelfOnMessagePump) {
@@ -241,7 +249,7 @@ TEST_F(ReceiverSessionTest, RegistersSelfOnMessagePump) {
   StrictMock<FakeClient> client;
 
   auto session = std::make_unique<ReceiverSession>(
-      &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>();
   SimpleMessagePort* raw_port = message_port.get();
   StrictMock<FakeClient> 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<FakeClient> 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>();
   SimpleMessagePort* raw_port = message_port.get();
   StrictMock<FakeClient> 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>();
   SimpleMessagePort* raw_port = message_port.get();
   StrictMock<FakeClient> 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>();
   SimpleMessagePort* raw_port = message_port.get();
   StrictMock<FakeClient> 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>();
   SimpleMessagePort* raw_port = message_port.get();
   StrictMock<FakeClient> 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>();
   SimpleMessagePort* raw_port = message_port.get();
   StrictMock<FakeClient> 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<const uint8_t> packet));
-};
-
 class MockConsumer : public Receiver::Consumer {
  public:
   MOCK_METHOD1(OnFramesReady, void(int next_frame_buffer_size));