Closed Bug 1556795 Opened 5 years ago Closed 5 years ago

RTCDataChannel.id logic needs an overhaul

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla69

Tracking Flags:

Tracking

Status

firefox69

---

fixed

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 1556795: (WIP) Generate DataChannel ids in accordance with the specification. 5 years ago Byron Campen [:bwc] (deleted), text/x-phabricator-request		Details
Bug 1556795: Re-enable some web-platform-tests related to DataChannel ids. r?jib 5 years ago Byron Campen [:bwc] (deleted), text/x-phabricator-request		Details
Bug 1556795: Allocate stream ids as soon as client/server is negotiated, and try to negotiate id limit increases after. r?ng 5 years ago Byron Campen [:bwc] (deleted), text/x-phabricator-request		Details
Bug 1556795: Simplifications related to DataChannel::mFlags. r?ng 5 years ago Byron Campen [:bwc] (deleted), text/x-phabricator-request		Details
Bug 1556795: Add some thread assertions, and remove an unused member. r?ng 5 years ago Byron Campen [:bwc] (deleted), text/x-phabricator-request		Details

Byron Campen [:bwc]

Assignee

Description

•

5 years ago

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.

Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo

Updated

•

5 years ago

Type: defect → task

Byron Campen [:bwc]

Assignee

Comment 1

•

5 years ago

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

Type: task → defect

Byron Campen [:bwc]

Assignee

Comment 2

•

5 years ago

Attached file Bug 1556795: (WIP) Generate DataChannel ids in accordance with the specification. (obsolete) (deleted) — Details

Byron Campen [:bwc]

Assignee

Comment 3

•

5 years ago

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

Byron Campen [:bwc]

Assignee

Comment 4

•

5 years ago

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

Byron Campen [:bwc]

Assignee

Comment 5

•

5 years ago

Attached file Bug 1556795: Re-enable some web-platform-tests related to DataChannel ids. r?jib (deleted) — Details

Byron Campen [:bwc]

Assignee

Comment 6

•

5 years ago

Attached file Bug 1556795: Allocate stream ids as soon as client/server is negotiated, and try to negotiate id limit increases after. r?ng (deleted) — Details

Depends on D35046

Byron Campen [:bwc]

Assignee

Comment 7

•

5 years ago

Attached file Bug 1556795: Simplifications related to DataChannel::mFlags. r?ng (deleted) — Details

Depends on D35047

Byron Campen [:bwc]

Assignee

Comment 8

•

5 years ago

Attached file Bug 1556795: Add some thread assertions, and remove an unused member. r?ng (deleted) — Details

Depends on D35048

Byron Campen [:bwc]

Assignee

Comment 9

•

5 years ago

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

Phabricator Automation

Updated

•

5 years ago

Attachment #9069963 - Attachment is obsolete: true

Byron Campen [:bwc]

Assignee

Comment 10

•

5 years ago

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

Byron Campen [:bwc]

Assignee

Comment 11

•

5 years ago

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

Pulsebot

Comment 12

•

5 years ago

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

Razvan Maries

Comment 13

•

5 years ago

bugherder

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

Status: NEW → RESOLVED

Closed: 5 years ago

status-firefox69: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla69

Andrei Ciure[:aciure]

Comment 14

•

5 years ago

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

Byron Campen [:bwc]

Assignee

Comment 15

•

5 years ago

Looks like this collided with 1558851.

Byron Campen [:bwc]

Assignee

Comment 16

•

5 years ago

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

Cosmin Sabou [:CosminS]

Updated

•

5 years ago

Status: RESOLVED → REOPENED

status-firefox69: fixed → ---

Resolution: FIXED → ---

Target Milestone: mozilla69 → ---

James Graham [:jgraham]

Comment 17

•

5 years ago

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

Byron Campen [:bwc]

Assignee

Comment 18

•

5 years ago

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

Byron Campen [:bwc]

Assignee

Comment 19

•

5 years ago

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

Pulsebot

Comment 20

•

5 years ago

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

Stefan Hindli [:stefan_hindli]

Comment 21

•

5 years ago

bugherder

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

Status: REOPENED → RESOLVED

Closed: 5 years ago → 5 years ago

status-firefox69: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla69

Nils Ohlmeier [:drno]

Updated

•

5 years ago

Regressed by: 1562341

Julien Cristau [:jcristau]

Updated

•

5 years ago

No longer regressed by: 1562341

Regressions: 1562341

Lennart Grahl [:lgrahl]

Comment 22

•

5 years ago

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

Randell Jesup [:jesup] (needinfo me)

Updated

•

3 years ago

Regressions: 1735972

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