Bug 1455647 - Part 2: Allow TransportLayers to be arranged in trees. r?drno draft
authorByron Campen [:bwc] <docfaraday@gmail.com>
Fri, 27 Apr 2018 16:57:28 -0500
changeset 806511 1f569a18b1ced688d2de0a3ba8d477ecafe50558
parent 806510 eec9ea9fa7b158251f01090b086433ef45f4745d
child 806512 4c9d2ddc79745326f6fbcfb9d230a51e0417f06b
push id112901
push userbcampen@mozilla.com
push dateSun, 10 Jun 2018 18:18:51 +0000
reviewersdrno
bugs1455647
milestone62.0a1
Bug 1455647 - Part 2: Allow TransportLayers to be arranged in trees. r?drno MozReview-Commit-ID: BQgNbCsmkke
media/mtransport/test/sctp_unittest.cpp
media/mtransport/test/transport_unittests.cpp
media/mtransport/transportflow.cpp
media/mtransport/transportflow.h
media/mtransport/transportlayer.cpp
media/mtransport/transportlayer.h
media/mtransport/transportlayerdtls.h
media/webrtc/signaling/gtest/mediapipeline_unittest.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
--- a/media/mtransport/test/sctp_unittest.cpp
+++ b/media/mtransport/test/sctp_unittest.cpp
@@ -134,18 +134,18 @@ class TransportTestPeer : public sigslot
   void ConnectSocket(TransportTestPeer *peer) {
     test_utils_->sts_target()->Dispatch(WrapRunnable(
         this, &TransportTestPeer::ConnectSocket_s, peer),
                                        NS_DISPATCH_SYNC);
   }
 
   void ConnectSocket_s(TransportTestPeer *peer) {
     loopback_->Connect(peer->loopback_);
-
-    ASSERT_EQ((nsresult)NS_OK, flow_->PushLayer(loopback_));
+    ASSERT_EQ((nsresult)NS_OK, loopback_->Init());
+    flow_->PushLayer(loopback_);
 
     loopback_->SignalPacketReceived.connect(this, &TransportTestPeer::PacketReceived);
 
     // SCTP here!
     ASSERT_TRUE(sctp_);
     std::cerr << "Calling usrsctp_bind()" << std::endl;
     int r = usrsctp_bind(sctp_, reinterpret_cast<struct sockaddr *>(
         &local_addr_), sizeof(local_addr_));
@@ -284,16 +284,17 @@ class TransportTestPeer : public sigslot
   }
 
 
  private:
   std::string name_;
   bool connected_;
   size_t sent_;
   size_t received_;
+  // Owns the TransportLayerLoopback, but basically does nothing else.
   RefPtr<TransportFlow> flow_;
   TransportLayerLoopback *loopback_;
 
   struct sockaddr_conn local_addr_;
   struct sockaddr_conn remote_addr_;
   struct socket *sctp_;
   nsCOMPtr<nsITimer> timer_;
   RefPtr<SendPeriodic> periodic_;
--- a/media/mtransport/test/transport_unittests.cpp
+++ b/media/mtransport/test/transport_unittests.cpp
@@ -547,21 +547,28 @@ class TransportTestPeer : public sigslot
   }
 
   void ConnectSocket_s(TransportTestPeer *peer) {
     nsresult res;
     res = loopback_->Init();
     ASSERT_EQ((nsresult)NS_OK, res);
 
     loopback_->Connect(peer->loopback_);
+    ASSERT_EQ((nsresult)NS_OK, loopback_->Init());
+    ASSERT_EQ((nsresult)NS_OK, logging_->Init());
+    ASSERT_EQ((nsresult)NS_OK, lossy_->Init());
+    ASSERT_EQ((nsresult)NS_OK, dtls_->Init());
+    dtls_->Chain(lossy_);
+    lossy_->Chain(logging_);
+    logging_->Chain(loopback_);
 
-    ASSERT_EQ((nsresult)NS_OK, flow_->PushLayer(loopback_));
-    ASSERT_EQ((nsresult)NS_OK, flow_->PushLayer(logging_));
-    ASSERT_EQ((nsresult)NS_OK, flow_->PushLayer(lossy_));
-    ASSERT_EQ((nsresult)NS_OK, flow_->PushLayer(dtls_));
+    flow_->PushLayer(loopback_);
+    flow_->PushLayer(logging_);
+    flow_->PushLayer(lossy_);
+    flow_->PushLayer(dtls_);
 
     if (dtls_->state() != TransportLayer::TS_ERROR) {
       // Don't execute these blocks if DTLS didn't initialize.
       TweakCiphers(dtls_->internal_fd());
       if (reuse_dhe_key_) {
         // TransportLayerDtls automatically sets this pref to false
         // so set it back for test.
         // This is pretty gross. Dig directly into the NSS FD. The problem
@@ -587,16 +594,27 @@ class TransportTestPeer : public sigslot
 
   void ConnectSocket(TransportTestPeer *peer) {
     RUN_ON_THREAD(test_utils_->sts_target(),
                   WrapRunnable(this, & TransportTestPeer::ConnectSocket_s,
                                peer),
                   NS_DISPATCH_SYNC);
   }
 
+  nsresult InitIce_s() {
+    nsresult rv = ice_->Init();
+    NS_ENSURE_SUCCESS(rv, rv);
+    rv = dtls_->Init();
+    NS_ENSURE_SUCCESS(rv, rv);
+    dtls_->Chain(ice_);
+    flow_->PushLayer(ice_);
+    flow_->PushLayer(dtls_);
+    return NS_OK;
+  }
+
   void InitIce() {
     nsresult res;
 
     // Attach our slots
     ice_ctx_->ctx()->SignalGatheringStateChange.
         connect(this, &TransportTestPeer::GatheringStateChange);
 
     char name[100];
@@ -615,21 +633,17 @@ class TransportTestPeer : public sigslot
     stream->SignalCandidate.
         connect(this, &TransportTestPeer::GotCandidate);
 
     // Create the transport layer
     ice_ = new TransportLayerIce();
     ice_->SetParameters(stream, 1);
 
     test_utils_->sts_target()->Dispatch(
-      WrapRunnable(flow_, &TransportFlow::PushLayer, ice_),
-      NS_DISPATCH_SYNC);
-
-    test_utils_->sts_target()->Dispatch(
-      WrapRunnableRet(&res, flow_, &TransportFlow::PushLayer, dtls_),
+      WrapRunnableRet(&res, this, &TransportTestPeer::InitIce_s),
       NS_DISPATCH_SYNC);
 
     ASSERT_EQ((nsresult)NS_OK, res);
 
     // Listen for media events
     dtls_->SignalPacketReceived.connect(this, &TransportTestPeer::PacketReceived);
     dtls_->SignalStateChange.connect(this, &TransportTestPeer::StateChanged);
 
@@ -1284,28 +1298,9 @@ TEST_F(TransportTest, TestDheOnlyFails) 
   SetDtlsPeer();
 
   // p2_ is the client
   // setting this on p1_ (the server) causes NSS to assert
   ConfigureOneCipher(p2_, TLS_DHE_RSA_WITH_AES_128_CBC_SHA);
   ConnectSocketExpectFail();
 }
 
-TEST(PushTests, LayerFail) {
-  RefPtr<TransportFlow> flow = new TransportFlow();
-  nsresult rv;
-  bool destroyed1, destroyed2;
-
-  rv = flow->PushLayer(new TransportLayerDummy(true, &destroyed1));
-  ASSERT_TRUE(NS_SUCCEEDED(rv));
-
-  rv = flow->PushLayer(new TransportLayerDummy(false, &destroyed2));
-  ASSERT_TRUE(NS_FAILED(rv));
-
-  ASSERT_EQ(true, destroyed1);
-  ASSERT_EQ(true, destroyed2);
-
-  rv = flow->PushLayer(new TransportLayerDummy(true, &destroyed1));
-  ASSERT_TRUE(NS_FAILED(rv));
-  ASSERT_EQ(true, destroyed1);
-}
-
 }  // end namespace
--- a/media/mtransport/transportflow.cpp
+++ b/media/mtransport/transportflow.cpp
@@ -9,18 +9,16 @@
 
 #include "logging.h"
 #include "runnable_utils.h"
 #include "transportflow.h"
 #include "transportlayer.h"
 
 namespace mozilla {
 
-MOZ_MTLOG_MODULE("mtransport")
-
 NS_IMPL_ISUPPORTS0(TransportFlow)
 
 // There are some hacks here to allow destruction off of
 // the main thread.
 TransportFlow::~TransportFlow() {
   // Push the destruction onto the STS thread. Note that there
   // is still some possibility that someone is accessing this
   // object simultaneously, but as long as smart pointer discipline
@@ -42,42 +40,21 @@ void TransportFlow::DestroyFinal(nsAutoP
 
 void TransportFlow::ClearLayers(std::deque<TransportLayer *>* layers) {
   while (!layers->empty()) {
     delete layers->front();
     layers->pop_front();
   }
 }
 
-nsresult TransportFlow::PushLayer(TransportLayer* layer) {
+void TransportFlow::PushLayer(TransportLayer* layer) {
   CheckThread();
-  if (!layers_) {
-    return NS_ERROR_FAILURE;
-  }
-
-  nsresult rv = layer->Init();
-  if (!NS_SUCCEEDED(rv)) {
-    MOZ_MTLOG(ML_ERROR, id_ << ": Layer initialization failed");
-    delete layer;
-
-    // Now destroy the rest of the flow, because it's no longer
-    // in an acceptable state.
-    ClearLayers(layers_.get());
-    layers_.reset();
-
-    return rv;
-  }
-
-  TransportLayer *old_layer = layers_->empty() ? nullptr : layers_->front();
   layers_->push_front(layer);
   EnsureSameThread(layer);
-
-  layer->Inserted(this, old_layer);
-
-  return NS_OK;
+  layer->SetFlowId(id_);
 }
 
 TransportLayer *TransportFlow::GetLayer(const std::string& id) const {
   CheckThread();
 
   if (layers_) {
     for (TransportLayer* layer : *layers_) {
       if (layer->id() == id)
--- a/media/mtransport/transportflow.h
+++ b/media/mtransport/transportflow.h
@@ -61,17 +61,17 @@ class TransportFlow final : public nsISu
 
   // Layer management. Note PushLayer() is not thread protected, so
   // either:
   // (a) Do it in the thread handling the I/O
   // (b) Do it before you activate the I/O system
   //
   // The flow takes ownership of the layers after a successful
   // push.
-  nsresult PushLayer(TransportLayer* layer);
+  void PushLayer(TransportLayer* layer);
 
   TransportLayer *GetLayer(const std::string& id) const;
 
   NS_DECL_THREADSAFE_ISUPPORTS
 
  private:
   ~TransportFlow();
 
--- a/media/mtransport/transportlayer.cpp
+++ b/media/mtransport/transportlayer.cpp
@@ -25,19 +25,18 @@ nsresult TransportLayer::Init() {
     state_ = TS_ERROR;
     return rv;
   }
   state_ = TS_INIT;
 
   return NS_OK;
 }
 
-void TransportLayer::Inserted(TransportFlow *flow, TransportLayer *downward) {
+void TransportLayer::Chain(TransportLayer *downward) {
   downward_ = downward;
-  flow_id_ = flow->id();
   MOZ_MTLOG(ML_DEBUG, LAYER_INFO << "Inserted: downward='" <<
     (downward ? downward->id(): "none") << "'");
 
   WasInserted();
 }
 
 void TransportLayer::SetState(State state, const char *file, unsigned line) {
   if (state != state_) {
--- a/media/mtransport/transportlayer.h
+++ b/media/mtransport/transportlayer.h
@@ -46,18 +46,19 @@ class TransportLayer : public sigslot::h
     downward_(nullptr) {}
 
   virtual ~TransportLayer() {}
 
   // Called to initialize
   nsresult Init();  // Called by Insert() to set up -- do not override
   virtual nsresult InitInternal() { return NS_OK; } // Called by Init
 
-  // Called when inserted into a flow
-  virtual void Inserted(TransportFlow *flow, TransportLayer *downward);
+  void SetFlowId(const std::string& flow_id) {flow_id_ = flow_id;}
+
+  virtual void Chain(TransportLayer *downward);
 
   // Downward interface
   TransportLayer *downward() { return downward_; }
 
   // Get the state
   State state() const { return state_; }
   // Must be implemented by derived classes
   virtual TransportResult SendPacket(const unsigned char *data, size_t len) = 0;
--- a/media/mtransport/transportlayerdtls.h
+++ b/media/mtransport/transportlayerdtls.h
@@ -18,17 +18,16 @@
 #include "mozilla/UniquePtr.h"
 #include "mozilla/TimeStamp.h"
 #include "nsCOMPtr.h"
 #include "nsIEventTarget.h"
 #include "nsITimer.h"
 #include "ScopedNSSTypes.h"
 #include "m_cpp_utils.h"
 #include "dtlsidentity.h"
-#include "transportflow.h"
 #include "transportlayer.h"
 
 namespace mozilla {
 
 struct Packet;
 
 class TransportLayerNSPRAdapter {
  public:
--- a/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp
+++ b/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp
@@ -199,27 +199,32 @@ class TransportInfo {
     dtls_->SetSrtpCiphers(ciphers);
     dtls_->SetIdentity(DtlsIdentity::Generate());
     dtls_->SetRole(client ? TransportLayerDtls::CLIENT :
       TransportLayerDtls::SERVER);
     dtls_->SetVerificationAllowAll();
   }
 
   void PushLayers() {
-    nsresult res;
+    if (NS_FAILED(loopback_->Init())) {
+      delete loopback_;
+      loopback_ = nullptr;
+    }
 
-    // Ignore error; if this fails, subsequent calls to PushLayer will fail also
-    (void)flow_->PushLayer(loopback_);
-    res = flow_->PushLayer(dtls_);
-    if (res != NS_OK) {
-      // These have already been deleted
-      loopback_ = nullptr;
+    if (NS_FAILED(dtls_->Init())) {
+      delete dtls_;
       dtls_ = nullptr;
     }
-    ASSERT_EQ((nsresult)NS_OK, res);
+
+    ASSERT_TRUE(loopback_);
+    ASSERT_TRUE(dtls_);
+
+    dtls_->Chain(loopback_);
+    flow_->PushLayer(loopback_);
+    flow_->PushLayer(dtls_);
   }
 
   void Connect(TransportInfo* peer) {
     MOZ_ASSERT(loopback_);
     MOZ_ASSERT(peer->loopback_);
 
     loopback_->Connect(peer->loopback_);
   }
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ -524,26 +524,27 @@ PeerConnectionMedia::UpdateTransportFlow
 
 // Accessing the PCMedia should be safe here because we shouldn't
 // have enqueued this function unless it was still active and
 // the ICE data is destroyed on the STS.
 static void
 FinalizeTransportFlow_s(RefPtr<PeerConnectionMedia> aPCMedia,
                         RefPtr<TransportFlow> aFlow, size_t aLevel,
                         bool aIsRtcp,
-                        nsAutoPtr<PtrVector<TransportLayer> > aLayerList)
+                        TransportLayerIce* aIceLayer,
+                        TransportLayerDtls* aDtlsLayer)
 {
-  TransportLayerIce* ice =
-      static_cast<TransportLayerIce*>(aLayerList->values.front());
-  ice->SetParameters(aPCMedia->ice_media_stream(aLevel),
-                     aIsRtcp ? 2 : 1);
-  for (auto& value : aLayerList->values) {
-    (void)aFlow->PushLayer(value); // TODO(bug 854518): Process errors.
-  }
-  aLayerList->values.clear();
+  aIceLayer->SetParameters(aPCMedia->ice_media_stream(aLevel),
+                           aIsRtcp ? 2 : 1);
+  // TODO(bug 854518): Process errors.
+  (void)aIceLayer->Init();
+  (void)aDtlsLayer->Init();
+  aDtlsLayer->Chain(aIceLayer);
+  aFlow->PushLayer(aIceLayer);
+  aFlow->PushLayer(aDtlsLayer);
 }
 
 static void
 AddNewIceStreamForRestart_s(RefPtr<PeerConnectionMedia> aPCMedia,
                             RefPtr<TransportFlow> aFlow,
                             size_t aLevel,
                             bool aIsRtcp)
 {
@@ -644,24 +645,20 @@ PeerConnectionMedia::UpdateTransportFlow
     alpn.insert(alpnDefault);
   }
   rv = dtls->SetAlpn(alpn, alpnDefault);
   if (NS_FAILED(rv)) {
     CSFLogError(LOGTAG, "Couldn't set ALPN");
     return rv;
   }
 
-  nsAutoPtr<PtrVector<TransportLayer> > layers(new PtrVector<TransportLayer>);
-  layers->values.push_back(ice.release());
-  layers->values.push_back(dtls.release());
-
   RefPtr<PeerConnectionMedia> pcMedia(this);
   rv = GetSTSThread()->Dispatch(
       WrapRunnableNM(FinalizeTransportFlow_s, pcMedia, flow, aLevel, aIsRtcp,
-                     layers),
+                     ice.release(), dtls.release()),
       NS_DISPATCH_NORMAL);
   if (NS_FAILED(rv)) {
     CSFLogError(LOGTAG, "Failed to dispatch FinalizeTransportFlow_s");
     return rv;
   }
 
   AddTransportFlow(aLevel, aIsRtcp, flow);