856644 - signaling_unittests regression: check for m=application no longer appropriate

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Eric Rescorla (:ekr)]

The signaling unit tests now fail:

[ RUN      ] SignalingTest.JustInit
Init Complete
Init Complete
Close
Close
[       OK ] SignalingTest.JustInit (7752 ms)
[ RUN      ] SignalingTest.CreateSetOffer
Init Complete
Init Complete
onCreateOfferSuccess = v=0
o=Mozilla-SIPUA-22.0a1 1882 0 IN IP4 0.0.0.0
s=SIP Call
t=0 0
a=ice-ufrag:806ad0d3
a=ice-pwd:96f6ae15239b07a021a071625e691263
a=fingerprint:sha-256 A1:D5:40:24:02:75:EC:9B:61:B7:FD:65:1D:52:6E:BE:B1:0E:7A:FA:CC:66:CF:84:65:D3:A1:91:82:28:3B:8A
m=audio 53170 RTP/SAVPF 109 0 8 101
c=IN IP4 192.168.129.33
a=rtpmap:109 opus/48000/2
a=ptime:20
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15
a=sendrecv
a=candidate:0 1 UDP 2113601791 192.168.129.33 53170 typ host
a=candidate:2 1 UDP 2112946431 172.16.26.1 50187 typ host
a=candidate:4 1 UDP 2112487679 192.168.134.1 49681 typ host
a=candidate:0 2 UDP 2113601790 192.168.129.33 58365 typ host
a=candidate:2 2 UDP 2112946430 172.16.26.1 50103 typ host
a=candidate:4 2 UDP 2112487678 192.168.134.1 51335 typ host
m=video 49258 RTP/SAVPF 120
c=IN IP4 192.168.129.33
a=rtpmap:120 VP8/90000
a=sendrecv
a=candidate:0 1 UDP 2113601791 192.168.129.33 49258 typ host
a=candidate:2 1 UDP 2112946431 172.16.26.1 64319 typ host
a=candidate:4 1 UDP 2112487679 192.168.134.1 61809 typ host
a=candidate:0 2 UDP 2113601790 192.168.129.33 50784 typ host
a=candidate:2 2 UDP 2112946430 172.16.26.1 57626 typ host
a=candidate:4 2 UDP 2112487678 192.168.134.1 57914 typ host

SDPSanityCheck flags for offer = 0x303 SHOULD_SEND_AUDIO SHOULD_RECV_AUDIO SHOULD_SEND_VIDEO SHOULD_RECV_VIDEO
/Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/test/signaling_unittests.cpp:961: Failure
Expected: (sdp.find("m=application")) != (std::string::npos), actual: 18446744073709551615 vs 18446744073709551615
onSetLocalDescriptionSuccess
Close
Close

Comment 1

[Eric Rescorla (:ekr)]

assigning to jesup b/c this looks like a result of the data channel changes.

Comment 2

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

Attachment: Make signaling_unittests understand the changes in datachannel negotiation

Comment 3

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

Comment on attachment 731999 [details] [diff] [review]
Make signaling_unittests understand the changes in datachannel negotiation

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

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +1219,3 @@
>  }
>  
>  TEST_F(SignalingTest, CreateOfferNoDataChannel)

Note: this test is pretty meaningless now, and likely should be removed.  If the other test worked, we could if we wanted test that it supresses m=application.

Better would be to remove the test, and remove the MozDontOfferDataChannel constraint.

Comment 4

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

Attachment: Make signaling_unittests understand the changes in datachannel negotiation

Comment 5

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

Attachment: Make signaling_unittests understand the changes in datachannel negotiation

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 732049 [details] [diff] [review]
Make signaling_unittests understand the changes in datachannel negotiation

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

lgtm

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3391233b3e31

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3391233b3e31

Comment 9

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

Comment on attachment 732049 [details] [diff] [review]
Make signaling_unittests understand the changes in datachannel negotiation

[Approval Request Comment]
Bug caused by (feature/regressing bug #): DataChannel SDP landing

User impact if declined: Signaling unit tests will fail on Aurora.  (Note: they aren't run on the test machines currently; we do run them on m-i/m-c on a personal/project continuous integration server/tinderbox.)  Not having this will mean anyone testing on Aurora (especially a patch for uplift) will have to manually apply this patch first.

Testing completed (on m-c, etc.): On m-c for some time and tested in CI

Risk to taking this patch (and alternatives if risky): nil - test code only

String or IDL/UUID changes made by this patch: none

Comment 10

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/7f5fc3bf0e76