Closed Bug 1556795 Opened 1 year ago Closed 1 year ago

RTCDataChannel.id logic needs an overhaul

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(4 files, 1 obsolete file)

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.

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

Type: task → defect
Attachment #9069963 - Attachment is obsolete: true
Pushed by bcampen@mozilla.com:
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

Looks like this collided with 1558851.

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla69 → ---

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

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

Pushed by bcampen@mozilla.com:
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
Regressed by: 1562341
No longer regressed by: 1562341
Regressions: 1562341

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

You need to log in before you can comment on or make changes to this bug.