1167414 - Handle return values of FallibleTArray functions in media/webrtc/signaling/

Status: NEW

(Core :: WebRTC: Signaling, defect, P4)

Description

[poiru]

Below is a list of unchecked fallible calls. After bug 968520 lands, these will cause 'unused result' warnings. Could you please let me know how you would like them to be fixed (e.g. ignore result, use MOZ_ASSERT/MOZ_ENSURE_TRUE, propagate error)? Feel free to fix them yourself.

media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2931:58:
  Byron Campen [:bwc]: report->mIceCandidateStats.Value().AppendElement(cand);

media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2968:59:
  Byron Campen [:bwc]: report->mIceCandidatePairStats.Value().AppendElement(s);

media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3047:74:
  Jan-Ivar Bruaroey: query->report->mInboundRTPStreamStats.Value().AppendElement(s);

media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3084:73:
  Jan-Ivar Bruaroey: query->report->mOutboundRTPStreamStats.Value().AppendElement(s);

media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3116:75:
  Jan-Ivar Bruaroey: query->report->mOutboundRTPStreamStats.Value().AppendElement(s);

media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3170:70:
  Jan-Ivar Bruaroey: query->report->mInboundRTPStreamStats.Value().AppendElement(s);

media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp:1008:69:
  Jan-Ivar Bruaroey: ctx->mStatsForClosedPeerConnections.AppendElement(*query->report);

media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp:274:69:
  Paul Kerr [:pkerr]: request->mResult.mReports.Value().AppendElement(*(query->report));

media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp:281:57:
  Paul Kerr [:pkerr]: request->mResult.mReports.Value().AppendElement(pc);

media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp:325:65:
  Paul Kerr [:pkerr]: nsLogs.AppendElement(NS_ConvertUTF8toUTF16(line.c_str()));

media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp:327:69:
  Paul Kerr [:pkerr]: nsLogs.AppendElement(NS_LITERAL_STRING("+++++++ END ++++++++"));

media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp:346:73:
  Paul Kerr [:pkerr]: request->mResult.AppendElement(NS_ConvertUTF8toUTF16(line.c_str()));

media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp:348:77:
  Paul Kerr [:pkerr]: request->mResult.AppendElement(NS_LITERAL_STRING("+++++++ END ++++++++"));

media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp:629:54:
  Paul Kerr [:pkerr]: request->mResult.mReports.Value().AppendElement(s);

media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp:665:39:
  Paul Kerr [:pkerr]: request->mResult.AppendElements(aLog);

Comment 1

[Byron Campen [:bwc]]

Do we have any way of making all of these arrays infallible? If not, I would say ignore the results.

Comment 2

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

These all used to be infallible. When can they fail?

Do these have custom allocators? I gave up reading the whole thread, but in Bug 967167 comment 6 it seemed like there would be an assumption of infalliblity in some cases?

If you could clarify that would help my understanding.

Comment 3

[poiru]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> These all used to be infallible. When can they fail?

The type of e.g. mIceCandidateStats is Sequence<RTCIceCandidateStats>, which inherits from FallibleTArray. As such, all these calls are currently implicitly fallible.

In bug 968520, we plan to make fallibility visible at the call site. So instead of e.g. 

  report->mIceCandidateStats.Value().AppendElement(cand);

you would have:

  report->mIceCandidateStats.Value().AppendElement(cand, fallible);

This doesn't actually change behaviour as the calls were already fallible. However, after bug 968520 lands fully, the compiler will emit 'unused result' warnings in these cases.

With the current plan, if you have a nsTArray, you will be able to choose between fallible and infallible operations. However, if you have a FallibleTArray (as we have here), you will only be able use fallible operations. Perhaps we should allow people to opt into infallible allocation even with a FallibleTArray. I asked about this in bug 968520 comment 83.

Comment 4

[Bulk Bug Changes for mreavy's org]

Mass change P3->P4 to align with new Mozilla triage process.