1425956 - Removing a track and later re-adding it to a peer connection causes InvalidSessionDescriptionError

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement, P2)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

STRs:
 1. Open https://jsfiddle.net/jib1/an44uhtd/ and share cam+mic
 2. check ☑ hold
 3. uncheck ☐ hold

Expected result:
  video appears, then freezes (separate bug)

Actual result:
  InvalidSessionDescriptionError: track id:{133942a0-a162-5c4b-b57f-6b35d4218a99} appears in more than one m-section at level 2

The first fiddle in the "Evolution of WebRTC" blog [2] is broken the same way.

Basically, we believe this is caused by a broken JSEP spec. [1]

[1] https://github.com/rtcweb-wg/jsep/issues/842
[2] https://blog.mozilla.org/webrtc/the-evolution-of-webrtc/

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Also, InvalidSessionDescriptionError isn't in the spec. See [1].

[1] https://github.com/w3c/webrtc-pc/issues/1702

Comment 2

[Byron Campen [:bwc]]

Attachment: Bug 1425956 - Part 1: Test-case that removes and re-adds a track that was negotiated previously.

Review commit: https://reviewboard.mozilla.org/r/208258/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208258/

Comment 3

[Byron Campen [:bwc]]

Attachment: Bug 1425956 - Part 2: Don't emit duplicate track ids in SDP.

Review commit: https://reviewboard.mozilla.org/r/208260/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208260/

Comment 4

[Byron Campen [:bwc]]

Attachment: Bug 1425956 - Part 3: Remove duplicate track ids on incoming SDP.

Review commit: https://reviewboard.mozilla.org/r/208262/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208262/

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8937584 [details]
Bug 1425956 - Part 1: Test-case that removes and re-adds a track that was negotiated previously.

https://reviewboard.mozilla.org/r/208258/#review214046

Lgtm modulo nits.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:662
(Diff revision 1)
> +    // pc1 is offerer
> +    offer = await pc1.createOffer();
> +    await pc2.setRemoteDescription(offer);
> +    await pc1.setLocalDescription(offer);
> +    answer = await pc2.createAnswer();
> +    await pc1.setRemoteDescription(answer);
> +    await pc2.setLocalDescription(answer);

Have you considered a reusable sub-function?

    await negotiate(pc1, pc2);

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:703
(Diff revision 1)
> +        }
> +      ]);
> +
> +    // pc1 is answerer. We need to create a new transceiver so pc1 will have
> +    // something to attach the re-added track to
> +    pc2.addTransceiver("audio");

This will fire a track event on pc1 after negotiation. Do we test that someplace else? If not, should we test it here?

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:723
(Diff revision 1)
> +        {
> +          receiver: {track: {}},
> +          currentDirection: "inactive"
> +        },
> +        {
> +          sender: {track: {}},

Isn't sender.track == null here?

Shouldn't we test receiver.track as well?

Comment 6

[Nils Ohlmeier [:drno]]

Comment on attachment 8937585 [details]
Bug 1425956 - Part 2: Don't emit duplicate track ids in SDP.

https://reviewboard.mozilla.org/r/208260/#review214062

This looks good for one round of offer-answer. But I'm not convinced yet that it won't cause other interop problems.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:390
(Diff revision 1)
> +            new SdpMsidAttributeList(mediaAttrs.GetMsid()));
> +        for (auto& msid : newMsids->mMsids) {
> +          msid.appdata = trackId;
> +        }
> +
> +        mediaAttrs.SetAttribute(newMsids.release());

Don't you need to this new IDs in a permanent lookup table to enable re-writing to the same fake track ID for every following renegotiation?

Or in other words: isn't this going to create new track IDs in new offer-answer round, which then will/might confuse a track based implementation?

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8937586 [details]
Bug 1425956 - Part 3: Remove duplicate track ids on incoming SDP.

https://reviewboard.mozilla.org/r/208262/#review214066

LGTM
Do we want additional JSEP unit tests for this to "describe" the expected behavior (given that this is not described in any spec as of right now)?

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8937584 [details]
Bug 1425956 - Part 1: Test-case that removes and re-adds a track that was negotiated previously.

https://reviewboard.mozilla.org/r/208258/#review214046

> This will fire a track event on pc1 after negotiation. Do we test that someplace else? If not, should we test it here?

We have tested this in other places plenty.

> Isn't sender.track == null here?
> 
> Shouldn't we test receiver.track as well?

What about receiver.track would we test? I suppose we could check the kind, but we already do that testing elsewhere.

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8937586 [details]
Bug 1425956 - Part 3: Remove duplicate track ids on incoming SDP.

https://reviewboard.mozilla.org/r/208262/#review214066

I don't know, maybe a test that inserts a dupe and just verifies that we don't barf?

Comment 10

[Byron Campen [:bwc]]

Attachment: Bug 1425956 - Part 1.1: Fix flaw in hasProps noticed during review where {} would match null.

Review commit: https://reviewboard.mozilla.org/r/208456/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208456/

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8937584 [details]
Bug 1425956 - Part 1: Test-case that removes and re-adds a track that was negotiated previously.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208258/diff/1-2/

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8937585 [details]
Bug 1425956 - Part 2: Don't emit duplicate track ids in SDP.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208260/diff/1-2/

Comment 13

[Byron Campen [:bwc]]

Comment on attachment 8937586 [details]
Bug 1425956 - Part 3: Remove duplicate track ids on incoming SDP.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208262/diff/1-2/

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8937749 [details]
Bug 1425956 - Part 1.1: Fix flaw in hasProps noticed during review where {} would match null.

https://reviewboard.mozilla.org/r/208456/#review214250

Comment 15

[Byron Campen [:bwc]]

Check on try, looks good so far.

Comment 16

[Nils Ohlmeier [:drno]]

Comment on attachment 8937585 [details]
Bug 1425956 - Part 2: Don't emit duplicate track ids in SDP.

https://reviewboard.mozilla.org/r/208260/#review214340

Copying over the MSID from the last stable SDP hopefully solves my concern about renegotiation.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76f26552f433
Part 1: Test-case that removes and re-adds a track that was negotiated previously. r=jib
https://hg.mozilla.org/integration/autoland/rev/ae1d83fc9eab
Part 1.1: Fix flaw in hasProps noticed during review where {} would match null. r=jib
https://hg.mozilla.org/integration/autoland/rev/a51a639d16c0
Part 2: Don't emit duplicate track ids in SDP. r=drno
https://hg.mozilla.org/integration/autoland/rev/8e682e487ef1
Part 3: Remove duplicate track ids on incoming SDP. r=drno

Comment 18

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/76f26552f433
https://hg.mozilla.org/mozilla-central/rev/ae1d83fc9eab
https://hg.mozilla.org/mozilla-central/rev/a51a639d16c0
https://hg.mozilla.org/mozilla-central/rev/8e682e487ef1