Closed Bug 1144432 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: drno, Assigned: bwc)

References

Details

(Whiteboard: [renegotiation])

Attachments

(4 files, 1 obsolete file)

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.
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.
Seeing as this is a fairly major interop problem, we should consider an uplift here, particularly considering that 38 is going to ESR.
(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.
Rank: 10
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [multistream]
Assignee: nobody → docfaraday
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.
Attached file MozReview Request: bz://1144432/bwc (obsolete) —
/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 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.
Attachment #8579040 - Flags: review?(martin.thomson)
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
Attachment #8579040 - Flags: review?(martin.thomson)
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
Attachment #8579040 - Flags: review?(martin.thomson)
Attachment #8579040 - Flags: review?(martin.thomson) → review+
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 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
Attachment #8579040 - Flags: review+
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
Flags: needinfo?(docfaraday)
Attachment #8579040 - Flags: review+
Flags: needinfo?(docfaraday)
Whiteboard: [multistream] → [renegotiation]
Flags: needinfo?(docfaraday)
Did we want to try to uplift this?
Flags: needinfo?(rjesup)
Depends on: 1147400
(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.
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?
(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).
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.
Flags: needinfo?(rjesup)
Flags: needinfo?(drno)
Flags: needinfo?(docfaraday)
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.
Flags: needinfo?(docfaraday)
Attachment #8579040 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(drno)
Hmm, we're going to need to pull in bug 1133866 in order to uplift so the patches apply.
Try run based on aurora with these patches:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=895b21dce6f9
Attachment #8579040 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8579040 - Attachment is obsolete: true
Attachment #8619790 - Flags: review+
Attachment #8619791 - Flags: review+
Attachment #8619792 - Flags: review+
Attachment #8619793 - Flags: review+
You need to log in before you can comment on or make changes to this bug.