Fix SenderSocketFactory integration issues

This change fixes several problems with integrating SenderSocketFactory
into Chrome:
 - Errors in OnMessage that lead to the destruction of the CastSocket
   need to be deferred because CastSocket is the caller.
 - Fixes a use-after-move.
 - Adds audio-only flag to CastSocket after authentication completes for
   parity with Chrome.
 - Move util/logging.h use into cc file because of name collision in
   Chrome.

Bug: openscreen:59
Change-Id: Ic1b577315400109b4e57e2eb5149b0c9d7836b73
Reviewed-on: https://chromium-review.googlesource.com/c/openscreen/+/2051213
Commit-Queue: Brandon Tolsch <btolsch@chromium.org>
Reviewed-by: Ryan Keane <rwkeane@google.com>

3 files changed, 37 insertions, 15 deletions

diff --git a/cast/sender/BUILD.gn b/cast/sender/BUILD.gn
index fd295b92..dfbc947e 100644
--- a/cast/sender/BUILD.gn
+++ b/cast/sender/BUILD.gn
@@ -13,7 +13,6 @@ source_set("channel") {
   ]
 
   deps = [
-    "../../util",
     "../common:channel",
     "../common/certificate/proto:certificate_proto",
     "../common/channel/proto:channel_proto",
@@ -22,6 +21,7 @@ source_set("channel") {
   public_deps = [
     "../../platform",
     "../../third_party/boringssl",
+    "../../util",
     "../common:certificate",
     "../common:channel",
   ]
diff --git a/cast/sender/channel/sender_socket_factory.cc b/cast/sender/channel/sender_socket_factory.cc
index 6f8d9a51..8259685f 100644
--- a/cast/sender/channel/sender_socket_factory.cc
+++ b/cast/sender/channel/sender_socket_factory.cc
@@ -9,6 +9,7 @@
 #include "cast/sender/channel/message_util.h"
 #include "platform/base/tls_connect_options.h"
 #include "util/crypto/certificate_utils.h"
+#include "util/logging.h"
 
 using ::cast::channel::CastMessage;
 
@@ -25,12 +26,20 @@ bool operator<(int32_t a,
   return b && a < b->socket->socket_id();
 }
 
-SenderSocketFactory::SenderSocketFactory(Client* client) : client_(client) {
+SenderSocketFactory::SenderSocketFactory(Client* client,
+                                         TaskRunner* task_runner)
+    : client_(client), task_runner_(task_runner) {
   OSP_DCHECK(client);
+  OSP_DCHECK(task_runner);
 }
 
 SenderSocketFactory::~SenderSocketFactory() = default;
 
+void SenderSocketFactory::set_factory(TlsConnectionFactory* factory) {
+  OSP_DCHECK(factory);
+  factory_ = factory;
+}
+
 void SenderSocketFactory::Connect(const IPEndpoint& endpoint,
                                   DeviceMediaPolicy media_policy,
                                   CastSocket::Client* client) {
@@ -72,8 +81,8 @@ void SenderSocketFactory::OnConnected(
     return;
   }
 
-  auto socket = std::make_unique<CastSocket>(std::move(connection), this,
-                                             GetNextSocketId());
+  auto socket = MakeSerialDelete<CastSocket>(
+      task_runner_, std::move(connection), this, GetNextSocketId());
   pending_auth_.emplace_back(
       new PendingAuth{endpoint, media_policy, std::move(socket), client,
                       AuthContext::Create(), std::move(peer_cert.value())});
@@ -150,21 +159,26 @@ void SenderSocketFactory::OnMessage(CastSocket* socket, CastMessage message) {
   }
 
   ErrorOr<CastDeviceCertPolicy> policy_or_error = AuthenticateChallengeReply(
-      message, (*it)->peer_cert.get(), (*it)->auth_context);
+      message, pending->peer_cert.get(), pending->auth_context);
   if (policy_or_error.is_error()) {
+    OSP_DLOG_WARN << "Authentication failed for " << pending->endpoint
+                  << " with error: " << policy_or_error.error();
     client_->OnError(this, pending->endpoint, policy_or_error.error());
     return;
   }
 
   if (policy_or_error.value() == CastDeviceCertPolicy::kAudioOnly &&
-      pending->media_policy != DeviceMediaPolicy::kAudioOnly) {
+      pending->media_policy == DeviceMediaPolicy::kIncludesVideo) {
     client_->OnError(this, pending->endpoint,
                      Error::Code::kCastV2ChannelPolicyMismatch);
     return;
   }
+  pending->socket->set_audio_only(policy_or_error.value() ==
+                                  CastDeviceCertPolicy::kAudioOnly);
 
   pending->socket->SetClient(pending->client);
-  client_->OnConnected(this, pending->endpoint, std::move(pending->socket));
+  client_->OnConnected(this, pending->endpoint,
+                       std::unique_ptr<CastSocket>(pending->socket.release()));
 }
 
 }  // namespace cast
diff --git a/cast/sender/channel/sender_socket_factory.h b/cast/sender/channel/sender_socket_factory.h
index 094530dd..4b867e2c 100644
--- a/cast/sender/channel/sender_socket_factory.h
+++ b/cast/sender/channel/sender_socket_factory.h
@@ -14,9 +14,10 @@
 #include "cast/common/channel/cast_socket.h"
 #include "cast/common/channel/proto/cast_channel.pb.h"
 #include "cast/sender/channel/cast_auth_util.h"
+#include "platform/api/task_runner.h"
 #include "platform/api/tls_connection_factory.h"
 #include "platform/base/ip_address.h"
-#include "util/logging.h"
+#include "util/serial_delete_ptr.h"
 
 namespace openscreen {
 namespace cast {
@@ -35,19 +36,25 @@ class SenderSocketFactory final : public TlsConnectionFactory::Client,
   };
 
   enum class DeviceMediaPolicy {
+    kNone = 0,
     kAudioOnly,
     kIncludesVideo,
   };
 
-  // |client| must outlive |this|.
-  explicit SenderSocketFactory(Client* client);
+  // |client| and |task_runner| must outlive |this|.
+  SenderSocketFactory(Client* client, TaskRunner* task_runner);
   ~SenderSocketFactory();
 
-  void set_factory(TlsConnectionFactory* factory) {
-    OSP_DCHECK(factory);
-    factory_ = factory;
-  }
+  // |factory| cannot be nullptr and must outlive |this|.
+  void set_factory(TlsConnectionFactory* factory);
 
+  // Begins connecting to a Cast device at |endpoint|.  If a successful
+  // connection is made, including device authentication, the new CastSocket
+  // will be passed to |client_|'s OnConnected method.  The new CastSocket will
+  // have its client set to |client|.  If any part of the connection process
+  // fails, |client_|'s OnError method is called instead.  This includes if the
+  // device's media policy, as determined by authentication, is audio-only and
+  // |media_policy| is kIncludesVideo.
   void Connect(const IPEndpoint& endpoint,
                DeviceMediaPolicy media_policy,
                CastSocket::Client* client);
@@ -73,7 +80,7 @@ class SenderSocketFactory final : public TlsConnectionFactory::Client,
   struct PendingAuth {
     IPEndpoint endpoint;
     DeviceMediaPolicy media_policy;
-    std::unique_ptr<CastSocket> socket;
+    SerialDeletePtr<CastSocket> socket;
     CastSocket::Client* client;
     AuthContext auth_context;
     bssl::UniquePtr<X509> peer_cert;
@@ -91,6 +98,7 @@ class SenderSocketFactory final : public TlsConnectionFactory::Client,
                  ::cast::channel::CastMessage message) override;
 
   Client* const client_;
+  TaskRunner* const task_runner_;
   TlsConnectionFactory* factory_ = nullptr;
   std::vector<PendingConnection> pending_connections_;
   std::vector<std::unique_ptr<PendingAuth>> pending_auth_;