Fix leak of RTCPeerConnectionHandler if PeerConnection.close() is called from js.

content/renderer/media/rtc_peer_connection_handler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/rtc_peer_connection_handler.h"
 
 #include <string>
 #include <utility>
 #include <vector>
 
@@ skipping 778 matching lines @@
 
  private:
   const base::WeakPtr<RTCPeerConnectionHandler> handler_;
   const scoped_refptr<base::SingleThreadTaskRunner> main_thread_;
 };
 
 RTCPeerConnectionHandler::RTCPeerConnectionHandler(
     blink::WebRTCPeerConnectionHandlerClient* client,
     PeerConnectionDependencyFactory* dependency_factory)
     : client_(client),
+      is_closed_(false),
       dependency_factory_(dependency_factory),
       weak_factory_(this) {
+  CHECK(client_),
   g_peer_connection_handlers.Get().insert(this);
 }
 
 RTCPeerConnectionHandler::~RTCPeerConnectionHandler() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   stop();
 
   g_peer_connection_handlers.Get().erase(this);
   if (peer_connection_tracker_)
     peer_connection_tracker_->UnregisterPeerConnection(this);
   STLDeleteValues(&remote_streams_);
 
   UMA_HISTOGRAM_COUNTS_10000(
       "WebRTC.NumDataChannelsPerPeerConnection", num_data_channels_created_);
 }
 
 // static
 void RTCPeerConnectionHandler::DestructAllHandlers() {
   std::set<RTCPeerConnectionHandler*> handlers(

[tommi (sloooow) - chröme, 2015/11/12 14:53:19]

do we actually need this copy?  Could this simply be a reference instead?
e.g.

std::set<RTCPeerConnectionHandler*>& handlers =
    g_peer_connection_handlers.Get();

or even do:

for (auto* handler : g_peer_connection_handlers.Get())
  handler->client_->releasePeerConnectionHandler();

[perkj_chrome, 2015/11/12 15:48:38]

needs copy since  g_peer_connection_handlers is modified, ie, the item is
removed from g_peer_connection_handlers as a consequence of calling
releasePeerConnectionHandler.

       g_peer_connection_handlers.Get().begin(),
       g_peer_connection_handlers.Get().end());
   for (auto handler : handlers) {

[tommi (sloooow) - chröme, 2015/11/12 14:53:18]

nit: could make it explicit that handler is always a pointer (never copy
by-value)
nit: also remove braces since it's only a single line now

[perkj_chrome, 2015/11/12 15:48:38]

Done.

-    if (handler->client_)
-      handler->client_->releasePeerConnectionHandler();
+    handler->client_->releasePeerConnectionHandler();
   }
 }
 
 // static
 void RTCPeerConnectionHandler::ConvertOfferOptionsToConstraints(
     const blink::WebRTCOfferOptions& options,
     RTCMediaConstraints* output) {
   output->AddMandatory(
       webrtc::MediaConstraintsInterface::kOfferToReceiveAudio,
       options.offerToReceiveAudio() > 0 ? "true" : "false",
@@ skipping 466 matching lines @@
     const std::string& track_id,
     blink::WebMediaStreamSource::Type track_type) {
   DCHECK(thread_checker_.CalledOnValidThread());
   signaling_thread()->PostTask(FROM_HERE,
       base::Bind(&GetStatsOnSignalingThread, native_peer_connection_, level,
                  make_scoped_refptr(observer), track_id, track_type));
 }
 
 void RTCPeerConnectionHandler::CloseClientPeerConnection() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (client_)
+  if (!is_closed_)
     client_->closePeerConnection();
 }
 
 blink::WebRTCDataChannelHandler* RTCPeerConnectionHandler::createDataChannel(
     const blink::WebString& label, const blink::WebRTCDataChannelInit& init) {
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::createDataChannel");
   DVLOG(1) << "createDataChannel label "
            << base::UTF16ToUTF8(base::StringPiece16(label));
 
@@ skipping 50 matching lines @@
   if (peer_connection_tracker_)
     peer_connection_tracker_->TrackCreateDTMFSender(this, track);
 
   return new RtcDtmfSenderHandler(sender);
 }
 
 void RTCPeerConnectionHandler::stop() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG(1) << "RTCPeerConnectionHandler::stop";
 
-  if (!client_ || !native_peer_connection_.get())
+  if (!is_closed_ || !native_peer_connection_.get())
     return;  // Already stopped.
 
   if (peer_connection_tracker_)
     peer_connection_tracker_->TrackStop(this);
 
   native_peer_connection_->Close();
 
-  // The client_ pointer is not considered valid after this point and no further
-  // callbacks must be made.
-  client_ = nullptr;
+  // This object may no longer forward call backs to blink.
+  is_closed_ = true;

[tommi (sloooow) - chröme, 2015/11/12 14:53:19]

now that client_ is never set to nullptr, can we make it const?

[perkj_chrome, 2015/11/12 15:48:37]

Done.

 }
 
 void RTCPeerConnectionHandler::OnSignalingChange(
     webrtc::PeerConnectionInterface::SignalingState new_state) {
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::OnSignalingChange");
 
   blink::WebRTCPeerConnectionHandlerClient::SignalingState state =
       GetWebKitSignalingState(new_state);
   if (peer_connection_tracker_)
     peer_connection_tracker_->TrackSignalingStateChange(this, state);
-  if (client_)
+  if (!is_closed_)
     client_->didChangeSignalingState(state);
 }
 
 // Called any time the IceConnectionState changes
 void RTCPeerConnectionHandler::OnIceConnectionChange(
     webrtc::PeerConnectionInterface::IceConnectionState new_state) {
   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::OnIceConnectionChange");
   DCHECK(thread_checker_.CalledOnValidThread());
   ReportICEState(new_state);
   if (new_state == webrtc::PeerConnectionInterface::kIceConnectionChecking) {
@@ skipping 15 matching lines @@
         "WebRTC.PeerConnection.TimeToConnect",
         base::TimeTicks::Now() - ice_connection_checking_start_);
     }
   }
 
   track_metrics_.IceConnectionChange(new_state);
   blink::WebRTCPeerConnectionHandlerClient::ICEConnectionState state =
       GetWebKitIceConnectionState(new_state);
   if (peer_connection_tracker_)
     peer_connection_tracker_->TrackIceConnectionStateChange(this, state);
-  if(client_)
+  if(!is_closed_)

[tommi (sloooow) - chröme, 2015/11/12 14:53:18]

nit: add space

[perkj_chrome, 2015/11/12 15:48:38]

Done.

     client_->didChangeICEConnectionState(state);
 }
 
 // Called any time the IceGatheringState changes
 void RTCPeerConnectionHandler::OnIceGatheringChange(
     webrtc::PeerConnectionInterface::IceGatheringState new_state) {
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::OnIceGatheringChange");
 
   if (new_state == webrtc::PeerConnectionInterface::kIceGatheringComplete) {
     // If ICE gathering is completed, generate a NULL ICE candidate,
     // to signal end of candidates.
-    if (client_) {
+    if (!is_closed_) {
       blink::WebRTCICECandidate null_candidate;
       client_->didGenerateICECandidate(null_candidate);
     }
 
     UMA_HISTOGRAM_COUNTS_100("WebRTC.PeerConnection.IPv4LocalCandidates",
                              num_local_candidates_ipv4_);
 
     UMA_HISTOGRAM_COUNTS_100("WebRTC.PeerConnection.IPv6LocalCandidates",
                              num_local_candidates_ipv6_);
   } else if (new_state ==
@@ skipping 36 matching lines @@
 
   if (peer_connection_tracker_) {
     peer_connection_tracker_->TrackAddStream(
         this, s->webkit_stream(), PeerConnectionTracker::SOURCE_REMOTE);
   }
 
   PerSessionWebRTCAPIMetrics::GetInstance()->IncrementStreamCounter();
 
   track_metrics_.AddStream(MediaStreamTrackMetrics::RECEIVED_STREAM,
                            s->webrtc_stream().get());
-  if (client_)
+  if (!is_closed_)
     client_->didAddRemoteStream(s->webkit_stream());
 }
 
 void RTCPeerConnectionHandler::OnRemoveStream(
     const scoped_refptr<webrtc::MediaStreamInterface>& stream) {
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::OnRemoveStreamImpl");
   RemoteStreamMap::iterator it = remote_streams_.find(stream.get());
   if (it == remote_streams_.end()) {
     NOTREACHED() << "Stream not found";
     return;
   }
 
   track_metrics_.RemoveStream(MediaStreamTrackMetrics::RECEIVED_STREAM,
                               stream.get());
   PerSessionWebRTCAPIMetrics::GetInstance()->DecrementStreamCounter();
 
   scoped_ptr<RemoteMediaStreamImpl> remote_stream(it->second);
   const blink::WebMediaStream& webkit_stream = remote_stream->webkit_stream();
   DCHECK(!webkit_stream.isNull());
   remote_streams_.erase(it);
 
   if (peer_connection_tracker_) {
     peer_connection_tracker_->TrackRemoveStream(
         this, webkit_stream, PeerConnectionTracker::SOURCE_REMOTE);
   }
 
-  if (client_)
+  if (!is_closed_)
     client_->didRemoveRemoteStream(webkit_stream);
 }
 
 void RTCPeerConnectionHandler::OnDataChannel(
     scoped_ptr<RtcDataChannelHandler> handler) {
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::OnDataChannelImpl");
 
   if (peer_connection_tracker_) {
     peer_connection_tracker_->TrackCreateDataChannel(
         this, handler->channel().get(), PeerConnectionTracker::SOURCE_REMOTE);
   }
 
-  if (client_)
+  if (!is_closed_)
     client_->didAddRemoteDataChannel(handler.release());
 }
 
 void RTCPeerConnectionHandler::OnIceCandidate(
     const std::string& sdp, const std::string& sdp_mid, int sdp_mline_index,
     int component, int address_family) {
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::OnIceCandidateImpl");
   blink::WebRTCICECandidate web_candidate;
   web_candidate.initialize(base::UTF8ToUTF16(sdp),
                            base::UTF8ToUTF16(sdp_mid),
                            sdp_mline_index);
   if (peer_connection_tracker_) {
     peer_connection_tracker_->TrackAddIceCandidate(
         this, web_candidate, PeerConnectionTracker::SOURCE_LOCAL, true);
   }
 
   // Only the first m line's first component is tracked to avoid
   // miscounting when doing BUNDLE or rtcp mux.
   if (sdp_mline_index == 0 && component == 1) {
     if (address_family == AF_INET) {
       ++num_local_candidates_ipv4_;
     } else if (address_family == AF_INET6) {
       ++num_local_candidates_ipv6_;
     } else {
       NOTREACHED();
     }
   }
-  if (client_)
+  if (!is_closed_)
     client_->didGenerateICECandidate(web_candidate);
 }
 
 webrtc::SessionDescriptionInterface*
 RTCPeerConnectionHandler::CreateNativeSessionDescription(
     const std::string& sdp, const std::string& type,
     webrtc::SdpParseError* error) {
   webrtc::SessionDescriptionInterface* native_desc =
       dependency_factory_->CreateSessionDescription(type, sdp, error);
 
@@ skipping 70 matching lines @@
 }
 
 void RTCPeerConnectionHandler::ResetUMAStats() {
   DCHECK(thread_checker_.CalledOnValidThread());
   num_local_candidates_ipv6_ = 0;
   num_local_candidates_ipv4_ = 0;
   ice_connection_checking_start_ = base::TimeTicks();
   memset(ice_state_seen_, 0, sizeof(ice_state_seen_));
 }
 }  // namespace content