Closed Bug 1256720 Opened 8 years ago Closed 8 years ago

Bad assertion in nr_ice_media_stream_check_timer_cb

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bwc, Unassigned)

Details

Attachments

(1 file)

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;
(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.
Rank: 25
Priority: -- → P2
Attachment #8734497 - Flags: review?(drno) → review+
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.
Attachment #8734497 - Attachment description: MozReview Request: Bug 1256720: Remove a bad assertion, and simplify some code. r?drno → MozReview Request: Bug 1256720: Remove a bad assertion, and simplify some code. r=drno
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/
https://hg.mozilla.org/mozilla-central/rev/169b38772095
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.