We need to support ICE restart/update to support renegotiation and to handle things like IP address change, interfaces going down, etc.

Byron -- Do you think we should bump this to a P2 and try to get this landed in the next one to three releases (i.e. Fx 45 or earlier)?

I think it would be a good idea to support this in the next ESR, but I'm not seeing any rush.

Thanks, Byron. The next ESR makes sense to me. Since the next ESR is Fx45 (which uplifts to Aurora on Dec 14), I'm going to bump this to a P2.

We (Tokbox) need this to support reconnecting after a one of the peers networking changes - most frequently with mobile devices changing from wifi to cellular. While Firefox on the desktop itself doesn't have to restart as often, when a peer connection is between desktop and a mobile device we need Firefox to be able to handle the case of getting new ice candidates. (My testing shows that where Chrome creates the iceRestart: true offer, and then sends new ice candidates, Firefox will not use these new ice candidates and return the peer connection's iceConnectionState to connected)

(In reply to Patrick Quinn-Graham [:pqg] from comment #5) > (My testing shows that where Chrome creates the iceRestart: true offer, and > then sends new ice candidates, Firefox will not use these new ice candidates > and return the peer connection's iceConnectionState to connected) I would expect us to outright reject the reoffer, since we have checking in place that bails when either the ice-ufrag or ice-pwd changes...

Byron -- Are you planning to take this one? I think it still makes sense to land this in time for Fx45 (the next ESR). If you prefer, I can find someone else to take it.

I really doubt I'm going to have time for this.

Hi, may I know what is the status of this ticket? Would this happen to go to release version 46?

*version 49

(In reply to leticia.choo from comment #11) > Hi, may I know what is the status of this ticket? Would this happen to go to > release version 49? Michael (the bug owner) has made a bunch of progress on this ticket and will be uploading patches for review soon. If reviews and testing go well, this should be in Firefox version 48. You'll see it all happen on this bug; so stay tuned.

Review commit: https://reviewboard.mozilla.org/r/41031/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41031/

Review commit: https://reviewboard.mozilla.org/r/41033/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41033/

Review commit: https://reviewboard.mozilla.org/r/41035/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41035/

Review commit: https://reviewboard.mozilla.org/r/41037/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41037/

Review commit: https://reviewboard.mozilla.org/r/41039/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41039/

Review commit: https://reviewboard.mozilla.org/r/41041/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41041/

Review commit: https://reviewboard.mozilla.org/r/41071/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41071/

Review commit: https://reviewboard.mozilla.org/r/41155/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41155/

Comment on attachment 8732188 [details] MozReview Request: Bug 906986 - Disabling TestSrflxCandPairingFilter until Bug 1226838 is fixed. https://reviewboard.mozilla.org/r/41031/#review37871

Comment on attachment 8732189 [details] MozReview Request: Bug 906986 - Rework fix for Bug 1241690 to avoid reliance on NrIceCtx inside NrIceMediaStream. https://reviewboard.mozilla.org/r/41033/#review37873

Comment on attachment 8732190 [details] MozReview Request: Bug 906986 - Use the streams from the ice_ctx, don't hold a separate set. https://reviewboard.mozilla.org/r/41035/#review37875 ::: media/mtransport/test/ice_unittest.cpp:636 (Diff revision 1) > - // The size of streams_ is not going to change out from under us, so should > + // The size of streams is not going to change out from under us, so should > // be safe here. I think this comment needs to change, since it was based on the fact that we had a main-thread cache of the streams that would not be modified on STS. I still think we're ok, because every change to the streams on STS is done with a synchronous dispatch from main. ::: media/mtransport/test/ice_unittest.cpp:754 (Diff revision 1) > std::cerr << "Stream " << i << " component " << j+1 << std::endl; > > NrIceCandidate *local; > NrIceCandidate *remote; > > - nsresult res = streams_[i]->GetActivePair(j+1, &local, &remote); > + nsresult res = ice_ctx_->GetStream(i)->GetActivePair(j+1, &local, &remote); Fold. ::: media/mtransport/test/ice_unittest.cpp:881 (Diff revision 1) > + index = i; > + break; Just put the stuff after the loop in here followed by a return, get rid of |index|, and add an assert aafter the loop. ::: media/mtransport/test/ice_unittest.cpp:899 (Diff revision 1) > > nsresult GetCandidatePairs_s(size_t stream_index, > std::vector<NrIceCandidatePair>* pairs) > { > MOZ_ASSERT(pairs); > - if (stream_index >= streams_.size() || !streams_[stream_index]) { > + if (stream_index >= ice_ctx_->GetStreamCount() || !ice_ctx_->GetStream(stream_index)) { Fold. ::: media/mtransport/test/ice_unittest.cpp:1075 (Diff revision 1) > - if (!streams_[stream]) { > + if (!ice_ctx_->GetStream(stream)) { > ADD_FAILURE() << "No such stream " << stream; > return; > } > > - ASSERT_TRUE(NS_SUCCEEDED(streams_[stream]->SendPacket(component, data, len))); > + ASSERT_TRUE(NS_SUCCEEDED(ice_ctx_->GetStream(stream)->SendPacket(component, data, len))); Fold. ::: media/mtransport/test/ice_unittest.cpp:1106 (Diff revision 1) > NS_DISPATCH_SYNC); > } > > void DisableComponent_s(size_t stream, int component_id) { > - ASSERT_LT(stream, streams_.size()); > - ASSERT_TRUE(streams_[stream]) << "No such stream " << stream; > + ASSERT_LT(stream, ice_ctx_->GetStreamCount()); > + ASSERT_TRUE(ice_ctx_->GetStream(stream).get()) << "No such stream " << stream; Fold.

Comment on attachment 8732191 [details] MozReview Request: Bug 906986 - Genericize some of the test setup and improve logging. https://reviewboard.mozilla.org/r/41037/#review37877 ::: media/mtransport/test/ice_unittest.cpp:505 (Diff revision 1) > > - > for (size_t i=0; i < candidates_in.size(); i++) { > std::string candidate(FilterCandidate(candidates_in[i])); > if (!candidate.empty()) { > - std::cerr << "Returning candidate: " << candidate << std::endl; > + std::cerr << name_ << " Returning candidate: " << candidate << std::endl; Fold. ::: media/mtransport/test/ice_unittest.cpp:749 (Diff revision 1) > if (!ice_ctx_->GetStream(i)) { > continue; > } > > for (size_t j=0; j < ice_ctx_->GetStream(i)->components(); ++j) { > - std::cerr << "Stream " << i << " component " << j+1 << std::endl; > + std::cerr << name_ << " Stream " << i << " component " << j+1 << std::endl; Fold. ::: media/mtransport/test/ice_unittest.cpp:843 (Diff revision 1) > (void)ctx; > if (state != NrIceCtx::ICE_CTX_GATHER_COMPLETE) { > return; > } > > std::cerr << "Gathering complete for " << name_ << std::endl; Maybe reorder this for consistency? ::: media/mtransport/test/ice_unittest.cpp:1036 (Diff revision 1) > - std::cerr << "Stream ready " << stream->name() << " ct=" << ready_ct_ << std::endl; > + std::cerr << "Stream ready for " << stream->name() << " ct=" << ready_ct_ << std::endl; > DumpCandidatePairs_s(stream); > } > void StreamFailed(NrIceMediaStream *stream) { > - std::cerr << "Stream failed " << stream->name() << " ct=" << ready_ct_ << std::endl; > + std::cerr << "Stream failed for " << stream->name() << " ct=" << ready_ct_ << std::endl; Fold. Also, is stream->name() going to be the same thing as name_? Maybe should reorder for consistency. ::: media/mtransport/test/ice_unittest.cpp:1051 (Diff revision 1) > - std::cerr << "ICE checking " << name_ << std::endl; > + std::cerr << "ICE reached checking for " << name_ << std::endl; > ice_reached_checking_ = true; > break; > case NrIceCtx::ICE_CTX_OPEN: > - std::cerr << "ICE completed " << name_ << std::endl; > + std::cerr << "ICE completed for " << name_ << std::endl; Maybe reorder these for consistency? ::: media/mtransport/test/ice_unittest.cpp:1374 (Diff revision 1) > > class IceConnectTest : public ::testing::Test { > public: > IceConnectTest() : > initted_(false), > + test_stun_server_initedd_(false), initted? ::: media/mtransport/test/ice_unittest.cpp:1413 (Diff revision 1) > } > > - void Init(bool allow_loopback, bool enable_tcp, bool default_only = false) { > - if (!initted_) { > + void Init(bool allow_loopback, > + bool enable_tcp, > + bool default_only = false, > + bool setupStunServers = true) { Might as well fix this to use underscores while we're at it. ::: media/mtransport/test/ice_unittest.cpp:1428 (Diff revision 1) > + InitPeer(p2_, setupStunServers); > + > initted_ = true; > } > > - bool Gather(unsigned int waitTime = kDefaultTimeout, > + void InitPeer(IceTestPeer* peer, bool setupStunServers = true) { Same here. ::: media/mtransport/test/ice_unittest.cpp:1435 (Diff revision 1) > - UseTestStunServer(); > + InitTestStunServer(); > + peer->UseTestStunServer(); UseTestStunServer() already inits this, we should probably decide whether we want the init repsonsibility on the caller or the callee, and stick with it. ::: media/mtransport/test/ice_unittest.cpp:1542 (Diff revision 1) > + ASSERT_TRUE(caller->ready_ct() == 0 && > + caller->ice_complete() == 0 && > + caller->ice_reached_checking() == 0); > + ASSERT_TRUE(callee->ready_ct() == 0 && > + callee->ice_complete() == 0 && > + callee->ice_reached_checking() == 0); These probably need to be individual ASSERT_EQ or ASSERT_FALSE, so it is easy to tell which condition failed. ::: media/mtransport/test/ice_unittest.cpp:1549 (Diff revision 1) > // IceTestPeer::Connect grabs attributes from the first arg, and gives them > // to |this|, meaning that p2_->Connect(p1_, ...) simulates p1 sending an > // offer to p2. Order matters here because it determines which peer is > // controlling. > - p2_->Connect(p1_, TRICKLE_NONE); > - p1_->Connect(p2_, TRICKLE_NONE); > + caller->Connect(callee, TRICKLE_NONE); > + callee->Connect(caller, TRICKLE_NONE); > > - ASSERT_TRUE_WAIT(p1_->ready_ct() == 1 && p2_->ready_ct() == 1, > + ASSERT_TRUE_WAIT(callee->ready_ct() == 1 && caller->ready_ct() == 1, > kDefaultTimeout); > - ASSERT_TRUE_WAIT(p1_->ice_complete() && p2_->ice_complete(), > + ASSERT_TRUE_WAIT(callee->ice_complete() && caller->ice_complete(), > kDefaultTimeout); > - AssertCheckingReached(); > > - p1_->DumpAndCheckActiveCandidates(); > - p2_->DumpAndCheckActiveCandidates(); > + ASSERT_TRUE(callee->ice_reached_checking()); > + ASSERT_TRUE(caller->ice_reached_checking()); > + > + callee->DumpAndCheckActiveCandidates(); > + caller->DumpAndCheckActiveCandidates(); |caller| and |callee| are backwards here (see the comment). Also, the comment needs to be updated. ::: media/mtransport/test/ice_unittest.cpp:1655 (Diff revision 1) > + bool expectTxFailure = false, > + bool expectRxFailure = false) { Use underscores instead of camelcase. ::: media/mtransport/test/ice_unittest.cpp:1663 (Diff revision 1) > - WrapRunnable(p1_.get(), > + WrapRunnable(p1, > &IceTestPeer::SendPacket, 0, 1, > reinterpret_cast<const unsigned char *>("TEST"), 4), > NS_DISPATCH_SYNC); > - ASSERT_EQ(1u, p1_->sent()); > - ASSERT_TRUE_WAIT(p2_->received() == 1, 1000); > + if (expectTxFailure) { > + ASSERT_NE(1u, p1->sent()); We probably want ASSERT_EQ(0,...) ::: media/mtransport/test/ice_unittest.cpp:1669 (Diff revision 1) > + } else { > + ASSERT_EQ(1u, p1->sent()); > + } > + if (expectRxFailure) { > + usleep(1000); > + ASSERT_TRUE(p2->received() == 0); ASSERT_EQ ::: media/mtransport/test/ice_unittest.cpp:3010 (Diff revision 1) > } > > TEST_F(IceConnectTest, TestHostCandPairingFilter) { > + Init(false, false, false, false); > AddStream("first", 1); > - ASSERT_TRUE(Gather(kDefaultTimeout, false)); > + ASSERT_TRUE(Gather(kDefaultTimeout)); We can probably get rid of the kDefaultTimeout here too?

Comment on attachment 8732192 [details] MozReview Request: Bug 906986 - Wrap NrIceCtx in NrIceCtxHandler which will allow us to handle ice restart. https://reviewboard.mozilla.org/r/41039/#review37891 ::: media/mtransport/nricectx.h:221 (Diff revision 1) > - bool tcp_enabled = true, > - bool allow_link_local = false, > + static bool Initialize(NrIceCtx* ice_ctx, > + bool hide_non_default); Let's make this a member function instead of a static. ::: media/mtransport/nricectx.cpp:381 (Diff revision 1) > - bool hide_non_default, > - Policy policy) { > - RefPtr<NrIceCtx> ctx = new NrIceCtx(name, offerer, policy); > > +void > +NrIceCtx::InitializeCryptoAndLogging(bool allow_loopback, Hmm. This name is misleading, because a bunch of config for ICE is done here (timers, priorities, and other stuff). InitializeLoggingCryptoAndIceConfig is a mouthful though. InitializeGlobals might describe the overall intent better, although it is less specific. ::: media/mtransport/nricectxhandler.h:8 (Diff revision 1) > + > +#include "nricectx.h" > + > +namespace mozilla { > + > +class NrIceCtxHandler : public NrIceCtx { So, I've looked at the subsequent patch, and it seems to me that we can just put all this stuff in NrIceCtx and call it a day. The only thing this ends up wrapping is the provisional NrIceCtx for the restart, which NrIceCtx can handle on its own.

Comment on attachment 8732385 [details] MozReview Request: Bug 906986 - Fix linux build error in tests. r=drno,bwc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41155/diff/1-2/

Comment on attachment 8732193 [details] MozReview Request: Bug 906986 - Ice restart and tests. https://reviewboard.mozilla.org/r/41041/#review37901 ::: dom/media/tests/mochitest/mochitest.ini:236 (Diff revision 1) > +[test_peerConnection_restartIceNoBundle.html] > +[test_peerConnection_restartIce.html] > +[test_peerConnection_restartIceLocalRollback.html] > +[test_peerConnection_restartIceLocalAndRemoteRollback.html] You're going to need some skip-ifs here, probably based on some of the other renegotiation tests. ::: dom/media/tests/mochitest/test_peerConnection_restartIce.html:18 (Diff revision 1) > + > + var test; > + runNetworkTest(function (options) { > + test = new PeerConnectionTest(options); > + > + addRenegotiation(test.chain, Indentation on these is a bit much. ::: dom/media/tests/mochitest/test_peerConnection_restartIceLocalAndRemoteRollback.html:70 (Diff revision 1) > + // for now, only use one stream, because rollback doesn't seem to > + // like multiple streams Let's put a bug# here. ::: dom/media/tests/mochitest/test_peerConnection_restartIceLocalRollback.html:32 (Diff revision 1) > + function PC_REMOTE_EXPECT_ICE_CHECKING(test) { > + test.pcRemote.iceCheckingRestartExpected = true; > + }, > + > + // causes an ice restart and then rolls it back > + // (does not result in sending and offer) s/and/an/ ::: dom/media/tests/mochitest/test_peerConnection_restartIceLocalRollback.html:43 (Diff revision 1) > + return test.setLocalDescription(test.pcLocal, > + offer, Trailing ws. ::: dom/media/tests/mochitest/test_peerConnection_restartIceLocalRollback.html:51 (Diff revision 1) > + }); > + }, > + function PC_LOCAL_ROLLBACK(test) { > + return test.setLocalDescription( > + test.pcLocal, > + new RTCSessionDescription({ type: "rollback", Trailing ws. ::: dom/media/tests/mochitest/test_peerConnection_restartIceLocalRollback.html:62 (Diff revision 1) > + return test.pcLocal.endOfTrickleIce; > + } > + ] > + ); > + > + // for now, only use one stream, because rollback doesn't seem to Bug # here too. ::: media/mtransport/nricectx.cpp:466 (Diff revision 1) > +{ > + char* ufrag; > + int r; > + > + if ((r=nr_ice_get_new_ice_ufrag(&ufrag))) > + return ""; Let's MOZ_CRASH if this happens. ::: media/mtransport/nricectx.cpp:481 (Diff revision 1) > +{ > + char* pwd; > + int r; > + > + if ((r=nr_ice_get_new_ice_pwd(&pwd))) > + return ""; Same here. ::: media/mtransport/nricectx.cpp:521 (Diff revision 1) > } > > if (hide_non_default) > flags |= NR_ICE_CTX_FLAGS_ONLY_DEFAULT_ADDRS; > > - r = nr_ice_ctx_create(const_cast<char *>(ctx->name_.c_str()), > + r = nr_ice_ctx_create_with_credentials(const_cast<char *>(ctx->name_.c_str()), Fold. ::: media/mtransport/nricectxhandler.cpp:61 (Diff revision 1) > + ConnectionState tmp_connection_state = ctx1.connection_state_; > + GatheringState tmp_gathering_state = ctx1.gathering_state_; > + nr_ice_ctx *tmp_ctx = ctx1.ctx_; > + nr_ice_peer_ctx *tmp_peer_ctx = ctx1.peer_; > + nr_ice_handler_vtbl *tmp_ice_handler_vtbl = ctx1.ice_handler_vtbl_; > + nr_ice_handler *tmp_ice_handler = ctx1.ice_handler_; > + bool tmp_trickle = ctx1.trickle_; > + nsCOMPtr<nsIEventTarget> tmp_sts_target = ctx1.sts_target_; std::swap should work on all this stuff. Or, we could avoid this entirely by having a handler object contain the NrIceCtx (but this means we would need to have a whole bunch of handler.ctx()->blah scattered around). ::: media/mtransport/test/ice_unittest.cpp:572 (Diff revision 1) > } > bool ice_complete() { return ice_complete_; } > bool ice_reached_checking() { return ice_reached_checking_; } > size_t received() { return received_; } > size_t sent() { return sent_; } > + void resetStats() { received_ = sent_ = 0; } If we taught SendReceive to verify that |received_| and |sent_| increased by 1 (instead of equaled 1), we wouldn't need this right? ::: media/mtransport/test/ice_unittest.cpp:2276 (Diff revision 1) > + InitPeer(p3_); > + p3_->AddStream(1); > + > + p2_->AddStream(1); > + ASSERT_TRUE(GatherCallerAndCallee(p2_, p3_)); > + std::cout << "-------------------------------------------------" << std::endl; Fold. ::: media/mtransport/test/ice_unittest.cpp:2319 (Diff revision 1) > + InitPeer(p3_); > + p3_->AddStream(1); > + > + p2_->AddStream(1); > + ASSERT_TRUE(GatherCallerAndCallee(p2_, p3_)); > + std::cout << "-------------------------------------------------" << std::endl; Fold. ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:979 (Diff revision 1) > + char buf[10]; > + > + if(r=nr_ice_random_string(buf,8)) Let's give a name to these magic constants. ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:989 (Diff revision 1) > + ABORT(r); > + > + _status=0; > + abort: > + if(_status) { > + RFREE(*ufrag); We probably want to zero out |ufrag| too. ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:997 (Diff revision 1) > + char buf[40]; > + > + if(r=nr_ice_random_string(buf,32)) Let's give a name to these magic constants. ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:1007 (Diff revision 1) > + ABORT(r); > + > + _status=0; > + abort: > + if(_status) { > + RFREE(*pwd); We probably want to zero out |pwd| too. ::: media/mtransport/transportlayerice.h:66 (Diff revision 1) > RefPtr<NrIceCtx> ctx_; > RefPtr<NrIceMediaStream> stream_; > int component_; > + > + // used to hold the old ice ctx, stream, and component > + // note: holding the old ctx and component are probably overkill (mjf) The component is definitely not going to change, and ctx_ seems to only be used to set thread_, so we can eliminate it entirely. We should be able to get away with just old_stream_. ::: media/mtransport/transportlayerice.cpp:92 (Diff revision 1) > TransportLayerIce::TransportLayerIce(const std::string& name) > - : name_(name), ctx_(nullptr), stream_(nullptr), component_(0) {} > + : name_(name), > + ctx_(nullptr), stream_(nullptr), component_(0), > + old_ctx_(nullptr), old_stream_(nullptr), old_component_(0) > +{ > + // setup happens later Trailing ws. ::: media/mtransport/transportlayerice.cpp:106 (Diff revision 1) > int component) { > + // If SetParameters is called and we already have a stream_, this means > + // we're handling an ICE restart. We need to hold the old stream until > + // we know the new stream is working. > + if (stream_ && !old_stream_) { > + MOZ_ASSERT(stream_ != stream); // make sure we're getting a different stream Fold. ::: media/mtransport/transportlayerice.cpp:141 (Diff revision 1) > +void > +TransportLayerIce::ResetOldStream() > +{ Convention here seems to be to have all this on one line. ::: media/mtransport/transportlayerice.cpp:148 (Diff revision 1) > +{ > + if (old_stream_ == nullptr) { > + return; // no work to do > + } > + // ICE Ready on the new stream, we can forget the old stream now > + MOZ_MTLOG(ML_INFO, LAYER_INFO << "ResetOldStream(" << old_stream_->name() << "," Fold. ::: media/mtransport/transportlayerice.cpp:158 (Diff revision 1) > +void > +TransportLayerIce::RestoreOldStream() > +{ All on one line. ::: media/mtransport/transportlayerice.cpp:167 (Diff revision 1) > + stream_->SignalReady.disconnect(this); > + stream_->SignalFailed.disconnect(this); > + stream_->SignalPacketReceived.disconnect(this); > + ctx_ = old_ctx_; > + stream_ = old_stream_; > + component_ = old_component_; We need to set the old stream to nullptr, right? ::: media/mtransport/transportlayerice.cpp:178 (Diff revision 1) > +} > + > TransportResult TransportLayerIce::SendPacket(const unsigned char *data, > size_t len) { > CheckThread(); > nsresult res = stream_->SendPacket(component_, data, len); We probably want to fall back to old_stream_ if set (since we unset it when |stream_| is ready). Alternately, we could do |new_stream_| instead, and promote it once it succeeds. ::: media/mtransport/transportlayerice.cpp:197 (Diff revision 1) > CheckThread(); > + MOZ_MTLOG(ML_INFO, LAYER_INFO << "ICE Ready(" << stream->name() << "," > + << component_ << ")"); > TL_SET_STATE(TS_OPEN); > + // Reset old ice stream if new stream is good after ice restart > + // Possible to get IceReady call for old stream after restart? ?mjf? > + ResetOldStream(); This could be called on the old stream during an ICE restart, yes. You probably need to compare |stream| against |stream_| and bail if they don't match. Then, when a restore happens, we call either IceReady or IceFailed (or nothing) depending on the state of the stream we just restored. ::: media/mtransport/transportlayerice.cpp:208 (Diff revision 1) > + MOZ_MTLOG(ML_INFO, LAYER_INFO << "ICE Failed(" << stream->name() << "," > + << component_ << ")"); > TL_SET_STATE(TS_ERROR); We should only do this stuff if |stream| matches |stream_| ::: media/mtransport/transportlayerice.cpp:211 (Diff revision 1) > + // Restore old ice stream if new stream fails during ice restart > + // Possible to get IceFailed call for old stream after restart? ?mjf? > + RestoreOldStream(); This is not our call. If the new stream fails, we're sunk. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h:65 (Diff revision 1) > virtual bool > RemoteIsIceLite() const override > { > return mRemoteIsIceLite; > } > + Trailing ws. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:153 (Diff revision 1) > mIceUfrag = ufrag; > mIcePwd = pwd; > > + if (mCurrentLocalDescription.get()) { > + mLocalIsIceRestarting = true; > + } If we do an ICE restart, and then rollback, I'm pretty sure we still have the new credentials and the old ones are lost. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:647 (Diff revision 1) > > // Undo track assignments from a previous call to CreateOffer > // (ie; if the track has not been negotiated yet, it doesn't necessarily need > // to stay in the same m-section that it was in) > for (JsepSendingTrack& trackWrapper : mLocalTracks) { > - if (!trackWrapper.mTrack->GetNegotiatedDetails()) { > + if (!trackWrapper.mTrack->GetNegotiatedDetails() || mLocalIsIceRestarting) { I don't think an ICE restart should be causing us to move tracks around. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:658 (Diff revision 1) > > // Make the basic SDP that is common to offer/answer. > nsresult rv = CreateGenericSDP(&sdp); > NS_ENSURE_SUCCESS(rv, rv); > > - if (mCurrentLocalDescription) { > + if (mCurrentLocalDescription && !mLocalIsIceRestarting) { I don't see anything in here that we don't want to do for an ICE restart; the sticky params in here are the mid and rtcp-mux, which make sense to leave alone. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1215 (Diff revision 1) > + // check for mismatch ufrag/pwd indicating ice restart > + bool iceRestarting = false; > + if (mCurrentRemoteDescription.get()) { > + for (size_t i = 0; > + !iceRestarting && i < mCurrentRemoteDescription->GetMediaSectionCount(); > + ++i) { > + if (mSdpHelper.MsectionIsDisabled(parsed->GetMediaSection(i)) || > + mSdpHelper.MsectionIsDisabled(mCurrentRemoteDescription->GetMediaSection(i))) { > + continue; > + } > + > + const SdpAttributeList& newAttrs( > + parsed->GetMediaSection(i).GetAttributeList()); > + const SdpAttributeList& oldAttrs( > + mCurrentRemoteDescription->GetMediaSection(i).GetAttributeList()); > + > + if ((newAttrs.GetIceUfrag() != oldAttrs.GetIceUfrag()) || > + (newAttrs.GetIcePwd() != oldAttrs.GetIcePwd())) { > + MOZ_MTLOG(ML_DEBUG, "Mismatched ice creds indicate ICE restart"); > + iceRestarting = true; > + } > + } > + } If we error out on partial ICE restart (see below), we just need to check the first m-section here. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1557 (Diff revision 1) > - mSdpHelper.AreOldTransportParamsValid(oldAnswer, newOffer, i)) { > + mSdpHelper.AreOldTransportParamsValid(oldAnswer, newOffer, i) && > + !(mLocalIsIceRestarting || mRemoteIsIceRestarting) > + ) { If we check for (and error out on) partial ICE restarts (see below), we can update AreOldTransportParamsValid to do this on a section-by-section basis, which is where we'll want to be once we work out how to set ICE credentials on a stream-by-stream basis. It should also let us remove mLocalIsIceRestarting altogether. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp (Diff revision 1) > - for (size_t i = 0; > - i < mCurrentRemoteDescription->GetMediaSectionCount(); > - ++i) { > - if (mSdpHelper.MsectionIsDisabled(description.GetMediaSection(i)) || > - mSdpHelper.MsectionIsDisabled(mCurrentRemoteDescription->GetMediaSection(i))) { > - continue; > - } > - > - const SdpAttributeList& newAttrs( > - description.GetMediaSection(i).GetAttributeList()); > - const SdpAttributeList& oldAttrs( > - mCurrentRemoteDescription->GetMediaSection(i).GetAttributeList()); > - > - if ((newAttrs.GetIceUfrag() != oldAttrs.GetIceUfrag()) || > - (newAttrs.GetIcePwd() != oldAttrs.GetIcePwd())) { > - JSEP_SET_ERROR("ICE restart is unsupported at this time " > - "(new remote description changes either the ice-ufrag " > - "or ice-pwd)" << > - "ice-ufrag (old): " << oldAttrs.GetIceUfrag() << > - "ice-ufrag (new): " << newAttrs.GetIceUfrag() << > - "ice-pwd (old): " << oldAttrs.GetIcePwd() << > - "ice-pwd (new): " << newAttrs.GetIcePwd()); > - return NS_ERROR_INVALID_ARG; > - } > - } > - I would not completely get rid of this; we still don't support _partial_ ICE restarts (ie; some m-sections but not others). Right now a partial ICE restart is going to fail in hard-to-diagnose ways. I recommend breaking out a SdpHelper::IceCredentialsDiffer(msection1, msection2) function, and using it here, in the code that sets up mRemoteIsIceRestarting, and in SdpHelper::AreOldTransportParamsValid. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1553 (Diff revision 1) > + if (aOptions.mIceRestart.isSome() && *(aOptions.mIceRestart)) { > + CSFLogInfo(logTag, "Offerer restarting ice"); > + std::string ufrag = mMedia->ice_ctx()->GetNewUfrag(); > + std::string pwd = mMedia->ice_ctx()->GetNewPwd(); > + > + mMedia->BeginIceRestart(ufrag, pwd); What happens if CreateOffer({iceRestart: true}) is called repeatedly without actually doing a SetLocalDescription? I think we end up asserting... we should handle this case. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1616 (Diff revision 1) > + if (mJsepSession->RemoteIsIceRestarting()) { > + CSFLogInfo(logTag, "Answerer restarting ice"); > + std::string ufrag = mMedia->ice_ctx()->GetNewUfrag(); > + std::string pwd = mMedia->ice_ctx()->GetNewPwd(); > + > + mMedia->BeginIceRestart(ufrag, pwd); Repeated CreateAnswer is problematic too. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3078 (Diff revision 1) > + if (mIceConnectionState == PCImplIceConnectionState::Connected || > + mIceConnectionState == PCImplIceConnectionState::Completed) { > + if (mMedia->ice_ctx()->IsRestarting()) { > + mMedia->FinalizeIceRestart(); > + } > + } We should probably finalize on failure too; we don't want an extra ctx hanging around if/when that happens. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:610 (Diff revision 1) > + std::string(ufrag), // Make copies. > + std::string(pwd)), These will get copied automatically.

Comment on attachment 8732385 [details] MozReview Request: Bug 906986 - Fix linux build error in tests. r=drno,bwc https://reviewboard.mozilla.org/r/41155/#review38113

Comment on attachment 8732188 [details] MozReview Request: Bug 906986 - Disabling TestSrflxCandPairingFilter until Bug 1226838 is fixed. https://reviewboard.mozilla.org/r/41031/#review38233

Comment on attachment 8732189 [details] MozReview Request: Bug 906986 - Rework fix for Bug 1241690 to avoid reliance on NrIceCtx inside NrIceMediaStream. https://reviewboard.mozilla.org/r/41033/#review38243

Comment on attachment 8732298 [details] MozReview Request: Bug 906986 - Removed an assert missed during cleanup. https://reviewboard.mozilla.org/r/41071/#review38245

Comment on attachment 8732385 [details] MozReview Request: Bug 906986 - Fix linux build error in tests. r=drno,bwc https://reviewboard.mozilla.org/r/41155/#review38247

Comment on attachment 8732193 [details] MozReview Request: Bug 906986 - Ice restart and tests. https://reviewboard.mozilla.org/r/41041/#review38249 ::: dom/media/tests/mochitest/mochitest.ini:236 (Diff revision 1) > +[test_peerConnection_restartIceNoBundle.html] > +[test_peerConnection_restartIce.html] > +[test_peerConnection_restartIceLocalRollback.html] > +[test_peerConnection_restartIceLocalAndRemoteRollback.html] These need to be moved above the [test_zmedia_cleanup.html] entry, because that has to be the least "test" getting executed on the B2G. Usually I'm trying to keep the test names in here in alphabetical order. ::: dom/media/tests/mochitest/test_peerConnection_restartIce.html:19 (Diff revision 1) > + var test; > + runNetworkTest(function (options) { > + test = new PeerConnectionTest(options); > + > + addRenegotiation(test.chain, > + [ Can we add the same comment as in the two other test cases here so people understand what this suppose to do? ::: dom/media/tests/mochitest/test_peerConnection_restartIce.html:20 (Diff revision 1) > + function PC_SET_OFFER_OPTION(test) { > + test.setOfferOptions({ iceRestart: true }); > + }, Each function in test.chain needs to start with PC_LOCAL or PC_REMOTE, because steeplechase splits the test based on that prefix. If this affects both sides, you need to make it two steps like you did below for setting the |iceCheckingRestartExpected|. ::: dom/media/tests/mochitest/test_peerConnection_restartIceLocalAndRemoteRollback.html:22 (Diff revision 1) > + var firstNegotiationSize = test.chain.commands.length; > + > + addRenegotiation(test.chain, > + [ > + // causes a full, normal ice restart > + function PC_SET_OFFER_OPTION(test) { Split into two and use PC_LOCAL and PC_REMOTE. And less indentation. ::: dom/media/tests/mochitest/test_peerConnection_restartIceLocalAndRemoteRollback.html:36 (Diff revision 1) > + ] > + ); > + > + test.chain.replaceAfter('PC_REMOTE_CREATE_ANSWER', > + [ > + function PC_LOCAL_SETUP_ICE_HANDLER(test) { I see this is just copied from renegotiation, but I'm wondering why we are setting up the Ice Candidate Handler again... ::: dom/media/tests/mochitest/test_peerConnection_restartIceLocalAndRemoteRollback.html:66 (Diff revision 1) > + // Rolling back should shut down gathering > + function PC_LOCAL_WAIT_FOR_END_OF_TRICKLE(test) { > + return test.pcLocal.endOfTrickleIce; > + }, > + ], > + firstNegotiationSize // Second PC_REMOTE_SET_REMOTE_DESCRIPTION Can we please clarify in the comment here, that this tells the replaceAfter() to only touch the second PC_REMOTE_CREATE_ANSWER (as we have two idenitically named steps now I got confused which one is going to taken). ::: dom/media/tests/mochitest/test_peerConnection_restartIceLocalRollback.html:21 (Diff revision 1) > + test = new PeerConnectionTest(options); > + > + addRenegotiation(test.chain, > + [ > + // causes a full, normal ice restart > + function PC_SET_OFFER_OPTION(test) { Same here. Split in two. ::: media/mtransport/nricectx.cpp:459 (Diff revision 1) > +std::string > +NrIceCtx::GetNewUfrag() I'm wondering if it would be more consistent with the general nICEr behavior to have GetNewUfrag() and GetNewPwd() return numbers/error codes and return the strings as the arguments? ::: media/mtransport/nricectx.cpp:493 (Diff revision 1) > + std::string ufrag = GetNewUfrag(); > + std::string pwd = GetNewPwd(); To be safe lets check the return values here and error out in case these failed. ::: media/mtransport/nricectx.cpp:526 (Diff revision 1) > + MOZ_ASSERT(ufrag == ctx->ctx_->ufrag); > + MOZ_ASSERT(pwd == ctx->ctx_->pwd); Are you concerned that nr_ice_ctx_create_with_credentials() can actually fail to set these properly or are these just left over from development? ::: media/mtransport/nricectxhandler.cpp:99 (Diff revision 1) > + std::string ufrag = GetNewUfrag(); > + std::string pwd = GetNewPwd(); Check return values. ::: media/mtransport/nricectxhandler.cpp:108 (Diff revision 1) > + > +void > +NrIceCtxHandler::BeginIceRestart(const std::string& ufrag, > + const std::string& pwd) > +{ > + MOZ_ASSERT(!restart_ctx, "existing ice restart in progress"); What about production code? Shouldn't we error out here in case a service in production manages to restart twice to fast? ::: media/mtransport/nricectxhandler.cpp:123 (Diff revision 1) > + hide_non_default, > + ufrag, > + pwd)); > + > + ++restart_count; > + restart_ctx = new_ctx; If I'm not mistaken |restart_ctx| now holds all the references to the original/old ctx, or? If that is the case then I find the variable naming a little confusing. ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.h:185 (Diff revision 1) > +int nr_ice_get_new_ice_ufrag(char** ufrag); > +int nr_ice_get_new_ice_pwd(char** pwd); I'm a little bit confused: why do we have ufrag and pwd generators in two places, mtransport and here down in nICEr? Are these here in nICEr only for unit testing? Can wen then at least mark them as such if not even #ifdef them to be only available in the unit tests? ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:979 (Diff revision 1) > + char buf[10]; > + > + if(r=nr_ice_random_string(buf,8)) Why did the buffer length got shortened by factor 10? Not that I think we really 100 bytes as before, but is 10 really a safe choice here? ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:983 (Diff revision 1) > + int r,_status; > + char buf[10]; > + > + if(r=nr_ice_random_string(buf,8)) > + ABORT(r); > + if(!(*ufrag=r_strdup(buf))) nr_ice_ctx_create_with_credentials() dupes these again. And nr_ice_random_string() is static. Do we need this r_strdup() call here? ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:997 (Diff revision 1) > + char buf[40]; > + > + if(r=nr_ice_random_string(buf,32)) Same here: from 100 down to 40? ::: media/mtransport/transportlayerice.cpp:170 (Diff revision 1) > + ctx_ = old_ctx_; > + stream_ = old_stream_; > + component_ = old_component_; I think the old_* variables need to be reset after these assignments, or? ::: media/webrtc/signaling/src/jsep/JsepSession.h:90 (Diff revision 1) > // Set up the ICE And DTLS data. > virtual nsresult SetIceCredentials(const std::string& ufrag, > const std::string& pwd) = 0; > virtual nsresult SetBundlePolicy(JsepBundlePolicy policy) = 0; > virtual bool RemoteIsIceLite() const = 0; > + virtual bool RemoteIsIceRestarting() const = 0; RemoteIceIsRestarting? ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h:38 (Diff revision 1) > + mLocalIsIceRestarting(false), > + mRemoteIsIceRestarting(false), *IceIsRestarting sounds more natural to me (the non native speaker :D ) ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1233 (Diff revision 1) > + const SdpAttributeList& oldAttrs( > + mCurrentRemoteDescription->GetMediaSection(i).GetAttributeList()); > + > + if ((newAttrs.GetIceUfrag() != oldAttrs.GetIceUfrag()) || > + (newAttrs.GetIcePwd() != oldAttrs.GetIcePwd())) { > + MOZ_MTLOG(ML_DEBUG, "Mismatched ice creds indicate ICE restart"); "Mismatched" sounds like an error. How about "Found new ICE creds, restarting ICE" or something like that? ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp:202 (Diff revision 1) > +static void > +AddNewIceStreamForRestart_s(RefPtr<PeerConnectionMedia> aPCMedia, > + RefPtr<TransportFlow> aFlow, > + size_t aLevel, > + bool aIsRtcp) > +{ > + TransportLayerIce* ice = > + static_cast<TransportLayerIce*>(aFlow->GetLayer("ice")); > + ice->SetParameters(aPCMedia->ice_ctx(), > + aPCMedia->ice_media_stream(aLevel), > + aIsRtcp ? 2 : 1); > +} > + Seeing this makes me want to see a mochitest with rtcp-mux off and an ICE restart. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1550 (Diff revision 1) > + std::string ufrag = mMedia->ice_ctx()->GetNewUfrag(); > + std::string pwd = mMedia->ice_ctx()->GetNewPwd(); Check return values. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1613 (Diff revision 1) > + std::string ufrag = mMedia->ice_ctx()->GetNewUfrag(); > + std::string pwd = mMedia->ice_ctx()->GetNewPwd(); Check return value. I only want to prevent that we send out an offer or answer with empty ufrag:pwd. Maybe it's easier to catch this special corner case in one of the functions you are calling with the new credentials.

Comment on attachment 8732191 [details] MozReview Request: Bug 906986 - Genericize some of the test setup and improve logging. https://reviewboard.mozilla.org/r/41037/#review38299 ::: media/mtransport/test/ice_unittest.cpp:1496 (Diff revision 1) > + // to be useful, this method should be called before Init > + ASSERT_FALSE(initted_); This is actually not true. For my consent refresh testing I actually want to block UDP in the middle of the call. So for at least BlockUdp() I would prefer to remove this assert. ::: media/mtransport/test/ice_unittest.cpp:1542 (Diff revision 1) > + ASSERT_TRUE(caller->ready_ct() == 0 && > + caller->ice_complete() == 0 && > + caller->ice_reached_checking() == 0); > + ASSERT_TRUE(callee->ready_ct() == 0 && > + callee->ice_complete() == 0 && Trailing WS

Review commit: https://reviewboard.mozilla.org/r/41863/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41863/

Comment on attachment 8733695 [details] MozReview Request: Bug 906986 - Convert NrIceCtxHandler to composition instead of inheriting from NrIceCtx https://reviewboard.mozilla.org/r/41863/#review38505 ::: media/mtransport/nricectx.h:196 (Diff revision 1) > std::string alpn_; > }; > > > class NrIceCtx { > + friend class NrIceCtxHandler; Do we need this? It looks like we're using accessors everywhere... ::: media/mtransport/nricectxhandler.h:17 (Diff revision 1) > bool offerer, > bool allow_loopback = false, > bool tcp_enabled = true, > bool allow_link_local = false, > bool hide_non_default = false, > - Policy policy = ICE_POLICY_ALL); > + NrIceCtx::Policy policy = NrIceCtx::ICE_POLICY_ALL); This line is running too long. ::: media/mtransport/nricectxhandler.h:27 (Diff revision 1) > + RefPtr<NrIceCtx> ctx() { return current_ctx; } > + > void BeginIceRestart(); // used in testing > void BeginIceRestart(const std::string& ufrag, > const std::string& pwd); > bool IsRestarting() { return (restart_ctx != nullptr); } const ::: media/mtransport/nricectxhandler.h:38 (Diff revision 1) > bool offerer, > - Policy policy) > + NrIceCtx::Policy policy); > - : NrIceCtx(name, offerer, policy), > - restart_ctx(nullptr), restart_count(0) {} > - > NrIceCtxHandler(); // disable Use =delete for this. ::: media/mtransport/nricectxhandler.h:43 (Diff revision 1) > - virtual ~NrIceCtxHandler(); > - > + ~NrIceCtxHandler() {} > + DISALLOW_COPY_ASSIGN(NrIceCtxHandler); > - void SwapContextData(NrIceCtxHandler& ctx1, NrIceCtxHandler& ctx2); > > -private: > - RefPtr<NrIceCtxHandler> restart_ctx; // for while restart is in progress > + RefPtr<NrIceCtx> current_ctx; > + RefPtr<NrIceCtx> restart_ctx; // for while restart is in progress The current naming makes it look like this is the new ctx, not the old one. ::: media/mtransport/nricectxhandler.cpp:75 (Diff revision 1) > NrIceCtxHandler::BeginIceRestart(const std::string& ufrag, > const std::string& pwd) > { > MOZ_ASSERT(!restart_ctx, "existing ice restart in progress"); > - RefPtr<NrIceCtxHandler> new_ctx = new NrIceCtxHandler(name_, offerer_, policy_); > + RefPtr<NrIceCtx> new_ctx = new NrIceCtx(this->current_ctx->name(), > + this->current_ctx->offerer(), Actually, the old value doesn't matter since it can change with each offer/answer. That said, we're just setting this to true all the time, but we can hard-code here and remove the offerer() function. ::: media/mtransport/test/ice_unittest.cpp:559 (Diff revision 1) > bool is_ready_s(size_t stream) { > - if (!ice_ctx_->GetStream(stream)) { > + if (!ice_ctx_->ctx()->GetStream(stream)) { > EXPECT_TRUE(false) << "No such stream " << stream; > return false; > } > - return ice_ctx_->GetStream(stream)->state() == NrIceMediaStream::ICE_OPEN; > + return ice_ctx_->ctx()->GetStream(stream)->state() == NrIceMediaStream::ICE_OPEN; Fold. ::: media/mtransport/test/ice_unittest.cpp:945 (Diff revision 1) > } > } > - ASSERT_NE(index, -1u); > + ASSERT_NE(index, -1); > > - ASSERT_GT(remote_->ice_ctx_->GetStreamCount(), index); > - nsresult res = remote_->ice_ctx_->GetStream(index)->ParseTrickleCandidate( > + ASSERT_GT(remote_->ice_ctx_->ctx()->GetStreamCount(), index); > + nsresult res = Trailing ws ::: media/mtransport/test/ice_unittest.cpp:1134 (Diff revision 1) > - if (!ice_ctx_->GetStream(stream)) { > + if (!ice_ctx_->ctx()->GetStream(stream)) { > ADD_FAILURE() << "No such stream " << stream; > return; > } > > - ASSERT_TRUE(NS_SUCCEEDED(ice_ctx_->GetStream(stream)->SendPacket(component, data, len))); > + ASSERT_TRUE(NS_SUCCEEDED(ice_ctx_->ctx()->GetStream(stream)->SendPacket(component, data, len))); Fold. ::: media/mtransport/test/ice_unittest.cpp:1165 (Diff revision 1) > NS_DISPATCH_SYNC); > } > > void DisableComponent_s(size_t stream, int component_id) { > - ASSERT_LT(stream, ice_ctx_->GetStreamCount()); > - ASSERT_TRUE(ice_ctx_->GetStream(stream).get()) << "No such stream " << stream; > + ASSERT_LT(stream, ice_ctx_->ctx()->GetStreamCount()); > + ASSERT_TRUE(ice_ctx_->ctx()->GetStream(stream).get()) << "No such stream " << stream; Fold. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:361 (Diff revision 1) > } > if (NS_FAILED(rv = mDNSResolver->Init())) { > CSFLogError(logTag, "%s: Failed to initialize dns resolver", __FUNCTION__); > return rv; > } > - if (NS_FAILED(rv = mIceCtx->SetResolver(mDNSResolver->AllocateResolver()))) { > + if (NS_FAILED(rv = mIceCtxHdlr->ctx()->SetResolver(mDNSResolver->AllocateResolver()))) { Fold. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:365 (Diff revision 1) > - mIceCtx->SignalGatheringStateChange.connect( > + mIceCtxHdlr->ctx()->SignalGatheringStateChange.connect( > this, > &PeerConnectionMedia::IceGatheringStateChange_s); > - mIceCtx->SignalConnectionStateChange.connect( > + mIceCtxHdlr->ctx()->SignalConnectionStateChange.connect( > this, > &PeerConnectionMedia::IceConnectionStateChange_s); Common code here. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:405 (Diff revision 1) > - RefPtr<NrIceMediaStream> stream = mIceCtx->CreateStream(os.str().c_str(), > + RefPtr<NrIceMediaStream> stream = mIceCtxHdlr->CreateStream(os.str().c_str(), > aComponentCount); Fix indent. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:622 (Diff revision 1) > + if (mIceCtxHdlr->IsRestarting()) { > + mIceCtxHdlr->ctx()->SignalGatheringStateChange.connect( > + this, > + &PeerConnectionMedia::IceGatheringStateChange_s); > + mIceCtxHdlr->ctx()->SignalConnectionStateChange.connect( > + this, > + &PeerConnectionMedia::IceConnectionStateChange_s); > + } Fix indent. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:623 (Diff revision 1) > + mIceCtxHdlr->ctx()->SignalGatheringStateChange.connect( > + this, > + &PeerConnectionMedia::IceGatheringStateChange_s); > + mIceCtxHdlr->ctx()->SignalConnectionStateChange.connect( > + this, > + &PeerConnectionMedia::IceConnectionStateChange_s); It probably makes sense to break this out into a function, since this is used in the c'tor. Also, we should probably disconnect from the old NrIceCtx when we begin an ICE restart (the old one might not have been ready after all, and I doubt we want two ICE ctx firing signals). This means we'll need to reconnect when a rollback happens. We'll also need to fire an initial state update whenever we switch which NrIceCtx we're listening to. We probably need a function like ConnectSignals(NrIceCtx *new, NrIceCtx* old=nullptr) that disconnects from |old| (if set), connects to |new|, and fires state updates for gathering and connection if the old and new ctx differ.

https://reviewboard.mozilla.org/r/41037/#review37877 > Fold. Also, is stream->name() going to be the same thing as name_? Maybe should reorder for consistency. stream->name() and name\_ are not the same.

https://reviewboard.mozilla.org/r/41041/#review37901 > These will get copied automatically. Weird, because I specifically copied this from elsewhere.

https://reviewboard.mozilla.org/r/41863/#review38505 > Do we need this? It looks like we're using accessors everywhere... Depends on whether we move everything back into NrIceCtx (from NrIceCtxHandler) or not. If we keep the handler, we need friend to construct the NrIceCtx from NrIceCtxHandler.

https://reviewboard.mozilla.org/r/41041/#review37901 > std::swap should work on all this stuff. Or, we could avoid this entirely by having a handler object contain the NrIceCtx (but this means we would need to have a whole bunch of handler.ctx()->blah scattered around). Fixed by containing the NrIceCtx in NrIceCtxHandler

https://reviewboard.mozilla.org/r/41041/#review37901 > Fold. Is the standard 80 or 79 characters?

https://reviewboard.mozilla.org/r/41041/#review37901 > Is the standard 80 or 79 characters? Did I mess up here? I wish reviewboard made it a little easier to check this.

https://reviewboard.mozilla.org/r/41041/#review37901 > Did I mess up here? I wish reviewboard made it a little easier to check this. I specifically ended these right at 80 chars (my editor shades everything wider than 80).

https://reviewboard.mozilla.org/r/41041/#review38249 > I see this is just copied from renegotiation, but I'm wondering why we are setting up the Ice Candidate Handler again... I'm not really sure why it was written that way either. I just reused it.

https://reviewboard.mozilla.org/r/41041/#review38249 > To be safe lets check the return values here and error out in case these failed. Byron asked that GetNewUfrag and GetNewPwd use MOZ_CRASH if they are unable return a valid ufrag/pwd. Is that sufficient?

https://reviewboard.mozilla.org/r/41041/#review38249 > Are you concerned that nr_ice_ctx_create_with_credentials() can actually fail to set these properly or are these just left over from development? I left those as a hint for the next developer to come along to know what was expectd. If you'd rather me replace them with a comment, I can do that.

https://reviewboard.mozilla.org/r/41037/#review37877 > UseTestStunServer() already inits this, we should probably decide whether we want the init repsonsibility on the caller or the callee, and stick with it. Unfortunately, there are too many methods named UseTestStunServer in this file. IceConnectTest::UseTestStunServer does call InitTestStunServer. It also possible to be spinning up IceTestPeers separately, and this ensures that InitTestStunServer is called in that case. There is a guard in InitTestStunServer to make sure only works on the first call.

https://reviewboard.mozilla.org/r/41041/#review38249 > I'm a little bit confused: why do we have ufrag and pwd generators in two places, mtransport and here down in nICEr? > Are these here in nICEr only for unit testing? Can wen then at least mark them as such if not even #ifdef them to be only available in the unit tests? The generators in mtransport are actually calling these functions in nICEr. Originally the ice creds were generated in nICEr, but it was only done within nr_ice_ctx_create. During ICE restart, PeerConnectionImpl generates new creds and uses them to begin the ICE restart and set them in JsepSessionImpl.

https://reviewboard.mozilla.org/r/41041/#review38249 > Why did the buffer length got shortened by factor 10? > Not that I think we really 100 bytes as before, but is 10 really a safe choice here? We're only generating an 8 character random string. After Byron's request, these are now defined using a constant: #define ICE_UFRAG_LEN 8 ... char buf[ICE_UFRAG_LEN+1]; if(r=nr_ice_random_string(buf,ICE_UFRAG_LEN)) > Same here: from 100 down to 40? We're only generating a 32 character random string. After Byron's request, these are now defined using a constant: #define ICE_PWD_LEN 32 ... char buf[ICE_PWD_LEN+1]; if(r=nr_ice_random_string(buf,ICE_PWD_LEN))

https://reviewboard.mozilla.org/r/41041/#review38249 > nr_ice_ctx_create_with_credentials() dupes these again. And nr_ice_random_string() is static. Do we need this r_strdup() call here? These functions are also called in NrIceCtx::GetNewUfrag() and NrIceCtx::GetNewPwd(), so the dup is necessary.

Comment on attachment 8732190 [details] MozReview Request: Bug 906986 - Use the streams from the ice_ctx, don't hold a separate set. https://reviewboard.mozilla.org/r/41035/#review38835

https://reviewboard.mozilla.org/r/41041/#review38249 > RemoteIceIsRestarting? Changed as suggested. > *IceIsRestarting sounds more natural to me (the non native speaker :D ) Changed as suggested.

Comment on attachment 8732192 [details] MozReview Request: Bug 906986 - Wrap NrIceCtx in NrIceCtxHandler which will allow us to handle ice restart. https://reviewboard.mozilla.org/r/41039/#review38865 ::: media/mtransport/nricemediastream.cpp:188 (Diff revision 1) > NrIceMediaStream::Create(NrIceCtx *ctx, > const std::string& name, > int components) { > RefPtr<NrIceMediaStream> stream = > new NrIceMediaStream(ctx, name, components); > + MOZ_ASSERT(stream->ctx_ == ctx->ctx()); Is this a left over from testing? I don't see how NrIceMediaStream() should assign something else here then the provided ctx.

Comment on attachment 8733695 [details] MozReview Request: Bug 906986 - Convert NrIceCtxHandler to composition instead of inheriting from NrIceCtx https://reviewboard.mozilla.org/r/41863/#review38873 ::: media/mtransport/nricectxhandler.cpp:16 (Diff revision 1) > + bool offerer, > + NrIceCtx::Policy policy) Indentation seems off here. One WS less or indent further to the bracket? ::: media/mtransport/nricectxhandler.cpp:73 (Diff revision 1) > + > void > NrIceCtxHandler::BeginIceRestart(const std::string& ufrag, > const std::string& pwd) > { > MOZ_ASSERT(!restart_ctx, "existing ice restart in progress"); If I'm not mistaken MOZ_ASSERT gets compiled out in release builds. And I'm concerned that out in the wild the chances are way higher that someone will manage to get overlapping ICE restarts. Let's log an error and return after the assert to be safe. ::: media/mtransport/nricectxhandler.cpp:88 (Diff revision 1) > - restart_ctx = new_ctx; > + restart_ctx = current_ctx; > + current_ctx = new_ctx; s/restart_ctx/rollback_ctx/ or something which makes it more clear that this is the previous ctx we need for failure roll backs? ::: media/mtransport/test/ice_unittest.cpp:322 (Diff revision 1) > > nr_socket_factory *fac; > int r = nat_->create_socket_factory(&fac); > MOZ_ASSERT(!r); > if (!r) { > - nr_ice_ctx_set_socket_factory(ice_ctx_->ctx(), fac); > + nr_ice_ctx_set_socket_factory(ice_ctx_->ctx()->ctx(), fac); hmm ctx->ctx->ctx looks pretty ugly and confusing

https://reviewboard.mozilla.org/r/41863/#review38873 > Indentation seems off here. One WS less or indent further to the bracket? Fixed

https://reviewboard.mozilla.org/r/41039/#review38865 > Is this a left over from testing? > I don't see how NrIceMediaStream() should assign something else here then the provided ctx. Just a bread crumb for the next developer to come along, and a guard in case some implementation detail changes.

https://reviewboard.mozilla.org/r/41039/#review37891 > So, I've looked at the subsequent patch, and it seems to me that we can just put all this stuff in NrIceCtx and call it a day. The only thing this ends up wrapping is the provisional NrIceCtx for the restart, which NrIceCtx can handle on its own. Byron and I agreed that composition in NrIceCtxHandler was the way to go.

Review commit: https://reviewboard.mozilla.org/r/43119/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43119/

Comment on attachment 8736136 [details] MozReview Request: Bug 906986 - Fixing review issues. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43119/diff/1-2/

Comment on attachment 8736136 [details] MozReview Request: Bug 906986 - Fixing review issues. https://reviewboard.mozilla.org/r/43119/#review39853 ::: media/mtransport/nricectxhandler.h:17 (Diff revision 1) > bool offerer, > bool allow_loopback = false, > bool tcp_enabled = true, > bool allow_link_local = false, > bool hide_non_default = false, > - NrIceCtx::Policy policy = NrIceCtx::ICE_POLICY_ALL); > + NrIceCtx::Policy policy = Trailing ws. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1545 (Diff revision 1) > CSFLogDebug(logTag, "CreateOffer()"); > > nsresult nrv; > if (aOptions.mIceRestart.isSome() && *(aOptions.mIceRestart)) { CreateOffer can be called multiple times before calling SetLocalDescription, perhaps waffling between having an ICE restart and not. We need to be able to handle any transition between these two states. A really simple approach would be to always roll back ICE restarts when CreateOffer (or CreateAnswer) is called, and then handle new requests for an ICE restart, but that could be wasteful if we are restarting now and were also restarting before. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:361 (Diff revision 1) > } > if (NS_FAILED(rv = mDNSResolver->Init())) { > CSFLogError(logTag, "%s: Failed to initialize dns resolver", __FUNCTION__); > return rv; > } > - if (NS_FAILED(rv = mIceCtxHdlr->ctx()->SetResolver(mDNSResolver->AllocateResolver()))) { > + if (NS_FAILED(rv = Trailing ws. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:401 (Diff revision 1) > static_cast<unsigned>(aLevel), > static_cast<unsigned>(aComponentCount)); > > std::ostringstream os; > os << mParentName << " aLevel=" << aLevel; > - RefPtr<NrIceMediaStream> stream = mIceCtxHdlr->CreateStream(os.str().c_str(), > + RefPtr<NrIceMediaStream> stream = Trailing ws. ::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:70 (Diff revision 1) > return NS_OK; > } > > bool > SdpHelper::AreOldTransportParamsValid(const Sdp& oldAnswer, > + const Sdp& oldOffer, We don't really want the old offer, we want the present offerer's old SDP (we talked about this already, just making sure nothing gets forgotten in the shuffle).

Comment on attachment 8736136 [details] MozReview Request: Bug 906986 - Fixing review issues. https://reviewboard.mozilla.org/r/43119/#review39849 A few smaller things to clean up, but in general looks good to me. ::: dom/media/tests/mochitest/test_peerConnection_restartIce.html:21 (Diff revision 1) > + function PC_LOCAL_SET_OFFER_OPTION(test) { > - test.setOfferOptions({ iceRestart: true }); > + test.setOfferOptions({ iceRestart: true }); > - }, > + }, We only need this on one side (could be - just checking)? ::: media/mtransport/nricectxhandler.h:17 (Diff revision 1) > bool offerer, > bool allow_loopback = false, > bool tcp_enabled = true, > bool allow_link_local = false, > bool hide_non_default = false, > - NrIceCtx::Policy policy = NrIceCtx::ICE_POLICY_ALL); > + NrIceCtx::Policy policy = Trailing WS ::: media/mtransport/nricectxhandler.cpp:83 (Diff revision 1) > + MOZ_MTLOG(ML_ERROR, "Existing ice restart in progress"); > + return false; // ice restart already in progress > + } > + > RefPtr<NrIceCtx> new_ctx = new NrIceCtx(this->current_ctx->name(), > - this->current_ctx->offerer(), > + true, // offerer We are always hard coded the offerer? Isn't this one of the bugs Byron recently found? Or do we always need to be in the offerer role to re-start ice? ::: media/mtransport/test/ice_unittest.cpp:292 (Diff revision 1) > IceTestPeer(const std::string& name, bool offerer, > bool allow_loopback = false, bool enable_tcp = true, > bool allow_link_local = false, bool hide_non_default = false) : > name_(name), > ice_ctx_(NrIceCtxHandler::Create(name, offerer, allow_loopback, > - enable_tcp, allow_link_local, hide_non_default)), > + enable_tcp, allow_link_local, Trailing WS ::: media/mtransport/test/ice_unittest.cpp:1166 (Diff revision 1) > NS_DISPATCH_SYNC); > } > > void DisableComponent_s(size_t stream, int component_id) { > ASSERT_LT(stream, ice_ctx_->ctx()->GetStreamCount()); > - ASSERT_TRUE(ice_ctx_->ctx()->GetStream(stream).get()) << "No such stream " << stream; > + ASSERT_TRUE(ice_ctx_->ctx()->GetStream(stream).get()) << "No such stream " Trailing WS ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:976 (Diff revision 1) > } > > return 0; > } > > +#define ICE_UFRAG_LEN 8 Can we move the defines to the top of the file? ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:996 (Diff revision 1) > + *ufrag = 0; > } > return(_status); > } > > +#define ICE_PWD_LEN 32 Move to the top ::: media/mtransport/transportlayerice.cpp:141 (Diff revision 1) > -{ > if (old_stream_ == nullptr) { > return; // no work to do > } > // ICE Ready on the new stream, we can forget the old stream now > - MOZ_MTLOG(ML_INFO, LAYER_INFO << "ResetOldStream(" << old_stream_->name() << "," > + MOZ_MTLOG(ML_INFO, LAYER_INFO << "ResetOldStream(" << old_stream_->name() Trailing WS ::: media/mtransport/transportlayerice.cpp:154 (Diff revision 1) > -{ > if (old_stream_ == nullptr) { > return; // no work to do > } > // ICE Failed on the new stream, we need to restore the old stream now > - MOZ_MTLOG(ML_INFO, LAYER_INFO << "RestoreOldStream(" << old_stream_->name() << "," > + MOZ_MTLOG(ML_INFO, LAYER_INFO << "RestoreOldStream(" << old_stream_->name() Trailing WS ::: media/mtransport/transportlayerice.cpp:162 (Diff revision 1) > + if (stream_->state() == NrIceMediaStream::ICE_OPEN) { > + IceReady(stream_); > + } else if (stream_->state() == NrIceMediaStream::ICE_CLOSED) { > + IceFailed(stream_); > + } What do we do in the ICE_CONNECTING state? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:361 (Diff revision 1) > } > if (NS_FAILED(rv = mDNSResolver->Init())) { > CSFLogError(logTag, "%s: Failed to initialize dns resolver", __FUNCTION__); > return rv; > } > - if (NS_FAILED(rv = mIceCtxHdlr->ctx()->SetResolver(mDNSResolver->AllocateResolver()))) { > + if (NS_FAILED(rv = Trailing WS ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:401 (Diff revision 1) > static_cast<unsigned>(aLevel), > static_cast<unsigned>(aComponentCount)); > > std::ostringstream os; > os << mParentName << " aLevel=" << aLevel; > - RefPtr<NrIceMediaStream> stream = mIceCtxHdlr->CreateStream(os.str().c_str(), > + RefPtr<NrIceMediaStream> stream = Trailing WS

https://reviewboard.mozilla.org/r/43119/#review39849 > We are always hard coded the offerer? Isn't this one of the bugs Byron recently found? > Or do we always need to be in the offerer role to re-start ice? We always tell nICEr that we are the offerer, yes. We do inform nICEr whether we're controlling or not, elsewhere. Once bug 1258753 lands, the only thing this ends up being used for is setting the initial value of |controlling| (which is subsequently overridden).

https://reviewboard.mozilla.org/r/43119/#review39849 > We always tell nICEr that we are the offerer, yes. We do inform nICEr whether we're controlling or not, elsewhere. Once bug 1258753 lands, the only thing this ends up being used for is setting the initial value of |controlling| (which is subsequently overridden). Byron specifically said to set offerer to true here. > Trailing WS Done > Trailing WS Done

https://reviewboard.mozilla.org/r/43119/#review39849 > Trailing WS Done > Trailing WS Done

https://reviewboard.mozilla.org/r/43119/#review39849 > We only need this on one side (could be - just checking)? Yep - the ice restart option only applies when creating the offer. Restart on the answer side is handled by looking at the previous and new offer to see if those ICE credentials are different, not through an option.

https://reviewboard.mozilla.org/r/43119/#review39849 > What do we do in the ICE_CONNECTING state? If we're restoring the old stream and it is in the ICE_CONNECTING state, there are no events to fire here (yet). Once the restored stream moves to ICE_OPEN or ICE_CLOSED the IceReady/IceFailed calls will fire.

Review commit: https://reviewboard.mozilla.org/r/43317/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43317/

Review commit: https://reviewboard.mozilla.org/r/43919/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43919/

Review commit: https://reviewboard.mozilla.org/r/43921/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43921/

Review commit: https://reviewboard.mozilla.org/r/43923/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43923/

Review commit: https://reviewboard.mozilla.org/r/43925/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43925/

Review commit: https://reviewboard.mozilla.org/r/43927/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43927/

Review commit: https://reviewboard.mozilla.org/r/43929/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43929/

Comment on attachment 8737364 [details] MozReview Request: Bug 906986 - Ice restart and tests. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43929/diff/1-2/

Comment on attachment 8737364 [details] MozReview Request: Bug 906986 - Ice restart and tests. https://reviewboard.mozilla.org/r/43929/#review40777 ::: media/mtransport/nricectxhandler.cpp:81 (Diff revision 2) > + // reconstitute the hide_non_default bool based on the flags stored in the > + // original ctx flags > + bool hide_non_default = > + current_ctx->ctx()->flags & NR_ICE_CTX_FLAGS_ONLY_DEFAULT_ADDRS; Hmm. I think I'd rather this be chosen by the caller, and for PeerConnectionMedia to just choose based on prefs like it does for any other ctx. This would mean we wouldn't need to access the flags from main. ::: media/mtransport/transportlayerice.cpp:105 (Diff revision 2) > + if (stream_ && !old_stream_) { > + MOZ_ASSERT(stream_ != stream); // make sure we're getting a different stream > + MOZ_ASSERT(old_stream_ == nullptr); Asserting that old_stream_ is set is probably overkill given that we just checked. ::: media/mtransport/transportlayerice.cpp:153 (Diff revision 2) > + > +void TransportLayerIce::RestoreOldStream() { > + if (old_stream_ == nullptr) { > + return; // no work to do > + } > + // ICE Failed on the new stream, we need to restore the old stream now This happens only on rollback right? ICE failure on the new stream doesn't let us fall back. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1569 (Diff revision 2) > + if (!restartIce && mMedia->ice_ctx_hdlr()->IsRestarting()) { > + RollbackIceRestart(); > + } Seems like this can roll back an ICE restart from a previous offer/answer exchange, which isn't right. The tricky bit here is we are finalizing when ICE succeeds on the new ctx, but we aren't doing anything when offer/answer completes; once that happens, we are not allowed to roll back anymore, although we can accept traffic on the old ctx for a bit. We probably need another function like CommitIceRestart (point of no return, although the old ctx can remain active), and FinalizeIceRestart can clean up the old ctx. In CreateOffer, if an ICE restart is asked for, we need to make room for a new ctx one way or another. If we have an ICE restart ctx that can be rolled back, we do that. If we have a _committed_ ICE restart, then we're kinda forced to finalize even though we aren't ready. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1648 (Diff revision 2) > + if (mJsepSession->RemoteIceIsRestarting()) { > + CSFLogInfo(logTag, "Answerer restarting ice"); > + nrv = SetupIceRestart(); This can be called multiple times for the same offer/answer exchange. Building on the committed/not-committed distinction above, we should do the following: if (Committed ICE restart) finalize if (no ICE restart) start ICE restart ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2876 (Diff revision 2) > + if (rollback && mMedia->ice_ctx_hdlr()->IsRestarting()) { > + RollbackIceRestart(); > + } We should only rollback non-committed ICE restarts here, otherwise we might get one from a previous offer/answer exchange.

Comment on attachment 8737364 [details] MozReview Request: Bug 906986 - Ice restart and tests. https://reviewboard.mozilla.org/r/43929/#review40825 LGTM ::: dom/media/tests/mochitest/test_peerConnection_restartIceNoRtcpMux.html:17 (Diff revision 2) > + }); > + > + var test; > + runNetworkTest(function (options) { > + options = options || { }; > + options.rtcpmux = false; Sorry for spotting this now: but can we make this a no-bundle & no-rtcp-mux test please? I know we have an extra test case for no-rtcp-mux with bundle still in place. But that is really only for covering really strange setups. As the no-rtcp-mux & no-bundle version contains even more ICE streams and therefore is kind of a super set of this here with even more streams I think it would better to have that checked (and it's also more likely to be hit in the field with some legacy device).

Comment on attachment 8737359 [details] MozReview Request: Bug 906986 - Disabling TestSrflxCandPairingFilter until Bug 1226838 is fixed. https://reviewboard.mozilla.org/r/43919/#review41025

Comment on attachment 8737360 [details] MozReview Request: Bug 906986 - Rework fix for Bug 1241690 to avoid reliance on NrIceCtx inside NrIceMediaStream. https://reviewboard.mozilla.org/r/43921/#review41029

Comment on attachment 8737361 [details] MozReview Request: Bug 906986 - Use the streams from the ice_ctx, don't hold a separate set. https://reviewboard.mozilla.org/r/43923/#review41031

Comment on attachment 8737362 [details] MozReview Request: Bug 906986 - Genericize some of the test setup and improve logging. https://reviewboard.mozilla.org/r/43925/#review41033

Comment on attachment 8737363 [details] MozReview Request: Bug 906986 - Wrap NrIceCtx in NrIceCtxHandler which will allow us to handle ice restart. https://reviewboard.mozilla.org/r/43927/#review41035

Comment on attachment 8737359 [details] MozReview Request: Bug 906986 - Disabling TestSrflxCandPairingFilter until Bug 1226838 is fixed. https://reviewboard.mozilla.org/r/43919/#review41053

Comment on attachment 8737360 [details] MozReview Request: Bug 906986 - Rework fix for Bug 1241690 to avoid reliance on NrIceCtx inside NrIceMediaStream. https://reviewboard.mozilla.org/r/43921/#review41055

Comment on attachment 8737361 [details] MozReview Request: Bug 906986 - Use the streams from the ice_ctx, don't hold a separate set. https://reviewboard.mozilla.org/r/43923/#review41057

Comment on attachment 8737362 [details] MozReview Request: Bug 906986 - Genericize some of the test setup and improve logging. https://reviewboard.mozilla.org/r/43925/#review41059

Comment on attachment 8737363 [details] MozReview Request: Bug 906986 - Wrap NrIceCtx in NrIceCtxHandler which will allow us to handle ice restart. https://reviewboard.mozilla.org/r/43927/#review41061

https://reviewboard.mozilla.org/r/43929/#review40825 > Sorry for spotting this now: but can we make this a no-bundle & no-rtcp-mux test please? > > I know we have an extra test case for no-rtcp-mux with bundle still in place. But that is really only for covering really strange setups. > As the no-rtcp-mux & no-bundle version contains even more ICE streams and therefore is kind of a super set of this here with even more streams I think it would better to have that checked (and it's also more likely to be hit in the field with some legacy device). I went ahead and just added another test that does both no-bundle and no-rtcp-mux.

Review commit: https://reviewboard.mozilla.org/r/44575/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44575/

Comment on attachment 8737364 [details] MozReview Request: Bug 906986 - Ice restart and tests. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43929/diff/2-3/

Comment on attachment 8738551 [details] MozReview Request: Bug 906986 - Rework rollback/finalize to include a committed state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44575/diff/1-2/

Comment on attachment 8737364 [details] MozReview Request: Bug 906986 - Ice restart and tests. https://reviewboard.mozilla.org/r/43929/#review41305

Comment on attachment 8738551 [details] MozReview Request: Bug 906986 - Rework rollback/finalize to include a committed state. https://reviewboard.mozilla.org/r/44575/#review41307 ::: media/mtransport/nricectxhandler.h:11 (Diff revision 2) > namespace mozilla { > > class NrIceCtxHandler { > public: > + enum RestartState { ICE_RESTART_NONE, > + ICE_RESTART_IN_PROGRESS, I think I would call this ICE_RESTART_PROVISIONAL. ::: media/mtransport/nricectxhandler.h:57 (Diff revision 2) > DISALLOW_COPY_ASSIGN(NrIceCtxHandler); > > RefPtr<NrIceCtx> current_ctx; > RefPtr<NrIceCtx> old_ctx; // for while restart is in progress > int restart_count; // used to differentiate streams between restarted ctx > + RestartState restart_state; This is being used on both main (reads) and STS (reads and writes). It might be better to handle the thread dispatch from main to STS within NrIceCtxHandler::Commit/Finalize/Rollback instead of in PCMedia, and update/check the state only on main. Or, this could be moved to PCMedia, to avoid having parts of this class accessible on main only, and other parts on STS only. I'm on the fence between these two options, I'll leave it up to you. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2905 (Diff revision 2) > - if (rollback && mMedia->ice_ctx_hdlr()->IsRestarting()) { > + if (rollback && > + mMedia->ice_ctx_hdlr()->GetRestartState() == > + NrIceCtxHandler::ICE_RESTART_IN_PROGRESS) { > RollbackIceRestart(); > + } else if (mMedia->ice_ctx_hdlr()->IsRestarting()) { > + mMedia->CommitIceRestart(); > } This will read better if you wrap the whole thing in an if (ICE_RESTART_PROVISIONAL) check; that's the only case where we ought to do anything here anyhow.

Comment on attachment 8738551 [details] MozReview Request: Bug 906986 - Rework rollback/finalize to include a committed state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44575/diff/2-3/

Comment on attachment 8738551 [details] MozReview Request: Bug 906986 - Rework rollback/finalize to include a committed state. https://reviewboard.mozilla.org/r/44575/#review41345

Comment on attachment 8738551 [details] MozReview Request: Bug 906986 - Rework rollback/finalize to include a committed state. https://reviewboard.mozilla.org/r/44575/#review41421

last try run, I had to poke manually to get it to finish up: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59d35143789e

there are conflicts like applying 3040c6537ad7 patching file media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp Hunk #9 FAILED at 767 1 out of 10 hunks FAILED -- saving rejects to file media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp.rej patch failed to apply abort: fix up the working directory and run hg transplant --continue could you take a look, thanks!

(In reply to Carsten Book [:Tomcat] from comment #98) > there are conflicts like > > applying 3040c6537ad7 > patching file > media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp > Hunk #9 FAILED at 767 > 1 out of 10 hunks FAILED -- saving rejects to file > media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp.rej > patch failed to apply > abort: fix up the working directory and run hg transplant --continue > > could you take a look, thanks! Looks like the failure is localized to EnsureIceGathering_s: @@ -660,31 +838,32 @@ PeerConnectionMedia::GatherIfReady() { &PeerConnectionMedia::EnsureIceGathering_s)); PerformOrEnqueueIceCtxOperation(runnable); } void PeerConnectionMedia::EnsureIceGathering_s() { if (mProxyServer) { - mIceCtx->SetProxyServer(*mProxyServer); + mIceCtxHdlr->ctx()->SetProxyServer(*mProxyServer); } // Start gathering, but only if there are streams - for (size_t i = 0; i < mIceCtx->GetStreamCount(); ++i) { - if (mIceCtx->GetStream(i)) { - mIceCtx->StartGathering(); + for (size_t i = 0; i < mIceCtxHdlr->ctx()->GetStreamCount(); ++i) { + if (mIceCtxHdlr->ctx()->GetStream(i)) { + mIceCtxHdlr->ctx()->StartGathering(); return; } } // If there are no streams, we're probably in a situation where we've rolled // back while still waiting for our proxy configuration to come back. Make // sure content knows that the rollback has stuck wrt gathering. - IceGatheringStateChange_s(mIceCtx.get(), NrIceCtx::ICE_CTX_GATHER_COMPLETE); + IceGatheringStateChange_s(mIceCtxHdlr->ctx().get(), + NrIceCtx::ICE_CTX_GATHER_COMPLETE); } nsresult PeerConnectionMedia::AddTrack(DOMMediaStream& aMediaStream, const std::string& streamId, MediaStreamTrack& aTrack, const std::string& trackId) {

Now there are a bunch of conflicts on the final commit. Could you rebase these so they can cleanly apply?

So I actually managed to get this rebased only to find out that "Ice restart and tests." needs DOM peer review due to the webidl changes. That was a fun use of my time...

I'll stash away the rebased patches on the off chance you get a DOM peer review and want to re-land. Feel free to ping me when you're ready.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #102) > I'll stash away the rebased patches on the off chance you get a DOM peer > review and want to re-land. Feel free to ping me when you're ready. My apologies - I didn't know about the DOM peer review. I'll get on that asap.

Comment on attachment 8737364 [details] MozReview Request: Bug 906986 - Ice restart and tests. https://reviewboard.mozilla.org/r/43929/#review41715 I guess I need to r- because of not following the spec. Trying to follow the spec's webidl as closely as possible helps with maintaining the code and also with reviewing the code. So, either fix the webidl or explain why we're doing something else than what is in the spec. ::: dom/webidl/RTCPeerConnection.webidl:65 (Diff revision 3) > }; > > dictionary RTCOfferOptions : RTCOfferAnswerOptions { > long offerToReceiveVideo; > long offerToReceiveAudio; > - // boolean iceRestart = false; // Not implemented (Bug 906986) > + boolean iceRestart; Per spec iceRestart defaults to false. Why we don't have that? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1537 (Diff revision 3) > if (aOptions.mOfferToReceiveVideo.WasPassed()) { > options.mOfferToReceiveVideo = > mozilla::Some(size_t(aOptions.mOfferToReceiveVideo.Value())); > } > > + if (aOptions.mIceRestart.WasPassed()) { So per spec we should always set options.mIceRestart here. No need to deal with WasPassed() or anything, since aOptions.mIceRestart just has a valid value always.

Review commit: https://reviewboard.mozilla.org/r/45275/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45275/

Comment on attachment 8739441 [details] MozReview Request: Bug 906986 - give RTCOfferOptions.iceRestart default value per spec. Looks like mozreview isn't working properly atm. Marking r+ here.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a392a2f7b117 https://hg.mozilla.org/integration/mozilla-inbound/rev/0786093c2c50 https://hg.mozilla.org/integration/mozilla-inbound/rev/b752c6a3ed5a https://hg.mozilla.org/integration/mozilla-inbound/rev/603dd13fc4e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/f6dd0039f260 https://hg.mozilla.org/integration/mozilla-inbound/rev/651e8c7355ba https://hg.mozilla.org/integration/mozilla-inbound/rev/dbde614eac1b

https://hg.mozilla.org/mozilla-central/rev/a392a2f7b117 https://hg.mozilla.org/mozilla-central/rev/0786093c2c50 https://hg.mozilla.org/mozilla-central/rev/b752c6a3ed5a https://hg.mozilla.org/mozilla-central/rev/603dd13fc4e7 https://hg.mozilla.org/mozilla-central/rev/f6dd0039f260 https://hg.mozilla.org/mozilla-central/rev/651e8c7355ba https://hg.mozilla.org/mozilla-central/rev/dbde614eac1b

First pass at ICE restart info for sheppy: ICE restart is the process of exchanging new information about the network connection between the peers of a call. This is useful for establishing connectivity in cases where failures are detected by the application or when the application recognizes that network conditions have changed, for example, on mobile when moving from cellular to WiFi networks. ICE restart is really no different than the initial ICE processing except that during restart, media continues to flow over the original network connections. Currently only full ICE restart (all media streams in the session) is supported. However, partial ICE restart will be supported in the future which will allow ICE restart on individual media streams. In both the full restart and partial restart cases, either peer can initiate ICE restart by sending an updated offer with new ice-ufrag and ice-pwd attributes.

(In reply to Michael Froman [:mjf] from comment #109) > First pass at ICE restart info for sheppy: > > ICE restart is the process of exchanging new information about the > network connection between the peers of a call. This is useful for > establishing connectivity in cases where failures are detected by the > application or when the application recognizes that network conditions > have changed, for example, on mobile when moving from cellular to WiFi > networks. ICE restart is really no different than the initial ICE > processing except that during restart, media continues to flow over the > original network connections. > > Currently only full ICE restart (all media streams in the session) is > supported. However, partial ICE restart will be supported in the future > which will allow ICE restart on individual media streams. In both the > full restart and partial restart cases, either peer can initiate ICE > restart by sending an updated offer with new ice-ufrag and ice-pwd > attributes. Thank you for this info! Very helpful. When the people who know these things share with the writers, we get all kinds of happy.

(In reply to Michael Froman [:mjf] from comment #109) > Currently only full ICE restart (all media streams in the session) is > supported. However, partial ICE restart will be supported in the future > which will allow ICE restart on individual media streams. In both the > full restart and partial restart cases, either peer can initiate ICE > restart by sending an updated offer with new ice-ufrag and ice-pwd > attributes. Regarding full vs. partial restarts: by "session" do you mean "connection" or does a full ICE restart renegotiate every RTCPeerConnection?

I've added info about ICE restart here: https://developer.mozilla.org/en-US/docs/Web/API/WebRTC_API/Session_lifetime#ICE_restart This is tentative pending answers to the above question, plus this one: Does ICE restart ever trigger automatically, or does the Web site/app have to detect appropriate conditions and trigger it explicitly?

(In reply to Eric Shepherd [:sheppy] from comment #111) > (In reply to Michael Froman [:mjf] from comment #109) > > > Currently only full ICE restart (all media streams in the session) is > > supported. However, partial ICE restart will be supported in the future > > which will allow ICE restart on individual media streams. In both the > > full restart and partial restart cases, either peer can initiate ICE > > restart by sending an updated offer with new ice-ufrag and ice-pwd > > attributes. > > Regarding full vs. partial restarts: by "session" do you mean "connection" > or does a full ICE restart renegotiate every RTCPeerConnection? ICE restart is within the RTPPeerConnection, and is triggered on the offerer side by setting the iceRestart option to true in the RTCOfferOptions.

(In reply to Eric Shepherd [:sheppy] from comment #112) > I've added info about ICE restart here: > https://developer.mozilla.org/en-US/docs/Web/API/WebRTC_API/ > Session_lifetime#ICE_restart > > This is tentative pending answers to the above question, plus this one: Does > ICE restart ever trigger automatically, or does the Web site/app have to > detect appropriate conditions and trigger it explicitly? The answerer side is triggered automatically when new ice-ufrag and ice-pwd attributes are detected in the offer. The offerer side requires setting the iceRestart option to true in the RTCOfferOptions. I think that has to be done from the site/app.

Pages updated: https://developer.mozilla.org/en-US/docs/Web/API/WebRTC_API/Session_lifetime#ICE_restart https://developer.mozilla.org/en-US/docs/Web/Events/iceconnectionstatechange https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/48