984580 - Fire ICE connection states "completed" when ICE checks have stopped, as well as "closed" when closed to match spec

Status: NEW

(Core :: WebRTC: Networking, defect, P2)

Description

[Byron Campen [:bwc]]

Right now all we support is the connected state, but there is an additional completed state we should be reporting.

Comment 1

[Byron Campen [:bwc]]

Attachment: Part 1: Reconcile the existing values in NrIceCtx::ConnectionState with the language used in WebRTC for the same states.

First, reconcile what we do have.

Comment 2

[Byron Campen [:bwc]]

So, after giving this a little thought, knowing that ICE is 'completed' is not really possible. At any point, we could receive a new check request, causing us to send a triggered check for a previously failed pair. We could also get a new (high priority) trickle candidate. The only useful thing we can say here (barring a timer we set that permanently disables any more checks from being sent) is whether we currently have any checks scheduled or not, and this can change at any time.

Internally, maybe we should decompose this into a couple of booleans; isChecking and isConnected. The four combinations of these get us the 'failed', 'checking', 'completed', and 'connected' states. Toss in gathering state, (gathering is true, checking and connected both false), and we get 'new' too. A transition from connected or completed back to checking is what disconnected was intended to indicate, I bet; it is not fundamentally different from an ICE perspective from the present 'checking' state. 'closed' looks completely redundant to me.

Comment 3

[Eric Rescorla (:ekr)]

(In reply to Byron Campen [:bwc] from comment #2)
> So, after giving this a little thought, knowing that ICE is 'completed' is
> not really possible. At any point, we could receive a new check request,
> causing us to send a triggered check for a previously failed pair. We could
> also get a new (high priority) trickle candidate.

There is a last trickle candidate in the protocol, though it isn't
currently supported by nICEr.


> The only useful thing we
> can say here (barring a timer we set that permanently disables any more
> checks from being sent) is whether we currently have any checks scheduled or
> not, and this can change at any time.
> 
> Internally, maybe we should decompose this into a couple of booleans;
> isChecking and isConnected. The four combinations of these get us the
> 'failed', 'checking', 'completed', and 'connected' states. Toss in gathering
> state, (gathering is true, checking and connected both false), and we get
> 'new' too. A transition from connected or completed back to checking is what
> disconnected was intended to indicate, I bet; it is not fundamentally
> different from an ICE perspective from the present 'checking' state.
> 'closed' looks completely redundant to me.

Comment 4

[Byron Campen [:bwc]]

Attachment: Part 2: Teach nICEr to fire a callback when all checks are completed.

Work in progress

Comment 5

[Maire Reavy [:mreavy]]

Still relevant?

Comment 6

[Byron Campen [:bwc]]

Yes.

Comment 7

[Nils Ohlmeier [:drno]]

FYI I noticed yesterday that spec now describes the connection state 'completed' as the combination of connection state reached 'connected' & gathering state reached 'completed'.

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I think we should up-prioritize this. We're also missing "closed" state it seems.

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Fippo brought up the good point that this now depends on bug 1318167.

Comment 10

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Bug 1318167 was fixed in 68, so we can reap the rewards by finishing this now.