ICE role switch when answerer starts renegotiation

RESOLVED FIXED in Firefox 49

Status

()

P1
normal
Rank:
10
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: drno, Assigned: drno)

Tracking

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox46 unaffected, firefox47 unaffected, firefox48 unaffected, firefox49 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

test_peerConnection_answererAddSecondAudioStream.html shows that we are switching the ICE roles controlled and controlling after the answerer renegotiated an additional stream.

But RFC 5245 says in section 5.2 https://tools.ietf.org/html/rfc5245#section-5.2
  "Once roles are determined for a session, they persist unless ICE is restarted."
backlog: --- → webrtc/webaudio+
Rank: 10
Created attachment 8748087 [details]
MozReview Request: Bug 1269486: prevent JsepSession from overwritting IceControlling

Review commit: https://reviewboard.mozilla.org/r/50151/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50151/
Attachment #8748087 - Flags: review?(docfaraday)
I'm contemplating if we should also figure out a way to make this SetControlling() call only once:
https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#599
Or have SetControlling() internally ignore any call after the initial, first call.

What do you think Byron?
Flags: needinfo?(docfaraday)

Updated

3 years ago
Attachment #8748087 - Flags: review?(docfaraday)
Comment on attachment 8748087 [details]
MozReview Request: Bug 1269486: prevent JsepSession from overwritting IceControlling

https://reviewboard.mozilla.org/r/50151/#review47003

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1274
(Diff revision 1)
>                                           const UniquePtr<Sdp>& remote)
>  {
>    bool remoteIceLite =
>        remote->GetAttributeList().HasAttribute(SdpAttribute::kIceLiteAttribute);
>  
> +  if (mNegotiations == 0) {

Since this is a rule in the ICE spec, not the JSEP spec, it probably makes sense to push this over to mtransport (right now, this would be NrIceCtx::SetControlling).

Updated

3 years ago
Flags: needinfo?(docfaraday)
Created attachment 8748347 [details]
MozReview Request: Bug 1269486: allow setting ICE controller only once. r=bwc

Review commit: https://reviewboard.mozilla.org/r/50231/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50231/
Attachment #8748087 - Attachment is obsolete: true
Attachment #8748347 - Flags: review?(docfaraday)
Comment on attachment 8748347 [details]
MozReview Request: Bug 1269486: allow setting ICE controller only once. r=bwc

https://reviewboard.mozilla.org/r/50231/#review47065

::: media/mtransport/nricectx.h:374
(Diff revision 1)
>  
>    ConnectionState connection_state_;
>    GatheringState gathering_state_;
>    const std::string name_;
>    bool offerer_;
> +  bool ice_controller_set_;

I'd name this ice_controlling_set_, but yeah this looks good.
Attachment #8748347 - Flags: review?(docfaraday) → review+
Comment on attachment 8748347 [details]
MozReview Request: Bug 1269486: allow setting ICE controller only once. r=bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50231/diff/1-2/
Attachment #8748347 - Attachment description: MozReview Request: Bug 1269486: allow setting ICE controller only once → MozReview Request: Bug 1269486: allow setting ICE controller only once. r=bwc
Created attachment 8748755 [details]
MozReview Request: Bug 1269486: tests to verify new ICE controller setting restriction. r=mjf

Review commit: https://reviewboard.mozilla.org/r/50515/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50515/
Attachment #8748755 - Flags: review?(mfroman)
Attachment #8748347 - Attachment is obsolete: true
Created attachment 8748769 [details]
MozReview Request: Bug 1269486: allow setting ICE controller only once. r=bwc

Review commit: https://reviewboard.mozilla.org/r/50525/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50525/
Attachment #8748769 - Flags: review?(docfaraday)
Comment on attachment 8748755 [details]
MozReview Request: Bug 1269486: tests to verify new ICE controller setting restriction. r=mjf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50515/diff/1-2/
Comment on attachment 8748769 [details]
MozReview Request: Bug 1269486: allow setting ICE controller only once. r=bwc

https://reviewboard.mozilla.org/r/50525/#review47303

Not sure why rb didn't carry forward my review...
Attachment #8748769 - Flags: review?(docfaraday) → review+
(In reply to Byron Campen [:bwc] (PTO Apr 15) from comment #10)
> Not sure why rb didn't carry forward my review...

Because I followed the suggestion for the hg mozreview plugin and cherry picked the change set with the tests in it. But that wiped out the previous review request completely, including the r+ :-(
Comment on attachment 8748755 [details]
MozReview Request: Bug 1269486: tests to verify new ICE controller setting restriction. r=mjf

https://reviewboard.mozilla.org/r/50515/#review47333

::: media/mtransport/test/ice_unittest.cpp:2623
(Diff revision 2)
> +  ASSERT_EQ(NrIceCtx::ICE_CONTROLLED, p1_->GetControlling());
> +  ASSERT_EQ(NrIceCtx::ICE_CONTROLLING, p2_->GetControlling());
> +
> +  p2_->RestartIce();
> +  ASSERT_FALSE(p2_->gathering_complete());
> +  // ICE restart should allow us to set controll role again

s/controll/control/

::: media/mtransport/test/ice_unittest.cpp:2626
(Diff revision 2)
> +  p2_->RestartIce();
> +  ASSERT_FALSE(p2_->gathering_complete());
> +  // ICE restart should allow us to set controll role again
> +  p2_->SetControlling(NrIceCtx::ICE_CONTROLLED);
> +  ASSERT_EQ(NrIceCtx::ICE_CONTROLLED, p2_->GetControlling());
> +  // But still only allowed to set controll role once

s/controll/control/

::: media/mtransport/test/ice_unittest.cpp:2635
(Diff revision 2)
> +  mozilla::UniquePtr<IceTestPeer> p3_;
> +  p3_ = MakeUnique<IceTestPeer>("P3", test_utils_, true, false,
> +                                false, false, false);
> +  InitPeer(p3_.get());
> +  p3_->AddStream(1);
> +  // Set controll role for p3 accordingly (w/o role conflict)

s/controll/control/
Attachment #8748755 - Flags: review?(mfroman) → review+
Comment on attachment 8748755 [details]
MozReview Request: Bug 1269486: tests to verify new ICE controller setting restriction. r=mjf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50515/diff/2-3/
status-firefox46: --- → unaffected
status-firefox47: --- → unaffected
status-firefox48: --- → unaffected

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d22d6009d64
https://hg.mozilla.org/mozilla-central/rev/3c03f242568d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.