1256720 - Bad assertion in nr_ice_media_stream_check_timer_cb

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

The if-statement here does not test for whether all streams are complete, just one, so there is no reason to expect that the number of active streams is 0:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c#328

What we probably want to do instead is:

if (timer_multiplier == 0) timer_multiplier = 1;

Comment 1

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #0)
> What we probably want to do instead is:
> 
> if (timer_multiplier == 0) timer_multiplier = 1;

I guess there is (or should be) no case where active_streams is 0, but ice_state is not COMPLETED and we therefore want the timer_val to be 0.

Comment 2

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1256720: Remove a bad assertion, and simplify some code. r=drno

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

Comment 3

[Nils Ohlmeier [:drno]]

Comment on attachment 8734497 [details]
MozReview Request: Bug 1256720: Remove a bad assertion, and simplify some code. r=drno

https://reviewboard.mozilla.org/r/42285/#review38869

::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c:323
(Diff revision 1)
>    {
>      int r,_status;
>      nr_ice_media_stream *stream=cb_arg;
>      nr_ice_cand_pair *pair = 0;
> -    int timer_val;
> -    int timer_multiplier;
> +    int timer_multiplier=stream->pctx->active_streams ? stream->pctx->active_streams : 1;
> +    int timer_val=stream->pctx->ctx->Ta*timer_multiplier;

To be on the safe side an assert(timer_val>0) below this or before setting the timer would be nice.

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8734497 [details]
MozReview Request: Bug 1256720: Remove a bad assertion, and simplify some code. r=drno

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

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/169b38772095

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/169b38772095