852665 - Report WebRTC transport termination (e.g. iceConnectionState=disconnected)

Status: RESOLVED FIXED

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

Description

[Eric Rescorla (:ekr)]

We need some way to report that transport has failed.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Not sure without more info if this is blocking, but I suspect it isn't

Comment 2

[Adam Roach [:abr]]

Justin's opinion appears to be that this should be communicated with an iceConnectionState change to "failed"; see http://lists.w3.org/Archives/Public/public-webrtc/2013Apr/0059.html

It is highly likely that Chrome will implement this behavior. Unless we have a reason to push for some other mechanism, I would propose that we do the same.

Comment 3

[Suhas]

I have been following webrtc discussion group on this particular issue for sometime. iceconnectionstate report seems to be reasonable but it is not complete if the reason for failure is reported with granularity and unambigously. I am not sure what is the spec's plan on this though ...

Comment 4

[Suhas]

i meant, if the failure "isn't" reported :)

Comment 8

[Adam Ullman]

Is there any recommended way now to detect whether a PeerConnection has failed? As far as I can tell the PeerConnection state still never changes to "failed". Is there some other workaround? Right now in Chrome we wait for the "loadedmetadata" event on the Video Element and timeout. On Firefox this Event fires too soon and so we have no way of detecting when a PeerConnection has failed.

Comment 9

[Eric Rescorla (:ekr)]

Are you looking for negotiation failure or ICE failure?

Comment 10

[Adam Ullman]

I think ICE failure but I'm not clear on the difference, it's ICE negotiation right? I basically just want to be able to detect any time a connection between peers has failed and be able to provide a reasonable error message to the user.

Comment 11

[Eric Rescorla (:ekr)]

Well, so there are cases where we can't negotiate media, e.g., where the answer
contains more m-lines than the offer and other cases where the signaling is fine
but it turns out that we can't establish an ICE connection. The former isn't
really signaled by the API but if it were it would be by the PeerConnection state.
Errors in the SDP just cause error callbacks generally.

http://dev.w3.org/2011/webrtc/editor/webrtc.html#rtcpeerstate-enum


The latter is indicated by the ICE state:
http://dev.w3.org/2011/webrtc/editor/webrtc.html#rtciceconnectionstate-enum

Comment 12

[Adam Ullman]

Ah, I wasn't checking oniceconnectionstatechange, just onsignalingstatechange, thanks. I see that the iceConnectionState is changing to 'failed' when the PeerConnection fails. Does this mean that this bug should be resolved?

Comment 13

[Nils Ohlmeier [:drno]]

(In reply to Adam Ullman from comment #12)
> Ah, I wasn't checking oniceconnectionstatechange, just
> onsignalingstatechange, thanks. I see that the iceConnectionState is
> changing to 'failed' when the PeerConnection fails. Does this mean that this
> bug should be resolved?

My understanding is that you wanted to know when an initial ICE connection never got established. This works by monitoring the iceConnectionState.
The second part, which I believe this ticket is about, is to get notified that an existing connection broke while you are using it.

Comment 14

[damon.oehlman]

It would be great to see this get resolved in a manner consistent with the chrome implementation.  At present when working with firefox we have no reliable way of determining when the remote party has disconnected using the RTCPeerConnection object alone (i.e. you can use your signalling server to provide information regarding peers leaving, but having to rely on that mechanism in a P2P networking situation is pretty poor).

Comment 15

[Tanase Butcaru]

I can confirm that oniceconnectionstatechange is not fired if iceConnectionState is disconnected. In Chrome browser works without problems (there's a delay of ~5 seconds, but it's ok).
Does somebody have a workaround for the moment until this gets solved?

Comment 16

[Silvia Pfeiffer]

Can the priority on this be increased somehow? We'd really appreciate correct iceConnectionState change events for disconnected and closed peer connections: http://w3c.github.io/webrtc-pc/#rtciceconnectionstate-enum

Comment 17

[wez.yee]

BUMP

Comment 18

[Nils Ohlmeier [:drno]]

I think implementing ICE consent freshness (bug 929977) is the only reasonable approach to detect an ICE connection failure after a connection initially got established.

Comment 19

[Nils Ohlmeier [:drno]]

Important point in the discussion here: I assume that most comments here are actually interested in the use case where the far end of an established PeerConnection suddenly dis-appeared, e.g. laptop closed, device out of battery, device lost IP access, but locally everything is still working fine.

Comment 20

[Nils Ohlmeier [:drno]]

As failed and closed are actually states which are reached I think we are talking here "only" about the disconnected state: http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js?mark=1146-1149#1119

Comment 21

[aleth [:aleth]]

(In reply to Nils Ohlmeier [:drno] from comment #19)
> Important point in the discussion here: I assume that most comments here are
> actually interested in the use case where the far end of an established
> PeerConnection suddenly dis-appeared, e.g. laptop closed, device out of
> battery, device lost IP access, but locally everything is still working fine.

I don't think a local connection loss (e.g. wifi goes down) is currently reported either? API consumers would want to know about those too. Though of course they might be two separate cases to handle...

Comment 22

[Igor]

Any updates so far? when this will be fixed?

Comment 23

[Byron Campen [:bwc]]

It is hard to say. We have a lot on our plate this year.

Comment 24

[mishramukul194]

Hi Guies and Babes.

I am getting the following error in twilio integration instead i am having Live credential with that Twilio ****.

the error is : 
ICE negotiation with Twilio failed. Call will terminate.
could you please suggest me any solution for that coz i  think its webrtc problem on browser based.
i have tested on every browser though.

please help

Comment 25

[Adam Roach [:abr]]

It seems very unlikely that I'll have an opportunity to work on WebRTC platform development for the foreseeable future. I'm unassigning this bug so that someone else might pick it up.

Comment 26

[Igor]

In a case that nobody will work on this issue, Do you have any recommendations how to handle peer disconnected state in Firefox?

Comment 27

[Byron Campen [:bwc]]

Once consent freshness (bug 929977, which drno is working on) is done, it should be fairly easy to implement this.

Comment 28

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

(In reply to Igor from comment #26)
> In a case that nobody will work on this issue, Do you have any
> recommendations how to handle peer disconnected state in Firefox?

One workaround is http://stackoverflow.com/questions/35577657/detect-offline-peer-in-webrtc-connection/35591205#35591205

Comment 29

[Tom Brückner]

This is really annoying, since it requires heavy workarounds. I was tempted to use a keep-alive ping until I gladly discovered Jan-Ivar's solution. Hope the bug will be fixed soon...

Comment 30

[Maire Reavy [:mreavy]]

Consent freshness (bug 929977) will land early in Fx49 Nightly (in about 2 weeks).  As Byron says in Comment 27, once that lands, this is fairly easy to implement. So I'm bumping this to a P1.  I'd like to target landing this in Fx49 (before Fx49 uplifts to Dev Edition).

Nils -- Do you want to take this as a follow on to bug 929977?

Comment 31

[Tom Brückner]

Thanks for the quick reaction, highly appreciated! :-)

Comment 32

[Igor]

Great to see the progress here

I can confirm that Jan-Ivar's solution works and we already implemented it
But hope to see a true fix here soon

Comment 33

[Prerak Jain]

When can we expect this issue to be resolved.

Comment 34

[Nils Ohlmeier [:drno]]

Since bug 929977 has made it's way into Firefox 49 which has become the official Firefox release recently Firefox will switch the ICE connection state to 'failed' if ICE consent freshness fails. Note that per ICE consent refresh RFC the timeout is fairly high at 30 seconds.

As there is still work going on on the ICE state transition flows I don't think it makes sense to consider switching to other states right now.

I'm inclined to actually close this bug as Firefox now reports if a transport has failed.

Comment 35

[Prerak Jain]

It takes too long to get the "failed" state ~30 secs before that client has no way to know about the network issues, "disconnected" state is helpful to know the momentary network glitches (chrome usually takes 5 sec to detect and trigger disconnected state change) and this can is used to let the user know about brief network issues and can help in improving the user experience.
 
When you say you are going to close, are you not going to implement all the ice states mentioned in http://w3c.github.io/webrtc-pc/#rtciceconnectionstate-enum or deferring it for now ?

Comment 36

[Nils Ohlmeier [:drno]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=226681c0ab37

Comment 37

[Nils Ohlmeier [:drno]]

Attachment: Bug 852665: add support for ICE disconnected state.

Review commit: https://reviewboard.mozilla.org/r/89672/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89672/

Comment 38

[Byron Campen [:bwc]]

Comment on attachment 8806172 [details]
Bug 852665: add support for ICE disconnected state.

https://reviewboard.mozilla.org/r/89672/#review89282

::: media/mtransport/nricectx.cpp:627
(Diff revision 1)
>    // Create the handler objects
>    ice_handler_vtbl_ = new nr_ice_handler_vtbl();
>    ice_handler_vtbl_->select_pair = &NrIceCtx::select_pair;
>    ice_handler_vtbl_->stream_ready = &NrIceCtx::stream_ready;
>    ice_handler_vtbl_->stream_failed = &NrIceCtx::stream_failed;
> -  ice_handler_vtbl_->ice_completed = &NrIceCtx::ice_completed;
> +  ice_handler_vtbl_->ice_completed = &NrIceCtx::ice_connected;

Should we rename this in the vtbl too?

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1289
(Diff revision 1)
> +void nr_ice_component_consent_calc_consent_timer(nr_ice_component *comp)
> +  {
> +    uint16_t trange, trand, tval;
> +    void *buf = &trand;
> +
> +    trange = NR_ICE_CONSENT_TIMER_DEFAULT / 100 * 20;

I guess rounding error will be smaller if you multiply by 20 first.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1291
(Diff revision 1)
> +    uint16_t trange, trand, tval;
> +    void *buf = &trand;
> +
> +    trange = NR_ICE_CONSENT_TIMER_DEFAULT / 100 * 20;
> +    tval = NR_ICE_CONSENT_TIMER_DEFAULT - trange;
> +    if (!nr_crypto_random_bytes(buf, sizeof(trand)))

I feel like ((UCHAR*)&trand, sizeof(trand)) would be easier to read.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1346
(Diff revision 1)
> -      tval += (trand % (trange * 2));
> -
> -    if (comp->ctx->test_timer_divider)
> -      tval = tval / comp->ctx->test_timer_divider;
>  
> -    NR_ASYNC_TIMER_SET(tval, nr_ice_component_consent_timer_cb, comp,
> +    NR_ASYNC_TIMER_SET(comp->consent_ctx->maximum_transmits_timeout_ms + 100,

What's the + 100 for?

Comment 39

[Nils Ohlmeier [:drno]]

Comment on attachment 8806172 [details]
Bug 852665: add support for ICE disconnected state.

https://reviewboard.mozilla.org/r/89672/#review89282

> What's the + 100 for?

Originally I thought I could leverage the ICE transaction timer to kick off the next round of consent tries and wanted to avoid that the order of the two timers would get swapped. But we need an external timer any way to kick off the first round of consent, and then later we would have to calculate the difference between our calculated random time out value and the actual time it took until we got a reply or things timed out. So I left it with the extra timer. I'm going to remove the extra 100ms.

Comment 40

[Nils Ohlmeier [:drno]]

Comment on attachment 8806172 [details]
Bug 852665: add support for ICE disconnected state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89672/diff/1-2/

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dec79837f590
add support for ICE disconnected state. r=bwc

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dec79837f590

Comment 43

[Eric Shepherd [:sheppy]]

I’ve updated the documentation. Please let me know if I’ve missed anything important:

https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection#RTCIceConnectionState_enum (see also the compat table at the end of the page)
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/iceConnectionState#Browser_compatibility

https://developer.mozilla.org/en-US/Firefox/Releases/52#WebRTC