From 141aacbf0b2df237e3e509ead278b770a2a04ad7 Mon Sep 17 00:00:00 2001 From: zhihuang Date: Tue, 29 Aug 2017 13:23:53 -0700 Subject: [PATCH] Fix the Chromium crash when creating an answer without a remote description. Replacing the RTC_DCHECK with an if condition so that the method GetOptionsForAnswer won't crash when there is no remote description. WebRtcSessionDescriptionFactory will handle the null remote description. TBR=deadbeef@webrtc.org BUG=chromium:757830 Review-Url: https://codereview.webrtc.org/3006723002 Cr-Commit-Position: refs/heads/master@{#19589} --- webrtc/pc/peerconnection.cc | 25 +++--- webrtc/pc/peerconnectioninterface_unittest.cc | 81 +++++++++++-------- 2 files changed, 60 insertions(+), 46 deletions(-) diff --git a/webrtc/pc/peerconnection.cc b/webrtc/pc/peerconnection.cc index 6112357ce1..e22a09ece0 100644 --- a/webrtc/pc/peerconnection.cc +++ b/webrtc/pc/peerconnection.cc @@ -1787,18 +1787,19 @@ void PeerConnection::GetOptionsForAnswer( rtc::Optional audio_index; rtc::Optional video_index; rtc::Optional data_index; - // There should be a pending remote description that's an offer... - RTC_DCHECK(session_->remote_description()); - RTC_DCHECK(session_->remote_description()->type() == - SessionDescriptionInterface::kOffer); - // Generate m= sections that match those in the offer. - // Note that mediasession.cc will handle intersection our preferred direction - // with the offered direction. - GenerateMediaDescriptionOptions( - session_->remote_description(), - cricket::RtpTransceiverDirection(send_audio, recv_audio), - cricket::RtpTransceiverDirection(send_video, recv_video), &audio_index, - &video_index, &data_index, session_options); + if (session_->remote_description()) { + // The pending remote description should be an offer. + RTC_DCHECK(session_->remote_description()->type() == + SessionDescriptionInterface::kOffer); + // Generate m= sections that match those in the offer. + // Note that mediasession.cc will handle intersection our preferred + // direction with the offered direction. + GenerateMediaDescriptionOptions( + session_->remote_description(), + cricket::RtpTransceiverDirection(send_audio, recv_audio), + cricket::RtpTransceiverDirection(send_video, recv_video), &audio_index, + &video_index, &data_index, session_options); + } cricket::MediaDescriptionOptions* audio_media_description_options = !audio_index ? nullptr diff --git a/webrtc/pc/peerconnectioninterface_unittest.cc b/webrtc/pc/peerconnectioninterface_unittest.cc index ead950cca7..c23a79d597 100644 --- a/webrtc/pc/peerconnectioninterface_unittest.cc +++ b/webrtc/pc/peerconnectioninterface_unittest.cc @@ -3771,6 +3771,53 @@ TEST_F(PeerConnectionInterfaceTest, EXPECT_TRUE(video_content->rejected); } +// This test ensures OnRenegotiationNeeded is called when we add track with +// MediaStream -> AddTrack in the same way it is called when we add track with +// PeerConnection -> AddTrack. +// The test can be removed once addStream is rewritten in terms of addTrack +// https://bugs.chromium.org/p/webrtc/issues/detail?id=7815 +TEST_F(PeerConnectionInterfaceTest, MediaStreamAddTrackRemoveTrackRenegotiate) { + CreatePeerConnectionWithoutDtls(); + rtc::scoped_refptr stream( + pc_factory_->CreateLocalMediaStream(kStreamLabel1)); + pc_->AddStream(stream); + rtc::scoped_refptr audio_track( + pc_factory_->CreateAudioTrack("audio_track", nullptr)); + rtc::scoped_refptr video_track( + pc_factory_->CreateVideoTrack( + "video_track", pc_factory_->CreateVideoSource( + std::unique_ptr( + new cricket::FakeVideoCapturer())))); + stream->AddTrack(audio_track); + EXPECT_TRUE_WAIT(observer_.renegotiation_needed_, kTimeout); + observer_.renegotiation_needed_ = false; + + stream->AddTrack(video_track); + EXPECT_TRUE_WAIT(observer_.renegotiation_needed_, kTimeout); + observer_.renegotiation_needed_ = false; + + stream->RemoveTrack(audio_track); + EXPECT_TRUE_WAIT(observer_.renegotiation_needed_, kTimeout); + observer_.renegotiation_needed_ = false; + + stream->RemoveTrack(video_track); + EXPECT_TRUE_WAIT(observer_.renegotiation_needed_, kTimeout); + observer_.renegotiation_needed_ = false; +} + +// Tests that creating answer would fail gracefully without being crashed if the +// remote description is unset. +TEST_F(PeerConnectionInterfaceTest, CreateAnswerWithoutRemoteDescription) { + CreatePeerConnection(); + // Creating answer fails because the remote description is unset. + std::unique_ptr answer; + EXPECT_FALSE(DoCreateAnswer(&answer, nullptr)); + + // Createing answer succeeds when the remote description is set. + CreateOfferAsRemoteDescription(); + EXPECT_TRUE(DoCreateAnswer(&answer, nullptr)); +} + class PeerConnectionMediaConfigTest : public testing::Test { protected: void SetUp() override { @@ -3891,37 +3938,3 @@ TEST(RTCConfigurationTest, ComparisonOperators) { PeerConnectionInterface::RTCConfigurationType::kAggressive); EXPECT_NE(a, h); } - -// This test ensures OnRenegotiationNeeded is called when we add track with -// MediaStream -> AddTrack in the same way it is called when we add track with -// PeerConnection -> AddTrack. -// The test can be removed once addStream is rewritten in terms of addTrack -// https://bugs.chromium.org/p/webrtc/issues/detail?id=7815 -TEST_F(PeerConnectionInterfaceTest, MediaStreamAddTrackRemoveTrackRenegotiate) { - CreatePeerConnectionWithoutDtls(); - rtc::scoped_refptr stream( - pc_factory_->CreateLocalMediaStream(kStreamLabel1)); - pc_->AddStream(stream); - rtc::scoped_refptr audio_track( - pc_factory_->CreateAudioTrack("audio_track", nullptr)); - rtc::scoped_refptr video_track( - pc_factory_->CreateVideoTrack( - "video_track", pc_factory_->CreateVideoSource( - std::unique_ptr( - new cricket::FakeVideoCapturer())))); - stream->AddTrack(audio_track); - EXPECT_TRUE_WAIT(observer_.renegotiation_needed_, kTimeout); - observer_.renegotiation_needed_ = false; - - stream->AddTrack(video_track); - EXPECT_TRUE_WAIT(observer_.renegotiation_needed_, kTimeout); - observer_.renegotiation_needed_ = false; - - stream->RemoveTrack(audio_track); - EXPECT_TRUE_WAIT(observer_.renegotiation_needed_, kTimeout); - observer_.renegotiation_needed_ = false; - - stream->RemoveTrack(video_track); - EXPECT_TRUE_WAIT(observer_.renegotiation_needed_, kTimeout); - observer_.renegotiation_needed_ = false; -}