1344946 - Ensure that mozRtt is initialized before use

Status: RESOLVED DUPLICATE of bug 1344970

(Core :: WebRTC, defect, P2)

Description

[Nico Grunbaum [:ng, @chew:mozilla.org]]

It is possible for GetRTCPReceiverReport to return with a success code without having initialized mozRtt.

Comment 1

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1344946 - ensure that mozRtt is initialized;

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

Comment 2

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

Comment on attachment 8844231 [details]
Bug 1344946 - ensure that mozRtt is initialized;

https://reviewboard.mozilla.org/r/117752/#review119442

r=me with overflow addressed.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:824
(Diff revision 1)
>  
> +    int64_t rtt = mRecvStream->GetRtt();
> +    if (rtt < 0) { // Indicates that RTT is unavailable
> +      return false;
> +    }
> +    MOZ_ASSERT(rtt <= UINT32_MAX);

I think we want INT32_MAX here, or we can end up returning negative numbers in the next line.

Note that MOZ_ASSERT will crash in debug builds, so are we sure the lower-level code can never ever return values bigger than this? If not, should we crop instead, and/or log a warning?

Comment 3

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8844231 [details]
Bug 1344946 - ensure that mozRtt is initialized;

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

Comment 4

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8844231 [details]
Bug 1344946 - ensure that mozRtt is initialized;

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

Comment 5

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

Comment on attachment 8844231 [details]
Bug 1344946 - ensure that mozRtt is initialized;

https://reviewboard.mozilla.org/r/117752/#review119474

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:820
(Diff revision 3)
> +    int64_t rtt = mRecvStream->GetRtt();
> +    if (rtt < 0) { // Indicates that RTT is unavailable
> +      return false;
> +    }

Failing to provide any stats because we can't calculate RTT is a bad idea.  I just opened a spec issue on this because RoundTripTime in the spec isn't defined for the "I can't calculate it" case.

My patch for SSRCs tripped over this, and I fixed it by adding an "else { *rttMs = 0; }" in bug 1337810

Comment 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Fixed in bug 1337810.
This patch is superseded by the fix in Bug 1344970.