From 0f620f4e318a162e7a251133e7a8ddea5188b9bb Mon Sep 17 00:00:00 2001 From: tommi Date: Thu, 9 Jul 2015 03:25:02 -0700 Subject: [PATCH] Make sure we process all pending offer/answer requests before terminating. This fixes a bug in the WebRtcSessionDescriptionFactory where messages would be dropped or worse yet processed after the factory was deleted. BUG=chromium:507307 Review URL: https://codereview.webrtc.org/1231823002 Cr-Commit-Position: refs/heads/master@{#9557} --- talk/app/webrtc/dtlsidentitystore.cc | 1 - talk/app/webrtc/dtlsidentitystore_unittest.cc | 1 - talk/app/webrtc/peerconnection.cc | 1 + talk/app/webrtc/webrtcsession.cc | 1 + talk/app/webrtc/webrtcsession_unittest.cc | 32 ++++++++++++++++ .../webrtc/webrtcsessiondescriptionfactory.cc | 38 ++++++++++++++----- .../webrtc/webrtcsessiondescriptionfactory.h | 15 ++++---- 7 files changed, 70 insertions(+), 19 deletions(-) diff --git a/talk/app/webrtc/dtlsidentitystore.cc b/talk/app/webrtc/dtlsidentitystore.cc index abd35e1fab..554a84a06f 100644 --- a/talk/app/webrtc/dtlsidentitystore.cc +++ b/talk/app/webrtc/dtlsidentitystore.cc @@ -31,7 +31,6 @@ #include "webrtc/base/logging.h" using webrtc::DTLSIdentityRequestObserver; -using webrtc::WebRtcSessionDescriptionFactory; namespace webrtc { diff --git a/talk/app/webrtc/dtlsidentitystore_unittest.cc b/talk/app/webrtc/dtlsidentitystore_unittest.cc index c0b204a85f..12f58feea5 100644 --- a/talk/app/webrtc/dtlsidentitystore_unittest.cc +++ b/talk/app/webrtc/dtlsidentitystore_unittest.cc @@ -33,7 +33,6 @@ #include "webrtc/base/ssladapter.h" using webrtc::DtlsIdentityStore; -using webrtc::WebRtcSessionDescriptionFactory; static const int kTimeoutMs = 10000; diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc index 487b975ce3..0a243077bc 100644 --- a/talk/app/webrtc/peerconnection.cc +++ b/talk/app/webrtc/peerconnection.cc @@ -338,6 +338,7 @@ PeerConnection::PeerConnection(PeerConnectionFactory* factory) } PeerConnection::~PeerConnection() { + ASSERT(signaling_thread()->IsCurrent()); if (mediastream_signaling_) mediastream_signaling_->TearDown(); if (stream_handler_container_) diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc index a3ff934dbc..2533328968 100644 --- a/talk/app/webrtc/webrtcsession.cc +++ b/talk/app/webrtc/webrtcsession.cc @@ -499,6 +499,7 @@ WebRtcSession::WebRtcSession( } WebRtcSession::~WebRtcSession() { + ASSERT(signaling_thread()->IsCurrent()); // Destroy video_channel_ first since it may have a pointer to the // voice_channel_. if (video_channel_) { diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc index c0938d11ea..0111f53cee 100644 --- a/talk/app/webrtc/webrtcsession_unittest.cc +++ b/talk/app/webrtc/webrtcsession_unittest.cc @@ -3852,6 +3852,38 @@ TEST_F(WebRtcSessionTest, TestSetSocketOptionBeforeBundle) { EXPECT_EQ(8000, option_val); } +// Test creating a session, request multiple offers, destroy the session +// and make sure we got success/failure callbacks for all of the requests. +// Background: crbug.com/507307 +TEST_F(WebRtcSessionTest, CreateOffersAndShutdown) { + Init(); + + rtc::scoped_refptr observers[100]; + PeerConnectionInterface::RTCOfferAnswerOptions options; + options.offer_to_receive_audio = + RTCOfferAnswerOptions::kOfferToReceiveMediaTrue; + + for (auto& o : observers) { + o = new WebRtcSessionCreateSDPObserverForTest(); + session_->CreateOffer(o, options); + } + + session_.reset(); + + // Make sure we process pending messages on the current (signaling) thread + // before checking we we got our callbacks. Quit() will do this and then + // immediately exit. We won't need the queue after this point anyway. + rtc::Thread::Current()->Quit(); + + for (auto& o : observers) { + // We expect to have received a notification now even if the session was + // terminated. The offer creation may or may not have succeeded, but we + // must have received a notification which, so the only invalid state + // is kInit. + EXPECT_NE(WebRtcSessionCreateSDPObserverForTest::kInit, o->state()); + } +} + // TODO(bemasc): Add a TestIceStatesBundle with BUNDLE enabled. That test // currently fails because upon disconnection and reconnection OnIceComplete is // called more than once without returning to IceGatheringGathering. diff --git a/talk/app/webrtc/webrtcsessiondescriptionfactory.cc b/talk/app/webrtc/webrtcsessiondescriptionfactory.cc index aab24cf065..1909b0ed78 100644 --- a/talk/app/webrtc/webrtcsessiondescriptionfactory.cc +++ b/talk/app/webrtc/webrtcsessiondescriptionfactory.cc @@ -40,6 +40,8 @@ namespace webrtc { namespace { static const char kFailedDueToIdentityFailed[] = " failed because DTLS identity request failed"; +static const char kFailedDueToSessionShutdown[] = + " failed because the session was shut down"; static const uint64 kInitSessionVersion = 2; @@ -183,6 +185,18 @@ WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory( } WebRtcSessionDescriptionFactory::~WebRtcSessionDescriptionFactory() { + ASSERT(signaling_thread_->IsCurrent()); + + // Fail any requests that were asked for before identity generation completed. + FailPendingRequests(kFailedDueToSessionShutdown); + + // Process all pending notifications in the message queue. If we don't do + // this, requests will linger and not know they succeeded or failed. + rtc::MessageList list; + signaling_thread_->Clear(this, rtc::MQID_ANY, &list); + for (auto& msg : list) + OnMessage(&msg); + transport_desc_factory_.set_identity(NULL); } @@ -400,6 +414,19 @@ void WebRtcSessionDescriptionFactory::InternalCreateAnswer( PostCreateSessionDescriptionSucceeded(request.observer, answer); } +void WebRtcSessionDescriptionFactory::FailPendingRequests( + const std::string& reason) { + ASSERT(signaling_thread_->IsCurrent()); + while (!create_session_description_requests_.empty()) { + const CreateSessionDescriptionRequest& request = + create_session_description_requests_.front(); + PostCreateSessionDescriptionFailed(request.observer, + ((request.type == CreateSessionDescriptionRequest::kOffer) ? + "CreateOffer" : "CreateAnswer") + reason); + create_session_description_requests_.pop(); + } +} + void WebRtcSessionDescriptionFactory::PostCreateSessionDescriptionFailed( CreateSessionDescriptionObserver* observer, const std::string& error) { CreateSessionDescriptionMsg* msg = new CreateSessionDescriptionMsg(observer); @@ -422,16 +449,7 @@ void WebRtcSessionDescriptionFactory::OnIdentityRequestFailed(int error) { LOG(LS_ERROR) << "Async identity request failed: error = " << error; identity_request_state_ = IDENTITY_FAILED; - std::string msg = kFailedDueToIdentityFailed; - while (!create_session_description_requests_.empty()) { - const CreateSessionDescriptionRequest& request = - create_session_description_requests_.front(); - PostCreateSessionDescriptionFailed( - request.observer, - ((request.type == CreateSessionDescriptionRequest::kOffer) ? - "CreateOffer" : "CreateAnswer") + msg); - create_session_description_requests_.pop(); - } + FailPendingRequests(kFailedDueToIdentityFailed); } void WebRtcSessionDescriptionFactory::SetIdentity( diff --git a/talk/app/webrtc/webrtcsessiondescriptionfactory.h b/talk/app/webrtc/webrtcsessiondescriptionfactory.h index 41798a485f..860532dec9 100644 --- a/talk/app/webrtc/webrtcsessiondescriptionfactory.h +++ b/talk/app/webrtc/webrtcsessiondescriptionfactory.h @@ -133,6 +133,8 @@ class WebRtcSessionDescriptionFactory : public rtc::MessageHandler, void InternalCreateOffer(CreateSessionDescriptionRequest request); void InternalCreateAnswer(CreateSessionDescriptionRequest request); + // Posts failure notifications for all pending session description requests. + void FailPendingRequests(const std::string& reason); void PostCreateSessionDescriptionFailed( CreateSessionDescriptionObserver* observer, const std::string& error); @@ -145,17 +147,16 @@ class WebRtcSessionDescriptionFactory : public rtc::MessageHandler, std::queue create_session_description_requests_; - rtc::Thread* signaling_thread_; - MediaStreamSignaling* mediastream_signaling_; + rtc::Thread* const signaling_thread_; + MediaStreamSignaling* const mediastream_signaling_; cricket::TransportDescriptionFactory transport_desc_factory_; cricket::MediaSessionDescriptionFactory session_desc_factory_; uint64 session_version_; rtc::scoped_ptr identity_service_; - rtc::scoped_refptr - identity_request_observer_; - WebRtcSession* session_; - std::string session_id_; - cricket::DataChannelType data_channel_type_; + rtc::scoped_refptr identity_request_observer_; + WebRtcSession* const session_; + const std::string session_id_; + const cricket::DataChannelType data_channel_type_; IdentityRequestState identity_request_state_; DISALLOW_COPY_AND_ASSIGN(WebRtcSessionDescriptionFactory);