From aa98c3c2e83a3bc0dd706deb8ae40ff9194e9f13 Mon Sep 17 00:00:00 2001 From: EKR Date: Tue, 27 May 2014 13:13:43 -0700 Subject: [PATCH] Bug 1015409 - Fix trickle between CreateOffer() and SetLocal(). r=bwc, a=lsblakk --- .../signaling/src/sipcc/core/gsm/fsmdef.c | 43 +++++----- .../signaling/test/signaling_unittests.cpp | 84 ++++++++++++++----- 2 files changed, 86 insertions(+), 41 deletions(-) diff --git a/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c b/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c index 91e60c06eb83e..193d6271e6242 100755 --- a/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c +++ b/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c @@ -765,7 +765,7 @@ static sm_function_t fsmdef_function_table[FSMDEF_S_MAX][CC_MSG_MAX] = /* CC_MSG_ADDSTREAM */ fsmdef_ev_addstream, /* CC_MSG_REMOVESTREAM */ fsmdef_ev_removestream, /* CC_MSG_ADDCANDIDATE */ fsmdef_ev_addcandidate, - /* CC_MSG_FOUNDCANDIDATE */ fsmdef_ev_default + /* CC_MSG_FOUNDCANDIDATE */ fsmdef_ev_foundcandidate }, /* FSMDEF_S_HAVE_LOCAL_PRANSWER -------------------------------------------- */ @@ -4252,14 +4252,16 @@ fsmdef_ev_foundcandidate(sm_event_t *event) { return (SM_RC_END); } - /* Distinguish between the following two cases: + /* Distinguish between the following three cases: 1. CreateOffer() has been called but SetLocalDesc() has not. 2. We are mid-call. + 3. We have a remote offer and CreateAnswer() has been called + but SetLocalDesc() has not - Both of these are in state STABLE but only in one do we - pass up trickle candidates. In the other we buffer them - and send them later. + The first two of these are in state STABLE but only in + the second do pass up trickle candidates. In the other + two we buffer them and send them later. */ /* Smuggle the entire candidate structure in a string */ PR_snprintf(candidate_tmp, sizeof(candidate_tmp), "%d\t%s\t%s", @@ -4267,27 +4269,26 @@ fsmdef_ev_foundcandidate(sm_event_t *event) { (char *)msg->data.candidate.mid, (char *)msg->data.candidate.candidate); - if (fcb->state == FSMDEF_S_STABLE) { - if (!dcb->sdp->dest_sdp) { - fsmdef_candidate_t *buffered_cand = NULL; + if ((fcb->state == FSMDEF_S_STABLE && !dcb->sdp->dest_sdp) + || fcb->state == FSMDEF_S_HAVE_REMOTE_OFFER) { + fsmdef_candidate_t *buffered_cand = NULL; - FSM_DEBUG_SM(DEB_F_PREFIX"dcb->sdp->dest_sdp is null." - "assuming CreateOffer called but not SetLocal...\n", - DEB_F_PREFIX_ARGS(FSM, __FUNCTION__)); - - buffered_cand = (fsmdef_candidate_t *)cpr_malloc(sizeof(fsmdef_candidate_t)); - if (!buffered_cand) - return SM_RC_END; + FSM_DEBUG_SM(DEB_F_PREFIX"dcb->sdp->dest_sdp is null." + "assuming CreateOffer called but not SetLocal...\n", + DEB_F_PREFIX_ARGS(FSM, __FUNCTION__)); - buffered_cand->candidate = strlib_malloc(candidate_tmp, -1); + buffered_cand = (fsmdef_candidate_t *)cpr_malloc(sizeof(fsmdef_candidate_t)); + if (!buffered_cand) + return SM_RC_END; - if (sll_lite_link_head(&dcb->candidate_list, - (sll_lite_node_t *)buffered_cand) != SLL_LITE_RET_SUCCESS) - return SM_RC_END; + buffered_cand->candidate = strlib_malloc(candidate_tmp, -1); - /* Don't notify upward */ + if (sll_lite_link_head(&dcb->candidate_list, + (sll_lite_node_t *)buffered_cand) != SLL_LITE_RET_SUCCESS) return SM_RC_END; - } + + /* Don't notify upward */ + return SM_RC_END; } ui_ice_candidate_found(evFoundIceCandidate, fcb->state, line, call_id, diff --git a/media/webrtc/signaling/test/signaling_unittests.cpp b/media/webrtc/signaling/test/signaling_unittests.cpp index 0a428e19f9579..17ef3c9884090 100644 --- a/media/webrtc/signaling/test/signaling_unittests.cpp +++ b/media/webrtc/signaling/test/signaling_unittests.cpp @@ -144,7 +144,6 @@ static const std::string strSampleSdpAudioVideoNoIce = "a=candidate:1 1 UDP 2130706431 192.168.2.3 50007 typ host\r\n" "a=candidate:2 2 UDP 2130706431 192.168.2.4 50008 typ host\r\n"; - static const std::string strSampleCandidate = "a=candidate:1 1 UDP 2130706431 192.168.2.1 50005 typ host\r\n"; @@ -152,6 +151,27 @@ static const std::string strSampleMid = ""; static const unsigned short nSamplelevel = 2; +static const std::string strG711SdpOffer = + "v=0\r\n" + "o=- 1 1 IN IP4 148.147.200.251\r\n" + "s=-\r\n" + "b=AS:64\r\n" + "t=0 0\r\n" + "a=fingerprint:sha-256 F3:FA:20:C0:CD:48:C4:5F:02:5F:A5:D3:21:D0:2D:48:" + "7B:31:60:5C:5A:D8:0D:CD:78:78:6C:6D:CE:CC:0C:67\r\n" + "m=audio 9000 RTP/AVP 0 8 126\r\n" + "c=IN IP4 148.147.200.251\r\n" + "b=TIAS:64000\r\n" + "a=rtpmap:0 PCMU/8000\r\n" + "a=rtpmap:8 PCMA/8000\r\n" + "a=rtpmap:126 telephone-event/8000\r\n" + "a=candidate:0 1 udp 2130706432 148.147.200.251 9000 typ host\r\n" + "a=candidate:0 2 udp 2130706432 148.147.200.251 9005 typ host\r\n" + "a=ice-ufrag:cYuakxkEKH+RApYE\r\n" + "a=ice-pwd:bwtpzLZD+3jbu8vQHvEa6Xuq\r\n" + "a=sendrecv\r\n"; + + enum sdpTestFlags { SHOULD_SEND_AUDIO = (1<<0), @@ -2160,25 +2180,7 @@ TEST_F(SignalingTest, AudioOnlyG711Call) EnsureInit(); sipcc::MediaConstraints constraints; - std::string offer = - "v=0\r\n" - "o=- 1 1 IN IP4 148.147.200.251\r\n" - "s=-\r\n" - "b=AS:64\r\n" - "t=0 0\r\n" - "a=fingerprint:sha-256 F3:FA:20:C0:CD:48:C4:5F:02:5F:A5:D3:21:D0:2D:48:" - "7B:31:60:5C:5A:D8:0D:CD:78:78:6C:6D:CE:CC:0C:67\r\n" - "m=audio 9000 RTP/AVP 0 8 126\r\n" - "c=IN IP4 148.147.200.251\r\n" - "b=TIAS:64000\r\n" - "a=rtpmap:0 PCMU/8000\r\n" - "a=rtpmap:8 PCMA/8000\r\n" - "a=rtpmap:126 telephone-event/8000\r\n" - "a=candidate:0 1 udp 2130706432 148.147.200.251 9000 typ host\r\n" - "a=candidate:0 2 udp 2130706432 148.147.200.251 9005 typ host\r\n" - "a=ice-ufrag:cYuakxkEKH+RApYE\r\n" - "a=ice-pwd:bwtpzLZD+3jbu8vQHvEa6Xuq\r\n" - "a=sendrecv\r\n"; + const std::string& offer(strG711SdpOffer); std::cout << "Setting offer to:" << std::endl << indent(offer) << std::endl; a2_->SetRemote(TestObserver::OFFER, offer); @@ -2757,6 +2759,48 @@ TEST_F(SignalingAgentTest, CreateOfferSetLocalTrickleTestServer) { } +TEST_F(SignalingAgentTest, CreateAnswerSetLocalTrickleTestServer) { + TestStunServer::GetInstance()->SetActive(false); + TestStunServer::GetInstance()->SetResponseAddr( + kBogusSrflxAddress, kBogusSrflxPort); + + CreateAgent( + TestStunServer::GetInstance()->addr(), + TestStunServer::GetInstance()->port(), + false); + + std::string offer(strG711SdpOffer); + agent(0)->SetRemote(TestObserver::OFFER, offer, true, + PCImplSignalingState::SignalingHaveRemoteOffer); + ASSERT_EQ(agent(0)->pObserver->lastStatusCode, + sipcc::PeerConnectionImpl::kNoError); + + sipcc::MediaConstraints constraints; + agent(0)->CreateAnswer(constraints, offer, ANSWER_AUDIO, DONT_CHECK_AUDIO); + + // Verify that the bogus addr is not there. + ASSERT_FALSE(agent(0)->AnswerContains(kBogusSrflxAddress)); + + // Now enable the STUN server. + TestStunServer::GetInstance()->SetActive(true); + agent(0)->WaitForGather(); + + // There shouldn't be any candidates until SetLocal. + ASSERT_EQ(0U, agent(0)->MatchingCandidates(kBogusSrflxAddress)); + + agent(0)->SetLocal(TestObserver::ANSWER, agent(0)->answer()); + PR_Sleep(1000); // Give time for the message queues. + + // Verify that we got our candidates. + ASSERT_LE(2U, agent(0)->MatchingCandidates(kBogusSrflxAddress)); + + // Verify that the candidates appear in the answer. + size_t match; + match = agent(0)->getLocalDescription().find(kBogusSrflxAddress); + ASSERT_LT(0U, match); +} + + TEST_F(SignalingAgentTest, CreateUntilFailThenWait) { int i;