Data channel message size limitations #702

lgrahl · 2017-10-26T16:27:03Z

Recent changes in the W3C spec will ensure that a TypeError is thrown in case
a data channel message is being sent that is too large for the other peer to
receive. However, that size limitation needs to be available to the sender or
it's an annoying guessing game.

This PR adds both access to maxMessageSize and throws a TypeError in case
the message is too large for the other peer to receive.

googlebot · 2017-10-26T16:30:41Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-10-26T16:34:41Z

CLAs look good, thanks!

fippo · 2017-10-26T16:51:48Z

src/js/common_shim.js

+      // Note: 65535 bytes is the default value from the SDP spec. Also,
+      //       every implementation we know supports receiving 65535 bytes.
+      var maxMessageSize = 65535;
+      var match = arguments[0].sdp.match(/a=max-message-size:\s*(\d+)/);


you might consider requiring sdp and using matchPrefix -- its a sub-dependency of the edge shim currently so wouldn't increase "binary size"

lgrahl · 2017-10-26T17:51:55Z

This PR has a few issues I've detected. I'll tick them one by one and then it's ready to be merged (from my side):

The local maximum message size needs to be determined, too. Chrome for example will fail beyond 256 KiB even if the remote maximum message size is above that.
Even though Firefox<=56 to Firefox<=56 is capable of sending large messages to one another, this PR will prevent that from being possible. This one is tricky... I resolved this in a rather hacky way by looking for mozilla...THIS_IS_SDPARTA in the remote description's SDP.
a=max-message-size:0 would fail at the moment.
Tests needed.
Use matchPrefix instead of regular expressions (where possible).

lgrahl · 2017-10-28T18:39:03Z

The new tests should be fine now but Travis is still failing (in other tests) - ideas?

lgrahl · 2017-10-28T20:42:45Z

FWIW, here's a list of determined maxMessageSize values:

FF<=56 to FF<=56: 1073741823 bytes (slightly incorrect but fine)
FF<=56 to FF>=57: 1073741823 bytes (slightly incorrect but fine)
FF<=56 to Chrome: 16384 bytes
FF<=56 to RAWRTC: 16384 bytes (very incorrect but fine)
FF>=57 to FF<=56: 1073741823 bytes (slightly incorrect but fine)
FF>=57 to FF>=57: 1073741823 bytes
FF>=57 to Chrome: 65535 bytes
FF>=57 to RAWRTC: 1073741823 bytes
Chrome to FF<=56: 65535 bytes
Chrome to FF>=57: 65535 bytes
Chrome to Chrome: 65535 bytes
Chrome to RAWRTC: 65535 bytes
RAWRTC to FF<=56: 65535 bytes
RAWRTC to FF>=57: 1073741823 bytes
RAWRTC to Chrome: 65535 bytes
RAWRTC to RAWRTC: unlimited (0)

Couldn't test Safari as I don't own any Apple products.
If you want to know why the incorrect's are fine, just ping me.

lgrahl · 2017-10-31T11:06:29Z

There's one last quirk remaining: I don't know why FF >= 57 doesn't raise an exception in the tests when sending a message that's too large. It does behave correctly when testing this by hand.

lgrahl · 2017-11-03T20:57:16Z

I'm done. Can be reviewed. Some FF stable test timed out on Travis and needs to be restarted (runs fine locally).

alvestrand

Nice add, and nice tests, but I worry about the tests that have started failing with AssertionError, where you just patched the expected files (test/e2e/expectations/chrome-beta).

alvestrand · 2017-11-09T07:10:38Z

src/js/common_shim.js

+    };
+
+    var getFirefoxVersion = function(description) {
+      // TODO: Is there a better solution for detecting Firefox?


If this is for detecting the remote browser's Firefox version, please call it detectRemoteFirefoxVersion. For local browser, we have utils.detectBrowser().

alvestrand · 2017-11-09T07:15:04Z

src/js/common_shim.js

+    var getCanSendMaxMessageSize = function(description, remoteIsFirefox) {
+      // Every implementation we know can send at least 64 KiB.
+      // Note: Although Chrome is technically able to send up to 256 KiB, the
+      //       data does not reach the other peer reliably.


If you have an issue number for Chrome here, please add it. (If not, please please please file one!)

Good point. I'll check if there is one already or file it.

Tracked here

alvestrand · 2017-11-09T07:18:10Z

test/e2e/expectations/chrome-beta

 PASS addTrack throws an exception if the track has already been added
 PASS addTrack throws an exception if the track has already been added via addStream
-PASS addTrack throws an exception if addStream is called with a stream containing a track already added
+ERR addTrack throws an exception if addStream is called with a stream containing a track already added AssertionError


Why have these tests started failling with the patch?

Ah, it wasn't my intention to commit this. There was an issue with tests but @fippo told me it might have been a new Chrome beta version. I'll rebase against master and we'll see if it works.

alvestrand · 2017-11-09T07:22:52Z

test/e2e/maxMessageSize.js

+        /^a=max-message-size:\s*(\d+)\s*$/gm, '');
+      description.sdp = description.sdp.replace(
+        /(^m=application\s+\d+\s+[\w\/]*SCTP.*$)/m,
+        '$1\r\na=max-message-size:' + maxMessageSize);


Won't this insert a second max-message-size line into your SDP if it already has one?

No, the replace in line 40 will remove the entire a=max-message-size line if there is one.

alvestrand · 2017-11-09T07:24:24Z

test/e2e/maxMessageSize.js

+  it('send largest possible single message', () => {
+    // Note: Patching to 65535 here as anything beyond that will take too long.
+    const maxMessageSize = 65535;
+    const patchMaxMessageSize = patchMaxMessageSizeFactory(maxMessageSize);


Will this test always fail if the negotiated size is < 65535?

The short answer is yes, but it's a little bit more complicated than that. Every implementation supports 64 KiB unless one that doesn't handle EOR correctly fiddles with usrsctp's buffer sizes (256 KiB send/receive buffer by default results in the partial delivery size of 64 KiB) which no one has done so far. But there's at least one case where the negotiated size is 16 KiB (Firefox to Chrome). That's not a problem here because these tests will always create a peer connection with the same browser.

fippo · 2017-11-09T11:54:43Z

src/js/common_shim.js

+      // Note: 65535 bytes is the default value from the SDP spec. Also,
+      //       every implementation we know supports receiving 65535 bytes.
+      var maxMessageSize = 65535;
+      var match = SDPUtils.matchPrefix(description.sdp, 'a=max-message-size:');


where is the sdp package imported? I don't understand why this works ;-)

It was already imported. :)

https://github.com/lgrahl/adapter/blob/ba8c0fa4ed39cc79f66ff418f0a292a83ae7b6ef/src/js/common_shim.js#L11

why does github not show it in the diff? Also note that you need to add it to package.json, currently it is a child dependeny

Because I didn't add it. It was already there. It looks like it is in package.json already? https://github.com/webrtc/adapter/blob/master/package.json#L23

I need new 👓

No worries. Me too. 😁 And it's better to double-check.

lgrahl · 2017-11-09T12:04:18Z

Rebased against master.

fippo · 2017-11-09T12:26:26Z

src/js/common_shim.js

+    }
+
+    var sctpInDescription = function(description) {
+      var match = description.sdp.match(/m=application\s+\d+\s+[\w/]*SCTP/);


how about this:

var sections = SDPUtils.splitSections(description.sdp); sections.shift(); return sections.some(function(mediaSection) { var mLine = SDPUtils.parseMLine(mediaSection); return mLine && mLine.kind === 'application' && mLine.protocol === 'DTLS/SCTP'; });

if we have SDPUtils anyway... needs a min version of 2.4.0 however

Personally, I don't mind either solutions. Be aware that it may not always be DTLS/SCTP but also UDP/DTLS/SCTP and TCP/DTLS/SCTP, see: https://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-26#section-4.2

alvestrand · 2017-11-11T04:47:06Z

Someone modified the DTMF test to use a transceiver. Transceivers aren't in Chrome stable.
I don't understand how this could be caused by this PR, so perhaps mainline is busted?

fippo · 2017-11-11T08:48:09Z

@alvestrand no. The transceiver check fails (https://travis-ci.org/webrtc/adapter/jobs/299697848#L2760) but that is in the expectation file.

The DTMF/addTrack test which fails against expectation has been flaky (on travis) for quite some time. More so with experimental webplatform features if I recall correctly. Hitting rerun (which requires commit bit unfortunately) a couple of times usually solves it :-/

(I would only do so after squashing)

lgrahl · 2017-11-28T11:46:37Z

Anything left preventing this from being merged?

KaptenJansson · 2017-12-08T12:41:51Z

Set for major release since this a new shim?

fippo · 2017-12-08T12:43:04Z

minor version is for new features. Unless 🐔 🐔 🐔

lgrahl · 2017-12-16T14:28:05Z

~~Ping.~~

Edit: I just found a minor bug: The default MMS should be 65536 instead of 65535... always gets me. 😒 Will push a fix.

Edit: This also affects FF >= 57. If we can't get a backport fix, this PR will need updating once more.

lgrahl · 2017-12-21T13:12:46Z

FF 57 will not be fixed, so I'll add a fix to this PR.
I'll also add some further tests to go through a lot of negotiated MMS cases because they're pretty complicated now.

lgrahl · 2017-12-21T22:44:36Z

Some tests disconnected and need to be restarted. (Some still fail in other places but I guess you know that already as it's also happening on master.)

…ilable.

Fix handle maxMessageSize 0 case properly Fix handle corner case for maxMessageSize when sending from FF to FF Fix handle yet another corner case for maxMessageSize when sending from FF < 57 (uses 16 KiB chunks)

… well There's a bug in the implementation of FF 57, so DOMError is raised instead of TypeError. I hope we can land a hotfix for that but it shouldn't prevent us from merging this now.

…sageSize`

…ror (apparently, I've misinterpreted the spec and this is how it should be done) Fix max message size tests

…l maximum message size at all) Remove MMS 0 case test (not really applicable to an e2e test)

…cy reasons

…error

…ated

…han 64 KiB)

… messages of arbitrary size

…n various combinations Fix default remote maximum message size for Firefox Fix default remote message size for Firefox 57 Don't apply the 'send' shim to Firefox >= 57 Export commonShim to window.adapter as well

lgrahl · 2017-12-21T23:06:07Z

Rebased against master.
Currently investigating why the test runner doesn't catch the exception raised by the native send call of Firefox >= 57.

Add a MMS test for subsequent data channels The send shim has been re-enabled for Firefox >= 57 due to the following bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1426831

Update expected MMS for FF >= 57

lgrahl · 2017-12-26T13:50:06Z

Resolved the problem with the test. It turned out to be a bug in Firefox and I had to dramatically decrease the maximum message size.

Please, don't be irritated by me adding more commits. It's just that I didn't want to hold back these changes. It's good to merge now (from my side) and I'll backlog further changes in another branch until this is getting merged.

Edit: URL fixed.

fippo · 2017-12-26T15:01:36Z

finding bugs by writing code... 👍 👍 👍

KaptenJansson · 2018-01-12T08:37:14Z

@alvestrand any thoughts?

alvestrand · 2018-01-12T12:34:43Z

Looks good to me, and the "update branch" button worked (for a wonder). I'll merge it once the checks complete.

googlebot added the cla: no label Oct 26, 2017

googlebot added cla: yes and removed cla: no labels Oct 26, 2017

fippo reviewed Oct 26, 2017

View reviewed changes

fippo approved these changes Oct 26, 2017

View reviewed changes

alvestrand reviewed Nov 9, 2017

View reviewed changes

fippo reviewed Nov 9, 2017

View reviewed changes

lgrahl force-pushed the dc-max-message-size branch from ba8c0fa to 7ac36d9 Compare November 9, 2017 11:56

fippo reviewed Nov 9, 2017

View reviewed changes

This was referenced Nov 29, 2017

webrtc: Test RTCSctpTransport.maxMessageSize attribute web-platform-tests/wpt#8495

Merged

RTCSctpTransport: Specify special cases for maxMessageSize w3c/webrtc-pc#1656

Merged

KaptenJansson approved these changes Dec 8, 2017

View reviewed changes

lgrahl mentioned this pull request Dec 16, 2017

Add data channel benchmarks saltyrtc/saltyrtc-task-webrtc-js#20

Open

2 tasks

Add dummy RTCSctpTransport to make the 'maxMessageSize' attribute ava…

78e250a

…ilable.

lgrahl added 16 commits December 22, 2017 00:04

Fix maxMessageSize also depends on the local maximum message size

f84ce3d

Fix handle maxMessageSize 0 case properly Fix handle corner case for maxMessageSize when sending from FF to FF Fix handle yet another corner case for maxMessageSize when sending from FF < 57 (uses 16 KiB chunks)

Fix handle maxMessageSize 0 case properly (again)

5d8cec2

Fix broken maxMessageSize tests

4dd2784

Fix apply TypeError shim when sending on data channels to FF >= 57 as…

22a2918

… well There's a bug in the implementation of FF 57, so DOMError is raised instead of TypeError. I hope we can land a hotfix for that but it shouldn't prevent us from merging this now.

Replace regular expression with SDPUtils.matchPrefix where useful

e78bcce

Fix regression introduced by using SDPUtils.matchPrefix in `getMaxMes…

d268eed

…sageSize`

Fix throw DOMException of type TypeError instead of throwing a TypeEr…

7de1187

…ror (apparently, I've misinterpreted the spec and this is how it should be done) Fix max message size tests

Fix determine maximum message size correctly (do not rely on the loca…

9c29712

…l maximum message size at all) Remove MMS 0 case test (not really applicable to an e2e test)

Fix also apply the 'TypeError on send' shim to FF >= 57 for consisten…

d6cf288

…cy reasons

Fix expect tests to pass that were accidentally changed to expect to …

8bfca85

…error

Rename getFirefoxVersion to getRemoteFirefoxVersion

c5c683b

Replace regex with SDPUtils for checking whether SCTP is being negoti…

0711e9d

…ated

Reference Chrome data channel send issue (cannot reliably send more t…

64600a6

…han 64 KiB)

Set maxMessageSize to positive infinity in case both peers can handle…

5604ef3

… messages of arbitrary size

Fix default remote maximum message size (65535 -> 65536)

dac3ecf

Add test cases for the expected and negotiated maximum message size i…

ded8522

…n various combinations Fix default remote maximum message size for Firefox Fix default remote message size for Firefox 57 Don't apply the 'send' shim to Firefox >= 57 Export commonShim to window.adapter as well

lgrahl force-pushed the dc-max-message-size branch from b88320b to ded8522 Compare December 21, 2017 23:04

lgrahl added 3 commits December 22, 2017 14:36

Re-enable the send shim for Firefox >= 57

9aa3d15

Add a MMS test for subsequent data channels The send shim has been re-enabled for Firefox >= 57 due to the following bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1426831

Update e2e test expectations

6aea672

Add workaround for https://bugzilla.mozilla.org/show_bug.cgi?id=1426831

359e6df

Update expected MMS for FF >= 57

Merge branch 'master' into dc-max-message-size

2d69512

Merge branch 'master' into dc-max-message-size

d96f5cc

KaptenJansson merged commit ba25960 into webrtcHacks:master Jan 16, 2018

lgrahl mentioned this pull request Mar 8, 2018

Maximum message size can be dropped saltyrtc/saltyrtc-meta#130

Closed

lgrahl deleted the dc-max-message-size branch March 29, 2018 12:58

Uh oh!

Data channel message size limitations #702

Data channel message size limitations #702

Uh oh!

Conversation

lgrahl commented Oct 26, 2017

Uh oh!

googlebot commented Oct 26, 2017

Uh oh!

googlebot commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgrahl commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgrahl commented Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgrahl commented Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgrahl commented Oct 31, 2017

Uh oh!

lgrahl commented Nov 3, 2017

Uh oh!

alvestrand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgrahl Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgrahl Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgrahl commented Nov 9, 2017

Uh oh!

fippo Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvestrand commented Nov 11, 2017

Uh oh!

fippo commented Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgrahl commented Oct 26, 2017 •

edited

Loading

lgrahl commented Oct 28, 2017 •

edited

Loading

lgrahl commented Oct 28, 2017 •

edited

Loading

lgrahl Nov 9, 2017 •

edited

Loading

lgrahl Nov 9, 2017 •

edited

Loading

fippo Nov 9, 2017 •

edited

Loading

fippo commented Nov 11, 2017 •

edited

Loading

lgrahl commented Dec 16, 2017 •

edited

Loading

lgrahl commented Dec 21, 2017 •

edited

Loading

lgrahl commented Dec 26, 2017 •

edited

Loading