Inconsistencies and race conditions in updating negotiation-needed flag

[soareschen]

In some cases, the negotiationneeded event could be fired twice when there is only one needed.

The part that confuse me is in step 10 for setting session description:

• If connection's signaling state is now stable, update the negotiation-needed flag. If connection's [[ needNegotiation]] slot was true both before and after this update, queue a task that runs the following steps:
  2. If connection's [[needNegotiation]] slot is false, abort these steps.
  3. Fire a simple event named negotiationneeded at connection.

The steps are similar but not entirely the same as the firing event step in the steps to update the negotiation-needed flag. The rationale for this step seems to be to defer the negotiationneeded event to be fired only when the connection go back to stable state. But it doesn't make sense together with step 2 in updating the negotiation-needed flag:

2. If connection's signaling state is not "stable", abort these steps.

The step seems to suggest that for any SDP changes when the connection state is not stable, the [[needNegotiation]] internal slot is never updated and the negotiationneeded event is never fired.

So it is not clear when the condition applies for connection's [[needNegotiation]] slot was true both before and after the session description update. The only cases that I can think of is by creating race conditions, e.g.:

• Generate offer with createOffer().
• Call a method that can synchronously trigger updating negotiation, i.e. createDataChannel() or addTransceiver(). This will manage to get past "If connection's signaling state is not "stable", abort these steps.", because the connection state is still stable, and [[needNegotiation]] is then set to true.
• In the next synchronous step, or in some async race condition cases, call setLocalDescription() in parallel. This sets the description without the newly created m= section.
• The event firing in update the negotiation-needed flag is done asynchronously in a task. So it can be fired after the connection state change to non-stable.
• Now call setRemoteDescription() with answer to change the connection back to stable state. The condition for last step of setting session description applies, and a second negotiationneeded event is fired.

I am not sure if the scenario happens by design or by accident. Would be better if there are additional non-normative section to describe how the negotiationneeded event should be handled, and describe the event behavior in a human-friendly way.

[taylor-b]

So it is not clear when the condition applies for connection's [[needNegotiation]] slot was true both before and after the session description update.

It's intended more to handle cases like this:

1. Data channel or track added. "negotiation-needed" flag set.
2. Offer created/sent in response to negotiationneeded event.
3. Something happens that requires renegotiation before the answer is received (like adding a track).
4. Answer received. Still needs more negotiation, so negotiationneeded fires again.
5. New offer created in response to second negotiationneeded. Assuming no more tracks are added, this offer/answer exchange finishes without requiring more negotiation.

I agree that there should be an explanation for how negotiationneeded is intended to be used (as shown in the examples below). But, if used as intended, I don't see how there could be inconsistencies/race conditions.

[soareschen]

Got it. I think I misread step 6.2 as "set [[needNegotiation]] slot to false", when it actually is "If connection's [[needNegotiation]] slot is false, abort these steps.". That's why there were confusion.

Closing this issue.

[taylor-b]

Actually, I realized a potential race condition when looking at the example code. Suppose getUserMedia leads to addTrack, which leads to negotiationneeded if the signaling state is stable, which leads to createOffer, etc. And also, the application is prepared to receive a remote offer, triggering setRemoteDescription, createAnswer, etc.

Now, suppose the remote offer arrives first, and getUserMedia finishes while the setRemoteDescription promise is still unresolved. Then negotiationneeded will fire, and the createOffer operation will be added to the operations queue after the setRemoteDescription operation. Which would lead to an "invalid signaling state" error when setLocalDescription(offer) is called later. I think. @jan-ivar, can you confirm?

In short: It's not quite enough to say "abort these steps if the signaling state isn't stable". We really would need to do something like "abort these steps if the signaling state isn't stable or the operations queue isn't empty". Then, we'd also need to move the delayed "negotiation needed?" check from "when applying a description transitions the signaling state to stable" to "when the last operation is removed from the operations queue and the signaling state is stable".

[jan-ivar]

@taylor-b Naively it seems to be in the app domain to avoid the inherent glare between "offer now" and an incoming offer, so InvalidStateError seems right to me. I'm loathe to add dependencies to the internal queue, since I consider it mostly a legacy thing. Unless I'm missing some ill behavior on negotiationneeded's part here?

[taylor-b]

@jan-ivar I guess you're right. Even if we address the scenario I described by pushing negotiationneeded further out, the inverse problem could happen, where setRemoteDescription is called while the createOffer promise triggered by negotiationneeded is still pending.

So I'll close this again for the time being.