This cl was originally reviewed in https://codereview.chromium.org/14…

…38153002/. Patchset 1 is a pure re-upload.

Pachset 2 fix a bug in the original cl and adds a unit test.

Fix leak of RTCPeerConnectionHandler if PeerConnection.close() is called from js.
This fixes a bug where RTCPeerConnectionHandler::client_ is set to null when RTCPeerConnectionHandler.stop() is called.
RTCPeerConnectionHandler.stop() is a pretty bad name (override from blink::WebRTCPeerConnectionHandler) since it is triggered when JS or the browser process want to close a PeerConnection.
Since client_ was set to null, RTCPeerConnectionHandler::DestructAllHandlers did not delete RTCPeerConnectionHandler.

BUG=542132

Review URL: https://codereview.chromium.org/1439973006

Cr-Commit-Position: refs/heads/master@{#359784}

content/renderer/media/mock_peer_connection_impl.h

@@ -61,9 +61,8 @@ class MockPeerConnectionImpl : public webrtc::PeerConnectionInterface {
     NOTIMPLEMENTED();
     return PeerConnectionInterface::kIceGatheringNew;
   }
-  void Close() override {
-    NOTIMPLEMENTED();
-  }
+
+  MOCK_METHOD0(Close, void());
 
   const webrtc::SessionDescriptionInterface* local_description() const override;
   const webrtc::SessionDescriptionInterface* remote_description()

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,17 +1382,16 @@ 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;
 }
 
 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);
 }
 

content/renderer/media/rtc_peer_connection_handler.h

@@ -225,8 +225,14 @@ class CONTENT_EXPORT RTCPeerConnectionHandler
 
   base::ThreadChecker thread_checker_;
 
-  // |client_| is a weak pointer, and is valid until stop() has returned.
-  blink::WebRTCPeerConnectionHandlerClient* client_;
+  // |client_| is a weak pointer to the blink object (blink::RTCPeerConnection)
+  // that owns this object.
+  // It is valid for the lifetime of this object.
+  blink::WebRTCPeerConnectionHandlerClient* const client_;
+  // True if this PeerConnection has been closed.
+  // After the PeerConnection has been closed, this object may no longer
+  // forward callbacks to blink.
+  bool is_closed_;
 
   // |dependency_factory_| is a raw pointer, and is valid for the lifetime of
   // RenderThreadImpl.

content/renderer/media/rtc_peer_connection_handler_unittest.cc

@@ -231,6 +231,7 @@ class RTCPeerConnectionHandlerTest : public ::testing::Test {
 
     mock_peer_connection_ = pc_handler_->native_peer_connection();
     ASSERT_TRUE(mock_peer_connection_);
+    EXPECT_CALL(*mock_peer_connection_, Close());
   }
 
   void TearDown() override {
@@ -334,6 +335,11 @@ TEST_F(RTCPeerConnectionHandlerTest, Destruct) {
   pc_handler_.reset(NULL);
 }
 
+TEST_F(RTCPeerConnectionHandlerTest, DestructAllHandlers) {
+  EXPECT_CALL(*mock_client_.get(), releasePeerConnectionHandler())
+      .Times(1);
+  RTCPeerConnectionHandler::DestructAllHandlers();
+}
 TEST_F(RTCPeerConnectionHandlerTest, CreateOffer) {
   blink::WebRTCSessionDescriptionRequest request;
   blink::WebMediaConstraints options;