diff options
author | perkj <perkj@chromium.org> | 2015-11-12 09:08:27 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-11-12 17:09:15 +0000 |
commit | 9596af111d9e82b0e27c9fab01fa688515623b1f (patch) | |
tree | 332f4ef3385224d591c7d1242db700bf22c489a5 /content | |
parent | 848308f02c47d35b47da1f3e44129a0d287ea0cb (diff) | |
download | chromium_src-9596af111d9e82b0e27c9fab01fa688515623b1f.zip chromium_src-9596af111d9e82b0e27c9fab01fa688515623b1f.tar.gz chromium_src-9596af111d9e82b0e27c9fab01fa688515623b1f.tar.bz2 |
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/1438153002
Cr-Commit-Position: refs/heads/master@{#359334}
Diffstat (limited to 'content')
3 files changed, 30 insertions, 18 deletions
diff --git a/content/renderer/media/rtc_peer_connection_handler.cc b/content/renderer/media/rtc_peer_connection_handler.cc index 6dfe0d9..193ab94 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()) 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); } diff --git a/content/renderer/media/rtc_peer_connection_handler.h b/content/renderer/media/rtc_peer_connection_handler.h index 8e8af25..fa45d6a 100644 --- a/content/renderer/media/rtc_peer_connection_handler.h +++ b/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. diff --git a/content/renderer/media/rtc_peer_connection_handler_unittest.cc b/content/renderer/media/rtc_peer_connection_handler_unittest.cc index 6cbdd1d..88fad70 100644 --- a/content/renderer/media/rtc_peer_connection_handler_unittest.cc +++ b/content/renderer/media/rtc_peer_connection_handler_unittest.cc @@ -334,6 +334,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; |