Modify PeerConnection for end-to-end QuicDataChannel usage

webrtc/p2p/quic/quictransportchannel.cc

Index: webrtc/p2p/quic/quictransportchannel.cc
diff --git a/webrtc/p2p/quic/quictransportchannel.cc b/webrtc/p2p/quic/quictransportchannel.cc
index 7e68612388b85e85b5ca522a64c0a91f3b354375..e366e83501f20450e115b1954d511e175a67c716 100644
--- a/webrtc/p2p/quic/quictransportchannel.cc
+++ b/webrtc/p2p/quic/quictransportchannel.cc
@@ -123,6 +123,7 @@ namespace cricket {
 
 QuicTransportChannel::QuicTransportChannel(TransportChannelImpl* channel)
     : TransportChannelImpl(channel->transport_name(), channel->component()),
+      Transport("", nullptr),

[pthatcher1, 2016/03/30 20:34:50]

Why not use channel->transport_name() for the transport name?

[mikescarlett, 2016/04/05 19:58:53]

Transport isn't subclassed anymore

       worker_thread_(rtc::Thread::Current()),
       channel_(channel),
       helper_(worker_thread_) {
@@ -153,9 +154,15 @@ QuicTransportChannel::QuicTransportChannel(TransportChannelImpl* channel)
       net::QuicTime::Delta::FromSeconds(kIdleConnectionStateLifetime));
   // Set the bytes reserved for the QUIC connection ID to zero.
   config_.SetBytesForConnectionIdToSend(0);
+  // Provide the Transport base class with a pointer to this channel by
+  // implicitly triggering Transport::CreateTransportChannel.
+  Transport::CreateChannel(0);
 }
 
-QuicTransportChannel::~QuicTransportChannel() {}
+QuicTransportChannel::~QuicTransportChannel() {
+  // Required to avoid assertion error in Transport::~Transport().
+  Transport::DestroyAllChannels();
+}
 
 bool QuicTransportChannel::SetLocalCertificate(
     const rtc::scoped_refptr<rtc::RTCCertificate>& certificate) {
@@ -441,6 +448,8 @@ bool QuicTransportChannel::CreateQuicSession() {
       this, &QuicTransportChannel::OnHandshakeComplete);
   quic_->SignalConnectionClosed.connect(
       this, &QuicTransportChannel::OnConnectionClosed);
+  quic_->SignalIncomingStream.connect(this,
+                                      &QuicTransportChannel::OnIncomingStream);
   return true;
 }
 
@@ -539,6 +548,7 @@ void QuicTransportChannel::OnConnectionClosed(net::QuicErrorCode error,
   // does not close due to failure.
   set_quic_state(QUIC_TRANSPORT_CLOSED);
   set_writable(false);
+  SignalClosed();
 }
 
 void QuicTransportChannel::OnProofValid(
@@ -567,4 +577,116 @@ void QuicTransportChannel::set_quic_state(QuicTransportState state) {
   quic_state_ = state;
 }
 
+bool QuicTransportChannel::NegotiateTransportDescription(
+    ContentAction local_role,
+    std::string* error_desc) {
+  if (!local_description() || !remote_description()) {
+    const std::string msg =
+        "Local and Remote description must be set before "
+        "transport descriptions are negotiated";
+    return BadTransportDescription(msg, error_desc);
+  }
+
+  rtc::SSLFingerprint* remote_fp =
+      remote_description()->identity_fingerprint.get();
+
+  if (remote_fp) {
+    if (!SetRemoteFingerprint(
+            remote_fp->algorithm,
+            reinterpret_cast<const uint8_t*>(remote_fp->digest.data()),
+            remote_fp->digest.size())) {
+      return BadTransportDescription("Failed to apply remote fingerprint.",
+                                     error_desc);
+    }
+  } else {
+    return BadTransportDescription(
+        "Local fingerprint supplied when caller didn't offer DTLS.",
+        error_desc);
+  }
+
+  // From RFC 4145, section-4.1, The following are the values that the
+  // 'setup' attribute can take in an offer/answer exchange:
+  //       Offer      Answer
+  //      ________________
+  //      active     passive / holdconn
+  //      passive    active / holdconn
+  //      actpass    active / passive / holdconn
+  //      holdconn   holdconn
+  //
+  // Set the role that is most conformant with RFC 5763, Section 5, bullet 1
+  // The endpoint MUST use the setup attribute defined in [RFC4145].
+  // The endpoint that is the offerer MUST use the setup attribute
+  // value of setup:actpass and be prepared to receive a client_hello
+  // before it receives the answer.  The answerer MUST use either a
+  // setup attribute value of setup:active or setup:passive.  Note that
+  // if the answerer uses setup:passive, then the DTLS handshake will
+  // not begin until the answerer is received, which adds additional
+  // latency. setup:active allows the answer and the DTLS handshake to
+  // occur in parallel.  Thus, setup:active is RECOMMENDED.  Whichever
+  // party is active MUST initiate a DTLS handshake by sending a
+  // ClientHello over each flow (host/port quartet).
+  // IOW - actpass and passive modes should be treated as server and
+  // active as client.

[pthatcher1, 2016/03/30 20:34:50]

Can you just delete this whole comment and write the following:

TODO(pthatcher): Move all the negotiation up into PeerConnection and out of the
Transport code.  Just have a SetRole or similar method here.

[mikescarlett, 2016/04/05 19:58:53]

Done. See QuicTransport in https://codereview.webrtc.org/1856943002.

+  ConnectionRole local_connection_role = local_description()->connection_role;
+  ConnectionRole remote_connection_role = remote_description()->connection_role;
+
+  bool is_remote_server = false;
+  if (local_role == CA_OFFER) {
+    if (local_connection_role != CONNECTIONROLE_ACTPASS) {
+      return BadTransportDescription(
+          "Offerer must use actpass value for setup attribute.", error_desc);
+    }
+
+    if (remote_connection_role == CONNECTIONROLE_ACTIVE ||
+        remote_connection_role == CONNECTIONROLE_PASSIVE ||
+        remote_connection_role == CONNECTIONROLE_NONE) {
+      is_remote_server = (remote_connection_role == CONNECTIONROLE_PASSIVE);
+    } else {
+      const std::string msg =
+          "Answerer must use either active or passive value "
+          "for setup attribute.";
+      return BadTransportDescription(msg, error_desc);
+    }
+    // If remote is NONE or ACTIVE it will act as client.
+  } else {
+    if (remote_connection_role != CONNECTIONROLE_ACTPASS &&
+        remote_connection_role != CONNECTIONROLE_NONE) {
+      return BadTransportDescription(
+          "Offerer must use actpass value for setup attribute.", error_desc);
+    }
+
+    if (local_connection_role == CONNECTIONROLE_ACTIVE ||
+        local_connection_role == CONNECTIONROLE_PASSIVE) {
+      is_remote_server = (local_connection_role == CONNECTIONROLE_ACTIVE);
+    } else {
+      const std::string msg =
+          "Answerer must use either active or passive value "
+          "for setup attribute.";
+      return BadTransportDescription(msg, error_desc);
+    }
+
+    // If local is passive, local will act as server.
+  }
+
+  rtc::SSLRole ssl_role = is_remote_server ? rtc::SSL_CLIENT : rtc::SSL_SERVER;
+  if (!SetSslRole(ssl_role)) {
+    return BadTransportDescription("Failed to set ssl role for the channel.",
+                                   error_desc);
+  }
+
+  // Now run the negotiation for the base class.
+  return Transport::NegotiateTransportDescription(local_role, error_desc);
+}
+
+ReliableQuicStream* QuicTransportChannel::CreateQuicStream() {

[pthatcher1, 2016/03/30 20:34:50]

Can you make it clear that the 0 here is the priority?

[mikescarlett, 2016/04/05 19:58:53]

Done.

+  if (quic_) {
+    return quic_->CreateOutgoingDynamicStream(0);
+  }
+  return nullptr;
+}
+
+void QuicTransportChannel::OnIncomingStream(ReliableQuicStream* stream) {
+  SignalIncomingStream(stream);
+}
+
 }  // namespace cricket