Fixing possible crash due to RefCountedChannel assignment operator.

Fixing possible crash due to RefCountedChannel assignment operator.

We relied on the default destructor of RefCountedChannel to destroy its
members in reverse initialization order (deleting the DTLS wrapper
before the underlying ICE channel).

However, std::vector also may use the default assignment operator, which
performs a member-wise copy in initialization order. Which results in
deleting the ICE channel before the DTLS one. This CL fixes this by
using a vector of pointers instead of structures, and uses RefCountedObject
to handle ref-counting.

BUG=chromium:672951
Committed: https://crrev.com/62802a1b0e668a279268b5f2d3d1ea9c10869961
Cr-Commit-Position: refs/heads/master@{#15583}

[Taylor Brandstetter, 2016-12-13 01:57:22]

The CQ bit was checked by deadbeef@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 01:57:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-13 02:22:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 02:22:11]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2016-12-13 02:22:41]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

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

honghaiz@webrtc.org changed reviewers:
+ honghaiz@webrtc.org

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

https://codereview.webrtc.org/2571683004/diff/1/webrtc/p2p/base/transportcont...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/2571683004/diff/1/webrtc/p2p/base/transportcont...
webrtc/p2p/base/transportcontroller.h:174:
RefCountedChannel(TransportChannelImpl* dtls, TransportChannelImpl* ice)
Why do we implement our custom RefCounted class instead of using the webrtc
template RefCountedObject?

https://codereview.webrtc.org/2571683004/diff/1/webrtc/p2p/base/transportcont...
webrtc/p2p/base/transportcontroller.h:183: // Need to delete the DTLS channel
first since it depends on ICE.
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.

https://codereview.webrtc.org/2571683004/diff/1/webrtc/p2p/base/transportcont...
webrtc/p2p/base/transportcontroller.h:205: std::unique_ptr<TransportChannelImpl>
ice_;
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:38:00]

Description was changed from

==========
Adding correct assignment operator for RefCountedChannel.

We relied on the default destructor of RefCountedChannel to destroy its
members in reverse initialization order (deleting the DTLS wrapper
before the underlying ICE channel).

However, std::vector also may use the default assignment operator, which
performs a member-wise copy in initialization order. Which results in
deleting the ICE channel before the DTLS one. This CL fixes this by
adding a correct assignment operator.

BUG=chromium:672951
==========

to

==========
Fixing possible crash due to RefCountedChannel assignment operator.

We relied on the default destructor of RefCountedChannel to destroy its
members in reverse initialization order (deleting the DTLS wrapper
before the underlying ICE channel).

However, std::vector also may use the default assignment operator, which
performs a member-wise copy in initialization order. Which results in
deleting the ICE channel before the DTLS one. This CL fixes this by
using a vector of pointers instead of structures, and uses RefCountedObject
to handle ref-counting.

BUG=chromium:672951
==========

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

Please take a look again. I'm using RefCountedObject now, and am just using raw
pointers in the vector so we don't have to worry about copy/assignment.

https://codereview.webrtc.org/2571683004/diff/1/webrtc/p2p/base/transportcont...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/2571683004/diff/1/webrtc/p2p/base/transportcont...
webrtc/p2p/base/transportcontroller.h:174:
RefCountedChannel(TransportChannelImpl* dtls, TransportChannelImpl* ice)
On 2016/12/13 19:02:20, honghaiz3 wrote:
> Why do we implement our custom RefCounted class instead of using the webrtc
> template RefCountedObject?

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.

https://codereview.webrtc.org/2571683004/diff/1/webrtc/p2p/base/transportcont...
webrtc/p2p/base/transportcontroller.h:183: // Need to delete the DTLS channel
first since it depends on ICE.
On 2016/12/13 19:02:20, honghaiz3 wrote:
> 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. 

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

https://codereview.webrtc.org/2571683004/diff/1/webrtc/p2p/base/transportcont...
webrtc/p2p/base/transportcontroller.h:205: std::unique_ptr<TransportChannelImpl>
ice_;
On 2016/12/13 19:02:20, honghaiz3 wrote:
> 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. 

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

[pthatcher1, 2016-12-13 23:36:56]

lgtm, with nit

https://codereview.webrtc.org/2571683004/diff/20001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportcontroller.cc (right):

https://codereview.webrtc.org/2571683004/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.cc:447: }
Look at at refcountedobject.h, that does that exact same thing as "delete
channel", doesn't it?

[Taylor Brandstetter, 2016-12-13 23:50:50]

https://codereview.webrtc.org/2571683004/diff/20001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportcontroller.cc (right):

https://codereview.webrtc.org/2571683004/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.cc:447: }
On 2016/12/13 23:36:56, pthatcher1 wrote:
> Look at at refcountedobject.h, that does that exact same thing as "delete
> channel", doesn't it?  

The destructor isn't public, it's expected to only be destroyed by the ref count
going to 0. We probably can remove this code later and just assert that
channels_ is empty when the destructor is called.

[Taylor Brandstetter, 2016-12-13 23:50:53]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-12-13 23:50:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-14 00:08:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 00:08:11]

Failed to apply patch for webrtc/p2p/base/transportcontroller.cc:
While running git apply --index -p1;
  error: patch failed: webrtc/p2p/base/transportcontroller.cc:389
  error: webrtc/p2p/base/transportcontroller.cc: patch does not apply

Patch:       webrtc/p2p/base/transportcontroller.cc
Index: webrtc/p2p/base/transportcontroller.cc
diff --git a/webrtc/p2p/base/transportcontroller.cc
b/webrtc/p2p/base/transportcontroller.cc
index
e4dc5afa21ae775b5b6cca401a7b2d136a19d151..7a4edffde3c2a3a7451b4e5eaddfdd79fa634b98
100644
--- a/webrtc/p2p/base/transportcontroller.cc
+++ b/webrtc/p2p/base/transportcontroller.cc
@@ -18,7 +18,7 @@
 #include "webrtc/base/thread.h"
 #include "webrtc/p2p/base/port.h"
 
-namespace cricket {
+namespace {
 
 enum {
   MSG_ICECONNECTIONSTATE,
@@ -29,11 +29,40 @@ enum {
 
 struct CandidatesData : public rtc::MessageData {
   CandidatesData(const std::string& transport_name,
-                 const Candidates& candidates)
+                 const cricket::Candidates& candidates)
       : transport_name(transport_name), candidates(candidates) {}
 
   std::string transport_name;
-  Candidates candidates;
+  cricket::Candidates candidates;
+};
+
+}  // namespace {
+
+namespace cricket {
+
+// This class groups the DTLS and ICE channels, and helps keep track of
+// how many external objects (BaseChannels) reference each channel.
+class TransportController::ChannelPair {
+ public:
+  // TODO(deadbeef): Change the types of |dtls| and |ice| to
+  // DtlsTransportChannelWrapper and P2PTransportChannelWrapper,
+  // once TransportChannelImpl is removed.
+  ChannelPair(TransportChannelImpl* dtls, TransportChannelImpl* ice)
+      : ice_(ice), dtls_(dtls) {}
+
+  // 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_;
+  std::unique_ptr<TransportChannelImpl> dtls_;
+
+  RTC_DISALLOW_COPY_AND_ASSIGN(ChannelPair);
 };
 
 TransportController::TransportController(rtc::Thread* signaling_thread,
@@ -225,7 +254,9 @@ TransportChannel*
TransportController::CreateTransportChannel_n(
       this, &TransportController::OnChannelStateChanged_n);
   dtls->SignalDtlsHandshakeError.connect(
       this, &TransportController::OnDtlsHandshakeError);
-  channels_.insert(channels_.end(), RefCountedChannel(dtls, ice))->AddRef();
+  RefCountedChannel* new_pair = new RefCountedChannel(dtls, ice);
+  new_pair->AddRef();
+  channels_.insert(channels_.end(), new_pair);
   bool channel_added = transport->AddChannel(dtls, component);
   RTC_DCHECK(channel_added);
   // Adding a channel could cause aggregate state to change.
@@ -245,8 +276,7 @@ void TransportController::DestroyTransportChannel_n(
                     << ", which doesn't exist.";
     return;
   }
-  it->DecRef();
-  if (it->ref() > 0) {
+  if ((*it)->Release() > 0) {
     return;
   }
   channels_.erase(it);
@@ -274,8 +304,8 @@ std::vector<std::string>
TransportController::transport_names_for_testing() {
 
 std::vector<TransportChannelImpl*> TransportController::channels_for_testing()
{
   std::vector<TransportChannelImpl*> ret;
-  for (RefCountedChannel& channel : channels_) {
-    ret.push_back(channel.dtls());
+  for (RefCountedChannel* channel : channels_) {
+    ret.push_back(channel->dtls());
   }
   return ret;
 }
@@ -338,27 +368,27 @@ void TransportController::OnMessage(rtc::Message* pmsg) {
   }
 }
 
-std::vector<TransportController::RefCountedChannel>::iterator
+std::vector<TransportController::RefCountedChannel*>::iterator
 TransportController::GetChannelIterator_n(const std::string& transport_name,
                                           int component) {
   RTC_DCHECK(network_thread_->IsCurrent());
-  return std::find_if(
-      channels_.begin(), channels_.end(),
-      [transport_name, component](const RefCountedChannel& channel) {
-        return channel.dtls()->transport_name() == transport_name &&
-               channel.dtls()->component() == component;
-      });
+  return std::find_if(channels_.begin(), channels_.end(),
+                      [transport_name, component](RefCountedChannel* channel) {
+                        return channel->dtls()->transport_name() ==
+                                   transport_name &&
+                               channel->dtls()->component() == component;
+                      });
 }
 
-std::vector<TransportController::RefCountedChannel>::const_iterator
+std::vector<TransportController::RefCountedChannel*>::const_iterator
 TransportController::GetChannelIterator_n(const std::string& transport_name,
                                           int component) const {
   RTC_DCHECK(network_thread_->IsCurrent());
   return std::find_if(
       channels_.begin(), channels_.end(),
-      [transport_name, component](const RefCountedChannel& channel) {
-        return channel.dtls()->transport_name() == transport_name &&
-               channel.dtls()->component() == component;
+      [transport_name, component](const RefCountedChannel* channel) {
+        return channel->dtls()->transport_name() == transport_name &&
+               channel->dtls()->component() == component;
       });
 }
 
@@ -381,7 +411,7 @@ const TransportController::RefCountedChannel*
TransportController::GetChannel_n(
     int component) const {
   RTC_DCHECK(network_thread_->IsCurrent());
   auto it = GetChannelIterator_n(transport_name, component);
-  return (it == channels_.end()) ? nullptr : &(*it);
+  return (it == channels_.end()) ? nullptr : *it;
 }
 
 TransportController::RefCountedChannel* TransportController::GetChannel_n(
@@ -389,7 +419,7 @@ TransportController::RefCountedChannel*
TransportController::GetChannel_n(
     int component) {
   RTC_DCHECK(network_thread_->IsCurrent());
   auto it = GetChannelIterator_n(transport_name, component);
-  return (it == channels_.end()) ? nullptr : &(*it);
+  return (it == channels_.end()) ? nullptr : *it;
 }
 
 JsepTransport* TransportController::GetOrCreateJsepTransport_n(
@@ -409,6 +439,13 @@ JsepTransport*
TransportController::GetOrCreateJsepTransport_n(
 void TransportController::DestroyAllChannels_n() {
   RTC_DCHECK(network_thread_->IsCurrent());
   transports_.clear();
+  for (RefCountedChannel* channel : channels_) {
+    // Even though these objects are normally ref-counted, if
+    // TransportController is deleted while they still have references, just
+    // remove all references.
+    while (channel->Release() > 0) {
+    }
+  }
   channels_.clear();
 }
 
@@ -430,7 +467,7 @@ void TransportController::SetIceConfig_n(const IceConfig&
config) {
 
   ice_config_ = config;
   for (auto& channel : channels_) {
-    channel.dtls()->SetIceConfig(ice_config_);
+    channel->dtls()->SetIceConfig(ice_config_);
   }
 }
 
@@ -439,7 +476,7 @@ void TransportController::SetIceRole_n(IceRole ice_role) {
 
   ice_role_ = ice_role;
   for (auto& channel : channels_) {
-    channel.dtls()->SetIceRole(ice_role_);
+    channel->dtls()->SetIceRole(ice_role_);
   }
 }
 
@@ -472,7 +509,7 @@ bool TransportController::SetLocalCertificate_n(
   }
   // ... and for the DTLS channel, which needs it for the DTLS handshake.
   for (auto& channel : channels_) {
-    bool set_cert_success = channel.dtls()->SetLocalCertificate(certificate);
+    bool set_cert_success = channel->dtls()->SetLocalCertificate(certificate);
     RTC_DCHECK(set_cert_success);
   }
   return true;
@@ -571,7 +608,7 @@ bool TransportController::SetRemoteTransportDescription_n(
 
 void TransportController::MaybeStartGathering_n() {
   for (auto& channel : channels_) {
-    channel.dtls()->MaybeStartGathering();
+    channel->dtls()->MaybeStartGathering();
   }
 }
 
@@ -669,7 +706,7 @@ void TransportController::SetMetricsObserver_n(
   RTC_DCHECK(network_thread_->IsCurrent());
   metrics_observer_ = metrics_observer;
   for (auto& channel : channels_) {
-    channel.dtls()->SetMetricsObserver(metrics_observer);
+    channel->dtls()->SetMetricsObserver(metrics_observer);
   }
 }
 
@@ -762,21 +799,21 @@ void TransportController::UpdateAggregateStates_n() {
   bool any_gathering = false;
   bool all_done_gathering = !channels_.empty();
   for (const auto& channel : channels_) {
-    any_receiving = any_receiving || channel.dtls()->receiving();
+    any_receiving = any_receiving || channel->dtls()->receiving();
     any_failed =
         any_failed ||
-        channel.dtls()->GetState() == TransportChannelState::STATE_FAILED;
-    all_connected = all_connected && channel.dtls()->writable();
+        channel->dtls()->GetState() == TransportChannelState::STATE_FAILED;
+    all_connected = all_connected && channel->dtls()->writable();
     all_completed =
-        all_completed && channel.dtls()->writable() &&
-        channel.dtls()->GetState() == TransportChannelState::STATE_COMPLETED &&
-        channel.dtls()->GetIceRole() == ICEROLE_CONTROLLING &&
-        channel.dtls()->gathering_state() == kIceGatheringComplete;
+        all_completed && channel->dtls()->writable() &&
+        channel->dtls()->GetState() == TransportChannelState::STATE_COMPLETED
&&
+        channel->dtls()->GetIceRole() == ICEROLE_CONTROLLING &&
+        channel->dtls()->gathering_state() == kIceGatheringComplete;
     any_gathering =
-        any_gathering || channel.dtls()->gathering_state() != kIceGatheringNew;
+        any_gathering || channel->dtls()->gathering_state() !=
kIceGatheringNew;
     all_done_gathering =
         all_done_gathering &&
-        channel.dtls()->gathering_state() == kIceGatheringComplete;
+        channel->dtls()->gathering_state() == kIceGatheringComplete;
   }…
(message too large)

[Taylor Brandstetter, 2016-12-14 00:12:10]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-12-14 00:12:11]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2571683004/#ps40001 (title:
"Merge with master")

[commit-bot: I haz the power, 2016-12-14 00:12:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-14 00:38:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481674330991150,
"parent_rev": "b10d4cc2ef66b9531baf5e85163d6bbe6f711678", "commit_rev":
"1ce58727842e118ddce298e1070670e3fdc37787"}

[commit-bot: I haz the power, 2016-12-14 00:38:40]

Description was changed from

==========
Fixing possible crash due to RefCountedChannel assignment operator.

We relied on the default destructor of RefCountedChannel to destroy its
members in reverse initialization order (deleting the DTLS wrapper
before the underlying ICE channel).

However, std::vector also may use the default assignment operator, which
performs a member-wise copy in initialization order. Which results in
deleting the ICE channel before the DTLS one. This CL fixes this by
using a vector of pointers instead of structures, and uses RefCountedObject
to handle ref-counting.

BUG=chromium:672951
==========

to

==========
Fixing possible crash due to RefCountedChannel assignment operator.

We relied on the default destructor of RefCountedChannel to destroy its
members in reverse initialization order (deleting the DTLS wrapper
before the underlying ICE channel).

However, std::vector also may use the default assignment operator, which
performs a member-wise copy in initialization order. Which results in
deleting the ICE channel before the DTLS one. This CL fixes this by
using a vector of pointers instead of structures, and uses RefCountedObject
to handle ref-counting.

BUG=chromium:672951

Review-Url: https://codereview.webrtc.org/2571683004
==========

[commit-bot: I haz the power, 2016-12-14 00:38:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-14 00:38:49]

Description was changed from

==========
Fixing possible crash due to RefCountedChannel assignment operator.

We relied on the default destructor of RefCountedChannel to destroy its
members in reverse initialization order (deleting the DTLS wrapper
before the underlying ICE channel).

However, std::vector also may use the default assignment operator, which
performs a member-wise copy in initialization order. Which results in
deleting the ICE channel before the DTLS one. This CL fixes this by
using a vector of pointers instead of structures, and uses RefCountedObject
to handle ref-counting.

BUG=chromium:672951

Review-Url: https://codereview.webrtc.org/2571683004
==========

to

==========
Fixing possible crash due to RefCountedChannel assignment operator.

We relied on the default destructor of RefCountedChannel to destroy its
members in reverse initialization order (deleting the DTLS wrapper
before the underlying ICE channel).

However, std::vector also may use the default assignment operator, which
performs a member-wise copy in initialization order. Which results in
deleting the ICE channel before the DTLS one. This CL fixes this by
using a vector of pointers instead of structures, and uses RefCountedObject
to handle ref-counting.

BUG=chromium:672951

Committed: https://crrev.com/62802a1b0e668a279268b5f2d3d1ea9c10869961
Cr-Commit-Position: refs/heads/master@{#15583}
==========

[commit-bot: I haz the power, 2016-12-14 00:38:51]

Patchset 3 (id:??) landed as
https://crrev.com/62802a1b0e668a279268b5f2d3d1ea9c10869961
Cr-Commit-Position: refs/heads/master@{#15583}

[honghaiz3, 2016-12-14 01:20:00]

lgtm

https://codereview.webrtc.org/2571683004/diff/40001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportcontroller.cc (right):

https://codereview.webrtc.org/2571683004/diff/40001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.cc:39: }  // namespace {
No left bracket.