850268 - Create a mochitest for testing a basic flow using addIceCandidate on a Peer Connection

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P3)

Description

[Jason Smith [:jsmith]]

Add a basic smoke test for the addIceCandidate function on a peer connection object.

Comment 1

[Jason Smith [:jsmith]]

This one feels a bit important because ice plays an important role in peer connections. We should at least do something basic here for sanity checks.

Comment 2

[Jason Smith [:jsmith]]

I can take this one.

So I've been experimenting with the addIceCandidate function to understand it against the signaling unit tests Eric pointed at me. I've managed to get addIceCandidate to throw an exception and hit an error callback. However, I've yet to hit the success callback.

I'm attaching my scratch pad code I've been experimenting with.

Eric - Can you give some input here on how and when success vs. error vs. exceptions get thrown? Is there a reference I could look at also for understanding the ice candidate syntax as well?

Comment 3

[Jason Smith [:jsmith]]

Attachment: addIceCandidate Scratchpad

Comment 4

[Eric Rescorla (:ekr)]

It should be an exception if you pass in something that's not an RTCIceCandidate at all.

It should be a failure if it's malformed (like no index), unparseable a-line, etc. [note that this last is going to be broken by the NS_DISPATCH_SYNC patch], or you're in the right state.

Otherwise success.

Comment 5

[Jason Smith [:jsmith]]

Attachment: Concept (non-working) patch v1

Comment 6

[Jason Smith [:jsmith]]

Here's the concept I've been exploring. I'm probably making some small mistake here, because I'm getting:

failed | uncaught exception - NS_ERROR_FAILURE: Invalid candidate passed to addIceCandidate! at file:///c:/Users/jsmith/Documents/mozilla-central/obj-i686-pc-mingw32/dist/bin/components/PeerConnection.js:592

Which occurs when the candidate or sdpMLineIndex is not defined:

    if (!cand.candidate || !cand.sdpMLineIndex) {
      throw new Components.Exception("Invalid candidate passed to addIceCandidate!");
    }

Although I'm confused, because I do have it defined in the test.

Comment 7

[Jason Smith [:jsmith]]

Comment on attachment 743819 [details] [diff] [review]
Concept (non-working) patch v1

Adam - Does the concept make sense here? And is there anything you see that's causing the failure?

Comment 8

[Adam Roach [:abr]]

Answered in IRC. The basic thrust is that "0" and "" both evaluate to false, so the check is going to fail. Whether it *should* fail is a different question, and one that I think Jason is opening another bug on.

Comment 9

[Adam Roach [:abr]]

Comment on attachment 743819 [details] [diff] [review]
Concept (non-working) patch v1

The test itself is going to blow up once we get proper state checking in place. You can't really add candidates to the remote SDP until you've set the remote SDP.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Adam Roach [:abr] from comment #9)
> Comment on attachment 743819 [details] [diff] [review]
> Concept (non-working) patch v1
> 
> The test itself is going to blow up once we get proper state checking in
> place. You can't really add candidates to the remote SDP until you've set
> the remote SDP.

Right...I remember getting that error when I did before the handshake happened. In this case though, the add ice candidates are being added after the remote SDP has been set though.

Comment 11

[Jason Smith [:jsmith]]

Attachment: Basic Add Ice Candidate Automation v1

Comment 12

[Jason Smith [:jsmith]]

Attachment: Basic Add Ice Candidate Automation v1

Comment 13

[Jason Smith [:jsmith]]

Comment on attachment 743973 [details] [diff] [review]
Basic Add Ice Candidate Automation v1

Turns out the other issue that wasn't working was a type-o in pc.js. The parameters were okay originally, given that we +1 the 0 to 1, so the value isn't false.

Ran this a few times - looks to be passing okay.

Also double checked - these add ice candidates are being added after the handshake is established, so the concern that we would get an error adding ice candidates won't happen.

This is now ready for review to get landed.

Comment 14

[Adam Roach [:abr]]

Comment on attachment 743973 [details] [diff] [review]
Basic Add Ice Candidate Automation v1

Review of attachment 743973 [details] [diff] [review]:
-----------------------------------------------------------------

r- because the test in its current form doesn't really test anything: feeding bogus information into the ICE stack doesn't check behavior other than "we don't crash if you do this slightly odd thing."

The way to test our behavior for addIceCandidate would look something like this: (1) Before setting the remote description, find and remove all the a=candidate lines (storing them for later) and change the IP address in the "c=" line so it no longer works; (2) run through the test step up to and including PC_REMOTE_SET_LOCAL_DESCRIPTION; (3) call addIceCandidate with the saved candidates (once for each candidate); (4) Run the two CHECK_MEDIA steps.

That sequence of events will verify that media can be set up even in the case that the original SDP contains no valid transport addresses, which is really the point of adding ICE candidates after the initial offer/answer exchange.

For complete coverage, I would actually have three tests: one where the four steps I describe above are applied to the local PC only; one where they're applied to the remote PC only; and one where they're applied to both. This will test en-block calls to trickle clients; trickle calls to en-bloc clients; and trickle calls to trickle clients, all of which really need to work.

::: dom/media/tests/mochitest/test_peerConnection_basicAddIceCandidate.html
@@ +14,5 @@
> +    title: "Basic addIceCandidate test"
> +  });
> +
> +  var iceCandidate = new mozRTCIceCandidate({
> +    "candidate": "a=candidate:1 1 UDP 2130706431 192.168.2.1 50005 typ host",

Although we do accept this format (because Chrome generates it), I believe the current understanding (to be clarified in the spec) is that that "a=" should not be included here. Once Chrome catches up with the spec, we'll be removing this "a=" stripping, which will break the test.

Comment 15

[Eric Rescorla (:ekr)]

> ::: dom/media/tests/mochitest/test_peerConnection_basicAddIceCandidate.html
> @@ +14,5 @@
> > +    title: "Basic addIceCandidate test"
> > +  });
> > +
> > +  var iceCandidate = new mozRTCIceCandidate({
> > +    "candidate": "a=candidate:1 1 UDP 2130706431 192.168.2.1 50005 typ host",
> 
> Although we do accept this format (because Chrome generates it), I believe
> the current understanding (to be clarified in the spec) is that that "a="
> should not be included here. Once Chrome catches up with the spec, we'll be
> removing this "a=" stripping, which will break the test.

This actually isn't even the format Chrome uses, because they have a \r\n at the end.

In any case, don't use the a= in front.

Comment 16

[Jason Smith [:jsmith]]

Don't know when I'll get back to working on this, but unassigning for now.

Comment 17

[Maire Reavy [:mreavy]]

Hey Nils, We said a couple of weeks ago that this really isn't needed for Fx32, but that if a patch is "close" to  being ready, we'd keep this on our radar for Fx 32.  I think it may make sense to push this out in favor of other, more important work.  What do you think?

Comment 18

[Nils Ohlmeier [:drno]]

(In reply to Maire Reavy [:mreavy] (Please needinfo me if you need my attention on a bug) from comment #17)
> Hey Nils, We said a couple of weeks ago that this really isn't needed for
> Fx32, but that if a patch is "close" to  being ready, we'd keep this on our
> radar for Fx 32.  I think it may make sense to push this out in favor of
> other, more important work.  What do you think?

Correct.
My only concern is that bug 1004530 shows that our trickle ICE is not working. It would be good to have the fix for bug 1004530 guarded by an actual test.

Comment 19

[Nils Ohlmeier [:drno]]

After thinking about it a little more I just realized that we don't have any way to either:
- verify that the connection is utilizing an iceCandidate
- or enforce connectivity to certain iceCandidate paths only
Therefore any test which we could get implemented would not test much. Lets push this out until we have one of the above ready and have a meaningful test.

Comment 20

[Nils Ohlmeier [:drno]]

getStats() might be a way to verify which iceCandidate is used on the transport layer.

Comment 21

[Nils Ohlmeier [:drno]]

Test utilizing addIceCandidate are available since quite some time.