796894 - Create Mochitest for data channel only connection (send/receive/disconnect)

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As part of the initial landing of the WebRTC feature some smoketests should ensure the basics for the Peer Connection feature are working. This bug covers the request for the following test:

Ensure a local data connection works:

* The connection can be established
* Data can be send/received
* Disconnect

Comment 1

[Jason Smith [:jsmith]]

The meta bug was indicated as blocking, but when we originally triaged this, we said not blocking. Which is it? Block or No Block?

Comment 2

[Maire Reavy [:mreavy]]

I believe it should block.  We want mochitests along with the feature itself.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So I was working on such a test yesterday and today and I wasn't able to get this working. Today I rearranged the datachannel test on webrtc-landing to mostly comply to our mochitests and I figured out that you will need at least a single mediastream attached to the peer connections to be able to create a datachannel. I find that weird given that there might be a lot of situations like chats where you do not need a stream at all.

Randell, can you please clarify? Is that expected or a bug?

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

This is a known bug (caused by us not hooking up to the SDP yet); we expect to fix it in the next week.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With bug 837035 fixed we should be able to implement that test now.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: WIP (no ondatachannel fired)

With this patch applied it's somewhat frustrating that the ondatachannel event is never fired. I have absolutely no idea why it is the case. Using the datachannel test from webrtc-landing instead and removing the second call to createDataChannel() everything works fine. But here the bidirectional setup of the channel is not done.

I already talked to Randell on IRC and he wants to have a look at it. Any other help is welcome too. Thanks!

Comment 7

[Jason Smith [:jsmith]]

Comment on attachment 739206 [details] [diff] [review]
WIP (no ondatachannel fired)

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

On the right track overall. I think I know what problem you are hitting - you aren't doing any data channel logic with the remote peer, so that might be the problem you are hitting here potentially.

::: dom/media/tests/mochitest/Makefile.in
@@ +44,4 @@
>    head.js \
>    mediaStreamPlayback.js \
>    pc.js \
> +  templates.js \

Good idea. I like the usage of splitting out the commands into a templates file.

::: dom/media/tests/mochitest/pc.js
@@ +335,5 @@
> +    value: function DCT_createDataChannel(options, onSuccess, onFailure) {
> +      this.pcLocal.createDataChannel(options);
> +
> +      if (!this.connected) {
> +        onSuccess();

I'm confused what the purpose of this logic is here. Can you clarify?

@@ +374,5 @@
> +  /**** Data channel properties and methods ****/
> +
> +  this.dataChannels = [ ];
> +
> +  this._pc.onconnection = function () {

Do we even use this event handler anymore? I thought this functionality was disabled now.

@@ +507,5 @@
> +    var label = '_channel' + this.dataChannels.length;
> +
> +    info("Create data channel '" + label + "' for " + this.label);
> +    var channel = this._pc.createDataChannel(label, options);
> +    channel.binaryType = "blob";

Why are we forcing a blob type here?

::: dom/media/tests/mochitest/templates.js
@@ +103,5 @@
> +      });
> +    }
> +  ],
> +  [
> +    'PC_LOCAL_CREATE_DATA_CHANNEL',

I only see one data channel getting created in this test for the local peer. Don't you need to do this for remote peer as well?

In fact, I actually don't see any remote data channel setup being done here. Maybe that's the problem you are hitting?

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jason Smith [:jsmith] from comment #7)

> On the right track overall. I think I know what problem you are hitting -
> you aren't doing any data channel logic with the remote peer, so that might
> be the problem you are hitting here potentially.

Thanks for the feedback Jason. The thing here is that you do not have to call createDataChannel() on both sides. It's enough to do it for the local peer. The correct setup is done bidirectional and the remote peer should get the created channel via ondatachannel. But even with this callback registered it's never called. This information came from Jesup a couple of days ago. I did similar things with the data channel test page on webrtc-landing and it seems to work there. So it might be something with our framework. Given that we haven't heard back from Randell yet, I will continue today in figuring out what's wrong.  

> ::: dom/media/tests/mochitest/pc.js
> @@ +335,5 @@
> > +    value: function DCT_createDataChannel(options, onSuccess, onFailure) {
> > +      this.pcLocal.createDataChannel(options);
> > +
> > +      if (!this.connected) {
> > +        onSuccess();
> 
> I'm confused what the purpose of this logic is here. Can you clarify?

The first call you have to do to create a datachannel is before createOffer. So we cannot wait until the event listeners get called. Any later call to add an additional datachannel (after the connection has been established) has to wait until the channel is setup. It's quiet complicated to handle all the different callbacks appropriately in the framework for mochitests so we will not end-up with a missing path and a global timeout. Let me finish this part and it will be clearer to understand. This is just working testing code.

> > +  this._pc.onconnection = function () {
> 
> Do we even use this event handler anymore? I thought this functionality was
> disabled now.

That's the only event which gets thrown for me when running the datachannel test. So it's still around, yes. Is there any bug you think of? 

> > +    var label = '_channel' + this.dataChannels.length;
> > +
> > +    info("Create data channel '" + label + "' for " + this.label);
> > +    var channel = this._pc.createDataChannel(label, options);
> > +    channel.binaryType = "blob";
> 
> Why are we forcing a blob type here?

It's testing code and not ready for any code related feedback or review. I want to see the functionality working first. And this is a copy from the datachannel testing page.

> > +    'PC_LOCAL_CREATE_DATA_CHANNEL',
> 
> I only see one data channel getting created in this test for the local peer.
> Don't you need to do this for remote peer as well?

See my above answer to this. That's the intended steps you will have to do as given by Randell. That's why I'm also seeking for his feedback.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> (In reply to Jason Smith [:jsmith] from comment #7)
> 
> > On the right track overall. I think I know what problem you are hitting -
> > you aren't doing any data channel logic with the remote peer, so that might
> > be the problem you are hitting here potentially.
> 
> Thanks for the feedback Jason. The thing here is that you do not have to
> call createDataChannel() on both sides. It's enough to do it for the local
> peer. The correct setup is done bidirectional and the remote peer should get
> the created channel via ondatachannel. But even with this callback
> registered it's never called. This information came from Jesup a couple of
> days ago. I did similar things with the data channel test page on
> webrtc-landing and it seems to work there. So it might be something with our
> framework. Given that we haven't heard back from Randell yet, I will
> continue today in figuring out what's wrong.  

Ah okay. I'm wondering if you are hitting the same problem I'm hitting over in bug 859195 with pc media flow checking mochitests. Probably something involving event handler callbacks?

Probably need to get a debugger running to figure this out.

> 
> > > +  this._pc.onconnection = function () {
> > 
> > Do we even use this event handler anymore? I thought this functionality was
> > disabled now.
> 
> That's the only event which gets thrown for me when running the datachannel
> test. So it's still around, yes. Is there any bug you think of? 

Hmm...that's weird. I thought this event handler became obsolete when we pulled the "connectDataConnection" functionality out. Randell can double check this, though.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jason Smith [:jsmith] from comment #9)
> Ah okay. I'm wondering if you are hitting the same problem I'm hitting over
> in bug 859195 with pc media flow checking mochitests. Probably something
> involving event handler callbacks?

I will spend some time now to convert the data test page to a simple mochitest. Given that this works I want to know if something is wrong when it gets run under automation. If that works, I can integrate the framework and most likely figure out if there is a bug.

> Hmm...that's weird. I thought this event handler became obsolete when we
> pulled the "connectDataConnection" functionality out. Randell can double
> check this, though.

It still exists. You can check that by running the test page: http://mozilla.github.io/webrtc-landing/data_test.html

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

onconnection is still fired, though less important now that queuing of createDataChannel()s exists.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I got closer to this issue. A stripped down version of the data channel test page from webrtc-landing is working fine, but fails once transformed into a mochitest. This is without our new framework - it's a simply copy and paste. I will continue to investigate.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: stripped down datachannel test as mochitest

This really simple mochitest fails the same way while the same code works fine in a standalone testcase.

Comment 14

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #13)
> Created attachment 739535 [details]
> stripped down datachannel test as mochitest
> 
> This really simple mochitest fails the same way while the same code works
> fine in a standalone testcase.

Hmm...interesting. Next week, I'll do the same thing with the PC media check flow test case to see if the same behavior happens.

Could this be a bug in mochitest itself potentially?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I moved out those problems to bug 863687 for now. Lets follow-up on there.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: WIP v1

First WIP of the update framework and 3 new data channel tests. It's not finished yet, but want to reach out for feedback if it looks ok to you. If that's the case I have to do code-cleanup and documentation.

There are still some TODOs listed and some bugs in Firefox I have to investigate and get filed. So please skip those parts, or if you have questions please ask. Thanks.

Comment 17

[Jason Smith [:jsmith]]

Comment on attachment 741601 [details] [diff] [review]
WIP v1

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

On the right track, although this needs some work in areas such as:

- Scope - keep this patch test-wise to focusing on the bare minimum smoke test (one mochitest)
- Decoupling of command logic
- onXXX handler workflow
- Non-determinism test behavior
- etc.

Probably is going to need another feedback pass.

::: dom/media/tests/mochitest/Makefile.in
@@ +11,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  
>  MOCHITEST_FILES = \
> +  test_dataChannel_basicDataOnly.html \
> +  test_dataChannel_multipleChannels.html \

I'd pull this out into a separate patch on a different bug.

@@ +12,5 @@
>  
>  MOCHITEST_FILES = \
> +  test_dataChannel_basicDataOnly.html \
> +  test_dataChannel_multipleChannels.html \
> +  test_dataChannel_noOffer.html \

As I mentioned on a separate patch, I still think this is inappropriate to call a data channel test. And this should not be part of the initial data channels patch.

::: dom/media/tests/mochitest/pc.js
@@ +260,5 @@
>  /**
> + * Closes the peer connection if it is active
> + */
> +PeerConnectionTest.prototype.close = function PCT_close() {
> +  if (this.connected) {

There's tests we might have that might intend to call close more than once as a negative test case. I don't think we should if blocking the close logic as a result.

@@ +332,5 @@
> + */
> +function DataChannelTest(options) {
> +  PeerConnectionTest.call(this, options);
> +
> +  this.chain.commands = commandsDataChannel;

Given that we've moved to a templates.js file, I think we should do instead here is accept a second parameter for a list of commands and use that as the commands by default. If it isn't defined, then use the default.

@@ +336,5 @@
> +  this.chain.commands = commandsDataChannel;
> +}
> +
> +DataChannelTest.prototype = Object.create(PeerConnectionTest.prototype, {
> +  close : {

Uhh...what's the point of having this method? You've already defined this in PC Test. There's nothing new be done here.

@@ +350,5 @@
> +    value : function DCT_createDataChannel(options, onSuccess, hasToWait) {
> +      hasToWait = (typeof hasToWait === "boolean") ? hasToWait : false;
> +
> +      if (hasToWait) {
> +        this.waitForDataChannelOpen(onSuccess);

This is binding two command pieces of logic together. This should be separated out.

@@ +354,5 @@
> +        this.waitForDataChannelOpen(onSuccess);
> +      }
> +
> +      var channel = this.pcLocal.createDataChannel(options);
> +      is(channel.binaryType, "blob", channel.id + " is of binary type 'blob'");

Any check-style logic really needs to be pulled out into a separate function and command per our command structure design of trying to aim for separation of test logic vs. checks.

@@ +358,5 @@
> +      is(channel.binaryType, "blob", channel.id + " is of binary type 'blob'");
> +
> +      // If we are already connected, the state will directly be 'open'
> +      var state = this.connected ? "open" : "connecting";
> +      is(channel.readyState, state, channel.id + " is in state: '" + state + "'");

The non-determinism of this test feels awkward. Can we make this more deterministic. Otherwise, we can have inconsistent test results.

@@ +372,5 @@
> +  /**
> +   *
> +   */
> +  send : {
> +    value : function DCT_send(data, onSuccess, options) {

Don't follow the naming with options.source and options.target.

@@ +375,5 @@
> +  send : {
> +    value : function DCT_send(data, onSuccess, options) {
> +      options = options || { };
> +      source = options.source || this.pcLocal.dataChannels[this.pcLocal.dataChannels.length - 1];
> +      target = options.target || this.pcRemote.dataChannels[this.pcRemote.dataChannels.length - 1];

What happens if someone by accident calls send with data channels provided for pcLocal or pcRemote?

@@ +377,5 @@
> +      options = options || { };
> +      source = options.source || this.pcLocal.dataChannels[this.pcLocal.dataChannels.length - 1];
> +      target = options.target || this.pcRemote.dataChannels[this.pcRemote.dataChannels.length - 1];
> +
> +      target.onmessage = function (dataRecv) {

Really confused on what's going on here.

Flush out what you are aiming to check here a bit more first. Then, I'll have comments on what to do here.

@@ +392,5 @@
> +
> +        onSuccess(target, data);
> +      }
> +
> +      source.send(data);

The problem I see in this command is that implementation is causing a side effect on the target data channel upon completing the method, but we shouldn't be doing that. After you finish your command, the data channel's onXXX handler should return to it's old state. Otherwise, the test could end up really strange non-deterministic behavior in some cases.

@@ +403,5 @@
> +  waitForDataChannelOpen : {
> +    value : function DCT_waitForDataChannel(onSuccess) {
> +      info("Register callbacks for 'ondatachannel' and 'onopen'");
> +      this.pcRemote.ondatachannel = function(channel) {
> +        channel.onopen = function (channel) {

Similar problem as stated above - this is causing a side effect on the data channel's onXXX handler upon exiting the command, but we shouldn't be doing that.

@@ +413,5 @@
> +
> +  /**
> +   *
> +   */
> +  waitForDataChannelOpen : {

Uh...why is this duplicated?

@@ +442,5 @@
> +  /**
> +   * Setup appropriate callbacks
> +   */
> +
> +  // TODO: We might want to use default callbacks so we can catch unexpected callbacks

That's actually probably better to do, actually. I was planning on doing that with media flow checking test.

@@ +452,5 @@
> +
> +  /**
> +   *
> +   */
> +  this._channel.onclose = function () {

This looks strange to define here. Here's what I'd expect instead:

By default, an onXXX handler should fire a test failure if the handler isn't expected to fire. If we're now expecting the onXXX handler to fire, the command in the test should set it directly temporarily, wait for it to fire, then complete the command and reset the onXXX handler to the old definition.

@@ +492,5 @@
> +  set binaryType(aType) {
> +    this._channel.binaryType = aType;
> +  },
> +
> +  get id() {

What's the purpose of having the id?

@@ +529,5 @@
>    this.constraints = [ ];
>    this.offerConstraints = {};
>    this.streams = [ ];
>  
> +  this.dataChannels = [ ];

There really needs to be a distinguishing difference between data channels the local peer has created vs. data channels captured by an ondatachannel callback.

@@ +530,5 @@
>    this.offerConstraints = {};
>    this.streams = [ ];
>  
> +  this.dataChannels = [ ];
> +  this.ondatachannel = null; // todo: might be a default function to catch unexpected events

Probably should do the default function, actually.

@@ +548,5 @@
>      self.attachMedia(event.stream, 'video', 'remote');
>    };
> +
> +
> +  this._pc.ondatachannel = function (aEvent) {

See the discussion above about order of execution with onXXX handlers. We're going to run into problems if we start placing too much in the constructor part, as the nature of the test will become hard to follow.

@@ +767,5 @@
>         this.label + ' has ' + this.constraints.length + ' local streams');
>  
> +    // TODO: change this when multiple incoming streams are supported (bug 834835)
> +    // is(this._pc.remoteStreams.length, constrainsRemote.length,
> +    //   this.label + ' has ' + constrainsRemote.length + ' remote streams');

Umm...why is this commented out?

In the context of a data only test, you should be checking that there are zero remoteStreams.

@@ +774,5 @@
>    /**
>     * Closes the connection
>     */
>    close : function PCW_close() {
> +    this._pc.close();

Don't understand why this was changed.

::: dom/media/tests/mochitest/templates.js
@@ +85,5 @@
> +
> +/**
> + * Default list of commands to execute for a Datachannel test.
> + */
> +var commandsDataChannel = [

I don't think this is what I'd call the right default list of commands. The default list of commands should really be the most basic smoke test that touches as much of the logic of data channels as possible. But this set of commands right now can't stand alone.

@@ +87,5 @@
> + * Default list of commands to execute for a Datachannel test.
> + */
> +var commandsDataChannel = [
> +  [
> +    'PC_LOCAL_GUM',

In the context of a data-only test, your goal isn't to do media logic at all. So I wouldn't expect to see the local and remote gUM logic.

@@ +103,5 @@
> +      });
> +    }
> +  ],
> +  [
> +    'PC_LOCAL_CREATE_DATA_CHANNEL',

This feels strange that there's only a single command involved here - I'd expect far more commands to be defined here. Which implies we need to decouple some of the logic.

::: dom/media/tests/mochitest/test_dataChannel_basicDataOnly.html
@@ +14,5 @@
> +    bug: "796894",
> +    title: "Basic datachannel only connection"
> +  });
> +
> +  var commands = [

The commands choice here feels strange - I don't think we should be binding a basic smoke test sending a string vs. a blob in the same smoketest.

::: dom/media/tests/mochitest/test_dataChannel_multipleChannels.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

This really isn't a smoke test of data channels feature. I'd pull this into a separate patch on a different bug.

@@ +16,5 @@
> +  });
> +
> +  var commands = [
> +    [
> +      'CREATE_DATA_CHANNEL',

Nit - I'd use a different name here.

::: dom/media/tests/mochitest/test_dataChannel_noOffer.html
@@ +11,5 @@
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    bug: "796894",
> +    title: "Multiple datachannel only connection"

This metadata doesn't look right.

@@ +27,5 @@
> +      }
> +    ]
> +  ];
> +
> +  var test = new DataChannelTest();

I generally don't agree with the design of the test file. And I don't think this should be part of this patch - it's part of a different bug where this test should be a part of.

There's zero data channel logic going on here. None at all. It's all peer connection logic as a result to change to how we generate offer SDPs without data channels calls.

In fact, I don't think this should be using the framework here at all - it's a one-off test that really should fall in line with the test_peerConnection_bugXXXXX tests - just do the callbacks directly in the test and do the m=application logic check as a result.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 741601 [details] [diff] [review]
WIP v1

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

Thanks for the feedback Jason. As I have said it's not ready yet, so lots of your comments apply to tasks I have to finish. Let me reply inline on those where you need more information.

::: dom/media/tests/mochitest/Makefile.in
@@ +11,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  
>  MOCHITEST_FILES = \
> +  test_dataChannel_basicDataOnly.html \
> +  test_dataChannel_multipleChannels.html \

That's necessary to verify that the framework is working as designed. I could pull it off to another bug but I don't see reason for this extra work.

@@ +12,5 @@
>  
>  MOCHITEST_FILES = \
> +  test_dataChannel_basicDataOnly.html \
> +  test_dataChannel_multipleChannels.html \
> +  test_dataChannel_noOffer.html \

Same as above, and yes it will not land together with the final patch on that bug but as patch on bug 856319. Any discussion about its naming I would like to defer to the other bug.

::: dom/media/tests/mochitest/pc.js
@@ +260,5 @@
>  /**
> + * Closes the peer connection if it is active
> + */
> +PeerConnectionTest.prototype.close = function PCT_close() {
> +  if (this.connected) {

I will check how this can be done best.

@@ +332,5 @@
> + */
> +function DataChannelTest(options) {
> +  PeerConnectionTest.call(this, options);
> +
> +  this.chain.commands = commandsDataChannel;

Good idea. I will get this in.

@@ +336,5 @@
> +  this.chain.commands = commandsDataChannel;
> +}
> +
> +DataChannelTest.prototype = Object.create(PeerConnectionTest.prototype, {
> +  close : {

It's not finished yet. I have to figure out how best wait for all the onclose events from data channels. I will make sure to have proper todo lines in the future.

@@ +350,5 @@
> +    value : function DCT_createDataChannel(options, onSuccess, hasToWait) {
> +      hasToWait = (typeof hasToWait === "boolean") ? hasToWait : false;
> +
> +      if (hasToWait) {
> +        this.waitForDataChannelOpen(onSuccess);

This cannot be easily separated out. If we would do that there is a high risk in missing the appropriate datachannel events when a peer connection has already been established. Here we do not have onSuccess callbacks as for setting up the peer connection. 

Situation 1: We call createDataChannel before sending the offer. Here we cannot wait given that the data channel is setup after the peer connection has been established.

Situation 2: We call createDataChannel after the peer connection has been setup. The data channel events will be sent with an unknown delay. We have to setup the handlers *before* we can call createDataChannel().

@@ +358,5 @@
> +      is(channel.binaryType, "blob", channel.id + " is of binary type 'blob'");
> +
> +      // If we are already connected, the state will directly be 'open'
> +      var state = this.connected ? "open" : "connecting";
> +      is(channel.readyState, state, channel.id + " is in state: '" + state + "'");

There are two states of the peer connection. Dependent on that we have specific states of the channel. Not sure if we can test it inside the chain. I will have to check that.

@@ +372,5 @@
> +  /**
> +   *
> +   */
> +  send : {
> +    value : function DCT_send(data, onSuccess, options) {

Would 'sourceChannel' and 'targetChannel' be better candidates?

@@ +375,5 @@
> +  send : {
> +    value : function DCT_send(data, onSuccess, options) {
> +      options = options || { };
> +      source = options.source || this.pcLocal.dataChannels[this.pcLocal.dataChannels.length - 1];
> +      target = options.target || this.pcRemote.dataChannels[this.pcRemote.dataChannels.length - 1];

Can you please explain? I don't understand this comment.

@@ +377,5 @@
> +      options = options || { };
> +      source = options.source || this.pcLocal.dataChannels[this.pcLocal.dataChannels.length - 1];
> +      target = options.target || this.pcRemote.dataChannels[this.pcRemote.dataChannels.length - 1];
> +
> +      target.onmessage = function (dataRecv) {

That's the event listener for the channel of the target peer when it receives a message sent by the other peer. It has to be setup *before* you actually send the message.

@@ +392,5 @@
> +
> +        onSuccess(target, data);
> +      }
> +
> +      source.send(data);

That's done in DataChannelWrapper. We do not directly work with the core DataChannel type here.

@@ +403,5 @@
> +  waitForDataChannelOpen : {
> +    value : function DCT_waitForDataChannel(onSuccess) {
> +      info("Register callbacks for 'ondatachannel' and 'onopen'");
> +      this.pcRemote.ondatachannel = function(channel) {
> +        channel.onopen = function (channel) {

Nope. It's getting reset. See the DataChannelWrapper implementation.

@@ +413,5 @@
> +
> +  /**
> +   *
> +   */
> +  waitForDataChannelOpen : {

Probably hit the wrong shortcut key in Pycharm which duplicates a block of code. I will remove it.

@@ +452,5 @@
> +
> +  /**
> +   *
> +   */
> +  this._channel.onclose = function () {

That's where the above mentioned default handlers would kick in. Right now we have empty function bodies but instead we will raise a failure.

@@ +492,5 @@
> +  set binaryType(aType) {
> +    this._channel.binaryType = aType;
> +  },
> +
> +  get id() {

The name of the datachannel is the same for pcLocal and pcRemote. To have an unique id you will have to combine both the pc label and the channel label.

@@ +529,5 @@
>    this.constraints = [ ];
>    this.offerConstraints = {};
>    this.streams = [ ];
>  
> +  this.dataChannels = [ ];

Why? Can you please explain? All the setup logic is done with createDataChannel. Later the same methods and properties are available on both sides.

@@ +548,5 @@
>      self.attachMedia(event.stream, 'video', 'remote');
>    };
> +
> +
> +  this._pc.ondatachannel = function (aEvent) {

We cannot put this code onto the prototype given that this will not be available at this time. I get a failure all the time. The only option would be global functions in this module, but then we do no longer have access to class members.

@@ +767,5 @@
>         this.label + ' has ' + this.constraints.length + ' local streams');
>  
> +    // TODO: change this when multiple incoming streams are supported (bug 834835)
> +    // is(this._pc.remoteStreams.length, constrainsRemote.length,
> +    //   this.label + ' has ' + constrainsRemote.length + ' remote streams');

I had to comment this out because it was simply not working. Now that bug 834835 has been fixed we can enable the checks again. Actually this code should have been updated with the patch on bug 834835. :/

::: dom/media/tests/mochitest/templates.js
@@ +87,5 @@
> + * Default list of commands to execute for a Datachannel test.
> + */
> +var commandsDataChannel = [
> +  [
> +    'PC_LOCAL_GUM',

We are in the framework here and not in the data-only test. If you want to totally clean separation between the test and the framework implementation we would have to file a separate bug for the latter.

@@ +103,5 @@
> +      });
> +    }
> +  ],
> +  [
> +    'PC_LOCAL_CREATE_DATA_CHANNEL',

No, we simply have to call createDataChannel() and that's all at this point.

::: dom/media/tests/mochitest/test_dataChannel_basicDataOnly.html
@@ +14,5 @@
> +    bug: "796894",
> +    title: "Basic datachannel only connection"
> +  });
> +
> +  var commands = [

This is for testing the implementation and it will be removed for the final patch.

::: dom/media/tests/mochitest/test_dataChannel_noOffer.html
@@ +27,5 @@
> +      }
> +    ]
> +  ];
> +
> +  var test = new DataChannelTest();

Most likely yes.

Comment 19

[Jason Smith [:jsmith]]

Comment on attachment 741601 [details] [diff] [review]
WIP v1

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

::: dom/media/tests/mochitest/pc.js
@@ +350,5 @@
> +    value : function DCT_createDataChannel(options, onSuccess, hasToWait) {
> +      hasToWait = (typeof hasToWait === "boolean") ? hasToWait : false;
> +
> +      if (hasToWait) {
> +        this.waitForDataChannelOpen(onSuccess);

The distinct nature of the situations stated above might argue that we might want to decouple those situations into separate commands. That might address my decoupling concern above.

@@ +358,5 @@
> +      is(channel.binaryType, "blob", channel.id + " is of binary type 'blob'");
> +
> +      // If we are already connected, the state will directly be 'open'
> +      var state = this.connected ? "open" : "connecting";
> +      is(channel.readyState, state, channel.id + " is in state: '" + state + "'");

Hmm...maybe the right way to resolve some of the coupling and determinism is similar to my comment above - if we have cases where createDataChannel has different results depending on when it's called, we might want to use separate commands and resulting checks as a result. That will reduce the complexity here.

@@ +372,5 @@
> +  /**
> +   *
> +   */
> +  send : {
> +    value : function DCT_send(data, onSuccess, options) {

Yeah, that works.

@@ +375,5 @@
> +  send : {
> +    value : function DCT_send(data, onSuccess, options) {
> +      options = options || { };
> +      source = options.source || this.pcLocal.dataChannels[this.pcLocal.dataChannels.length - 1];
> +      target = options.target || this.pcRemote.dataChannels[this.pcRemote.dataChannels.length - 1];

I think what I meant to say here is if pcLocal.dataChannels or pcRemote.dataChannels has a length of zero. You'll go out of bounds in that case.

@@ +377,5 @@
> +      options = options || { };
> +      source = options.source || this.pcLocal.dataChannels[this.pcLocal.dataChannels.length - 1];
> +      target = options.target || this.pcRemote.dataChannels[this.pcRemote.dataChannels.length - 1];
> +
> +      target.onmessage = function (dataRecv) {

The below discussion states that logic should be happening DataChannelWrapper though.

@@ +403,5 @@
> +  waitForDataChannelOpen : {
> +    value : function DCT_waitForDataChannel(onSuccess) {
> +      info("Register callbacks for 'ondatachannel' and 'onopen'");
> +      this.pcRemote.ondatachannel = function(channel) {
> +        channel.onopen = function (channel) {

I think I understand where I'm getting confused here. Some of the logic here is happening as part of DataChannelTest for setting the onXXX handlers, but should be encapsulated in the wrapper to help with cohesion.

@@ +452,5 @@
> +
> +  /**
> +   *
> +   */
> +  this._channel.onclose = function () {

True, but that would only allow to run this test once. And this handler is set to be turned on by default, when in reality, I'd only the turn on the handler when we are expecting the onXXX handler to fire.

@@ +529,5 @@
>    this.constraints = [ ];
>    this.offerConstraints = {};
>    this.streams = [ ];
>  
> +  this.dataChannels = [ ];

I think because I'd want the ability to query the local representation of the data channels differently than the remote representation of the data channels.

@@ +548,5 @@
>      self.attachMedia(event.stream, 'video', 'remote');
>    };
> +
> +
> +  this._pc.ondatachannel = function (aEvent) {

I'm confused by the above statement.

The way you could resolve this would follow something similar to what I'm doing in the media flow tests:

Set the ondatachannel event handler to the correct handler when we expect it to fire after the correct function is called. Otherwise, the default failure handler is used.

::: dom/media/tests/mochitest/templates.js
@@ +103,5 @@
> +      });
> +    }
> +  ],
> +  [
> +    'PC_LOCAL_CREATE_DATA_CHANNEL',

I think what I'm talking about here is the decoupling of logic mentioned in the other file. That might resolve the concern I'm talking about here, which may or may not result in additional commands potentially.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 741601 [details] [diff] [review]
WIP v1

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

Jason, please see my comments inline. If you still have questions lets really have a call so I can explain the difficulties.

Given that I will most likely not get feedback from Eric, I will upload a new version of the patch by today.

::: dom/media/tests/mochitest/pc.js
@@ +350,5 @@
> +    value : function DCT_createDataChannel(options, onSuccess, hasToWait) {
> +      hasToWait = (typeof hasToWait === "boolean") ? hasToWait : false;
> +
> +      if (hasToWait) {
> +        this.waitForDataChannelOpen(onSuccess);

As I have said above it's not that easy to separate them into their own commands. The events don't allow us to do that without having to add a lot more code to sync with the commands.

Also the above covers the default path for our data channel tests. If one or two of upcoming tests want to test this in more detail, those should handle those special cases.

@@ +358,5 @@
> +      is(channel.binaryType, "blob", channel.id + " is of binary type 'blob'");
> +
> +      // If we are already connected, the state will directly be 'open'
> +      var state = this.connected ? "open" : "connecting";
> +      is(channel.readyState, state, channel.id + " is in state: '" + state + "'");

I moved this out to the command blocks, so that we don't have to care about the state in the framework.

@@ +375,5 @@
> +  send : {
> +    value : function DCT_send(data, onSuccess, options) {
> +      options = options || { };
> +      source = options.source || this.pcLocal.dataChannels[this.pcLocal.dataChannels.length - 1];
> +      target = options.target || this.pcRemote.dataChannels[this.pcRemote.dataChannels.length - 1];

We will always have a valid data channel after the peer connection has been established. If not, it's a failure in the framework. We could add an if condition and a is() check but why caring about when we can do a check that way?

If a test has no data channel on purpose, it can also catch the exception and handle it appropriately by making the test not fail. If we would have caught the non existent data channel case with if() that would not be possible.

@@ +377,5 @@
> +      options = options || { };
> +      source = options.source || this.pcLocal.dataChannels[this.pcLocal.dataChannels.length - 1];
> +      target = options.target || this.pcRemote.dataChannels[this.pcRemote.dataChannels.length - 1];
> +
> +      target.onmessage = function (dataRecv) {

No, the DataChannelWrapper will have default handlers for the possible events only which will raise failures if they are called. Here we overwrite such a handler, because it will be valid, and we have to handle it here inside this method.

@@ +403,5 @@
> +  waitForDataChannelOpen : {
> +    value : function DCT_waitForDataChannel(onSuccess) {
> +      info("Register callbacks for 'ondatachannel' and 'onopen'");
> +      this.pcRemote.ondatachannel = function(channel) {
> +        channel.onopen = function (channel) {

No, but you answered yourself already below for the 'this._channel.onclose = function () {' case.

@@ +452,5 @@
> +
> +  /**
> +   *
> +   */
> +  this._channel.onclose = function () {

Why only once? All those event handlers will run as long as the data channel exists. Only when we expect it to get fired we update the appropriate callback like self.onclose() for the wrapper. Otherwise it will raise a failure.

@@ +529,5 @@
>    this.constraints = [ ];
>    this.offerConstraints = {};
>    this.streams = [ ];
>  
> +  this.dataChannels = [ ];

As far as I have seen both sides are totally equal after their creation. There is nothing special you could do with one of those sides. Everything happens in the createDataChannel() call.

@@ +548,5 @@
>      self.attachMedia(event.stream, 'video', 'remote');
>    };
> +
> +
> +  this._pc.ondatachannel = function (aEvent) {

We want to raise a failure if an event gets called when we do not expect it to be called. So we cannot move this code away. Also any custom ondatachannel handler on the DataChannelTest class should get a DataChannelWrapper and not an nsIDataChannel instance.

Comment 21

[Jason Smith [:jsmith]]

I think at this point, just build another followup patch I'll take a look. I think this might make sense on a 2nd pass.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: WIP v2

This patch modifies a lot what has been discussed since the last feedback cycle. So I want to start another feedback cycle. Sadly it breaks the peer connection tests, which I haven't been able to finish investigating yet. I just wanted to put the patch up so we can have a quicker process.

Also this patch introduces another intermittent leak on shutdown which I still have to investigate.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 750174 [details] [diff] [review]
WIP v2

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

::: dom/media/tests/mochitest/pc.js
@@ +822,5 @@
>         this.label + ' has ' + this.constraints.length + ' local streams');
>  
> +    // TODO: change this when multiple incoming streams are supported (bug 834835)
> +    // is(this._pc.remoteStreams.length, constrainsRemote.length,
> +    //   this.label + ' has ' + constrainsRemote.length + ' remote streams');

Just a note here... we cannot test this if no streams have been added, as what this test stands for. Means this code should have been updated when bug 834835 got fixed. I would like to care about it in a follow-up bug soon.

Comment 24

[Jason Smith [:jsmith]]

Comment on attachment 750174 [details] [diff] [review]
WIP v2

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

Looks good. Curious to more about that comment you just added though.

::: dom/media/tests/mochitest/pc.js
@@ +368,5 @@
> +
> +  closeDataChannel : {
> +    /**
> +     * Close the specified data channel
> +     */

Minor Nit - the commenting style feels strange. Couldn't we just do this before the start of the definition (aka before closeDataChannel here)?

@@ +409,5 @@
> +        // Setup event handlers to handle the new connection
> +        this.waitForDataChannelOpen(this.pcLocal, this.pcRemote, onSuccess);
> +      }
> +
> +      if (!hasToWait) {

Small nit - we don't need two if statements here. Just do an if and else statement:

if (hasToWait) {
   this.waitForDataChannelOpen...
} else {
   onSuccess(channel);
}

@@ +822,5 @@
>         this.label + ' has ' + this.constraints.length + ' local streams');
>  
> +    // TODO: change this when multiple incoming streams are supported (bug 834835)
> +    // is(this._pc.remoteStreams.length, constrainsRemote.length,
> +    //   this.label + ' has ' + constrainsRemote.length + ' remote streams');

Why? I don't follow.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #22)
> Also this patch introduces another intermittent leak on shutdown which I
> still have to investigate.

I have updated to tip and now I can no longer reproduce this leak. Looks like something has been fixed here or it's a bit harder to get it reproduced.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 750174 [details] [diff] [review]
WIP v2

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

::: dom/media/tests/mochitest/pc.js
@@ +368,5 @@
> +
> +  closeDataChannel : {
> +    /**
> +     * Close the specified data channel
> +     */

That will most likely not work, at least PyCharm blames about it. Given that closeDataChannel can have multiple sub-properties it would also be confusing. So I think this is correct.

@@ +409,5 @@
> +        // Setup event handlers to handle the new connection
> +        this.waitForDataChannelOpen(this.pcLocal, this.pcRemote, onSuccess);
> +      }
> +
> +      if (!hasToWait) {

Oh, yes. In the last patch I had it before the createDataChannel() but that didn't allow us to setup the handlers correctly. I'm not 100% certain this is the correct way but I will do some extensive testing once the patch is ready.

@@ +822,5 @@
>         this.label + ' has ' + this.constraints.length + ' local streams');
>  
> +    // TODO: change this when multiple incoming streams are supported (bug 834835)
> +    // is(this._pc.remoteStreams.length, constrainsRemote.length,
> +    //   this.label + ' has ' + constrainsRemote.length + ' remote streams');

data channel only without audio and video will still fail. I will re-check if I can fix it. Otherwise I will leave the current code and comment it out.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #25) 
> I have updated to tip and now I can no longer reproduce this leak. Looks
> like something has been fixed here or it's a bit harder to get it reproduced.

Got it reproduced. I filed bug 872978.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jason Smith [:jsmith] from comment #24)
> Comment on attachment 750174 [details] [diff] [review]
> >         this.label + ' has ' + this.constraints.length + ' local streams');
> >  
> > +    // TODO: change this when multiple incoming streams are supported (bug 834835)
> > +    // is(this._pc.remoteStreams.length, constrainsRemote.length,
> > +    //   this.label + ' has ' + constrainsRemote.length + ' remote streams');
> 
> Why? I don't follow.

I talked with Eric on IRC and we will keep this commented out for now. I filed bug 873049 for it.

Comment 29

[Jason Smith [:jsmith]]

Comment on attachment 750174 [details] [diff] [review]
WIP v2

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

::: dom/media/tests/mochitest/pc.js
@@ +368,5 @@
> +
> +  closeDataChannel : {
> +    /**
> +     * Close the specified data channel
> +     */

Technically that would goes against the style in dom/media/tests/mochitest. And I don't think we're intending to have multiple sub-properties for these functions - that's how the inheritance chain is setup. 

I'd rather stick with a consistent style here - which places the comments before as seen in the media stream playback JS file. A code editor shouldn't be the dictator of the style of the files involved.

@@ +822,5 @@
>         this.label + ' has ' + this.constraints.length + ' local streams');
>  
> +    // TODO: change this when multiple incoming streams are supported (bug 834835)
> +    // is(this._pc.remoteStreams.length, constrainsRemote.length,
> +    //   this.label + ' has ' + constrainsRemote.length + ' remote streams');

I don't think we should be commenting things out for all possible mochitests. That's going to cause us to lose coverage. I'd rather just disable this check for the specific test that this won't work with.

I still don't understand however what's failing here. remoteStreams should be zero here. What value are you getting?

Comment 31

[Jason Smith [:jsmith]]

Oh. I see what you are trying to do here. Here's a simple fix to the problem:

- Have checkMedia take in a 2nd parameter for expectedRemoteStreams
- Change the remoteStreams check to use that number instead
- The default command that calls into checkMedia uses 1 for peer connection commands
- Have two different sets of default commands for data channel
** The first is more no media streams involved - checkMedia uses 0 in this case
** The second is for when media streams are involved - checkMedia uses 1 in this case

Comment 33

[Jason Smith [:jsmith]]

As I stated in the other bug, there's many ways to work around this problem. I don't think fixing this in a followup is an option.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jason Smith [:jsmith] from comment #29)
> > +  closeDataChannel : {
> > +    /**
> > +     * Close the specified data channel
> > +     */
> 
> Technically that would goes against the style in dom/media/tests/mochitest.
> And I don't think we're intending to have multiple sub-properties for these
> functions - that's how the inheritance chain is setup. 

I think you misunderstand how this works. closeDataChannel is setup as property here but the underlying implementation is bound to the 'value' sub property. You cannot create the jsdoc for the outer property.

> I'd rather stick with a consistent style here - which places the comments
> before as seen in the media stream playback JS file.

Then this was most likely a failure. I will check locally how jsdoc-toolkit generates the documentation. Then we will 100% sure, which way is the correct one.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

This version fixes the breakage with the peerconnection tests in the close method. It also disables the media checks for those data channel tests which do not request any kind of media. We will fix that on bug 873049. I have also added all necessary jsdoc comments.

Jan also have checked the new webidl implementation with the WIP v2 patch and everything looks fine. We weren't able to find any regression for a local run.

I pushed the current version of the patch to try:
https://tbpl.mozilla.org/?tree=Try&rev=fb2864809c6b

Comment 36

[Jason Smith [:jsmith]]

Comment on attachment 750503 [details] [diff] [review]
Patch v1

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

Technically jesup has explicitly given you and I peer access to grant review on tests, so I can give reviews on these.

Looks fine on my end. I think the followup as cited is fine along with the way we have implemented the removal of the checks for now.

We probably want either review+ from either jesup or ekr to be able to land this.

Comment 37

[Jason Smith [:jsmith]]

Comment on attachment 750503 [details] [diff] [review]
Patch v1

Actually, abr could do the review here also.

So either abr, jesup, or ekr needs to give a r+ here.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It looks like that we cannot land this patch due to the intermittent leak I have experienced and filed as bug 872978.

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The tryserver shows some failures, leaks, and even a crash. Looks like some more work is necessary here.

Ubuntu64 opt:
11:58:07 INFO - 18353 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_dataChannel_basicVideo.html | Unexpected event 'onopen' fired for pcLocal_channel_0 @http://mochi.test:8888/tests/dom/media/tests/mochitest/pc.js:558
11:58:07 INFO - 18356 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_dataChannel_basicVideo.html | Unexpected event 'ondatachannel' fired for pcRemote @http://mochi.test:8888/tests/dom/media/tests/mochitest/pc.js:686

That's because i moved down the waitForDataChannelOpen() call in createDataChannel() and we register the custom handlers too late. I will move it back to the place it was before and have to figure out how to correctly setup the handlers.

OSX 10.6 opt (same as above plus crash):
https://tbpl.mozilla.org/php/getParsedLog.php?id=23042132&tree=Try

11:58:11  WARNING -  TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_dataChannel_multipleChannels.html | Exited with code 1 during test run
11:58:49  WARNING -  PROCESS-CRASH | /tests/dom/media/tests/mochitest/test_dataChannel_multipleChannels.html | application crashed [@ 0x7fffffe001a0]

11:58:49     INFO -   0  0x7fffffe001a0
11:58:49     INFO -      rbx = 0x00000001326d9700   r12 = 0x000000013259ebe0
11:58:49     INFO -      r13 = 0x0000000000000630   r14 = 0x000000000000015c
11:58:49     INFO -      r15 = 0x0000000000000154   rip = 0x00007fffffe001a0
11:58:49     INFO -      rsp = 0x0000000106f7d900   rbp = 0x0000000106f7d9f0
11:58:49     INFO -      Found by: given as instruction pointer in context
11:58:49     INFO -   1  libSystem.B.dylib + 0x617d
11:58:49     INFO -      rip = 0x00007fff8816e17e   rsp = 0x0000000106f7d908
11:58:49     INFO -      rbp = 0x0000000106f7d9f0
11:58:49     INFO -      Found by: stack scanning
11:58:49     INFO -   2  XUL!sctp_add_to_readq [sctputil.c:fb2864809c6b : 4747 + 0x19]
11:58:49     INFO -      rip = 0x00000001011e434c   rsp = 0x0000000106f7d910
11:58:49     INFO -      rbp = 0x0000000106f7d9f0
11:58:49     INFO -      Found by: stack scanning
11:58:49     INFO -   3  libmozglue.dylib!je_malloc [jemalloc.c:fb2864809c6b : 4247 + 0xc]
11:58:49     INFO -      rip = 0x000000010000e19f   rsp = 0x0000000106f7d930
11:58:49     INFO -      rbp = 0x0000000106f7d9f0
11:58:49     INFO -      Found by: stack scanning
11:58:49     INFO -   4  libSystem.B.dylib + 0x2c87
11:58:49     INFO -      rip = 0x00007fff8816ac88   rsp = 0x0000000106f7d990
11:58:49     INFO -      rbp = 0x0000000106f7d9f0
11:58:49     INFO -      Found by: stack scanning
11:58:49     INFO -   5  libSystem.B.dylib + 0x617d
11:58:49     INFO -      rip = 0x00007fff8816e17e   rsp = 0x0000000106f7d9a8
11:58:49     INFO -      rbp = 0x0000000106f7d9f0
11:58:49     INFO -      Found by: stack scanning
11:58:49     INFO -   6  XUL!sctp_build_readq_entry [sctp_indata.c:fb2864809c6b : 147 + 0x7]
11:58:49     INFO -      rip = 0x000000010119ea19   rsp = 0x0000000106f7d9b0
11:58:49     INFO -      rbp = 0x0000000106f7d9f0
11:58:49     INFO -      Found by: stack scanning

OSX 10.6 debug:
The leak as reported earlier.

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 750503 [details] [diff] [review]
Patch v1

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

I still would like to get feedback from Eric, even that I have to come up with another version of the patch. Would be good to hear if he agrees with the other code or if he has suggestions.

::: dom/media/tests/mochitest/pc.js
@@ +420,5 @@
> +
> +      var channel = this.pcLocal.createDataChannel(options);
> +
> +      if (hasToWait) {
> +        this.waitForDataChannelOpen(this.pcLocal, this.pcRemote, onSuccess);

This is causing us trouble. We really have to register the handlers BEFORE we call createDataChannel. Otherwise we get intermittent failures for unexpected ondatachannel and onopen events.

Comment 41

[Eric Rescorla (:ekr)]

Comment on attachment 750503 [details] [diff] [review]
Patch v1

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

This looks overall pretty good, though there are probably enough
issues that I would r- it if this were r? rather than f?.


You should get jesup's review for the DC semantics.

I wonder if we need more tests for the actual channel semantics.
Really big objects, fast sending, etc.

::: dom/media/tests/mochitest/head.js
@@ +140,5 @@
> + */
> +function getBlobContent(blob, onSuccess) {
> +  var reader = new FileReader();
> +
> +  reader.onload = function (event) {

Should we be also be registering for errors?

::: dom/media/tests/mochitest/pc.js
@@ +358,5 @@
> +     */
> +    value : function DCT_close(onSuccess) {
> +      var self = this;
> +
> +      function _closeChannels() {

Is there a way to make this structure clearer (i.e., non-recursive).

@@ +362,5 @@
> +      function _closeChannels() {
> +        var length = self.pcLocal.dataChannels.length;
> +
> +        if (length > 0) {
> +          self.closeDataChannel(length - 1, function () {

Does this have a failure callback? Or do things just time out?

@@ +415,5 @@
> +     *        has been established. This should only be false for the very first
> +     *        channel.
> +     */
> +    value : function DCT_createDataChannel(options, onSuccess, hasToWait) {
> +      hasToWait = (typeof hasToWait === "boolean") ? hasToWait : true;

How would it be not-boolean and yet true?

@@ +420,5 @@
> +
> +      var channel = this.pcLocal.createDataChannel(options);
> +
> +      if (hasToWait) {
> +        this.waitForDataChannelOpen(this.pcLocal, this.pcRemote, onSuccess);

Do we need a failure cb?

@@ +429,5 @@
> +
> +    }
> +  },
> +
> +  send : {

We could probably use a more evocative name:

sendData()?

@@ +452,5 @@
> +      target = options.targetChannel ||
> +               this.pcRemote.dataChannels[this.pcRemote.dataChannels.length - 1];
> +
> +      // Register event handler for the target channel
> +      target.onmessage = function (recv_data) {

Shouldn't this be registered at data channel setup?

@@ +482,5 @@
> +      //   1) source.channel -> onopen
> +      //   2) target -> ondatachannel
> +      //   3) target.channel -> onopen
> +
> +      sourceChannel.onopen = function () {

Wouldn't it make more sense to register these ahead of itme?

@@ +809,5 @@
> +    var label = 'channel_' + this.dataChannels.length;
> +    info("Create data channel '" + label + "' for " + this.label);
> +
> +    var channel = this._pc.createDataChannel(label, options);
> +    var wrapper = new DataChannelWrapper(channel, this);

Would it make more sense for createDataChannel to return a DCW?

@@ +906,5 @@
>      try {
>        this._pc.close();
>        info(this.label + ": Closed connection.");
> +    }
> +    catch (e) {

Isn't } else { clearer?

::: dom/media/tests/mochitest/templates.js
@@ +1,1 @@
> +/**

I have not double-checked these. I will assume that they came directly from pc.js without error.

::: dom/media/tests/mochitest/test_dataChannel_basicAudio.html
@@ +18,5 @@
> +  var test;
> +  runTest(function () {
> +    test = new DataChannelTest();
> +    test.setMediaConstraints([{audio: true}], [{audio: true}]);
> +    test.run();

What does this test? It looks like just setup, not data xfer?

::: dom/media/tests/mochitest/test_dataChannel_sendBlob.html
@@ +10,5 @@
> +<body>
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    bug: "XYZ",

This is missing  bug #.

@@ +14,5 @@
> +    bug: "XYZ",
> +    title: "Basic blob data channel message"
> +  });
> +
> +  var commands = [

Can we put this in template?

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 750503 [details] [diff] [review]
Patch v1

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

Thanks for your feedback Eric. See my comments inline. Also for additional tests those will come in with follow-up bugs. This one is just for the basic tests.

::: dom/media/tests/mochitest/head.js
@@ +140,5 @@
> + */
> +function getBlobContent(blob, onSuccess) {
> +  var reader = new FileReader();
> +
> +  reader.onload = function (event) {

Might be a good idea. I will check that.

::: dom/media/tests/mochitest/pc.js
@@ +358,5 @@
> +     */
> +    value : function DCT_close(onSuccess) {
> +      var self = this;
> +
> +      function _closeChannels() {

With all the events we have to wait for it will not be possible. It's equivalent but simpler than the getAllUserMedia() implementation.

@@ +362,5 @@
> +      function _closeChannels() {
> +        var length = self.pcLocal.dataChannels.length;
> +
> +        if (length > 0) {
> +          self.closeDataChannel(length - 1, function () {

Compared to the basic gUM and PeerConnection methods we do have events for datachannels. No onSuccess or onError callbacks can be registered. That means if events are not triggered by the given action we run into a timeout.

@@ +415,5 @@
> +     *        has been established. This should only be false for the very first
> +     *        channel.
> +     */
> +    value : function DCT_createDataChannel(options, onSuccess, hasToWait) {
> +      hasToWait = (typeof hasToWait === "boolean") ? hasToWait : true;

If the parameter has not been specified it will be 'undefined'. Given that hasToWait should be a boolean, we set true by default. But as I can see now, I missed to note that this parameter is optional in the jsdoc comment. I will add it for the next version of the patch.

@@ +420,5 @@
> +
> +      var channel = this.pcLocal.createDataChannel(options);
> +
> +      if (hasToWait) {
> +        this.waitForDataChannelOpen(this.pcLocal, this.pcRemote, onSuccess);

No, same as mentioned before. If the appropriate event is not triggered we timeout.

@@ +429,5 @@
> +
> +    }
> +  },
> +
> +  send : {

Sure. I will fix that.

@@ +452,5 @@
> +      target = options.targetChannel ||
> +               this.pcRemote.dataChannels[this.pcRemote.dataChannels.length - 1];
> +
> +      // Register event handler for the target channel
> +      target.onmessage = function (recv_data) {

No, if events are unexpectedly fired we want to register that as a failure. That's why the real event handler, which is located in DataChannelWrapper, set's up the failure callback.

@@ +482,5 @@
> +      //   1) source.channel -> onopen
> +      //   2) target -> ondatachannel
> +      //   3) target.channel -> onopen
> +
> +      sourceChannel.onopen = function () {

Yes. See my comment above in the createDataChannel() method. That's something I have to revert to the state I had before, when we registered those events before we called _pc.createDataChannel().

@@ +809,5 @@
> +    var label = 'channel_' + this.dataChannels.length;
> +    info("Create data channel '" + label + "' for " + this.label);
> +
> +    var channel = this._pc.createDataChannel(label, options);
> +    var wrapper = new DataChannelWrapper(channel, this);

I don't understand. That's exactly what this method does. Or do I miss something?

@@ +906,5 @@
>      try {
>        this._pc.close();
>        info(this.label + ": Closed connection.");
> +    }
> +    catch (e) {

I have never seen a try / else construct without catch. What should that be?

::: dom/media/tests/mochitest/templates.js
@@ +1,1 @@
> +/**

Ỳes, a pure copy and paste.

::: dom/media/tests/mochitest/test_dataChannel_basicAudio.html
@@ +18,5 @@
> +  var test;
> +  runTest(function () {
> +    test = new DataChannelTest();
> +    test.setMediaConstraints([{audio: true}], [{audio: true}]);
> +    test.run();

A default message is send through the channel by default. See the template file for a data channel setup. If we should get it removed from there and placed in all the individual tests, please let me know.

::: dom/media/tests/mochitest/test_dataChannel_sendBlob.html
@@ +10,5 @@
> +<body>
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    bug: "XYZ",

Oh, right. I will have to check if we already have a bug for it. If not I will take this one.

@@ +14,5 @@
> +    bug: "XYZ",
> +    title: "Basic blob data channel message"
> +  });
> +
> +  var commands = [

That depends on where we want to have the checks for sending data. In the template where those are getting executed for each and every test or in the test itself. What would you prefer?

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Still waiting for Eric's feedback regarding my last comment.

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 750503 [details] [diff] [review]
Patch v1

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

::: dom/media/tests/mochitest/pc.js
@@ +429,5 @@
> +
> +    }
> +  },
> +
> +  send : {

I thought more about it and I would not change its name. Right now it has the same name as the method of the underlying peer connection object. We could use sendData but it might harder to associate. What do you think?

Comment 45

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2

Updated version of the patch with all of the formerly proposed changes.

https://tbpl.mozilla.org/?tree=Try&rev=0c95ea692e9b

We are now stable in setting up the data channel during the peering phase and later. But sadly we still have issues on OS X:

00:44:50 INFO - 18199 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_dataChannel_basicAudio.html | The local and remote peers are connected

00:45:57 INFO - 18498 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_dataChannel_basicVideo.html | Test timed out.

Those failures only happen in opt builds but not debug builds. I have to further investigate those two issues. That's why no review request for now.

Comment 46

[Henrik Skupin [:whimboo][⌚️UTC+1]]

One more failure for win7 debug:

01:10:26 INFO - 18242 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_dataChannel_basicAudio.html | Channel is in state: 'closed' - got open, expected closed

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #45)
> 00:44:50 INFO - 18199 ERROR TEST-UNEXPECTED-FAIL |
> /tests/dom/media/tests/mochitest/test_dataChannel_basicAudio.html | The
> local and remote peers are connected

This is actually bug 875346. I will remove those checks for now.

Comment 48

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #45)
> Created attachment 753153 [details] [diff] [review]
> Patch v2
> 
> Updated version of the patch with all of the formerly proposed changes.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=0c95ea692e9b
> 
> We are now stable in setting up the data channel during the peering phase
> and later. But sadly we still have issues on OS X:
> 
> 00:44:50 INFO - 18199 ERROR TEST-UNEXPECTED-FAIL |
> /tests/dom/media/tests/mochitest/test_dataChannel_basicAudio.html | The
> local and remote peers are connected

This one I should have fixed now by introducing a helper function which checks the state of all 3 asynchronous callbacks which happen. When all of them happened, it will return.

> 00:45:57 INFO - 18498 ERROR TEST-UNEXPECTED-FAIL |
> /tests/dom/media/tests/mochitest/test_dataChannel_basicVideo.html | Test
> timed out.

This one seems to be a recursion in WebrtcVideoSessionConduit. It seems to end-up in a loop and never fires the requested event. Roc, do you know?

 0:40.90 1196398336[7fada0190a70]: [ViECaptureThrea|WebrtcVideoSessionConduit] VideoConduit.cpp:656: SendPacket Channel 0, len 862
 0:40.90 -342624512[1691130]: [Socket Thread|WebrtcVideoSessionConduit] VideoConduit.cpp:603: ReceivedRTPPacket: Channel 0, Len 862
 0:40.90 -2134907136[2509590]: [firefox-bin|WebrtcVideoSessionConduit] VideoConduit.cpp:595: SendVideoFrame Inserted A Frame
 0:40.90 -2134907136[2509590]: [firefox-bin|WebrtcVideoSessionConduit] VideoConduit.cpp:595: SendVideoFrame Inserted A Frame
 0:40.91 -2145425664[7fad84000980]: [ViECaptureThrea|WebrtcVideoSessionConduit] VideoConduit.cpp:656: SendPacket Channel 0, len 862
 0:40.91 -342624512[1691130]: [Socket Thread|WebrtcVideoSessionConduit] VideoConduit.cpp:603: ReceivedRTPPacket: Channel 0, Len 862
 0
[...]

I will further investigate once I fixed the known issues in the tests.

Comment 49

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: NSPR log (datachannel:5)

This NSPR log shows the datachannel activity until test_dataChannel_basicVideo.html, where the test hangs during shutdown of the datachannels. I can see that something is wrong there but I would kindly like Randells eyes on it. He might know what's going on. At this point we are destroying two datachannels right after each other.

I will try to get it reproduced with signaling:5 too, if that gives even more information.

Comment 50

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: NSPR log (datachannel:5)

Sorry, I uploaded the wrong log file. That's the right one now.

Comment 51

[Jason Smith [:jsmith]]

Comment on attachment 753783 [details]
NSPR log (datachannel:5)

This isn't an appropriate use of the feedback flag. Clearing.

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Henrik Skupin (:whimboo) from comment #48)
> > 00:45:57 INFO - 18498 ERROR TEST-UNEXPECTED-FAIL |
> > /tests/dom/media/tests/mochitest/test_dataChannel_basicVideo.html | Test
> > timed out.
> 
> This one seems to be a recursion in WebrtcVideoSessionConduit. It seems to
> end-up in a loop and never fires the requested event. Roc, do you know?

Not off the top of my head...

Comment 53

[Randell Jesup [:jesup] (needinfo me)]

Well, what's likely going on is that something isn't telling it to shut down - when "in a call" it will produce sequences like this - just means it's active.  The question is why isn't the call ending.  That require more analysis and in particular signaling analysis (and mochitests have signaling logging by default; no change needed there).  There are already-filed intermittents on the "doesn't shut down the call" bugs.

Comment 54

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 753783 [details]
NSPR log (datachannel:5)

***failed: setsockopt RESET, errno 114

114 is EALREADY

The patches I've been running on Try should I think plug a leak that could be triggered by this error (you can only have one outstanding Close (or set of Close()es) at a time, and we weren't retrying once the current one finished.

This isn't the only source of leaks however; I'm running a new Try after more cleanup of the shutdown/Close() code

Comment 55

[Henrik Skupin [:whimboo][⌚️UTC+1]]

A helpful log from a tryserver run might be this one:
https://tbpl.mozilla.org/php/getParsedLog.php?id=23306747&tree=Try&full=1#error0

Would this help or is a log with datachannel:5, and signaling:5 preferred?

I will recheck the close methods for datachannel, but I'm sure that we close them sequentially in the tests. There is no overlap.

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: NSPR log for datachannel onclose (datachannel:5,signaling:5)

I was able to get this one more time, and now with both datachannel and signaling log information.

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v3

With the last update of the patch I only see the known leak and an intermittent hang in test_dataChannel_basicAudioVideo.html (https://tbpl.mozilla.org/php/getParsedLog.php?id=23365237&tree=Try&full=1), which I have already mentioned before. But I can't see that the tests are responsible for. Please let me know how you want to continue on that. Shall I file a new bug for it or do we already have a similar intermittent open bug for peerconnection_basicAudioVideo?

Here the try server results on all platforms:
https://tbpl.mozilla.org/?tree=Try&rev=1c8e776607a5

Comment 58

[Jason Smith [:jsmith]]

Comment on attachment 753935 [details] [diff] [review]
Patch v3

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

review- for some cleanup work needed. See comments below.

::: dom/media/tests/mochitest/head.js
@@ +215,5 @@
> +
> +/**
> + * Generates a callback function fired only for unexpected events happening.
> + *
> + * @param {String} description

Nit - Fixed this in the media flow checking tests, but there's missing parameter definition here.

::: dom/media/tests/mochitest/pc.js
@@ +275,5 @@
> +  this.pcLocal.close();
> +  this.pcRemote.close();
> +  this.connected = false;
> +
> +  onSuccess();

Why is an async callback needed here? Why can't this be synchronous?

@@ +392,5 @@
> +      var self = this;
> +
> +      // Register handler for remote channel, cause we have to wait until
> +      // the current close operation has been finished.
> +      remoteChannel.onclose = function () {

Nit - I'd add an ok(true) test to indicate that you've successfully received the onclose event as you expected.

@@ +398,5 @@
> +
> +        onSuccess(remoteChannel);
> +      };
> +
> +      localChannel.close();

Nit - Add an info statement here to indicate that you are closing the local channel.

@@ +412,5 @@
> +     *        Options for the data channel (see nsIPeerConnection)
> +     * @param {Function} onSuccess
> +     *        Callback when the creation was successful
> +     */
> +    value : function DCT_createDataChannel(options, onSuccess) {

The indirection in this function can be a bit hard to follow + making use of common logic found in the template. Can you remove the code duplication here and reduce the indirection?

@@ +423,5 @@
> +          is(localChannel.readyState, "open", localChannel.id + " is in state: 'open'");
> +          is(remoteChannel.readyState, "open", remoteChannel.id + " is in state: 'open'");
> +
> +          onSuccess(localChannel, remoteChannel);
> +        }

And what happens if this fails?

@@ +434,5 @@
> +      });
> +
> +      // Creat the datachannel and handle the local 'onopen' event
> +      this.pcLocal.createDataChannel(options, function (channel) {
> +        localChannel = channel;

Shouldn't you be calling check_next_test here?

@@ +462,5 @@
> +      target = options.targetChannel ||
> +               this.pcRemote.dataChannels[this.pcRemote.dataChannels.length - 1];
> +
> +      // Register event handler for the target channel
> +      target.onmessage = function (recv_data) {

Nit - I'd add an ok(true) here to indicate you've passed the test to receive an onmessage event on the target.

@@ +475,5 @@
> +
> +/**
> + * This class acts as a wrapper around a DataChannel instance.
> + *
> + * @param dataChannel

Nit - Missing definition of parameters here.

@@ +491,5 @@
> +   */
> +
> +  this.onclose = unexpectedEventAndFinish(this.id, 'onclose');
> +  this.onmessage = unexpectedEventAndFinish(this.id, 'onmessage');
> +  this.onopen = unexpectedEventAndFinish(this.id, 'onopen');

You need to define an onerror event handler here to execute unexpectedEventAndFinish if it's fired.

@@ +899,5 @@
> +   *
> +   * @param {Function} onDataChannelOpened
> +   *        Callback to execute when the data channel has been opened
> +   */
> +  registerDataChannelOpenEvents : function (onDataChannelOpened) {

This feels like a very confusing function to expose on the public API. It feels more appropriate to be internal.

::: dom/media/tests/mochitest/templates.js
@@ +158,5 @@
> +    }
> +  ],
> +  [
> +    'PC_REMOTE_SET_LOCAL_DESCRIPTION',
> +    function (test) {

All of the logic here definitely doesn't belong out in the template. We need a consistent flow within the framework itself for managing the async nature shown below. You need to universalize the logic between createDataChannel's use of similar logic below with the logic you have below making use of public facing APIs vs. internal APIs.

Also, there's a lot of code duplication present here. I'd clean this up.

@@ +187,5 @@
> +        check_next_test();
> +      });
> +
> +      test.pcRemote.setLocalDescription(test.pcRemote._last_answer, function () {
> +        test.connected = true;

Why would this be classified as connected at this point?

@@ +222,5 @@
> +  [
> +    'SEND_BLOB',
> +    function (test) {
> +      var contents = ["At vero eos et accusam et justo duo dolores et ea rebum."];
> +      var blob = new Blob(contents, { "type" : "text/plain" });

If you are going to test with blobs, I'd use something different than plain text preferably for the smoke test.

@@ +237,5 @@
> +      });
> +    }
> +  ],
> +  [
> +    'CREATE_SECOND_DATA_CHANNEL',

All commands including this one and below do not belong in the general smoke test template.

::: dom/media/tests/mochitest/test_dataChannel_basicAudioVideo.html
@@ +17,5 @@
> +
> +  var test;
> +  runTest(function () {
> +    test = new DataChannelTest();
> +    test.setMediaConstraints([{audio: true}, {video: true}],

One basic test missing here is the audio video combined case which uses:

[{video: true, audio: true}], [{video: true, audio: true}]

On that regard, I just noticed there's a bug in our PC smoke tests for not doing that in the combined test.

Comment 59

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 753935 [details] [diff] [review]
Patch v3

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

Generally looks pretty good, modulo jason's comments and the one item I noted

::: dom/media/tests/mochitest/pc.js
@@ +643,5 @@
> +  this._pc.onaddstream = function (aEvent) {
> +    info(self.label + ": 'onaddstream' event fired for " + aEvent.stream);
> +
> +    // TODO: Bug 834835 - Assume type is video until we get get{Audio,Video}Tracks.
> +    self.attachMedia(aEvent.stream, 'video', 'remote');

We now have getAudio/VideoTracks

Comment 60

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #59)
> Comment on attachment 753935 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 753935 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Generally looks pretty good, modulo jason's comments and the one item I noted
> 
> ::: dom/media/tests/mochitest/pc.js
> @@ +643,5 @@
> > +  this._pc.onaddstream = function (aEvent) {
> > +    info(self.label + ": 'onaddstream' event fired for " + aEvent.stream);
> > +
> > +    // TODO: Bug 834835 - Assume type is video until we get get{Audio,Video}Tracks.
> > +    self.attachMedia(aEvent.stream, 'video', 'remote');
> 
> We now have getAudio/VideoTracks

Note - I was going to fix that as part of bug 834837. I don't think we need to worry about that on this patch.

Comment 61

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 753935 [details] [diff] [review]
Patch v3

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

::: dom/media/tests/mochitest/pc.js
@@ +275,5 @@
> +  this.pcLocal.close();
> +  this.pcRemote.close();
> +  this.connected = false;
> +
> +  onSuccess();

Because it will become async in the future. See the above comment. This might be related to bug 878562. So I don't see a reason why this should be sync now, when we have to change all of our tests later. Also keep in mind that DataChannelTest.close is also async!

@@ +392,5 @@
> +      var self = this;
> +
> +      // Register handler for remote channel, cause we have to wait until
> +      // the current close operation has been finished.
> +      remoteChannel.onclose = function () {

That doesn't make much sense. A check like that is only helpful with a condition, but not when it would be always true.

@@ +398,5 @@
> +
> +        onSuccess(remoteChannel);
> +      };
> +
> +      localChannel.close();

This is already part of the DataChannelWrapper.close method. Why should we do this twice?

@@ +412,5 @@
> +     *        Options for the data channel (see nsIPeerConnection)
> +     * @param {Function} onSuccess
> +     *        Callback when the creation was successful
> +     */
> +    value : function DCT_createDataChannel(options, onSuccess) {

That's not that easy as writing this comment. As said earlier here we have to wait for an immediate response while during the setup of the peer connection you will have to wait until the connection is up. There is duplicated code, that's correct, but not sure how much this can be simplified. But I will check that.

@@ +423,5 @@
> +          is(localChannel.readyState, "open", localChannel.id + " is in state: 'open'");
> +          is(remoteChannel.readyState, "open", remoteChannel.id + " is in state: 'open'");
> +
> +          onSuccess(localChannel, remoteChannel);
> +        }

The usual timeout we run into as for any other kind of event.

@@ +434,5 @@
> +      });
> +
> +      // Creat the datachannel and handle the local 'onopen' event
> +      this.pcLocal.createDataChannel(options, function (channel) {
> +        localChannel = channel;

It should. Thanks for catching that.

@@ +462,5 @@
> +      target = options.targetChannel ||
> +               this.pcRemote.dataChannels[this.pcRemote.dataChannels.length - 1];
> +
> +      // Register event handler for the target channel
> +      target.onmessage = function (recv_data) {

As said above, I feel that's pretty useless when you always call it with true. Also please check back your feedback from earlier on this bug, which stated that I should not use any checks inside the framework! Why this change now after I removed all of them?

@@ +491,5 @@
> +   */
> +
> +  this.onclose = unexpectedEventAndFinish(this.id, 'onclose');
> +  this.onmessage = unexpectedEventAndFinish(this.id, 'onmessage');
> +  this.onopen = unexpectedEventAndFinish(this.id, 'onopen');

Will do, thanks for noting that. I simply missed it.

@@ +643,5 @@
> +  this._pc.onaddstream = function (aEvent) {
> +    info(self.label + ": 'onaddstream' event fired for " + aEvent.stream);
> +
> +    // TODO: Bug 834835 - Assume type is video until we get get{Audio,Video}Tracks.
> +    self.attachMedia(aEvent.stream, 'video', 'remote');

As mentioned earlier on this bug this is outside of the scope for this implementation. A separate bug has already been filed for it and when my query foo is good enough it should be bug 834837. So it's nothing we should really touch here.

@@ +899,5 @@
> +   *
> +   * @param {Function} onDataChannelOpened
> +   *        Callback to execute when the data channel has been opened
> +   */
> +  registerDataChannelOpenEvents : function (onDataChannelOpened) {

This exists because it's needed at several stages, here and in the template to setup the initial datachannel. Also tests which want to implement their own more detailed tests could participate from. Having this as a private method would require tests to re-define all of it.

::: dom/media/tests/mochitest/templates.js
@@ +158,5 @@
> +    }
> +  ],
> +  [
> +    'PC_REMOTE_SET_LOCAL_DESCRIPTION',
> +    function (test) {

Something we could do is to move this code into pc.js and create a method like createInitialDataChannel().

@@ +187,5 @@
> +        check_next_test();
> +      });
> +
> +      test.pcRemote.setLocalDescription(test.pcRemote._last_answer, function () {
> +        test.connected = true;

Because we are in the callback of setLocalDescription when the peer connection has been already established.

@@ +222,5 @@
> +  [
> +    'SEND_BLOB',
> +    function (test) {
> +      var contents = ["At vero eos et accusam et justo duo dolores et ea rebum."];
> +      var blob = new Blob(contents, { "type" : "text/plain" });

MIME types are blown away anyway. So whatever we use here will not make a difference. The content we get on the other side has to be identical.

@@ +237,5 @@
> +      });
> +    }
> +  ],
> +  [
> +    'CREATE_SECOND_DATA_CHANNEL',

Here we have a conflict between you and Eric, who wanted to have those in the template. Please clarify what you both want to have. Randell might also want to be included here. I really don't want to change that back and forth. Tell me what you really want, and you will get it.

Just in general having this code not in here, we would have to add an immense number of different tests to check e.g. multiple datachannels for audio and video (combined or not).

::: dom/media/tests/mochitest/test_dataChannel_basicAudioVideo.html
@@ +17,5 @@
> +
> +  var test;
> +  runTest(function () {
> +    test = new DataChannelTest();
> +    test.setMediaConstraints([{audio: true}, {video: true}],

Not doing what? Can you please be more specific? I don't understand that sentence.

Comment 62

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Jason, if you want to get some of the stuff discussed, you can find me in the meeting room 'Fail' through the whole day.

Comment 63

[Jason Smith [:jsmith]]

Comment on attachment 753935 [details] [diff] [review]
Patch v3

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

Note - with my media flow checking mochitests landing, you'll need to rebase this.

::: dom/media/tests/mochitest/templates.js
@@ +158,5 @@
> +    }
> +  ],
> +  [
> +    'PC_REMOTE_SET_LOCAL_DESCRIPTION',
> +    function (test) {

That works.

@@ +237,5 @@
> +      });
> +    }
> +  ],
> +  [
> +    'CREATE_SECOND_DATA_CHANNEL',

Why couldn't these commands be a part of a separate template for multiple channels? I'll followup with Eric on this though.

::: dom/media/tests/mochitest/test_dataChannel_basicAudioVideo.html
@@ +17,5 @@
> +
> +  var test;
> +  runTest(function () {
> +    test = new DataChannelTest();
> +    test.setMediaConstraints([{audio: true}, {video: true}],

The test you are doing above handles the case when you are doing an audio and video in separate streams and merging them. You also need the case when the audio and video come within the same stream. So you need a test that does:

{video: true, audio: true}

Comment 64

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v4

Updated patch for all comments taken care of. Keep in mind that the tests will fail due to the broken command chain handling on bug 784519. As long as this is not fixed I cannot finish up this test.

Comment 65

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #64)
> Created attachment 760470 [details] [diff] [review]
> Patch v4
> 
> Updated patch for all comments taken care of. Keep in mind that the tests
> will fail due to the broken command chain handling on bug 784519. As long as
> this is not fixed I cannot finish up this test.

I don't see how bug 784519 broke the tests on this patch. Please clarify.

Comment 66

[Henrik Skupin [:whimboo][⌚️UTC+1]]

For any clarification see the other bug.

Comment 67

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I pushed an updated version of this patch for bug 881658 to try:
https://tbpl.mozilla.org/?tree=Try&rev=03a4a0834945

If everything is green I will finalize it later today.

Comment 68

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #67)
> I pushed an updated version of this patch for bug 881658 to try:
> https://tbpl.mozilla.org/?tree=Try&rev=03a4a0834945

Finally, we got a completely green testrun! That's great to see. I will finalize the patch early tomorrow so that we can land it before the weekend!

Comment 69

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v4.1

Updated patch for the latest signaling and event handling changes. Also all other reported issues should have been fixed. If not, please let me know.

I pushed this version of the patch to try:
https://tbpl.mozilla.org/?tree=Try&rev=f410a89cfc7a

Comment 70

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v4.2

Just noticed that I missed to remove some of the .id references, which will not break the tests but result in undefined output.

New try run can be found here:
https://tbpl.mozilla.org/?tree=Try&rev=a84dbd60ff02

Comment 71

[Jason Smith [:jsmith]]

Comment on attachment 762623 [details] [diff] [review]
Patch v4.2

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

review+ with some comments. None of the comments made here are blockers to land the patch.

::: dom/media/tests/mochitest/pc.js
@@ +572,5 @@
> +      source.send(data);
> +    }
> +  },
> +
> +  waitForInitialDataChannel : {

Given that we're overriding the command definition of setLocalDescription for Data Channels, we might want to override the setLocalDescription function and use that instead here.

::: dom/media/tests/mochitest/templates.js
@@ +205,5 @@
> +    'PC_REMOTE_SET_LOCAL_DESCRIPTION',
> +    function (test) {
> +      test.waitForInitialDataChannel(function (localChannel, remoteChannel) {
> +        is(localChannel.readyState, "open", localChannel + " is in state: 'open'");
> +        is(remoteChannel.readyState, "open", remoteChannel + " is in state: 'open'");

Why not do these checks as part of waitForInitialDataChannel in the check_next_test function?

@@ +233,5 @@
> +    function (test) {
> +      var message = "Lorem ipsum dolor sit amet";
> +
> +      test.send(message, function (channel, data) {
> +        is(data, message, "Message correctly transmitted from pcLocal to pcRemote.");

Are there any common logic we can pull into the Data Channel Test framework for doing common checks for blobs and string messages that's evident in the templates.js file?

@@ +276,5 @@
> +      var message = "Lorem ipsum dolor sit amet";
> +
> +      test.send(message, function (channel, data) {
> +        is(channels.indexOf(channel), channels.length - 1, "Last channel used");
> +        is(data, message, "Received message has the correct content.");

How are you testing here that the other data channel didn't receive the message sent here?

@@ +305,5 @@
> +    function (test) {
> +      var channels = test.pcRemote.dataChannels;
> +
> +      test.closeDataChannel(channels.length - 1, function (channel) {
> +        is(channel.readyState, "closed", "Channel is in state: 'closed'");

Shouldn't you be testing that the other data channel's readyState has not gone to closed here?

Comment 72

[Jason Smith [:jsmith]]

Comment on attachment 762623 [details] [diff] [review]
Patch v4.2

Probably are going to want a second review from a WebRTC developer to ensure the implementation here aligns with underlying development code expectations.

Comment 73

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jason Smith [:jsmith] from comment #71)
> > +  waitForInitialDataChannel : {
> 
> Given that we're overriding the command definition of setLocalDescription
> for Data Channels, we might want to override the setLocalDescription
> function and use that instead here.

Good idea. The only thing is that we have to be smart this way and depend on the remotes signaling state of 'have-remote-offer'. With that checked first we can easily do that.

> ::: dom/media/tests/mochitest/templates.js
> @@ +205,5 @@
> > +    'PC_REMOTE_SET_LOCAL_DESCRIPTION',
> > +    function (test) {
> > +      test.waitForInitialDataChannel(function (localChannel, remoteChannel) {
> > +        is(localChannel.readyState, "open", localChannel + " is in state: 'open'");
> > +        is(remoteChannel.readyState, "open", remoteChannel + " is in state: 'open'");
> 
> Why not do these checks as part of waitForInitialDataChannel in the
> check_next_test function?

As you said earlier we should not do checks in the framework itself. If we are not sure what we want to do, please file a bug so that we can get clarification.

> > +    function (test) {
> > +      var message = "Lorem ipsum dolor sit amet";
> > +
> > +      test.send(message, function (channel, data) {
> > +        is(data, message, "Message correctly transmitted from pcLocal to pcRemote.");
> 
> Are there any common logic we can pull into the Data Channel Test framework
> for doing common checks for blobs and string messages that's evident in the
> templates.js file?

See above for checks within the framework.

> > +      var message = "Lorem ipsum dolor sit amet";
> > +
> > +      test.send(message, function (channel, data) {
> > +        is(channels.indexOf(channel), channels.length - 1, "Last channel used");
> > +        is(data, message, "Received message has the correct content.");
> 
> How are you testing here that the other data channel didn't receive the
> message sent here?

The other channel has the default handler setup for unexpectedEventAndFinish(). So if a message would have been send to that channel, we will get an error.

> > +    function (test) {
> > +      var channels = test.pcRemote.dataChannels;
> > +
> > +      test.closeDataChannel(channels.length - 1, function (channel) {
> > +        is(channel.readyState, "closed", "Channel is in state: 'closed'");
> 
> Shouldn't you be testing that the other data channel's readyState has not
> gone to closed here?

Why should it not have changed to closed? When we close the channel on one side via closeDataChannel() the remote peer will also be closed and fires the onclose event, which we are waiting for.

Comment 74

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v5

Changes made as requested to pull in the call to setLocalDescription for the final call on the remote peer. Lets wait for the outstanding review from Randell.

Comment 75

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #73)
> > > +    function (test) {
> > > +      var channels = test.pcRemote.dataChannels;
> > > +
> > > +      test.closeDataChannel(channels.length - 1, function (channel) {
> > > +        is(channel.readyState, "closed", "Channel is in state: 'closed'");
> > 
> > Shouldn't you be testing that the other data channel's readyState has not
> > gone to closed here?
> 
> Why should it not have changed to closed? When we close the channel on one
> side via closeDataChannel() the remote peer will also be closed and fires
> the onclose event, which we are waiting for.

I think I'm talking about the negative test case that does the following:

When I have two data channels setup between two peers, if I close one channel, then the other channel should not be closed.

In short, we're validating that closing one channel in peer communication doesn't cause some odd side effect to cause the other channel to close.

Comment 76

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jason Smith [:jsmith] from comment #75)
> When I have two data channels setup between two peers, if I close one
> channel, then the other channel should not be closed.
> 
> In short, we're validating that closing one channel in peer communication
> doesn't cause some odd side effect to cause the other channel to close.

Then the default handler which is unexpectedEventAndFinish() will be called and we fail.

Comment 77

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Randell, can you please tell me when you will have time to review this patch? I'm afraid it will bitrot again. Thanks.

Comment 78

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 763417 [details] [diff] [review]
Patch v5

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

::: dom/media/tests/mochitest/pc.js
@@ +586,5 @@
> +    value : function DCT_setLocalDescription(peer, desc, onSuccess) {
> +      // If the peer has a remote offer we are in the final call, and have
> +      // to wait for the datachannel connection to be open
> +      if (peer.signalingState === 'have-remote-offer') {
> +        this.waitForInitialDataChannel(peer, desc, onSuccess);

Since this also sets the LocalDescription, that should at least be mentioned.

@@ +598,5 @@
> +  },
> +
> +  waitForInitialDataChannel : {
> +    /**
> +     * Create an initial data channel before the peer connection has been established

Connected, not established, is a better way to describe it.

Comment 79

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Randell. I will do a last local run with an up2date nightly build and push it to inbound if all still passes. Not sure if we can make the 24.0 cut before the merge today. Lets hope so.

Comment 80

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #79)
> Thanks Randell. I will do a last local run with an up2date nightly build and
> push it to inbound if all still passes. Not sure if we can make the 24.0 cut
> before the merge today. Lets hope so.

Umm...let's not do that. As I indicated earlier, I think it would better if I pushed the patch on bug 831789 first, rebase this patch against those changes, and then land. The other

Comment 81

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #80)
> (In reply to Henrik Skupin (:whimboo) from comment #79)
> > Thanks Randell. I will do a last local run with an up2date nightly build and
> > push it to inbound if all still passes. Not sure if we can make the 24.0 cut
> > before the merge today. Lets hope so.
> 
> Umm...let's not do that. As I indicated earlier, I think it would better if
> I pushed the patch on bug 831789 first, rebase this patch against those
> changes, and then land. The other

Meant to say - The other approach is going a little more obtrusive.

Comment 82

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No, you are mixing up bug numbers. Your bug would need an update anyway due to the dependency on bug 881658. So I don't see why I should wait here with the patch being ready for landing, while yours will need an update on multiple places. We should get this patch landed, and you update yours based on the new template file.

Comment 83

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #82)
> No, you are mixing up bug numbers. Your bug would need an update anyway due
> to the dependency on bug 881658. So I don't see why I should wait here with
> the patch being ready for landing, while yours will need an update on
> multiple places. We should get this patch landed, and you update yours based
> on the new template file.

I don't think that's a good idea. The changes are minor to update against the changes that were made in that bug. It's going to be easier to merge the other way. The other direction I think is going far more obtrusive, as this patch makes cross-cutting changes.

Comment 84

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So you are saying that checks for media flow are more important as tests for various kind of datachannel setups? Please keep in mind that I will be out soon for a bit more than a week and I will not be able to update this patch before I leave. So this patch will sit around another 2-3 weeks.

Comment 85

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) [away 06/28 - 07/7] from comment #84)
> So you are saying that checks for media flow are more important as tests for
> various kind of datachannel setups? Please keep in mind that I will be out
> soon for a bit more than a week and I will not be able to update this patch
> before I leave. So this patch will sit around another 2-3 weeks.

Neither group of automation is more important - I'm just looking for the easiest path forward to merge both patches together.

If you are planning to be out soon, then let's do other approach then - you land your patch first, I'll rebase, and land after that (since I'm not going on PTO for at least another month).

Comment 86

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v6

Patch with updated comments and ready for landing. I talked with Ed and Ryan on IRC and we agreed to land it on inbound and miss the merge. So we have to get this patch landed on aurora later this week.

Comment 87

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This will most likely land for Firefox 25. So I don't set any target milestone yet. It can be done once merged to mozilla-central.

https://hg.mozilla.org/integration/mozilla-inbound/rev/923aca74a801

Comment 88

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/923aca74a801

Comment 89

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We will have to wait for the landing of the patch on bug 886417 before we can backport this patch to Aurora. For beta we would also need a modified patch without the code added by bug 784519.

Comment 90

[Jason Smith [:jsmith]]

This patch is a test only patch. I don't think that's appropriate to mark tracking-firefox for. It doesn't seem critical for release management to watch over this.

Comment 91

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ups, sure! It was just the habit for things I usually have to do. Those are not necessary.

Comment 92

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As talked with Randell, we might not want to backport this framework to beta given the necessary major overhaul of the patch for the signaling changes, which only have been landed for Firefox 24. So I'm going to wontfix this for Firefox 23.

For Aurora I pushed the patch to try:
https://tbpl.mozilla.org/?tree=Try&rev=77ab79cc4c45

If it sticks, I will land it on aurora today.

Comment 93

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Except a totally unrelated bustage on Android this is all green. So I pushed the patch to aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e2cfcae2605

Looks like we are done here!

Comment 94

[u279076]

Assuming no QA needed here.