1158959 - Mochitest messaging between PC's does not guarantee order

Status: NEW

(Core :: WebRTC, defect, P5)

Description

[Nils Ohlmeier [:drno]]

Currently our mochitests pass the equivalent of signaling messages between the two PeerConnection or the PeerConnectionWrappers as direct on indirect as they please.

E.g. Ice candidates travel like this:
pcLocal.onicecandidate(c) -> test.iceCandidateHandler(c) -> pcRemote.storeOrAddIceCandidate(c) -> pcRemote.addIceCandidate(c)

FYI the only reason the candidate first gets delivered back to the test level is to allow the test level to properly send the ice candidate to the signaling server in case of steeplechase (as you don't have access to pcRemote in that case).

Links to the code pieces:
https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/templates.js#210
https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#1343
https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#512
https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#1207

Leaving aside the promise which currently acts as a blocker before calling addIceCandidate() this means the ice candidate gets added to pcRemote directly from within the onicecandidate callback of pcLocal.

Where the SDP travels likes this:
test.createOffer() -> pcLocal.createOffer() -> pc.createOffer() -> stores offer in some vars -> test.sLD() -> ... -> copy vars around on the test level (aka PC_REMOTE_GET_OFFER) -> test.sRD() -> pcRemote.sRD() -> pc.sRD()

https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/templates.js#268
https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#371
https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#1027
https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/templates.js#287
https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/templates.js#296
https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/templates.js#311
https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#465

There are plenty of dispatches in the layers around the SDP.

We should add a generic signaling interface which the originator uses to "send" signaling related events to the other PeerConnection. On the receiving side each of received messages gets translated into an event of the given message type. Receiving an event with no event handler being registered should probably translate into an error (that is currently not the case with steeplechase - messages with no handler currently get stored until someone asks for a message of that type (or indefinitely)). The actual interface implementation would look a little different for steeplechase vs local. A queue in the send/recv interface would guarantee order of the messages.
In case of the local implementation we would potentially wait with the dispatching between send and receive until the test steps in templates.js proceed to the next test steps to simulate a kind of async behavior.

Comment 1

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

That, or collapse everything between PC_LOCAL_SET_LOCAL_DESCRIPTION and PC_REMOTE_SET_REMOTE_DESCRIPTION :-)

The key to signaling would seem to be to use it for both candidates and offers. Does this work right on steeplechase today?

Comment 2

[Nils Ohlmeier [:drno]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #1)
> That, or collapse everything between PC_LOCAL_SET_LOCAL_DESCRIPTION and
> PC_REMOTE_SET_REMOTE_DESCRIPTION :-)

What do you mean by "collapse"?
The answerer doesn't have to do signaling between these steps, but the offerer has to wait for answer to come back, or?

> The key to signaling would seem to be to use it for both candidates and
> offers.

In case of steeplechase the signaling is the same. Except again in both case the way from the event handler or success callback to the signaling server is different. And also on the receiving end the path from the signaling to the function calls is different.
 
> Does this work right on steeplechase today?

Yes it works in steeplechase today. But steeplechase does not execute the basicH264Video test case which seems to be the most timing critical in our case.
But my point here is/was more that in the non-steeplechase case the signaling paths are quite different and we should "fix" that.

Comment 3

[Bulk Bug Changes for mreavy's org]

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