Possibly racy replaceTrack()

[henbos]

Roughly speaking, replaceTrack() does this:

In parallel:
  Determine if negotiation is needed, if it is, reject promise.
  Otherwise queue a task that runs the following steps:
    Set track and resolve promise (unless pc is closed).

I think there's two issues with determining if negotiation is needed in parallel and then queuing a task to complete the operation:

1. It's possible that in-between checking that negotiation was not needed and running the task, the sender is modified such that negotiation would indeed be needed. The track is replaced even though it can't be sent seamlessly. Assuming we don't get encoding errors, how will the other endpoint react?
2. Performing the "is negotiation needed" check in parallel means it is racy, we don't know if it will occur before or after removeTrack(), for instance:

let promise = sender.replaceTrack(track);
// Will "is negotiation needed" happen before or after removeTrack()?
pc.removeTrack(sender);

I can see this code snippet resolving or rejecting the promise depending on a race. Do we care?

[henbos]

Note: I'm assuming that removeTrack() which changes direction would cause the "check if negotiation is needed" to reject the promise. Question: Do we need to add a clause for (direction != currentDirection) or say that "if direction is recvonly or inactive, reject the promise"?

[taylor-b]

It's possible that in-between checking that negotiation was not needed and running the task, the sender is modified such that negotiation would indeed be needed.

Are you imagining replaceTrack racing with setRemoteDescription (adding more restrictions to what can be sent) or setParameters (setting a specific codec)? That seems valid, since the ordering of those promises resolving isn't guaranteed, as far as I can tell.

Note: I'm assuming that removeTrack() which changes direction would cause the "check if negotiation is needed" to reject the promise.

I don't think the "determine if negotiation is needed" here is intended to take direction into account. It should be legal to call replaceTrack while the call is put "on hold," so that the correct track is sent as soon as the call is resumed. Otherwise the application would have to wait until it's taken off hold to call replaceTrack, and a frame from the old track could leak out, which isn't ideal.

But that's rather confusing, since section 4.7 defines what "negotiation needed" means, which includes direction. Given all the recent discussion, I think this step needs to be worded differently/expanded upon.

[henbos]

I don't think the "determine if negotiation is needed" here is intended to take direction into account.

Does that mean it should be valid to replaceTrack() after removeTrack(sender)? I suppose removeTrack() is just a sync way of doing replaceTrack(null); direction = null, nothing irreversible.

If so these tests are wrong to assert that the promise is rejected?
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setRemoteDescription-replaceTrack.https.html?l=106

[taylor-b]

If so these tests are wrong to assert that the promise is rejected?

Possibly, depending on the result of this discussion. Sorry for not thinking about this enough when reviewing the test...

[stefhak]

I can agree to that having both sync (add/removeTrack) and async methods (replaceTrack) operating on the same thing is not very clean. However, perhaps we can live with it?
Doing

let promise = sender.replaceTrack(track);
// Will "is negotiation needed" happen before or after removeTrack()?
pc.removeTrack(sender);

is not what you should do (adding await to the first operation would solve it I guess).

[taylor-b]

Given that replaceTrack no longer takes direction into account, and uses the operations queue, I think we're safe from any race conditions now. Closing this issue.