[webrtc-pc] JSEP reads & modifies transceivers in parallel, causing racy behavior. (#2476)

jan-ivar has just created a new issue for https://github.com/w3c/webrtc-pc:

== JSEP reads & modifies transceivers in parallel, causing racy behavior. ==
[Webrtc-pc](https://w3c.github.io/webrtc-pc/#set-description) runs JSEP in the background (*"In parallel, start the process to apply description as described in [JSEP]"* and *"If ... applied successfully"* will *"queue a task"* to main thread with results).

But [section 5.9](https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-25#section-5.9) ("Apply a Local Description") reads the transceiver list and read/writes the `mid` property ("[associated](https://w3c.github.io/webrtc-pc/#dfn-associated)"). Those are main-thread properties read/written from the wrong thread:
```
      *  If there is no RtpTransceiver associated with this m= section,
         find one and associate it with this m= section according to the
         following steps.  Note that this situation will only occur when
         applying an offer.

         +  Find the RtpTransceiver that corresponds to this m= section,
            using the mapping between transceivers and m= section
            indices established when creating the offer.

         +  Set the value of this RtpTransceiver's mid property to the
            MID of the m= section.
```
The read may be fine, since the only havoc JS can wreak here is stop a transceiver, which JSEP doesn't seem to care about. But it writes the `mid` which may race with JS-reads &  [ONN](https://w3c.github.io/webrtc-pc/#dfn-check-if-negotiation-is-needed). E.g.:
```js
const [transceiver] = pc.getTransceivers();

pc.setLocalDescription(offer); // note: no await
console.log(transceiver.mid); // null?
transceiver.sender.setStreams(streams); // will negotiationneeded fire?
```

[Section 5.10](https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-25#section-5.10) ("Apply a Remote Description") also reads the transceiver list & read/writes the `mid`:

      *  If the m= section is not associated with any RtpTransceiver
         (possibly because it was dissociated in the previous step),
         either find an RtpTransceiver or create one according to the
         following steps:

         +  If the m= section is sendrecv or recvonly, and there are
            RtpTransceivers of the same type that were added to the
            PeerConnection by addTrack and are not associated with any
            m= section and are not stopped, find the first (according to
            the canonical order described in Section 5.2.1) such
            RtpTransceiver.

         +  If no RtpTransceiver was found in the previous step, create
            one with a recvonly direction.

         +  Associate the found or created RtpTransceiver with the m=
            section by setting the value of the RtpTransceiver's mid
            property to the MID of the m= section, and establish a
            mapping between the transceiver and the index of the m=
            section.  If the m= section does not include a MID (i.e.,
            the remote endpoint does not support the MID extension),
            generate a value for the RtpTransceiver mid property,
            following the guidance for "a=mid" mentioned in
            Section 5.2.1.

This suffers from the same `mid` write problem, but additionally, the transceiver list changing under JSEP may cause racy behavior as well. E.g.:
```js
pc.setRemoteDescription(offer); // note: no await
pc.addTrack(track, stream); // Supposed to always get associated
```
Now, there's also language in _addTrack_ to associate post-SRD _success_, but it looks at **[[CurrentDescription]]**, so there's potentially the tiniest window between the steps above the queued SRD success-task, where the transceiver would not get associated. I was unable to verify this (the 3rd test in [this fiddle](https://jsfiddle.net/jib1/q2rgco6t/) fails in Safari, but for slightly different timing reasons).

Ironically, fixing this to not read across threads likely widens that time window. cc @docfaraday 

Please view or discuss this issue at https://github.com/w3c/webrtc-pc/issues/2476 using your GitHub account

Received on Saturday, 15 February 2020 01:22:02 UTC