1556795 - RTCDataChannel.id logic needs an overhaul

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

[Byron Campen [:bwc]]

Our implementation of DataChannel only assigns ids after the stream id limit has been successfully negotiated in SCTP. The webrtc-pc spec says to set these immediately, and make a best effort to negotiate the stream limit so they can be used. We need to update our code to implement this best-effort approach.

Comment 1

[Byron Campen [:bwc]]

Given that this difference leads to lots of buggy behavior, I think I'm going to call this a defect.

Comment 2

[Byron Campen [:bwc]]

Attachment: Bug 1556795: (WIP) Generate DataChannel ids in accordance with the specification.

Comment 3

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b969751e1515f18e546eb0d661694c6d9e4a179e

Comment 4

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dac88e6d411eb350ca12d9efc67f73ab584fc26

Comment 5

[Byron Campen [:bwc]]

Attachment: Bug 1556795: Re-enable some web-platform-tests related to DataChannel ids. r?jib

Comment 6

[Byron Campen [:bwc]]

Attachment: Bug 1556795: Allocate stream ids as soon as client/server is negotiated, and try to negotiate id limit increases after. r?ng

Depends on D35046

Comment 7

[Byron Campen [:bwc]]

Attachment: Bug 1556795: Simplifications related to DataChannel::mFlags. r?ng

Depends on D35047

Comment 8

[Byron Campen [:bwc]]

Attachment: Bug 1556795: Add some thread assertions, and remove an unused member. r?ng

Depends on D35048

Comment 9

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbff70d6a52ede90a7471ddd229fa739ed0fd0ce

Comment 10

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3521b305cf2f576e15d6677892880a6be095a836

Comment 11

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d888f131453bdcc0b6aab6f3e7038c697cd4cae8

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/112937d690e0
Re-enable some web-platform-tests related to DataChannel ids. r=jib
https://hg.mozilla.org/integration/autoland/rev/f8ce8930bcab
Allocate stream ids as soon as client/server is negotiated, and try to negotiate id limit increases after. r=ng
https://hg.mozilla.org/integration/autoland/rev/a87072937e2c
Simplifications related to DataChannel::mFlags. r=ng
https://hg.mozilla.org/integration/autoland/rev/05898e5d5434
Add some thread assertions, and remove an unused member. r=ng

Comment 13

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/112937d690e0
https://hg.mozilla.org/mozilla-central/rev/f8ce8930bcab
https://hg.mozilla.org/mozilla-central/rev/a87072937e2c
https://hg.mozilla.org/mozilla-central/rev/05898e5d5434

Comment 14

[Andrei Ciure[:aciure]]

Backed out 4 changesets (bug 1556795)for causing RTCPeerConnection-createDataChannel.html to perma fail

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=05898e5d54344e2e43181bfe6608b029771e39af

failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=252633743&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&searchStr=wpt10

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=252617868&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&searchStr=wpt16

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/cb03b8da2fbfc316a70b836d77483f8971c7fd8b

Comment 15

[Byron Campen [:bwc]]

Looks like this collided with 1558851.

Comment 16

[Byron Campen [:bwc]]

Yeah, it looks like the merge didn't go quite right here.

Comment 17

[James Graham [:jgraham]]

Oh sorry I was just preparing a patch to fix the metadata, but understandably the sheriffs decided to back out. I think the right patch is something like

diff --git a/testing/web-platform/meta/webrtc/RTCPeerConnection-createDataChannel.html.ini b/testing/web-platform/meta/webrtc/RTCPeerConnection-createDataChannel.html.ini
index b4dd12fbec66..c40c1ddaa60c 100644
--- a/testing/web-platform/meta/webrtc/RTCPeerConnection-createDataChannel.html.ini
+++ b/testing/web-platform/meta/webrtc/RTCPeerConnection-createDataChannel.html.ini
@@ -43,6 +43,3 @@
     expected: FAIL
     bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1550497
 
-  [Channels created (after setRemoteDescription) should have id assigned]
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=797135
-    expected: FAIL

Comment 18

[Byron Campen [:bwc]]

Yeah, I'm rebasing and rebuilding now. Should be able to re-land later today.

Comment 19

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3da19335b6c9bb4c57891c1a82954240ff85e013

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/947f43a8a712
Re-enable some web-platform-tests related to DataChannel ids. r=jib
https://hg.mozilla.org/integration/autoland/rev/64c2df149745
Allocate stream ids as soon as client/server is negotiated, and try to negotiate id limit increases after. r=ng
https://hg.mozilla.org/integration/autoland/rev/ea730cd703a2
Simplifications related to DataChannel::mFlags. r=ng
https://hg.mozilla.org/integration/autoland/rev/fb1e149d91bf
Add some thread assertions, and remove an unused member. r=ng

Comment 21

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/947f43a8a712
https://hg.mozilla.org/mozilla-central/rev/64c2df149745
https://hg.mozilla.org/mozilla-central/rev/ea730cd703a2
https://hg.mozilla.org/mozilla-central/rev/fb1e149d91bf

Comment 22

[Lennart Grahl [:lgrahl]]

I would suggest to wait until https://github.com/w3c/webrtc-pc/pull/2222 has been merged. This should drastically simplify things.