Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redundant queuing in gathering state computation risks eliding events #2892

Closed
jan-ivar opened this issue Aug 18, 2023 · 0 comments · Fixed by #2894
Closed

Redundant queuing in gathering state computation risks eliding events #2892

jan-ivar opened this issue Aug 18, 2023 · 0 comments · Fixed by #2894
Assignees

Comments

@jan-ivar
Copy link
Member

Similar to #2020 but for gathering states (we fixed this for connection states, but not gathering states).

Quoting the spec:

When the ICE Agent indicates that it began gathering a generation of candidates for an RTCIceTransport, the user agent MUST queue a task that runs the following steps: [...]

  1. Set transport.[[IceGathererState]] to gathering.

  2. Fire an event named gatheringstatechange at transport.

  3. Update the ICE gathering state of connection.

Since Update the ICE gathering state ALSO queues a task, we're queuing a task from inside another, to, among other things:

  1. Let newState be the value of deriving a new state value as described by the RTCIceGatheringState enum.

@docfaraday noticed this puts us at risk of eliding icegatheringstatechange events, since the state derived by the time the second queued task runs may be different.

The solution seems to be to fire both gatheringstatechange (on transport) and icegatheringstatechange (on connection) from the same task, like we did with the connection states in #2400 and #2444 to solve similar concerns.

Web compat

WPT coverage here is poor, but this fiddle shows only Safari is firing "gathering" on the ice transport at all (see sender.transport.iceTransport.ongatheringstatechange), so there should be time to align timing and ordering here with the rest of the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant