zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Modified peerconnection's "observer" slot to be nulled on close.

Browse Source 
This prevents usage of the observer post-close; modified the "usage
report notification" handler to not report when called post-close.
This fits the description of the original bug, so likely fixes it.

Bug: chromium:868337
Change-Id: Ic6757d2fb335203a6a6aacb2c9b52854b40332f7
Reviewed-on: https://webrtc-review.googlesource.com/91121
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Guido Urdaneta <guidou@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24164}
This commit is contained in:
Harald Alvestrand
2018-08-01 10:50:16 +02:00
committed by Commit Bot
parent 9014324bb1
commit 7a1c7f782a
3 changed files with 118 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

97
pc/peerconnection.cc
View File

@ -1130,7 +1130,7 @@ bool PeerConnection::AddStream(MediaStreamInterface* local_stream) {
} }
stats_->AddStream(local_stream); stats_->AddStream(local_stream);
observer_->OnRenegotiationNeeded(); Observer()->OnRenegotiationNeeded();
return true; return true;
} }
@ -1159,7 +1159,7 @@ void PeerConnection::RemoveStream(MediaStreamInterface* local_stream) {
if (IsClosed()) { if (IsClosed()) {
return; return;
} }
observer_->OnRenegotiationNeeded(); Observer()->OnRenegotiationNeeded();
} }
RTCErrorOr<rtc::scoped_refptr<RtpSenderInterface>> PeerConnection::AddTrack( RTCErrorOr<rtc::scoped_refptr<RtpSenderInterface>> PeerConnection::AddTrack(
@ -1187,7 +1187,7 @@ RTCErrorOr<rtc::scoped_refptr<RtpSenderInterface>> PeerConnection::AddTrack(
(IsUnifiedPlan() ? AddTrackUnifiedPlan(track, stream_ids) (IsUnifiedPlan() ? AddTrackUnifiedPlan(track, stream_ids)
: AddTrackPlanB(track, stream_ids)); : AddTrackPlanB(track, stream_ids));
if (sender_or_error.ok()) { if (sender_or_error.ok()) {
observer_->OnRenegotiationNeeded(); Observer()->OnRenegotiationNeeded();
stats_->AddTrack(track); stats_->AddTrack(track);
} }
return sender_or_error; return sender_or_error;
@ -1333,7 +1333,7 @@ RTCError PeerConnection::RemoveTrackNew(
"Couldn't find sender " + sender->id() + " to remove."); "Couldn't find sender " + sender->id() + " to remove.");
} }
} }
observer_->OnRenegotiationNeeded(); Observer()->OnRenegotiationNeeded();
return RTCError::OK(); return RTCError::OK();
} }
@ -1423,7 +1423,7 @@ PeerConnection::AddTransceiver(
transceiver->internal()->set_direction(init.direction); transceiver->internal()->set_direction(init.direction);
if (fire_callback) { if (fire_callback) {
observer_->OnRenegotiationNeeded(); Observer()->OnRenegotiationNeeded();
} }
return rtc::scoped_refptr<RtpTransceiverInterface>(transceiver); return rtc::scoped_refptr<RtpTransceiverInterface>(transceiver);
@ -1498,7 +1498,7 @@ PeerConnection::CreateAndAddTransceiver(
void PeerConnection::OnNegotiationNeeded() { void PeerConnection::OnNegotiationNeeded() {
RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK_RUN_ON(signaling_thread());
RTC_DCHECK(!IsClosed()); RTC_DCHECK(!IsClosed());
observer_->OnRenegotiationNeeded(); Observer()->OnRenegotiationNeeded();
} }
rtc::scoped_refptr<RtpSenderInterface> PeerConnection::CreateSender( rtc::scoped_refptr<RtpSenderInterface> PeerConnection::CreateSender(
@ -1723,7 +1723,7 @@ rtc::scoped_refptr<DataChannelInterface> PeerConnection::CreateDataChannel(
// Trigger the onRenegotiationNeeded event for every new RTP DataChannel, or // Trigger the onRenegotiationNeeded event for every new RTP DataChannel, or
// the first SCTP DataChannel. // the first SCTP DataChannel.
if (data_channel_type() == cricket::DCT_RTP || first_datachannel) { if (data_channel_type() == cricket::DCT_RTP || first_datachannel) {
observer_->OnRenegotiationNeeded(); Observer()->OnRenegotiationNeeded();
} }
NoteUsageEvent(UsageEvent::DATA_ADDED); NoteUsageEvent(UsageEvent::DATA_ADDED);
return DataChannelProxy::Create(signaling_thread(), channel.get()); return DataChannelProxy::Create(signaling_thread(), channel.get());
@ -2082,11 +2082,12 @@ RTCError PeerConnection::ApplyLocalDescription(
transceiver->internal()->set_fired_direction(media_desc->direction()); transceiver->internal()->set_fired_direction(media_desc->direction());
} }
} }
auto observer = Observer();
for (auto transceiver : remove_list) { for (auto transceiver : remove_list) {
observer_->OnRemoveTrack(transceiver->receiver()); observer->OnRemoveTrack(transceiver->receiver());
} }
for (auto stream : removed_streams) { for (auto stream : removed_streams) {
observer_->OnRemoveStream(stream); observer->OnRemoveStream(stream);
} }
} else { } else {
// Media channels will be created only when offer is set. These may use new // Media channels will be created only when offer is set. These may use new
@ -2488,20 +2489,21 @@ RTCError PeerConnection::ApplyRemoteDescription(
} }
} }
// Once all processing has finished, fire off callbacks. // Once all processing has finished, fire off callbacks.
auto observer = Observer();
for (auto transceiver : now_receiving_transceivers) { for (auto transceiver : now_receiving_transceivers) {
stats_->AddTrack(transceiver->receiver()->track()); stats_->AddTrack(transceiver->receiver()->track());
observer_->OnTrack(transceiver); observer->OnTrack(transceiver);
observer_->OnAddTrack(transceiver->receiver(), observer->OnAddTrack(transceiver->receiver(),
transceiver->receiver()->streams()); transceiver->receiver()->streams());
} }
for (auto stream : added_streams) { for (auto stream : added_streams) {
observer_->OnAddStream(stream); observer->OnAddStream(stream);
} }
for (auto transceiver : remove_list) { for (auto transceiver : remove_list) {
observer_->OnRemoveTrack(transceiver->receiver()); observer->OnRemoveTrack(transceiver->receiver());
} }
for (auto stream : removed_streams) { for (auto stream : removed_streams) {
observer_->OnRemoveStream(stream); observer->OnRemoveStream(stream);
} }
} }
@ -2576,10 +2578,11 @@ RTCError PeerConnection::ApplyRemoteDescription(
} }
// Iterate new_streams and notify the observer about new MediaStreams. // Iterate new_streams and notify the observer about new MediaStreams.
auto observer = Observer();
for (size_t i = 0; i < new_streams->count(); ++i) { for (size_t i = 0; i < new_streams->count(); ++i) {
MediaStreamInterface* new_stream = new_streams->at(i); MediaStreamInterface* new_stream = new_streams->at(i);
stats_->AddStream(new_stream); stats_->AddStream(new_stream);
observer_->OnAddStream( observer->OnAddStream(
rtc::scoped_refptr<MediaStreamInterface>(new_stream)); rtc::scoped_refptr<MediaStreamInterface>(new_stream));
} }
@ -3308,6 +3311,9 @@ void PeerConnection::Close() {
event_log_.reset(); event_log_.reset();
}); });
ReportUsagePattern(); ReportUsagePattern();
// The .h file says that observer can be discarded after close() returns.
// Make sure this is true.
observer_ = nullptr;
} }
void PeerConnection::OnMessage(rtc::Message* msg) { void PeerConnection::OnMessage(rtc::Message* msg) {
@ -3391,7 +3397,7 @@ void PeerConnection::CreateAudioReceiver(
auto receiver = RtpReceiverProxyWithInternal<RtpReceiverInternal>::Create( auto receiver = RtpReceiverProxyWithInternal<RtpReceiverInternal>::Create(
signaling_thread(), audio_receiver); signaling_thread(), audio_receiver);
GetAudioTransceiver()->internal()->AddReceiver(receiver); GetAudioTransceiver()->internal()->AddReceiver(receiver);
observer_->OnAddTrack(receiver, std::move(streams)); Observer()->OnAddTrack(receiver, std::move(streams));
NoteUsageEvent(UsageEvent::AUDIO_ADDED); NoteUsageEvent(UsageEvent::AUDIO_ADDED);
} }
@ -3409,7 +3415,7 @@ void PeerConnection::CreateVideoReceiver(
auto receiver = RtpReceiverProxyWithInternal<RtpReceiverInternal>::Create( auto receiver = RtpReceiverProxyWithInternal<RtpReceiverInternal>::Create(
signaling_thread(), video_receiver); signaling_thread(), video_receiver);
GetVideoTransceiver()->internal()->AddReceiver(receiver); GetVideoTransceiver()->internal()->AddReceiver(receiver);
observer_->OnAddTrack(receiver, std::move(streams)); Observer()->OnAddTrack(receiver, std::move(streams));
NoteUsageEvent(UsageEvent::VIDEO_ADDED); NoteUsageEvent(UsageEvent::VIDEO_ADDED);
} }
@ -3531,7 +3537,7 @@ void PeerConnection::SetIceConnectionState(IceConnectionState new_state) {
PeerConnectionInterface::kIceConnectionClosed); PeerConnectionInterface::kIceConnectionClosed);
ice_connection_state_ = new_state; ice_connection_state_ = new_state;
observer_->OnIceConnectionChange(ice_connection_state_); Observer()->OnIceConnectionChange(ice_connection_state_);
} }
void PeerConnection::OnIceGatheringChange( void PeerConnection::OnIceGatheringChange(
@ -3541,7 +3547,7 @@ void PeerConnection::OnIceGatheringChange(
return; return;
} }
ice_gathering_state_ = new_state; ice_gathering_state_ = new_state;
observer_->OnIceGatheringChange(ice_gathering_state_); Observer()->OnIceGatheringChange(ice_gathering_state_);
} }
void PeerConnection::OnIceCandidate( void PeerConnection::OnIceCandidate(
@ -3555,7 +3561,7 @@ void PeerConnection::OnIceCandidate(
candidate->candidate().address().IsPrivateIP()) { candidate->candidate().address().IsPrivateIP()) {
NoteUsageEvent(UsageEvent::PRIVATE_CANDIDATE_COLLECTED); NoteUsageEvent(UsageEvent::PRIVATE_CANDIDATE_COLLECTED);
} }
observer_->OnIceCandidate(candidate.get()); Observer()->OnIceCandidate(candidate.get());
} }
void PeerConnection::OnIceCandidatesRemoved( void PeerConnection::OnIceCandidatesRemoved(
@ -3564,7 +3570,7 @@ void PeerConnection::OnIceCandidatesRemoved(
if (IsClosed()) { if (IsClosed()) {
return; return;
} }
observer_->OnIceCandidatesRemoved(candidates); Observer()->OnIceCandidatesRemoved(candidates);
} }
void PeerConnection::ChangeSignalingState( void PeerConnection::ChangeSignalingState(
@ -3580,13 +3586,13 @@ void PeerConnection::ChangeSignalingState(
signaling_state_ = signaling_state; signaling_state_ = signaling_state;
if (signaling_state == kClosed) { if (signaling_state == kClosed) {
ice_connection_state_ = kIceConnectionClosed; ice_connection_state_ = kIceConnectionClosed;
observer_->OnIceConnectionChange(ice_connection_state_); Observer()->OnIceConnectionChange(ice_connection_state_);
if (ice_gathering_state_ != kIceGatheringComplete) { if (ice_gathering_state_ != kIceGatheringComplete) {
ice_gathering_state_ = kIceGatheringComplete; ice_gathering_state_ = kIceGatheringComplete;
observer_->OnIceGatheringChange(ice_gathering_state_); Observer()->OnIceGatheringChange(ice_gathering_state_);
} }
} }
observer_->OnSignalingChange(signaling_state_); Observer()->OnSignalingChange(signaling_state_);
} }
void PeerConnection::OnAudioTrackAdded(AudioTrackInterface* track, void PeerConnection::OnAudioTrackAdded(AudioTrackInterface* track,
@ -3595,7 +3601,7 @@ void PeerConnection::OnAudioTrackAdded(AudioTrackInterface* track,
return; return;
} }
AddAudioTrack(track, stream); AddAudioTrack(track, stream);
observer_->OnRenegotiationNeeded(); Observer()->OnRenegotiationNeeded();
} }
void PeerConnection::OnAudioTrackRemoved(AudioTrackInterface* track, void PeerConnection::OnAudioTrackRemoved(AudioTrackInterface* track,
@ -3604,7 +3610,7 @@ void PeerConnection::OnAudioTrackRemoved(AudioTrackInterface* track,
return; return;
} }
RemoveAudioTrack(track, stream); RemoveAudioTrack(track, stream);
observer_->OnRenegotiationNeeded(); Observer()->OnRenegotiationNeeded();
} }
void PeerConnection::OnVideoTrackAdded(VideoTrackInterface* track, void PeerConnection::OnVideoTrackAdded(VideoTrackInterface* track,
@ -3613,7 +3619,7 @@ void PeerConnection::OnVideoTrackAdded(VideoTrackInterface* track,
return; return;
} }
AddVideoTrack(track, stream); AddVideoTrack(track, stream);
observer_->OnRenegotiationNeeded(); Observer()->OnRenegotiationNeeded();
} }
void PeerConnection::OnVideoTrackRemoved(VideoTrackInterface* track, void PeerConnection::OnVideoTrackRemoved(VideoTrackInterface* track,
@ -3622,7 +3628,7 @@ void PeerConnection::OnVideoTrackRemoved(VideoTrackInterface* track,
return; return;
} }
RemoveVideoTrack(track, stream); RemoveVideoTrack(track, stream);
observer_->OnRenegotiationNeeded(); Observer()->OnRenegotiationNeeded();
} }
void PeerConnection::PostSetSessionDescriptionSuccess( void PeerConnection::PostSetSessionDescriptionSuccess(
@ -4264,7 +4270,7 @@ void PeerConnection::OnRemoteSenderRemoved(const RtpSenderInfo& sender_info,
RTC_NOTREACHED() << "Invalid media type"; RTC_NOTREACHED() << "Invalid media type";
} }
if (receiver) { if (receiver) {
observer_->OnRemoveTrack(receiver); Observer()->OnRemoveTrack(receiver);
} }
} }
@ -4279,7 +4285,7 @@ void PeerConnection::UpdateEndedRemoteMediaStreams() {
for (auto& stream : streams_to_remove) { for (auto& stream : streams_to_remove) {
remote_streams_->RemoveStream(stream); remote_streams_->RemoveStream(stream);
observer_->OnRemoveStream(std::move(stream)); Observer()->OnRemoveStream(std::move(stream));
} }
} }
@ -4451,7 +4457,7 @@ void PeerConnection::CreateRemoteRtpDataChannel(const std::string& label,
channel->SetReceiveSsrc(remote_ssrc); channel->SetReceiveSsrc(remote_ssrc);
rtc::scoped_refptr<DataChannelInterface> proxy_channel = rtc::scoped_refptr<DataChannelInterface> proxy_channel =
DataChannelProxy::Create(signaling_thread(), channel); DataChannelProxy::Create(signaling_thread(), channel);
observer_->OnDataChannel(std::move(proxy_channel)); Observer()->OnDataChannel(std::move(proxy_channel));
} }
rtc::scoped_refptr<DataChannel> PeerConnection::InternalCreateDataChannel( rtc::scoped_refptr<DataChannel> PeerConnection::InternalCreateDataChannel(
@ -4574,7 +4580,7 @@ void PeerConnection::OnDataChannelOpenMessage(
rtc::scoped_refptr<DataChannelInterface> proxy_channel = rtc::scoped_refptr<DataChannelInterface> proxy_channel =
DataChannelProxy::Create(signaling_thread(), channel); DataChannelProxy::Create(signaling_thread(), channel);
observer_->OnDataChannel(std::move(proxy_channel)); Observer()->OnDataChannel(std::move(proxy_channel));
NoteUsageEvent(UsageEvent::DATA_ADDED); NoteUsageEvent(UsageEvent::DATA_ADDED);
} }
@ -5957,7 +5963,15 @@ void PeerConnection::ReportUsagePattern() const {
static_cast<int>(UsageEvent::ICE_STATE_CONNECTED); static_cast<int>(UsageEvent::ICE_STATE_CONNECTED);
if ((usage_event_accumulator_ & bad_bits) == bad_bits && if ((usage_event_accumulator_ & bad_bits) == bad_bits &&
(usage_event_accumulator_ & good_bits) == 0) { (usage_event_accumulator_ & good_bits) == 0) {
observer_->OnInterestingUsage(usage_event_accumulator_); // If called after close(), we can't report, because observer may have
// been deallocated, and therefore pointer is null. Write to log instead.
if (observer_) {
Observer()->OnInterestingUsage(usage_event_accumulator_);
} else {
RTC_LOG(LS_INFO) << "Interesting usage signature "
<< usage_event_accumulator_
<< " observed after observer shutdown";
}
} }
} }
@ -6275,10 +6289,25 @@ bool PeerConnection::OnTransportChanged(
return ret; return ret;
} }
PeerConnectionObserver* PeerConnection::Observer() const {
// In earlier production code, the pointer was not cleared on close,
// which might have led to undefined behavior if the observer was not
// deallocated, or strange crashes if it was.
// We use CHECK in order to catch such behavior if it exists.
// TODO(hta): Remove or replace with DCHECK if nothing is found.
RTC_CHECK(observer_);
return observer_;
}
void PeerConnection::ClearStatsCache() { void PeerConnection::ClearStatsCache() {
if (stats_collector_) { if (stats_collector_) {
stats_collector_->ClearCachedStatsReport(); stats_collector_->ClearCachedStatsReport();
} }
} }
void PeerConnection::RequestUsagePatternReportForTesting() {
signaling_thread()->Post(RTC_FROM_HERE, this, MSG_REPORT_USAGE_PATTERN,
nullptr);
}
} // namespace webrtc } // namespace webrtc

4
pc/peerconnection.h
View File

@ -267,6 +267,7 @@ class PeerConnection : public PeerConnectionInternal,
void ReturnHistogramVeryQuicklyForTesting() { void ReturnHistogramVeryQuicklyForTesting() {
return_histogram_very_quickly_ = true; return_histogram_very_quickly_ = true;
} }
void RequestUsagePatternReportForTesting();
protected: protected:
~PeerConnection() override; ~PeerConnection() override;
@ -915,6 +916,9 @@ class PeerConnection : public PeerConnectionInternal,
RtpTransportInternal* rtp_transport, RtpTransportInternal* rtp_transport,
cricket::DtlsTransportInternal* dtls_transport) override; cricket::DtlsTransportInternal* dtls_transport) override;
// Returns the observer. Will crash on CHECK if the observer is removed.
PeerConnectionObserver* Observer() const;
sigslot::signal1<DataChannel*> SignalDataChannelCreated_; sigslot::signal1<DataChannel*> SignalDataChannelCreated_;
// Storing the factory as a scoped reference pointer ensures that the memory // Storing the factory as a scoped reference pointer ensures that the memory

51
pc/peerconnection_histogram_unittest.cc
View File

@ -91,6 +91,10 @@ class ObserverForUsageHistogramTest : public MockPeerConnectionObserver {
return interesting_usage_detected_; return interesting_usage_detected_;
} }
void ClearInterestingUsageDetector() {
interesting_usage_detected_ = absl::optional<int>();
}
private: private:
absl::optional<int> interesting_usage_detected_; absl::optional<int> interesting_usage_detected_;
RawWrapperPtr candidate_target_; // Note: Not thread-safe against deletions. RawWrapperPtr candidate_target_; // Note: Not thread-safe against deletions.
@ -454,6 +458,53 @@ TEST_F(PeerConnectionUsageHistogramTest, NotableUsageNoted) {
EXPECT_EQ(absl::make_optional(ObservedFingerprint()), EXPECT_EQ(absl::make_optional(ObservedFingerprint()),
caller->observer()->interesting_usage_detected()); caller->observer()->interesting_usage_detected());
} }
TEST_F(PeerConnectionUsageHistogramTest, NotableUsageOnEventFiring) {
auto caller = CreatePeerConnection();
caller->CreateDataChannel("foo");
caller->GenerateOfferAndCollectCandidates();
int expected_fingerprint = MakeUsageFingerprint(
{PeerConnection::UsageEvent::DATA_ADDED,
PeerConnection::UsageEvent::SET_LOCAL_DESCRIPTION_CALLED,
PeerConnection::UsageEvent::CANDIDATE_COLLECTED});
EXPECT_EQ(0, webrtc::metrics::NumSamples(kUsagePatternMetric));
caller->GetInternalPeerConnection()->RequestUsagePatternReportForTesting();
EXPECT_EQ_WAIT(1, webrtc::metrics::NumSamples(kUsagePatternMetric),
kDefaultTimeout);
EXPECT_TRUE(expected_fingerprint == ObservedFingerprint() ||
(expected_fingerprint |
static_cast<int>(
PeerConnection::UsageEvent::PRIVATE_CANDIDATE_COLLECTED)) ==
ObservedFingerprint());
EXPECT_EQ(absl::make_optional(ObservedFingerprint()),
caller->observer()->interesting_usage_detected());
}
TEST_F(PeerConnectionUsageHistogramTest,
NoNotableUsageOnEventFiringAfterClose) {
auto caller = CreatePeerConnection();
caller->CreateDataChannel("foo");
caller->GenerateOfferAndCollectCandidates();
int expected_fingerprint = MakeUsageFingerprint(
{PeerConnection::UsageEvent::DATA_ADDED,
PeerConnection::UsageEvent::SET_LOCAL_DESCRIPTION_CALLED,
PeerConnection::UsageEvent::CANDIDATE_COLLECTED,
PeerConnection::UsageEvent::CLOSE_CALLED});
EXPECT_EQ(0, webrtc::metrics::NumSamples(kUsagePatternMetric));
caller->pc()->Close();
EXPECT_EQ(1, webrtc::metrics::NumSamples(kUsagePatternMetric));
caller->GetInternalPeerConnection()->RequestUsagePatternReportForTesting();
caller->observer()->ClearInterestingUsageDetector();
EXPECT_EQ_WAIT(2, webrtc::metrics::NumSamples(kUsagePatternMetric),
kDefaultTimeout);
EXPECT_TRUE(expected_fingerprint == ObservedFingerprint() ||
(expected_fingerprint |
static_cast<int>(
PeerConnection::UsageEvent::PRIVATE_CANDIDATE_COLLECTED)) ==
ObservedFingerprint());
// After close, the usage-detection callback should NOT have been called.
EXPECT_FALSE(caller->observer()->interesting_usage_detected());
}
#endif #endif
#endif #endif