Closed Bug 1373015 Opened 7 years ago Closed 7 years ago

Permafailing dom/media/tests/mochitest/test_peerConnection_stats.html | candidate-pair.state is succeeded. value=cancelled

Categories

(Core :: WebRTC, defect, P2)

Product:
Core
Component:
WebRTC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: mjf)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:other])

Attachments

(1 file)

Bug 1373015 - fix permafail mochitest for RTCIceCandPairStats changes in Bug 1339906.
59 bytes, text/x-review-board-request
drno
: review+
Details 
The failing verification's suggest a PeerConnection which had not connected (0 bytes, state canceled etc).
But in the first full dump of the stats everything looks fine. My best guess right now is that the stats for some unknown reason get pulled to early to contain the "real" values of the call.
Assignee: nobody → mfroman
Rank: 21
Priority: -- → P2
(In reply to Nils Ohlmeier [:drno] from comment #1)
> The failing verification's suggest a PeerConnection which had not connected
> (0 bytes, state canceled etc).
> But in the first full dump of the stats everything looks fine. My best guess
> right now is that the stats for some unknown reason get pulled to early to
> contain the "real" values of the call.

When running locally (and on try before landing) I only saw pairs that were in state succeeded.  Not sure what changed on the timing, but I'm going to tweak the test to only look at most of the fields on the candidate pair stats when the state is succeeded.
Follow up: this happened today on my OSX machine, so it wasn't only and Android issue.
Comment on attachment 8878167 [details]
Bug 1373015 - fix permafail mochitest for RTCIceCandPairStats changes in Bug 1339906.

https://reviewboard.mozilla.org/r/149544/#review154202

I guess the edge case here is that in case of peer reflexive candidates we have one candidates in state canceled plus an identical candidate in the succeed state.
Though normally the canceled candidates should not show up in the stats. Would be interested to find out why sometimes canceled candidates pop up in the stats.
Attachment #8878167 - Flags: review?(drno) → review+
(In reply to Nils Ohlmeier [:drno] from comment #5)
> Would be interested to find out why sometimes canceled candidates pop up in
> the stats.
I'm going to go ahead and land this fix to squelch the oranges, and then I'll take a look at why the cancelled candidates are showing up in the stats.
Pushed by mfroman@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/4dffb09212c0
fix permafail mochitest for RTCIceCandPairStats changes in Bug 1339906. r=drno
There is currently no code in PeerConnectionImpl::RecordIceStats_s or NrIceMediaStream::GetCandidatePairs to filter candidate pairs that are in state cancelled.  Should I open a new bug to add code to RecordIceStats_s that does not add cancelled candidate pairs to the ice stats?  Note, this change would also keep them from showing on about:webrtc.
Flags: needinfo?(drno)
https://hg.mozilla.org/mozilla-central/rev/4dffb09212c0
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Whiteboard: [stockwell fixed:other]
(In reply to Michael Froman [:mjf] from comment #9)
> There is currently no code in PeerConnectionImpl::RecordIceStats_s or
> NrIceMediaStream::GetCandidatePairs to filter candidate pairs that are in
> state cancelled.  Should I open a new bug to add code to RecordIceStats_s
> that does not add cancelled candidate pairs to the ice stats?  Note, this
> change would also keep them from showing on about:webrtc.

It is there http://searchfox.org/mozilla-central/source/media/mtransport/nricemediastream.cpp#356
Not sure if I would keep investigating if that filter did not work, or if the RTCP stats were reporting some other legit canceled pair. Although since these are always host <-> host pair one wonders what pairs should legitimately get canceled...
Flags: needinfo?(drno)
(In reply to Nils Ohlmeier [:drno] from comment #14)
> (In reply to Michael Froman [:mjf] from comment #9)
> > There is currently no code in PeerConnectionImpl::RecordIceStats_s or
> > NrIceMediaStream::GetCandidatePairs to filter candidate pairs that are in
> > state cancelled.  Should I open a new bug to add code to RecordIceStats_s
> > that does not add cancelled candidate pairs to the ice stats?  Note, this
> > change would also keep them from showing on about:webrtc.
> 
> It is there
> http://searchfox.org/mozilla-central/source/media/mtransport/
> nricemediastream.cpp#356
> Not sure if I would keep investigating if that filter did not work, or if
> the RTCP stats were reporting some other legit canceled pair. Although since
> these are always host <-> host pair one wonders what pairs should
> legitimately get canceled...
I think that code is only picking the better of a duplicated pair, not filtering out cancelled pairs.  Interestingly, I typically see a cancelled pair when using Spark.
Flags: needinfo?(drno)
(In reply to Michael Froman [:mjf] from comment #15)
> (In reply to Nils Ohlmeier [:drno] from comment #14)
> > It is there
> > http://searchfox.org/mozilla-central/source/media/mtransport/
> > nricemediastream.cpp#356
> > Not sure if I would keep investigating if that filter did not work, or if
> > the RTCP stats were reporting some other legit canceled pair. Although since
> > these are always host <-> host pair one wonders what pairs should
> > legitimately get canceled...
> I think that code is only picking the better of a duplicated pair, not
> filtering out cancelled pairs.  Interestingly, I typically see a cancelled
> pair when using Spark.

My comment #5 was only considering duplicated pairs caused by peer reflexive candidates. Peer reflexive candidates can happen quite often in our CI system when ICE is quicker then the signaling path. So that is one way of getting a canceled pair.

The second way is if ICE succeeds already, but there are more pairs to be checked, which is then skipped, e.g. canceling TURN pairs. As our CI tests don't provide STUN or TURN servers, and are run on singled homed machines I don't think we should see any of these type of canceled pairs.

Which then leave us with my original question: of why do we see canceled duplicated pairs in the stats instead of the succeed pairs? And why only at the very early RTCP, but not in the later RTCP.
Flags: needinfo?(drno)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: