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..193ab94540154ec00b940a0bce5feaf084b99a42 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);
 }
 
@@ -817,13 +819,13 @@ RTCPeerConnectionHandler::~RTCPeerConnectionHandler() {
 
 // static
 void RTCPeerConnectionHandler::DestructAllHandlers() {
+  // Copy g_peer_connection_handlers since releasePeerConnectionHandler will
+  // remove an item.
   std::set<RTCPeerConnectionHandler*> handlers(
       g_peer_connection_handlers.Get().begin(),
       g_peer_connection_handlers.Get().end());
-  for (auto handler : handlers) {
-    if (handler->client_)
-      handler->client_->releasePeerConnectionHandler();
-  }
+  for (auto* handler : handlers)
+    handler->client_->releasePeerConnectionHandler();
 }
 
 // static
@@ -1309,7 +1311,7 @@ void RTCPeerConnectionHandler::GetStats(
 
 void RTCPeerConnectionHandler::CloseClientPeerConnection() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (client_)
+  if (!is_closed_)
     client_->closePeerConnection();
 }
 
@@ -1380,7 +1382,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())

[Taylor_Brandstetter, 2015/11/13 18:45:29]

Should be "is_closed", not "!is_closed". I think this is the source of the
clusterfuzz crashes. The PC is never actually being closed because of this, and
so observer objects are being dereferenced after being destroyed.

[tommi (sloooow) - chröme, 2015/11/13 18:57:23]

well spotted! :)

     return;  // Already stopped.
 
   if (peer_connection_tracker_)
@@ -1388,9 +1390,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;
 }
 
 void RTCPeerConnectionHandler::OnSignalingChange(
@@ -1402,7 +1403,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 +1439,7 @@ void RTCPeerConnectionHandler::OnIceConnectionChange(
       GetWebKitIceConnectionState(new_state);
   if (peer_connection_tracker_)
     peer_connection_tracker_->TrackIceConnectionStateChange(this, state);
-  if(client_)
+  if (!is_closed_)
     client_->didChangeICEConnectionState(state);
 }
 
@@ -1451,7 +1452,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 +1509,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 +1537,7 @@ void RTCPeerConnectionHandler::OnRemoveStream(
         this, webkit_stream, PeerConnectionTracker::SOURCE_REMOTE);
   }
 
-  if (client_)
+  if (!is_closed_)
     client_->didRemoveRemoteStream(webkit_stream);
 }
 
@@ -1550,7 +1551,7 @@ void RTCPeerConnectionHandler::OnDataChannel(
         this, handler->channel().get(), PeerConnectionTracker::SOURCE_REMOTE);
   }
 
-  if (client_)
+  if (!is_closed_)
     client_->didAddRemoteDataChannel(handler.release());
 }
 
@@ -1579,7 +1580,7 @@ void RTCPeerConnectionHandler::OnIceCandidate(
       NOTREACHED();
     }
   }
-  if (client_)
+  if (!is_closed_)
     client_->didGenerateICECandidate(web_candidate);
 }