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

RESOLVED FIXED in Firefox 39

Status

()

Core
WebRTC: Signaling
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

Trunk
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → docfaraday
(Assignee)

Comment 1

3 years ago
Looks like this won't be a small fix, unfortunately.
(Assignee)

Comment 2

3 years ago
Created attachment 8582090 [details]
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
(Assignee)

Comment 3

3 years ago
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
(Assignee)

Comment 4

3 years ago
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
(Assignee)

Updated

3 years ago
Attachment #8582090 - Flags: review?(martin.thomson)
I hope that next week is OK.
(Assignee)

Comment 7

3 years ago
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.
Attachment #8582090 - Flags: review?(martin.thomson)
(Assignee)

Comment 8

3 years ago
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
(Assignee)

Comment 9

3 years ago
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
Attachment #8582090 - Flags: review?(drno)
(Assignee)

Comment 10

3 years ago
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
Attachment #8582090 - Flags: review?(drno)
(Assignee)

Comment 11

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

Had to update transport_unittest

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d49d6eea2f4
Attachment #8582090 - Flags: review?(drno)
(Assignee)

Comment 12

3 years ago
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
Attachment #8582090 - Flags: review?(drno)
(Assignee)

Comment 13

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

Rebase.
Attachment #8582090 - Flags: review?(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.
(Assignee)

Comment 15

3 years ago
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 on attachment 8582090 [details]
MozReview Request: bz://1146462/bwc

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

Ship It!
Attachment #8582090 - Flags: review?(drno) → review+
https://hg.mozilla.org/mozilla-central/rev/6dd46eb4124a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Updated

3 years ago
Depends on: 1150966
(Assignee)

Comment 19

3 years ago
Comment on attachment 8582090 [details]
MozReview Request: bz://1146462/bwc
Attachment #8582090 - Attachment is obsolete: true
Attachment #8619848 - Flags: review+
(Assignee)

Comment 20

3 years ago
Created attachment 8619848 [details]
MozReview Request: Bug 1146462: Close ICE transports when m-sections are disabled.
You need to log in before you can comment on or make changes to this bug.