Closed Bug 852665 Opened 11 years ago Closed 8 years ago

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

Categories

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

Product:
Core
Component:
WebRTC: Networking
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: ekr, Assigned: drno)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [webrtc][blocking-webrtc-][spec-issue][webrtc-uplift])

Attachments

(1 file)

Bug 852665: add support for ICE disconnected state.
58 bytes, text/x-review-board-request
bwc
: review+
Details 
We need some way to report that transport has failed.
Not sure without more info if this is blocking, but I suspect it isn't
Whiteboard: [webrtc][blocking-webrtc?]
Assignee: nobody → ekr
Priority: -- → P2
Whiteboard: [webrtc][blocking-webrtc?] → [webrtc][blocking-webrtc-][spec-issue]
Assignee: ekr → adam
Whiteboard: [webrtc][blocking-webrtc-][spec-issue] → [webrtc][blocking-webrtc-][spec-issue][webrtc-uplift]
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.
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 ...
i meant, if the failure "isn't" reported :)
Blocks: Talkilla
Summary: Report WebRTC transport failures somehow → Report WebRTC transport failures (iceConnectionState=failed)
Summary: Report WebRTC transport failures (iceConnectionState=failed) → Report WebRTC transport termination (e.g. iceConnectionState=failed|disconnected|closed|)
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.
Are you looking for negotiation failure or ICE failure?
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.
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
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?
(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.
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).
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?
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
BUMP
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.
Depends on: 929977
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.
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
Summary: Report WebRTC transport termination (e.g. iceConnectionState=failed|disconnected|closed|) → Report WebRTC transport termination (e.g. iceConnectionState=disconnected)
(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...
backlog: --- → webRTC+
Rank: 27
Any updates so far? when this will be fixed?
It is hard to say. We have a lot on our plate this year.
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
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.
Assignee: adam → nobody
QA Contact: jsmith
In a case that nobody will work on this issue, Do you have any recommendations how to handle peer disconnected state in Firefox?
Once consent freshness (bug 929977, which drno is working on) is done, it should be fairly easy to implement this.
(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
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...
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?
Rank: 27 → 17
Flags: needinfo?(drno)
Priority: P2 → P1
Thanks for the quick reaction, highly appreciated! :-)
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
Assignee: nobody → drno
Flags: needinfo?(drno)
When can we expect this issue to be resolved.
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.
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 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?
Attachment #8806172 - Flags: review?(docfaraday) → review+
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.
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/dec79837f590
add support for ICE disconnected state. r=bwc
https://hg.mozilla.org/mozilla-central/rev/dec79837f590
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1320150
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: