Closed Bug 1024028 Opened 7 years ago Closed 6 years ago

ICE connection state never transitions to checking with a full trickle peer

Categories

(Core :: WebRTC: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(2 files, 1 obsolete file)

The only place we transition to checking happens here:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/nricectx.cpp#693

This does not happen if the peer has not yet supplied candidates:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/nricectx.cpp#685

This allows us to transition directly from new to connected, which is probably likely to confuse content, and also confuses some of our telemetry probes.
Blocks: 1041832
The spec defines the "checking" state as follows:

"The ICE Agent has received remote candidates on at least one component, and is checking candidate pairs but has not yet found a connection. In addition to checking, it may also still be gathering."

So, we'll need to add another callback to nICEr that fires when checks actually start.
Assignee: nobody → docfaraday
Attachment #8468633 - Flags: review?(drno)
Attachment #8468634 - Flags: review?(drno)
Comment on attachment 8468633 [details] [diff] [review]
Part 1: Test that checking is reached in ice_unittest.

Review of attachment 8468633 [details] [diff] [review]:
-----------------------------------------------------------------

Just one small NIT, but otherwise LGTM.

::: media/mtransport/test/ice_unittest.cpp
@@ +773,5 @@
> +        std::cerr << "ICE completed " << name_ << std::endl;
> +        ice_complete_ = true;
> +        break;
> +      case NrIceCtx::ICE_CTX_FAILED:
> +        break;

A default: with at least some error message about an unknown state would be nice.
Attachment #8468633 - Flags: review?(drno) → review+
Comment on attachment 8468634 [details] [diff] [review]
Part 2: Add an ice_checking callback that is fired when checking actually starts.

Review of attachment 8468634 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, besides some small optimizations.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.h
@@ +136,5 @@
>    nr_ice_foundation_head foundations;
>  
>    nr_ice_media_stream_head streams;           /* Media streams */
>    int stream_ct;
> +  UCHAR checks_started;

Whats the idea behind this flag?
It doesn't seem to get set or query anywhere, or?

::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c
@@ +576,5 @@
> +    if (!pctx->checks_started) {
> +      r_log(LOG_ICE,LOG_NOTICE,"ICE(%s): peer (%s) is now checking",pctx->ctx->label,pctx->label);
> +      pctx->checks_started = 1;
> +      if (pctx->handler && pctx->handler->vtbl->ice_checking) {
> +        pctx->handler->vtbl->ice_checking(pctx->handler->obj, pctx);

Shouldn't the checks_started flag get set after ice_checking()?
Theoretically you could end up setting the flag, but not making the state transition if you are missing the handler. And I assume that if check for handler is not there just for fun.
Attachment #8468634 - Flags: review?(drno) → review+
(In reply to Nils Ohlmeier [:drno] from comment #5)
> Comment on attachment 8468633 [details] [diff] [review]
> Part 1: Test that checking is reached in ice_unittest.
> 
> Review of attachment 8468633 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just one small NIT, but otherwise LGTM.
> 
> ::: media/mtransport/test/ice_unittest.cpp
> @@ +773,5 @@
> > +        std::cerr << "ICE completed " << name_ << std::endl;
> > +        ice_complete_ = true;
> > +        break;
> > +      case NrIceCtx::ICE_CTX_FAILED:
> > +        break;
> 
> A default: with at least some error message about an unknown state would be
> nice.

   I am of two minds on this; a default label can give you runtime detection of cases where someone added something to the enum, and out-of-bounds values due to memory corruption or something, whereas omitting it can get you compile-time detection of the former, but no detection of the latter. I feel like the compile-time check is worth it, since if we're out of bounds we're already in undefined territory, and adding a check is not likely to be all that useful for diagnostics.
(In reply to Nils Ohlmeier [:drno] from comment #6)
> Comment on attachment 8468634 [details] [diff] [review]
> Part 2: Add an ice_checking callback that is fired when checking actually
> starts.
> 
> Review of attachment 8468634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, besides some small optimizations.
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.h
> @@ +136,5 @@
> >    nr_ice_foundation_head foundations;
> >  
> >    nr_ice_media_stream_head streams;           /* Media streams */
> >    int stream_ct;
> > +  UCHAR checks_started;
> 
> Whats the idea behind this flag?
> It doesn't seem to get set or query anywhere, or?
> 

   Yeah, that should only be in ice_peer_ctx. Will remove.

> ::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c
> @@ +576,5 @@
> > +    if (!pctx->checks_started) {
> > +      r_log(LOG_ICE,LOG_NOTICE,"ICE(%s): peer (%s) is now checking",pctx->ctx->label,pctx->label);
> > +      pctx->checks_started = 1;
> > +      if (pctx->handler && pctx->handler->vtbl->ice_checking) {
> > +        pctx->handler->vtbl->ice_checking(pctx->handler->obj, pctx);
> 
> Shouldn't the checks_started flag get set after ice_checking()?
> Theoretically you could end up setting the flag, but not making the state
> transition if you are missing the handler. And I assume that if check for
> handler is not there just for fun.

   That handler is an optional callback that is either set at the very beginning, or not at all. For me, the principle of least surprise says that if I get an |ice_checking| callback, the state variables should indicate that checks have indeed started. I'm also not sure I want to turn the |checks_started| flag into a |ice_checking_callback_fired| flag, even though we aren't using it for anything else yet.
Attachment #8468634 - Attachment is obsolete: true
Comment on attachment 8476003 [details] [diff] [review]
Part 2: Add an ice_checking callback that is fired when checking actually starts.

Review of attachment 8476003 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=drno
Attachment #8476003 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/12c48b15b3f5
https://hg.mozilla.org/mozilla-central/rev/2954187fdb95
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.