Fixing possible crash due to RefCountedChannel assignment operator.

webrtc/p2p/base/transportcontroller.h

 /*
  *  Copyright 2015 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #ifndef WEBRTC_P2P_BASE_TRANSPORTCONTROLLER_H_
 #define WEBRTC_P2P_BASE_TRANSPORTCONTROLLER_H_
 
 #include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "webrtc/base/asyncinvoker.h"
+#include "webrtc/base/constructormagic.h"
 #include "webrtc/base/sigslot.h"
 #include "webrtc/base/sslstreamadapter.h"
 #include "webrtc/p2p/base/candidate.h"
 #include "webrtc/p2p/base/dtlstransportchannel.h"
 #include "webrtc/p2p/base/jseptransport.h"
 #include "webrtc/p2p/base/p2ptransportchannel.h"
 
 namespace rtc {
 class Thread;
 class PacketTransportInterface;
@@ skipping 133 matching lines @@
  private:
   void OnMessage(rtc::Message* pmsg) override;
 
   // This structure groups the DTLS and ICE channels, and helps keep track of
   // how many external objects (BaseChannels) reference each channel.
   struct RefCountedChannel {
     RefCountedChannel() = default;
     // TODO(deadbeef): Change the types of |dtls| and |ice| to
     // DtlsTransportChannelWrapper and P2PTransportChannelWrapper,
     // once TransportChannelImpl is removed.
-    explicit RefCountedChannel(TransportChannelImpl* dtls,
-                               TransportChannelImpl* ice)
+    RefCountedChannel(TransportChannelImpl* dtls, TransportChannelImpl* ice)

[honghaiz3, 2016/12/13 19:02:20]

Why do we implement our custom RefCounted class instead of using the webrtc
template RefCountedObject?

[Taylor Brandstetter, 2016/12/13 22:42:36]

I think just because no one has bothered to change it. I guess it makes sense to
do that now, instead of adding more code to the old thing. See new patchset.

         : ice_(ice), dtls_(dtls), ref_(0) {}
 
+    // Move assignment operator and move constructor needed since this goes in
+    // a vector.
+    RefCountedChannel(RefCountedChannel&& o)
+        : ice_(std::move(o.ice_)), dtls_(std::move(o.dtls_)), ref_(o.ref_) {
+    }
+    void operator=(RefCountedChannel&& o) {
+      // Need to delete the DTLS channel first since it depends on ICE.

[honghaiz3, 2016/12/13 19:02:20]

I wonder if we can better solve the dependency by letting DTLS channel own the
ICE channel. I understand we may need to have direct access to the ICE channel
per the standard, but we can always provide the access via DTLS channel. If we
do so, we don't need this struct. 

[Taylor Brandstetter, 2016/12/13 22:42:36]

We can discuss that separately, but I don't want to do that just to fix this
bug.

+      dtls_ = std::move(o.dtls_);
+      ice_ = std::move(o.ice_);
+      ref_ = o.ref_;
+    }
+
     void AddRef() { ++ref_; }
     void DecRef() {
       ASSERT(ref_ > 0);
       --ref_;
     }
     int ref() const { return ref_; }
 
     // Currently, all ICE-related calls still go through this DTLS channel. But
     // that will change once we get rid of TransportChannelImpl, and the DTLS
     // channel interface no longer includes ICE-specific methods.
     const TransportChannelImpl* dtls() const { return dtls_.get(); }
     TransportChannelImpl* dtls() { return dtls_.get(); }
     const TransportChannelImpl* ice() const { return ice_.get(); }
     TransportChannelImpl* ice() { return ice_.get(); }
 
    private:
     std::unique_ptr<TransportChannelImpl> ice_;

[honghaiz3, 2016/12/13 19:02:20]

Is it an overkill to use smart pointer here?
Since the whole purpose of this struct is to manage these two pointers, isn't it
much simpler to use the raw pointers here and delete the two pointers in the
destructor (where we can control the order of deletion)? This could have avoid
all the issues caused by any moving or copying of the object. 

[Taylor Brandstetter, 2016/12/13 22:42:36]

I prefer to use smart pointers where possible because it makes object ownership
more clear. But it may be a matter of preference.

     std::unique_ptr<TransportChannelImpl> dtls_;
     int ref_ = 0;
   };
 
   // Helper functions to get a channel or transport, or iterator to it (in case
   // it needs to be erased).
   std::vector<RefCountedChannel>::iterator GetChannelIterator_n(
       const std::string& transport_name,
       int component);
   std::vector<RefCountedChannel>::const_iterator GetChannelIterator_n(
@@ skipping 71 matching lines @@
   IceRole ice_role_ = ICEROLE_CONTROLLING;
   bool redetermine_role_on_ice_restart_;
   uint64_t ice_tiebreaker_ = rtc::CreateRandomId64();
   rtc::SSLProtocolVersion ssl_max_version_ = rtc::SSL_PROTOCOL_DTLS_12;
   rtc::scoped_refptr<rtc::RTCCertificate> certificate_;
   rtc::AsyncInvoker invoker_;
   // True if QUIC is used instead of DTLS.
   bool quic_ = false;
 
   webrtc::MetricsObserverInterface* metrics_observer_ = nullptr;
+
+  RTC_DISALLOW_COPY_AND_ASSIGN(TransportController);
 };
 
 }  // namespace cricket
 
 #endif  // WEBRTC_P2P_BASE_TRANSPORTCONTROLLER_H_