1157042 - PeerConnection.js reorders setLD success promise wrt onicecandidate

Status: NEW

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

Description

[Eric Rescorla (:ekr)]

The C++ generates them in the right order (setLD success then onicecandidate)
but then they come out in the wrong order.

Trace below.

53 INFO pcLocalEKR: SetLocalDescription()
EKR: onSetLocalDescriptionSuccess
57 INFO PeerConnectionWrapper (pcLocal)EKR: Internal, set local description
59 INFO pcLocalEKR: SetLocalDescription returned
63 INFO pcRemote: EKR: Calling setRemoteDescription
112 INFO pcRemoteEKR: SetLocalDescription()
EKR: onSetLocalDescriptionSuccess
EKR: foundIceCandidate
118 INFO pcLocal: EKR: iceCandidate = {"candidate":"candidate:0 1 UDP 2122252543 192.168.199.149 51020 typ host","sdpMid":"","sdpMLineIndex":0}
123 INFO PeerConnectionWrapper (pcRemote): EKR: adding ICE candidate {"candidate":"candidate:0 1 UDP 2122252543 192.168.199.149 51020 typ host","sdpMid":"","sdpMLineIndex":0}
EKR: foundIceCandidate
124 INFO pcLocal: EKR: iceCandidate = {"candidate":"candidate:0 2 UDP 2122252542 192.168.199.149 44004 typ host","sdpMid":"","sdpMLineIndex":0}
129 INFO PeerConnectionWrapper (pcRemote): EKR: adding ICE candidate {"candidate":"candidate:0 2 UDP 2122252542 192.168.199.149 44004 typ host","sdpMid":"","sdpMLineIndex":0}
EKR: foundIceCandidate
EKR: foundIceCandidate
131 INFO pcRemote: EKR: iceCandidate = {"candidate":"candidate:0 1 UDP 2122252543 192.168.199.149 52851 typ host","sdpMid":"","sdpMLineIndex":0}
EKR: foundIceCandidate
140 INFO PeerConnectionWrapper (pcRemote)EKR: Internal, set local description
142 INFO pcRemoteEKR: SetLocalDescription returned
148 INFO pcLocal: EKR: Calling setRemoteDescription
200 INFO PeerConnectionWrapper (pcLocal): EKR: adding ICE candidate {"candidate":"candidate:0 1 UDP 2122252543 192.168.199.149 52851 typ host","sdpMid":"","sdpMLineIndex":0}

Comment 1

[Eric Rescorla (:ekr)]

Attachment: Test for

Comment 2

[Eric Rescorla (:ekr)]

Attachment: Diagnostics

Comment 3

[Eric Rescorla (:ekr)]

How to read this is that the lines with #s in front come from the mochitest and the ones without come from Firefox.

As you can see, the second setLD succeeds in Firefox prior to foundIceCandidate firing, but then the ICE candidate callbacks in pc.js (i.e., the mochitests) fire *before* the setLD promise is resolved. This seems to point to some reordering happening in PeerConnection.js

Comment 4

[Eric Rescorla (:ekr)]

The repro for this is:
./mach mochitest-plain --e10s dom/media/tests/mochitest/test_peerConnection_basicH264Video.html

Comment 5

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

I've not been able to reproduce this problem per comment 4 on my OSX system in debug or opt builds. I used the patch here and my patch from Bug 1087551. Perhaps this is network topology dependent?

I'm also unable to reproduce any problems outside of the test framework using http://jsfiddle.net/0gk0cyn4

If others can reproduce that would be great!

Still, from code-inspection and yesterday's discussion on irc, we found no code-guarantee that the setLocalDescription success-callback would always run before onicecandidate could fire, so I think we still want to land my patch which makes this guarantee explicit. Let me know if you feel otherwise.

Uplifting seems unnecessary however (I would prefer if it rode the train actually).

Would it make sense for me to move that patch here and land it from this bug? Then Nils can keep his work separate.

Comment 6

[Eric Rescorla (:ekr)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #5)
> I've not been able to reproduce this problem per comment 4 on my OSX system
> in debug or opt builds. I used the patch here and my patch from Bug 1087551.
> Perhaps this is network topology dependent?
> 
> I'm also unable to reproduce any problems outside of the test framework
> using http://jsfiddle.net/0gk0cyn4
> 
> If others can reproduce that would be great!
>

It happens on my Linux box reliably.



> Still, from code-inspection and yesterday's discussion on irc, we found no
> code-guarantee that the setLocalDescription success-callback would always
> run before onicecandidate could fire, so I think we still want to land my
> patch which makes this guarantee explicit. Let me know if you feel otherwise.

I'd like to understand the entire space of what's busted before we start
doing piecemeal fixes.

Comment 7

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

(In reply to Eric Rescorla (:ekr) from comment #6)
> It happens on my Linux box reliably.

Perfect. With http://jsfiddle.net/0gk0cyn4 too?

> I'd like to understand the entire space of what's busted before we start
> doing piecemeal fixes.

Getting the test framework to a point where we're testing this without aid of candidate-caching sounds like a good idea.

Comment 8

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

Attachment: delay firing icecandidate until after setLocalDesciption promise resolves (4) r=mt

Moved here from Bug 1087551, as this bug describes the right problem. Unbitrot. Carrying forward r=mt.

Comment 9

[Bulk Bug Changes for mreavy's org]

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