1146462 - Disabling an m-section doesn't tear down the ICE stream

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Byron Campen [:bwc]]

While working on bug 952145, I've discovered that ICE streams are not being properly torn down (or disabled) when an m-section is disabled by renegotiation. The danger is that if that m-section is ever re-used (and isn't bundled), we will try to re-use the ICE stream, which probably will not work. I may be able to decompose the work on 952145 to get a small fix for this.

Comment 1

[Byron Campen [:bwc]]

Looks like this won't be a small fix, unfortunately.

Comment 2

[Byron Campen [:bwc]]

Attachment: MozReview Request: bz://1146462/bwc

/r/5917 - Bug 1146462: (WIP) Close ICE transports when m-sections are disabled.

Pull down this commit:

hg pull review -r aca2f18722ad447428efab8ba9743c519ea33d8b

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8582090 [details]
MozReview Request: bz://1146462/bwc

/r/5917 - Bug 1146462: Close ICE transports when m-sections are disabled.

Pull down this commit:

hg pull review -r 14fd792469dcf650dcdfeb7cdf00b371746cfdf9

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8582090 [details]
MozReview Request: bz://1146462/bwc

/r/5917 - Bug 1146462: Close ICE transports when m-sections are disabled.

Pull down this commit:

hg pull review -r 02d992ab485cca844940e435383b1ee86550363b

Comment 5

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db7698cb1fc5

Comment 6

[Martin Thomson [:mt:]]

I hope that next week is OK.

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8582090 [details]
MozReview Request: bz://1146462/bwc

Actually, on the way home I decided to simplify the behavior I was shooting for. I may ask drno once that change has been made.

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8582090 [details]
MozReview Request: bz://1146462/bwc

/r/5917 - Bug 1146462: Close ICE transports when m-sections are disabled.

Pull down this commit:

hg pull review -r cdb61f41d7b017384733f4cfd19bde81ab365703

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8582090 [details]
MozReview Request: bz://1146462/bwc

The tricky work in this patch was handling the case where the master bundle transport was disabled, and making sure that the appropriate candidate attributes were carried over into the reoffer. The new behavior when creating a reoffer is to give every m-section its own distinct transport/set of candidates (if the previous m-section was either a master bundle m-section, or unbundled, the old candidates/transport can safely be reused). This also means now we can handle m-sections being removed from the bundle, because everything has a dedicated transport just in case. The downside is we do a round of gathering on every reoffer where bundle is in use, but that's no too bad.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06bc5469efe8

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8582090 [details]
MozReview Request: bz://1146462/bwc

/r/5917 - Bug 1146462: Close ICE transports when m-sections are disabled.

Pull down this commit:

hg pull review -r e1057a122e9edfcb88110e60ab07dc8879f0d7b2

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8582090 [details]
MozReview Request: bz://1146462/bwc

Had to update transport_unittest

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d49d6eea2f4

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8582090 [details]
MozReview Request: bz://1146462/bwc

/r/5917 - Bug 1146462: Close ICE transports when m-sections are disabled.

Pull down this commit:

hg pull review -r d2761686b0fe37b93c770739566428e203536e9f

Comment 13

[Byron Campen [:bwc]]

Comment on attachment 8582090 [details]
MozReview Request: bz://1146462/bwc

Rebase.

Comment 14

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/5917/#review5171

::: media/mtransport/nricectx.cpp
(Diff revision 6)
> +NrIceCtx::SetStream(size_t index, NrIceMediaStream* stream) {
> +  if (index >= streams_.size()) {
> +    streams_.resize(index + 1);
> +  }
>  
> -  if (stream) {
> -    streams_.push_back(stream);
> +  if (streams_[index]) {
> +    streams_[index]->Close();
>    }
>  
> -  return stream;
> +  streams_[index] = stream;

Do we want an upper limit here, to prevent some faulty code creating 65000 streams?

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
(Diff revision 6)
> +    while(pctx){
> +      if(!nr_ice_peer_ctx_find_pstream(pctx, *streamp, &peer_stream)) {
> +        if(r=nr_ice_peer_ctx_remove_pstream(pctx, &peer_stream)) {
> +          ABORT(r);
> +        }
> +      }
> +
> +      pctx=STAILQ_NEXT(pctx,entry);
> +    }

Why do we continue to iterate through the peer contexts after we removed the stream?
Can the same stream be in multiple contexts? If that's the case we should probably continue through the contexts after encountering the first error.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 6)
> -  if (IsBundleSlave(*sdp, level)) {
> +  if (mState == kJsepStateStable) {
> +    const Sdp* answer(GetAnswer());
> +    if (IsBundleSlave(*answer, level)) {
> -    // We do not add candidate attributes to bundled m-sections unless they
> +      // We do not add candidate attributes to bundled m-sections unless they
> -    // are the "master" bundle m-section.
> +      // are the "master" bundle m-section.
> -    *skipped = true;
> +      *skipped = true;
> -    return NS_OK;
> +      return NS_OK;
> -  }
> +    }
> +  }
>  
>    *skipped = false;
>  
>    return AddCandidateToSdp(sdp, candidate, mid, level);

So if this gets called in haveRemoteOffer or haveLocalOffer we don't check for BundleSlave and just add to the SDP?

Besides the comments/questions I think a test which goes through dis-abling/removing and re-adding the same might be helpful.

Comment 15

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5917/#review5187

> Do we want an upper limit here, to prevent some faulty code creating 65000 streams?

Well, the worst that will happen is we'll waste some memory on a really sparse array, and spend extra time when we have to iterate it. It seems like this should only happen if we have that many m-sections. Probably not worth having an arbitrary sanity check, since if we end up in that situation we're likely dealing with memory corruption or something.

> Why do we continue to iterate through the peer contexts after we removed the stream?
> Can the same stream be in multiple contexts? If that's the case we should probably continue through the contexts after encountering the first error.

It can be in multiple contexts, yes, but not in the way we use nICEr. This is a SIP forking thing, and just like everywhere else it touches, we end up with an error handling conundrum that you noticed here. The error handling in nICEr is generally to stop whatever you're doing when an error is encountered, and given that this is a "should never happen" error (nr_ice_media_stream_destroy doesn't fail), I'm not sure I'd want the code to keep trying.

> So if this gets called in haveRemoteOffer or haveLocalOffer we don't check for BundleSlave and just add to the SDP?

Yeah, because we don't know for sure whether we're doing bundle or not this time.

There's a test in signaling_unittests. That should be enough I think.

Comment 16

[Nils Ohlmeier [:drno]]

Comment on attachment 8582090 [details]
MozReview Request: bz://1146462/bwc

https://reviewboard.mozilla.org/r/5915/#review5233

Ship It!

Comment 17

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd46eb4124a

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6dd46eb4124a

Comment 20

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1146462: Close ICE transports when m-sections are disabled.