1269486 - ICE role switch when answerer starts renegotiation

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Nils Ohlmeier [:drno]]

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."

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: 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/

Comment 2

[Nils Ohlmeier [:drno]]

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?

Comment 3

[Byron Campen [:bwc]]

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).

Comment 4

[Nils Ohlmeier [:drno]]

Attachment: 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/

Comment 5

[Byron Campen [:bwc]]

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.

Comment 6

[Nils Ohlmeier [:drno]]

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/

Comment 7

[Nils Ohlmeier [:drno]]

Attachment: 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/

Comment 8

[Nils Ohlmeier [:drno]]

Attachment: 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/

Comment 9

[Nils Ohlmeier [:drno]]

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 10

[Byron Campen [:bwc]]

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...

Comment 11

[Nils Ohlmeier [:drno]]

(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 12

[Michael Froman [:mjf]]

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/

Comment 13

[Nils Ohlmeier [:drno]]

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/

Comment 14

[Michael Froman [:mjf]]

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

carry forward r+

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d22d6009d64
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c03f242568d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d22d6009d64
https://hg.mozilla.org/mozilla-central/rev/3c03f242568d