From ddeec048c093da6e8e3dc17e599672681fa4def7 Mon Sep 17 00:00:00 2001 From: "jiayl@webrtc.org" Date: Thu, 12 Jun 2014 21:42:46 +0000 Subject: [PATCH] Revert r6390 "Adds end to end DataChannel tests." Flaky on linux_memcheck This reverts commit c3272a942f04f9dd0db3f6bf0d201bcf47c3fa08. TBR=wu@webrtc.org BUG=2626 Review URL: https://webrtc-codereview.appspot.com/13689004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@6420 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../webrtc/peerconnectionendtoend_unittest.cc | 197 ------------------ .../webrtc/test/mockpeerconnectionobservers.h | 5 +- .../webrtc/test/peerconnectiontestwrapper.cc | 12 -- .../webrtc/test/peerconnectiontestwrapper.h | 7 +- talk/media/sctp/sctpdataengine.cc | 11 +- 5 files changed, 10 insertions(+), 222 deletions(-) diff --git a/talk/app/webrtc/peerconnectionendtoend_unittest.cc b/talk/app/webrtc/peerconnectionendtoend_unittest.cc index f701e0635f..99597202b2 100644 --- a/talk/app/webrtc/peerconnectionendtoend_unittest.cc +++ b/talk/app/webrtc/peerconnectionendtoend_unittest.cc @@ -26,7 +26,6 @@ */ #include "talk/app/webrtc/test/peerconnectiontestwrapper.h" -#include "talk/app/webrtc/test/mockpeerconnectionobservers.h" #include "talk/base/gunit.h" #include "talk/base/logging.h" #include "talk/base/ssladapter.h" @@ -34,13 +33,6 @@ #include "talk/base/stringencode.h" #include "talk/base/stringutils.h" -#define MAYBE_SKIP_TEST(feature) \ - if (!(feature())) { \ - LOG(LS_INFO) << "Feature disabled... skipping"; \ - return; \ - } - -using webrtc::DataChannelInterface; using webrtc::FakeConstraints; using webrtc::MediaConstraintsInterface; using webrtc::MediaStreamInterface; @@ -50,7 +42,6 @@ namespace { const char kExternalGiceUfrag[] = "1234567890123456"; const char kExternalGicePwd[] = "123456789012345678901234"; -const size_t kMaxWait = 10000; void RemoveLinesFromSdp(const std::string& line_start, std::string* sdp) { @@ -126,9 +117,6 @@ class PeerConnectionEndToEndTest : public sigslot::has_slots<>, public testing::Test { public: - typedef std::vector > - DataChannelList; - PeerConnectionEndToEndTest() : caller_(new talk_base::RefCountedObject( "caller")), @@ -145,11 +133,6 @@ class PeerConnectionEndToEndTest EXPECT_TRUE(caller_->CreatePc(pc_constraints)); EXPECT_TRUE(callee_->CreatePc(pc_constraints)); PeerConnectionTestWrapper::Connect(caller_.get(), callee_.get()); - - caller_->SignalOnDataChannel.connect( - this, &PeerConnectionEndToEndTest::OnCallerAddedDataChanel); - callee_->SignalOnDataChannel.connect( - this, &PeerConnectionEndToEndTest::OnCalleeAddedDataChannel); } void GetAndAddUserMedia() { @@ -175,11 +158,6 @@ class PeerConnectionEndToEndTest callee_->WaitForCallEstablished(); } - void WaitForConnection() { - caller_->WaitForConnection(); - callee_->WaitForConnection(); - } - void SetupLegacySdpConverter() { caller_->SignalOnSdpCreated.connect( this, &PeerConnectionEndToEndTest::ConvertToLegacySdp); @@ -211,57 +189,6 @@ class PeerConnectionEndToEndTest LOG(LS_INFO) << "AddGiceCredsToCandidate: " << *sdp; } - void OnCallerAddedDataChanel(DataChannelInterface* dc) { - caller_signaled_data_channels_.push_back(dc); - } - - void OnCalleeAddedDataChannel(DataChannelInterface* dc) { - callee_signaled_data_channels_.push_back(dc); - } - - // Tests that |dc1| and |dc2| can send to and receive from each other. - void TestDataChannelSendAndReceive( - DataChannelInterface* dc1, DataChannelInterface* dc2) { - talk_base::scoped_ptr dc1_observer( - new webrtc::MockDataChannelObserver(dc1)); - - talk_base::scoped_ptr dc2_observer( - new webrtc::MockDataChannelObserver(dc2)); - - static const std::string kDummyData = "abcdefg"; - webrtc::DataBuffer buffer(kDummyData); - EXPECT_TRUE(dc1->Send(buffer)); - EXPECT_EQ_WAIT(kDummyData, dc2_observer->last_message(), kMaxWait); - - EXPECT_TRUE(dc2->Send(buffer)); - EXPECT_EQ_WAIT(kDummyData, dc1_observer->last_message(), kMaxWait); - - EXPECT_EQ(1U, dc1_observer->received_message_count()); - EXPECT_EQ(1U, dc2_observer->received_message_count()); - } - - void WaitForDataChannelsToOpen(DataChannelInterface* local_dc, - const DataChannelList& remote_dc_list, - size_t remote_dc_index) { - EXPECT_EQ_WAIT(DataChannelInterface::kOpen, local_dc->state(), kMaxWait); - - EXPECT_TRUE_WAIT(remote_dc_list.size() > remote_dc_index, kMaxWait); - EXPECT_EQ_WAIT(DataChannelInterface::kOpen, - remote_dc_list[remote_dc_index]->state(), - kMaxWait); - EXPECT_EQ(local_dc->id(), remote_dc_list[remote_dc_index]->id()); - } - - void CloseDataChannels(DataChannelInterface* local_dc, - const DataChannelList& remote_dc_list, - size_t remote_dc_index) { - local_dc->Close(); - EXPECT_EQ_WAIT(DataChannelInterface::kClosed, local_dc->state(), kMaxWait); - EXPECT_EQ_WAIT(DataChannelInterface::kClosed, - remote_dc_list[remote_dc_index]->state(), - kMaxWait); - } - ~PeerConnectionEndToEndTest() { talk_base::CleanupSSL(); } @@ -269,8 +196,6 @@ class PeerConnectionEndToEndTest protected: talk_base::scoped_refptr caller_; talk_base::scoped_refptr callee_; - DataChannelList caller_signaled_data_channels_; - DataChannelList callee_signaled_data_channels_; }; // Disable for TSan v2, see @@ -297,126 +222,4 @@ TEST_F(PeerConnectionEndToEndTest, DISABLED_CallWithLegacySdp) { WaitForCallEstablished(); } -// Verifies that a DataChannel created before the negotiation can transition to -// "OPEN" and transfer data. -TEST_F(PeerConnectionEndToEndTest, CreateDataChannelBeforeNegotiate) { - MAYBE_SKIP_TEST(talk_base::SSLStreamAdapter::HaveDtlsSrtp); - - CreatePcs(); - - webrtc::DataChannelInit init; - talk_base::scoped_refptr caller_dc( - caller_->CreateDataChannel("data", init)); - talk_base::scoped_refptr callee_dc( - callee_->CreateDataChannel("data", init)); - - Negotiate(); - WaitForConnection(); - - WaitForDataChannelsToOpen(caller_dc, callee_signaled_data_channels_, 0); - WaitForDataChannelsToOpen(callee_dc, caller_signaled_data_channels_, 0); - - TestDataChannelSendAndReceive(caller_dc, callee_signaled_data_channels_[0]); - TestDataChannelSendAndReceive(callee_dc, caller_signaled_data_channels_[0]); - - CloseDataChannels(caller_dc, callee_signaled_data_channels_, 0); - CloseDataChannels(callee_dc, caller_signaled_data_channels_, 0); -} - -// Verifies that a DataChannel created after the negotiation can transition to -// "OPEN" and transfer data. -TEST_F(PeerConnectionEndToEndTest, CreateDataChannelAfterNegotiate) { - MAYBE_SKIP_TEST(talk_base::SSLStreamAdapter::HaveDtlsSrtp); - - CreatePcs(); - - webrtc::DataChannelInit init; - - // This DataChannel is for creating the data content in the negotiation. - talk_base::scoped_refptr dummy( - caller_->CreateDataChannel("data", init)); - Negotiate(); - WaitForConnection(); - - // Creates new DataChannels after the negotiation and verifies their states. - talk_base::scoped_refptr caller_dc( - caller_->CreateDataChannel("hello", init)); - talk_base::scoped_refptr callee_dc( - callee_->CreateDataChannel("hello", init)); - - WaitForDataChannelsToOpen(caller_dc, callee_signaled_data_channels_, 1); - WaitForDataChannelsToOpen(callee_dc, caller_signaled_data_channels_, 0); - - TestDataChannelSendAndReceive(caller_dc, callee_signaled_data_channels_[1]); - TestDataChannelSendAndReceive(callee_dc, caller_signaled_data_channels_[0]); - - CloseDataChannels(caller_dc, callee_signaled_data_channels_, 1); - CloseDataChannels(callee_dc, caller_signaled_data_channels_, 0); -} - -// Verifies that DataChannel IDs are even/odd based on the DTLS roles. -TEST_F(PeerConnectionEndToEndTest, DataChannelIdAssignment) { - MAYBE_SKIP_TEST(talk_base::SSLStreamAdapter::HaveDtlsSrtp); - - CreatePcs(); - - webrtc::DataChannelInit init; - talk_base::scoped_refptr caller_dc_1( - caller_->CreateDataChannel("data", init)); - talk_base::scoped_refptr callee_dc_1( - callee_->CreateDataChannel("data", init)); - - Negotiate(); - WaitForConnection(); - - EXPECT_EQ(1U, caller_dc_1->id() % 2); - EXPECT_EQ(0U, callee_dc_1->id() % 2); - - talk_base::scoped_refptr caller_dc_2( - caller_->CreateDataChannel("data", init)); - talk_base::scoped_refptr callee_dc_2( - callee_->CreateDataChannel("data", init)); - - EXPECT_EQ(1U, caller_dc_2->id() % 2); - EXPECT_EQ(0U, callee_dc_2->id() % 2); -} - -// Verifies that the message is received by the right remote DataChannel when -// there are multiple DataChannels. -TEST_F(PeerConnectionEndToEndTest, - MessageTransferBetweenTwoPairsOfDataChannels) { - MAYBE_SKIP_TEST(talk_base::SSLStreamAdapter::HaveDtlsSrtp); - - CreatePcs(); - - webrtc::DataChannelInit init; - - talk_base::scoped_refptr caller_dc_1( - caller_->CreateDataChannel("data", init)); - talk_base::scoped_refptr caller_dc_2( - caller_->CreateDataChannel("data", init)); - - Negotiate(); - WaitForConnection(); - WaitForDataChannelsToOpen(caller_dc_1, callee_signaled_data_channels_, 0); - WaitForDataChannelsToOpen(caller_dc_2, callee_signaled_data_channels_, 1); - - talk_base::scoped_ptr dc_1_observer( - new webrtc::MockDataChannelObserver(callee_signaled_data_channels_[0])); - - talk_base::scoped_ptr dc_2_observer( - new webrtc::MockDataChannelObserver(callee_signaled_data_channels_[1])); - - const std::string message_1 = "hello 1"; - const std::string message_2 = "hello 2"; - - caller_dc_1->Send(webrtc::DataBuffer(message_1)); - EXPECT_EQ_WAIT(message_1, dc_1_observer->last_message(), kMaxWait); - - caller_dc_2->Send(webrtc::DataBuffer(message_2)); - EXPECT_EQ_WAIT(message_2, dc_2_observer->last_message(), kMaxWait); - - EXPECT_EQ(1U, dc_1_observer->received_message_count()); - EXPECT_EQ(1U, dc_2_observer->received_message_count()); -} #endif // if !defined(THREAD_SANITIZER) diff --git a/talk/app/webrtc/test/mockpeerconnectionobservers.h b/talk/app/webrtc/test/mockpeerconnectionobservers.h index 3ae2162bc7..e2de379309 100644 --- a/talk/app/webrtc/test/mockpeerconnectionobservers.h +++ b/talk/app/webrtc/test/mockpeerconnectionobservers.h @@ -90,7 +90,7 @@ class MockSetSessionDescriptionObserver class MockDataChannelObserver : public webrtc::DataChannelObserver { public: explicit MockDataChannelObserver(webrtc::DataChannelInterface* channel) - : channel_(channel), received_message_count_(0) { + : channel_(channel) { channel_->RegisterObserver(this); state_ = channel_->state(); } @@ -101,18 +101,15 @@ class MockDataChannelObserver : public webrtc::DataChannelObserver { virtual void OnStateChange() { state_ = channel_->state(); } virtual void OnMessage(const DataBuffer& buffer) { last_message_.assign(buffer.data.data(), buffer.data.length()); - ++received_message_count_; } bool IsOpen() const { return state_ == DataChannelInterface::kOpen; } const std::string& last_message() const { return last_message_; } - size_t received_message_count() const { return received_message_count_; } private: talk_base::scoped_refptr channel_; DataChannelInterface::DataState state_; std::string last_message_; - size_t received_message_count_; }; class MockStatsObserver : public webrtc::StatsObserver { diff --git a/talk/app/webrtc/test/peerconnectiontestwrapper.cc b/talk/app/webrtc/test/peerconnectiontestwrapper.cc index be70969db9..7d3664fd1c 100644 --- a/talk/app/webrtc/test/peerconnectiontestwrapper.cc +++ b/talk/app/webrtc/test/peerconnectiontestwrapper.cc @@ -103,13 +103,6 @@ bool PeerConnectionTestWrapper::CreatePc( return peer_connection_.get() != NULL; } -talk_base::scoped_refptr -PeerConnectionTestWrapper::CreateDataChannel( - const std::string& label, - const webrtc::DataChannelInit& init) { - return peer_connection_->CreateDataChannel(label, &init); -} - void PeerConnectionTestWrapper::OnAddStream(MediaStreamInterface* stream) { LOG(LS_INFO) << "PeerConnectionTestWrapper " << name_ << ": OnAddStream"; @@ -129,11 +122,6 @@ void PeerConnectionTestWrapper::OnIceCandidate( sdp); } -void PeerConnectionTestWrapper::OnDataChannel( - webrtc::DataChannelInterface* data_channel) { - SignalOnDataChannel(data_channel); -} - void PeerConnectionTestWrapper::OnSuccess(SessionDescriptionInterface* desc) { // This callback should take the ownership of |desc|. talk_base::scoped_ptr owned_desc(desc); diff --git a/talk/app/webrtc/test/peerconnectiontestwrapper.h b/talk/app/webrtc/test/peerconnectiontestwrapper.h index 05e9b623c5..46fefafbdb 100644 --- a/talk/app/webrtc/test/peerconnectiontestwrapper.h +++ b/talk/app/webrtc/test/peerconnectiontestwrapper.h @@ -52,10 +52,6 @@ class PeerConnectionTestWrapper bool CreatePc(const webrtc::MediaConstraintsInterface* constraints); - talk_base::scoped_refptr CreateDataChannel( - const std::string& label, - const webrtc::DataChannelInit& init); - // Implements PeerConnectionObserver. virtual void OnError() {} virtual void OnSignalingChange( @@ -64,7 +60,7 @@ class PeerConnectionTestWrapper webrtc::PeerConnectionObserver::StateType state_changed) {} virtual void OnAddStream(webrtc::MediaStreamInterface* stream); virtual void OnRemoveStream(webrtc::MediaStreamInterface* stream) {} - virtual void OnDataChannel(webrtc::DataChannelInterface* data_channel); + virtual void OnDataChannel(webrtc::DataChannelInterface* data_channel) {} virtual void OnRenegotiationNeeded() {} virtual void OnIceConnectionChange( webrtc::PeerConnectionInterface::IceConnectionState new_state) {} @@ -98,7 +94,6 @@ class PeerConnectionTestWrapper const std::string&> SignalOnIceCandidateReady; sigslot::signal1 SignalOnSdpCreated; sigslot::signal1 SignalOnSdpReady; - sigslot::signal1 SignalOnDataChannel; private: void SetLocalDescription(const std::string& type, const std::string& sdp); diff --git a/talk/media/sctp/sctpdataengine.cc b/talk/media/sctp/sctpdataengine.cc index 46b2ece435..017454f220 100644 --- a/talk/media/sctp/sctpdataengine.cc +++ b/talk/media/sctp/sctpdataengine.cc @@ -781,6 +781,7 @@ void SctpDataMediaChannel::OnStreamResetEvent( << ListStreams(open_streams_) << "], Q'd: [" << ListStreams(queued_reset_streams_) << "], Sent: [" << ListStreams(sent_reset_streams_) << "]"; + bool local_stream_reset_acknowledged = false; // If both sides try to reset some streams at the same time (even if they're // disjoint sets), we can get reset failures. @@ -791,6 +792,7 @@ void SctpDataMediaChannel::OnStreamResetEvent( sent_reset_streams_.begin(), sent_reset_streams_.end()); sent_reset_streams_.clear(); + local_stream_reset_acknowledged = true; } else if (evt->strreset_flags & SCTP_STREAM_RESET_INCOMING_SSN) { // Each side gets an event for each direction of a stream. That is, @@ -807,6 +809,7 @@ void SctpDataMediaChannel::OnStreamResetEvent( if (it != sent_reset_streams_.end()) { LOG(LS_VERBOSE) << "SCTP_STREAM_RESET_EVENT(" << debug_name_ << "): local sid " << stream_id << " acknowledged."; + local_stream_reset_acknowledged = true; sent_reset_streams_.erase(it); } else if ((it = open_streams_.find(stream_id)) @@ -837,9 +840,11 @@ void SctpDataMediaChannel::OnStreamResetEvent( } } - // Always try to send the queued RESET because this call indicates that the - // last local RESET or remote RESET has made some progress. - SendQueuedStreamResets(); + if (local_stream_reset_acknowledged) { + // This message acknowledges the last stream-reset request we sent out + // (only one can be outstanding at a time). Send out the next one. + SendQueuedStreamResets(); + } } // Puts the specified |param| from the codec identified by |id| into |dest|