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

content/renderer/media/rtc_peer_connection_handler.cc

Index: content/renderer/media/rtc_peer_connection_handler.cc
diff --git a/content/renderer/media/rtc_peer_connection_handler.cc b/content/renderer/media/rtc_peer_connection_handler.cc
index 6dfe0d9f2925f989f47eef6e5a7b6c04881d6ed5..b90875181345a2ee3f4db05131cd613a705d9eaa 100644
--- a/content/renderer/media/rtc_peer_connection_handler.cc
+++ b/content/renderer/media/rtc_peer_connection_handler.cc
@@ -796,8 +796,10 @@ 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);
 }
 
@@ -821,8 +823,7 @@ void RTCPeerConnectionHandler::DestructAllHandlers() {
       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();
   }
 }
 
@@ -1309,7 +1310,7 @@ void RTCPeerConnectionHandler::GetStats(
 
 void RTCPeerConnectionHandler::CloseClientPeerConnection() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (client_)
+  if (!is_closed_)
     client_->closePeerConnection();
 }
 
@@ -1380,7 +1381,7 @@ 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_)
@@ -1388,9 +1389,8 @@ void RTCPeerConnectionHandler::stop() {
 
   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(
@@ -1402,7 +1402,7 @@ void RTCPeerConnectionHandler::OnSignalingChange(
       GetWebKitSignalingState(new_state);
   if (peer_connection_tracker_)
     peer_connection_tracker_->TrackSignalingStateChange(this, state);
-  if (client_)
+  if (!is_closed_)
     client_->didChangeSignalingState(state);
 }
 
@@ -1438,7 +1438,7 @@ void RTCPeerConnectionHandler::OnIceConnectionChange(
       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);
 }
 
@@ -1451,7 +1451,7 @@ void 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);
     }
@@ -1508,7 +1508,7 @@ void RTCPeerConnectionHandler::OnAddStream(
 
   track_metrics_.AddStream(MediaStreamTrackMetrics::RECEIVED_STREAM,
                            s->webrtc_stream().get());
-  if (client_)
+  if (!is_closed_)
     client_->didAddRemoteStream(s->webkit_stream());
 }
 
@@ -1536,7 +1536,7 @@ void RTCPeerConnectionHandler::OnRemoveStream(
         this, webkit_stream, PeerConnectionTracker::SOURCE_REMOTE);
   }
 
-  if (client_)
+  if (!is_closed_)
     client_->didRemoveRemoteStream(webkit_stream);
 }
 
@@ -1550,7 +1550,7 @@ void RTCPeerConnectionHandler::OnDataChannel(
         this, handler->channel().get(), PeerConnectionTracker::SOURCE_REMOTE);
   }
 
-  if (client_)
+  if (!is_closed_)
     client_->didAddRemoteDataChannel(handler.release());
 }
 
@@ -1579,7 +1579,7 @@ void RTCPeerConnectionHandler::OnIceCandidate(
       NOTREACHED();
     }
   }
-  if (client_)
+  if (!is_closed_)
     client_->didGenerateICECandidate(web_candidate);
 }