[webrtc-pc] Inconsistencies and race conditions in updating negotiation-needed flag

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

== Inconsistencies and race conditions in updating negotiation-needed flag ==
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.

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

Received on Monday, 5 June 2017 04:35:18 UTC