1144432 - Re-enabling a previously deactivated m-section result in mid error

Status: RESOLVED FIXED

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

Description

[Nils Ohlmeier [:drno]]

After removing a video track the m-section correctly get marked as inactive in the offer and answer:

m=video 0 RTP/SAVPF 111
c=IN IP4 0.0.0.0
a=inactive
a=rtpmap:111 NULL/0

The subsequent attempt to add a track of the same type results in adding that m-section again with exactly the same mid, but that raises the following error:

Error name: InvalidSessionDescriptionError
Error message: New remote description changes mid for level 2, was '" now 'sdparta_2'

We should probably leave the mid even in inactive m-sections, but might also need to remember the id's in case the other side does not send us mid's for inactive sections.

Comment 1

[Byron Campen [:bwc]]

So, it looks like it is technically allowed to change the mid in renegotiation (the spec says they SHOULD remain the same). So, maybe we should just remove the check.

Comment 2

[Byron Campen [:bwc]]

Seeing as this is a fairly major interop problem, we should consider an uplift here, particularly considering that 38 is going to ESR.

Comment 3

[Maire Reavy [:mreavy]]

(In reply to Byron Campen [:bwc] from comment #2)
> Seeing as this is a fairly major interop problem, we should consider an
> uplift here, particularly considering that 38 is going to ESR.

I'd love to uplift the fix for this into 38 when we have it.  So let's get a patch up for review and landed in Nightly as soon as we can, and then we'll evaluate risk/reward for aurora uplift.

Comment 4

[Nils Ohlmeier [:drno]]

I could see a problem if a section with a label A, e.g. an audio section, suddenly shows up with label B as this might be an indicator that also the type or parameters might have changed.
But I don't see a problem with this particular check: changing from '' (nothing) to a new id seems safe.

Comment 5

[Byron Campen [:bwc]]

Attachment: MozReview Request: bz://1144432/bwc

/r/5573 - Bug 1144432: Allow mid to change in renegotiation, check that mid doesn't change in answer, and disable some checking when the m-section was previously disabled.

Pull down this commit:

hg pull review -r b0ccc57d17c84baf356fe7539850b97e2705daaf

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8579040 [details]
MozReview Request: bz://1144432/bwc

I need to also convince myself that permitting the mid to change won't break other code.

Comment 7

[Byron Campen [:bwc]]

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

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8579040 [details]
MozReview Request: bz://1144432/bwc

/r/5573 - Bug 1144432 - Part 1: Test-case
/r/5665 - Bug 1144432 - Part 2: Allow mid to change in renegotiation, check that mid doesn't change in answer, and disable some checking when the m-section was previously disabled.
/r/5667 - Bug 1144432 - Part 3: Disable unused m-sections in answer, and fix a null ptr bug.
/r/5669 - Bug 1144432 - Part 4: Unbust signaling_unittests

Pull down these commits:

hg pull review -r 2bd4b1a8d2f643375e82e0e11720c7043db8a49d

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8579040 [details]
MozReview Request: bz://1144432/bwc

Enough interdiff here to warrant a second pass

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

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8579040 [details]
MozReview Request: bz://1144432/bwc

https://reviewboard.mozilla.org/r/5571/#review4633

Ship It!

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
(Diff revision 2)
> +#ifdef MOZILLA_INTERNAL_API
> +  nsCOMPtr<nsIDocument> principal = mParent->GetWindow()->GetExtantDoc();
> +#else
> +  // For unit-tests
> +  nsCOMPtr<nsIScriptSecurityManager> secMan(
> +      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv));
> +  if (NS_FAILED(rv)) {
> +    CSFLogError(logTag, "%s: Failed to get IOService: %d",
> +        __FUNCTION__, (int)rv);
> +    CSFLogError(logTag, "%s: Failed to get securityManager: %d", __FUNCTION__, (int)rv);
> +    return;
> +  }
> +
> +  nsCOMPtr<nsIPrincipal> principal;
> +  rv = secMan->GetSystemPrincipal(getter_AddRefs(principal));
> +  if (NS_FAILED(rv)) {
> +    CSFLogError(logTag, "%s: Failed to get systemPrincipal: %d", __FUNCTION__, (int)rv);
> +    return;
> +  }
> +#endif
> +

I'm going to suggest that you:
a) factor this proxy setup out
b) move the invocation to ::Init
c) ignore the return status, because that only disables proxy use

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8579040 [details]
MozReview Request: bz://1144432/bwc

/r/5573 - Bug 1144432 - Part 1: Test-case
/r/5665 - Bug 1144432 - Part 2: Allow mid to change in renegotiation, check that mid doesn't change in answer, and disable some checking when the m-section was previously disabled.
/r/5667 - Bug 1144432 - Part 3: Disable unused m-sections in answer, and fix a null ptr bug.
/r/5669 - Bug 1144432 - Part 4: Unbust signaling_unittests

Pull down these commits:

hg pull review -r b2553541228b1b9589817c2056ae52b38b0754e6

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8579040 [details]
MozReview Request: bz://1144432/bwc

Incorporate feedback, carry forward r=mt

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

Comment 13

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9745d51c2f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/d70bf74a36d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/9efa1d056f16
https://hg.mozilla.org/integration/mozilla-inbound/rev/47772f47152b

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c9745d51c2f6
https://hg.mozilla.org/mozilla-central/rev/d70bf74a36d0
https://hg.mozilla.org/mozilla-central/rev/9efa1d056f16
https://hg.mozilla.org/mozilla-central/rev/47772f47152b

Comment 15

[Byron Campen [:bwc]]

Did we want to try to uplift this?

Comment 16

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #15)
> Did we want to try to uplift this?

I think this would be an important fix for our future ESR release, as it prevents services providers from simply looping through adding and removing the same stream over and over again.

Comment 17

[Byron Campen [:bwc]]

Is it easier to wait until 38 is ESR to uplift than it would be to uplift to Aurora (or Beta) once bug 1147400 is resolved?

Comment 18

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #17)
> Is it easier to wait until 38 is ESR to uplift than it would be to uplift to
> Aurora (or Beta) once bug 1147400 is resolved?

What I meant to say is: we should get this into 38 ASAP :-) (I think it is gets harder and harder to justify and uplift the longer we wait).

Comment 19

[Maire Reavy [:mreavy]]

Byron/Nils -- Can one of you write the uplift request for this to Fx38? I'm not aware of any unresolved regressions, and I'd like this to be in the first Beta of Fx38.  Thanks.

Comment 20

[Byron Campen [:bwc]]

Comment on attachment 8579040 [details]
MozReview Request: bz://1144432/bwc

I will need to create a backport for this, since only a small part of part 4 applies here. Also note that bug 1147400 will need to be uplifted with this, which is also a low-risk change.

Approval Request Comment
[Feature/regressing bug #]:

   Renegotiation (bug 1017888)

[User impact if declined]:

   Webrtc calls that remove a stream will be unable to add a stream of that type later.

[Describe test coverage new/current, TreeHerder]:

   A new test-case in jsep_session_unittest (in CI)

[Risks and why]:

   Risk is fairly low.

[String/UUID change made/needed]:

   None.

Comment 21

[Maire Reavy [:mreavy]]

Thanks, Byron, for writing the uplift request on this.  I just needinfo'd you on Bug 1147400 because it needs its own aurora-approval flag set.

Comment 22

[Byron Campen [:bwc]]

Hmm, we're going to need to pull in bug 1133866 in order to uplift so the patches apply.

Comment 23

[Byron Campen [:bwc]]

Try run based on aurora with these patches:

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

Comment 24

[Byron Campen [:bwc]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/21ee0f42cf6e
https://hg.mozilla.org/releases/mozilla-aurora/rev/c8adf5fc5564
https://hg.mozilla.org/releases/mozilla-aurora/rev/c8c05d6d9723
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bff0c3c1c6e

Comment 26

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1144432 - Part 2: Allow mid to change in renegotiation, check that mid doesn't change in answer, and disable some checking when the m-section was previously disabled.

Comment 27

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1144432 - Part 3: Disable unused m-sections in answer, and fix a null ptr bug.

Comment 28

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1144432 - Part 4: Unbust signaling_unittests

Comment 29

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1144432 - Part 1: Test-case