1769802 - setRemoteDescription and setLocalDescription identity validation failures leave JSEP in new state

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

If identity validation fails, we should not be leaving JSEP in a state where the description was successfully set.

We cannot really rely on the JSEP engine's rollback here, since that only works for offers. We need a more aggressive rollback here. This is at least partially related to bug 1423365, which we may want to revive.

Ultimately, we may need to allow JsepSessionImpl to be deep-copied, so PeerConnectionImpl can copy it, run the operation on the copy, and if that operation (and all other related operations, which may be in PeerConnection.jsm) succeeds, overwrite mJsepSession with the new copy, and update pointers to JsepTransceivers everywhere it makes sense. Otherwise, we discard the new JSEP session, leaving the original totally unchanged.

Comment 1

[Byron Campen [:bwc]]

Attachment: Bug 1769802: Test-cases for bug. r?jib

Comment 2

[Byron Campen [:bwc]]

Attachment: Bug 1769802: Deep clone stuff for SDP. r?ng

If we're going to clone the JSEP engine, first we need to be able to clone SDP

Depends on D150167

Comment 3

[Byron Campen [:bwc]]

Attachment: Bug 1769802: Key JsepTransceiver by UUID instead of a simple integer index. r?mjf

The integer index we're replacing here is based on the order in which
transceivers were added. If we clone the JSEP engine for an sRD that happens to
result in the creation of a transceiver, and at the same time JS calls
addTransceiver, we have a situation where the sRD transceiver is added first
to the cloned JSEP engine, but the addTransceiver transceiver is added first
to the old JSEP engine, resulting in them having the same index. So, let's just
use a proper key for this stuff.

Depends on D150168

Comment 4

[Byron Campen [:bwc]]

Attachment: Bug 1769802: Make the JSEP engine cloneable. r?mjf

We could go to the trouble of teaching the JSEP engine to succeed/fail
atomically, but even if we did, we still need to be able to undo a successfully
executed sRD or sLD when identity-related stuff (in JS) fails. The simplest way
to get undo functionality is to clone the JSEP engine, and perform the
operation on one of the copies.

Depends on D150169

Comment 5

[Byron Campen [:bwc]]

Attachment: Bug 1769802: Stop holding a reference to the JsepTransceiver in RTCRtpSender/Receiver. r?mjf

This will make it easier to replace the JsepTransceiver a given
RTCRtpTransceiver is using, which is necessary if we're going to be
deep-copying and replacing the JSEP engine.

Depends on D150170

Comment 6

[Byron Campen [:bwc]]

Attachment: Bug 1769802: Ensure that PeerConnectionImpl is in charge of unlinking (except when it shouldn't be, ugh). r?mjf

The previous changeset was causing nullptr crashes because RTCRtpTransceiver
was performing an unlink before PeerConnectionImpl got a chance to close. It is
likely that there were already ways to trigger this bug.

Depends on D150171

Comment 7

[Byron Campen [:bwc]]

Attachment: Bug 1769802: Call the success callback for sRD/sLD after the JS work is done. r?jib

This fixes a class of bugs where modifications to the RTCRtpTransceivers were
applied before sRD/sLD were completely done (while all of the JSEP and transport
stuff was finished, JS could still be working on identity-related stuff).

Depends on D150172

Comment 8

[Byron Campen [:bwc]]

Attachment: Bug 1769802: Ensure that sRD/sLD fail atomically. r?jib

This fixes the class of bugs where identity-related failures could leave the
JSEP engine in a different state than the JS-observable state, and also the
class of bugs where a failure in the JSEP engine could leave the JSEP engine in
a weird halfway state.

Depends on D150173

Comment 9

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/jobs?repo=try&revision=8c46ecbaeda5495e3e19e6406ff798455cc0a1a4
https://treeherder.mozilla.org/jobs?repo=try&revision=5268df1ea7500f4018ec199854e4d2a0629d8f2b
https://treeherder.mozilla.org/jobs?repo=try&revision=419168634151d2ef053e21c76e50b3ac6c1a2e83

Comment 10

[Byron Campen [:bwc]]

Attachment: Bug 1769802: Make sure we apply addTrack magic when redoing sRD. r?jib

Depends on D150174

Comment 11

[Byron Campen [:bwc]]

Try looks good so far:

https://treeherder.mozilla.org/jobs?repo=try&revision=0c8408990985d91831677b4ea0891142c1d48a99
https://treeherder.mozilla.org/jobs?repo=try&revision=eded99b3f2f0be56f8d1cb9ec9ff77e8f90399eb

I think I'll re-run the gtest suite, just in case.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7db66c7d7f46
Test-cases for bug. r=jib
https://hg.mozilla.org/integration/autoland/rev/58439a6601ff
Deep clone stuff for SDP. r=ng
https://hg.mozilla.org/integration/autoland/rev/87692ef0b35d
Key JsepTransceiver by UUID instead of a simple integer index. r=mjf
https://hg.mozilla.org/integration/autoland/rev/732e02234941
Make the JSEP engine cloneable. r=mjf
https://hg.mozilla.org/integration/autoland/rev/fb9d2e7e5fae
Stop holding a reference to the JsepTransceiver in RTCRtpSender/Receiver. r=mjf
https://hg.mozilla.org/integration/autoland/rev/b046b56d111f
Ensure that PeerConnectionImpl is in charge of unlinking (except when it shouldn't be, ugh). r=mjf
https://hg.mozilla.org/integration/autoland/rev/9c544f877fa4
Call the success callback for sRD/sLD after the JS work is done. r=jib,webidl,smaug
https://hg.mozilla.org/integration/autoland/rev/d25d7e866391
Ensure that sRD/sLD fail atomically. r=jib
https://hg.mozilla.org/integration/autoland/rev/52f3bf105790
Make sure we apply addTrack magic when redoing sRD. r=jib

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/35355 for changes under testing/web-platform/tests

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/7db66c7d7f46
https://hg.mozilla.org/mozilla-central/rev/58439a6601ff
https://hg.mozilla.org/mozilla-central/rev/87692ef0b35d
https://hg.mozilla.org/mozilla-central/rev/732e02234941
https://hg.mozilla.org/mozilla-central/rev/fb9d2e7e5fae
https://hg.mozilla.org/mozilla-central/rev/b046b56d111f
https://hg.mozilla.org/mozilla-central/rev/9c544f877fa4
https://hg.mozilla.org/mozilla-central/rev/d25d7e866391
https://hg.mozilla.org/mozilla-central/rev/52f3bf105790

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Copying crash signatures from duplicate bugs.