856319 - Don't offer m=application unless createDataChannel is called first

Status: VERIFIED FIXED

(Core :: WebRTC: Signaling, defect)

Description

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

We shouldn't offer m=application (DataChannels) unless createDataChannel() is called before createOffer().  

Once we support renegotiation, createDataChannel when there's no m=application already will cause negotiationneeded

Can't land until the dependencies do

Comment 1

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

Attachment: hook up createDataChannel-before-createOffer to SDP generation from bug 837035

Comment 2

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

Attachment: hacked up version of data_test to verify m=application control (and preset channels)

Comment 3

[Ethan Hugg [:ehugg]]

Comment on attachment 731480 [details] [diff] [review]
hook up createDataChannel-before-createOffer to SDP generation from bug 837035

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +709,5 @@
>    CSFLogDebug(logTag, "%s: making DOMDataChannel", __FUNCTION__);
>  
> +  // XXX stream_id of 0 might confuse things...
> +  mCall->addStream(0, 2, DATA);
> +  mHaveDataStream = true;

I see mHaveDataStream set but not used, is it needed since the addStream sets .enabled in the media_cap_tbl now?  Where you planning to use it later?  Perhaps I missed its usage.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3566,5 @@
>      /*
>       * This is temporary code to allow configuration of the two
>       * default streams. When multiple streams > 2 are supported this
>       * will be re-implemented.
> +     * Don't enable a media line if it wasn't already enabled.

This looks like you are fixing another bug with removestream.  What were the symptoms of this?

Comment 4

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

(In reply to Ethan Hugg [:ehugg] from comment #3)
> Comment on attachment 731480 [details] [diff] [review]
> hook up createDataChannel-before-createOffer to SDP generation from bug
> 837035
> 
> Review of attachment 731480 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +709,5 @@
> >    CSFLogDebug(logTag, "%s: making DOMDataChannel", __FUNCTION__);
> >  
> > +  // XXX stream_id of 0 might confuse things...
> > +  mCall->addStream(0, 2, DATA);
> > +  mHaveDataStream = true;
> 
> I see mHaveDataStream set but not used, is it needed since the addStream
> sets .enabled in the media_cap_tbl now?  Where you planning to use it later?
> Perhaps I missed its usage.

I'll revise.  I actually had a use intended, though the current patch works.

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
> @@ +3566,5 @@
> >      /*
> >       * This is temporary code to allow configuration of the two
> >       * default streams. When multiple streams > 2 are supported this
> >       * will be re-implemented.
> > +     * Don't enable a media line if it wasn't already enabled.
> 
> This looks like you are fixing another bug with removestream.  What were the
> symptoms of this?

Looking at it more closely, I'm not sure it would cause problems even when we add removestream.  I was concerned that removing a stream when one hadn't been added would cause it to add an m= line, but that can be checked for at a higher level.  I.e.:
  pc = new RTCPeer..()
  pc.addStream(AUDIO...)
  ...
  pc.removeStream(VIDEO..)

So long as removeStream() validates the arguments, this is fine as-is.

Comment 5

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

Attachment: hook up createDataChannel-before-createOffer to SDP generation from bug 837035

Comment 6

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

Comment on attachment 731527 [details] [diff] [review]
hook up createDataChannel-before-createOffer to SDP generation from bug 837035

Made the stuff in fsmdef assertions
Uses mHaveDataStream to avoid calling addStream multiple times (just an optimization)

Comment 7

[Ethan Hugg [:ehugg]]

Comment on attachment 731527 [details] [diff] [review]
hook up createDataChannel-before-createOffer to SDP generation from bug 837035

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

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3566,5 @@
>      /*
>       * This is temporary code to allow configuration of the two
>       * default streams. When multiple streams > 2 are supported this
>       * will be re-implemented.
> +     * Don't enable a media line if it wasn't already enabled.

I think this comment matches the old patch.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/29fb45427001

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/29fb45427001

Comment 10

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

I will have a mochitest for this change shortly.

Comment 11

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

Attachment: mochitest v1

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=de46a127bd46

Comment 12

[Jason Smith [:jsmith]]

Comment on attachment 732939 [details] [diff] [review]
mochitest v1

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

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

This isn't really a data channel test. Rename this?

::: dom/media/tests/mochitest/test_dataChannel_noOffer.html
@@ +16,5 @@
> +  });
> +
> +  var steps = [
> +    [
> +      "CHECK_NO_M_LINE",

This really should be part of the basic smoke test commands in a command such as "CHECK_SDP" - this would fall under the similar checks for a different refactoring we need to do elsewhere as well for the localDescription and remoteDescription refactoring.

Comment 13

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

Comment on attachment 732939 [details] [diff] [review]
mochitest v1

(In reply to Jason Smith [:jsmith] from comment #12)
> >  MOCHITEST_FILES = \
> > +  test_dataChannel_noOffer.html \
> 
> This isn't really a data channel test. Rename this?

This is a datachannel test. It only performs a negative and single check. But something which might be better here is to base it on the upcoming DataChannelTest class.

> > +  var steps = [
> > +    [
> > +      "CHECK_NO_M_LINE",
> 
> This really should be part of the basic smoke test commands in a command
> such as "CHECK_SDP" - this would fall under the similar checks for a
> different refactoring we need to do elsewhere as well for the
> localDescription and remoteDescription refactoring.

No, this cannot be part of such a command. This is a negative test and will only happen once in that specific test.

I will rework the test side by side with the basic dc tests.

Comment 14

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #13)
> Comment on attachment 732939 [details] [diff] [review]
> mochitest v1
> 
> (In reply to Jason Smith [:jsmith] from comment #12)
> > >  MOCHITEST_FILES = \
> > > +  test_dataChannel_noOffer.html \
> > 
> > This isn't really a data channel test. Rename this?
> 
> This is a datachannel test. It only performs a negative and single check.
> But something which might be better here is to base it on the upcoming
> DataChannelTest class.

No, it's not. This is a basic smoke test of a peer connection that changes the basis of the flow that does not involve data channels. I'd rather only use the data channel naming scheme for tests that actually involve data channels, which is not the case here.

> 
> > > +  var steps = [
> > > +    [
> > > +      "CHECK_NO_M_LINE",
> > 
> > This really should be part of the basic smoke test commands in a command
> > such as "CHECK_SDP" - this would fall under the similar checks for a
> > different refactoring we need to do elsewhere as well for the
> > localDescription and remoteDescription refactoring.
> 
> No, this cannot be part of such a command. This is a negative test and will
> only happen once in that specific test.

I disagree. It's not a negative test - this is part of the basic smoke test logic that changes how the PCs work by default. The test you have here adds an unnecessary mochitest, cause all you are doing here differently is appending on a command to an existing workflow that already exists checking an additional attribute.

There's no need to add an additional mochitest file here - just change the pc framework basis for what checks are additionally needed.

Comment 15

[Jason Smith [:jsmith]]

lgtm on trunk - a sanity data channel test is showing m=application in the sdp, where as a sanity test without data channels with peer connections is not showing m=application.

Comment 16

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

Attachment: mochitest v1

Not sure if a review from Jason would be enough to get this landed. Please let me know. For now asking for review from both of you. 

As Jason suggested we do not make use of the PeerConnectionTest framework for this test but do it via the basic commands. But I kept the dataChannel part in the filename given that is a negative test for datachannels and that's why connected to them. It's easier to find it that way.

Comment 17

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

Attachment: mochitest v1.1

I copied the code from my large datachannel patch and missed to actually remove the pc.js and templates.js inclusions.

Comment 18

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

Comment on attachment 750425 [details] [diff] [review]
mochitest v1.1

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

Test landed on inbound as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0dd7859a422

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0dd7859a422

Comment 20

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

Jesup, I assume we want to get those tests backported down to beta?

Comment 21

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

(In reply to Henrik Skupin (:whimboo) from comment #20)
> Jesup, I assume we want to get those tests backported down to beta?

Jesup. ping.

Comment 22

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #21)
> (In reply to Henrik Skupin (:whimboo) from comment #20)
> > Jesup, I assume we want to get those tests backported down to beta?
> 
> Jesup. ping.

I'd just ask for approval for this at this point if you want to backport it.

Comment 23

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

Yes.  Run a try of them on beta (and aurora if they're not there), and if it's green, mark for approval.

Comment 24

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

So I did try server runs on aurora and beta today. While aurora looks promising we totally failed on beta. This bug has been marked as been fixed in Firefox 22, so why do we fail so embarrassing?

aurora: https://tbpl.mozilla.org/?tree=Try&rev=2d646fedea99
beta: https://tbpl.mozilla.org/?tree=Try&rev=faa8ce1d69fe

So i will get this test landed on aurora but for beta I would like to hear back from Randell, what's going on.

Comment 25

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

Comment on attachment 750425 [details] [diff] [review]
mochitest v1.1

Landed test on mozilla-aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/e33bd71d1e01

Comment 26

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

(In reply to Henrik Skupin (:whimboo) from comment #24)
> So i will get this test landed on aurora but for beta I would like to hear
> back from Randell, what's going on.

I have seen bug 874850 but I don't think that it is related due to it's massive failure rate.

Comment 27

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

The problem with the test on beta is because of: {"name":"INTERNAL_ERROR","message":"Could not create local SDP for offer; cause = ERR"}. Don't we have support landed for '{ mandatory: { OfferToReceiveAudio: true} };' on beta, which this test needs? Is it a still not fixed issue on beta? If not, how can we change it to make it working? Using mozGetUserMedia() doesn't seem to work too.