1464917 - Unexpected retransmissions on a completely unreliable data channel

Status: RESOLVED FIXED

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

Description

[Mario Carbajal]

Attachment: datachanneldebug.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180516032328

Steps to reproduce:

Created a datachannel with maxRetransmits set to 0

Send data through that datachannel ensuring some amount of packet loss is happening.


Actual results:

Despite being an unreliable channel and the packets being lost the messages are sometimes (or perhaps always) being resent.


Expected results:

Messages sent through the unreliable datachannel should not be resent.


I'm attaching logs obtained using these instructions: https://github.com/nplab/WebRTC-Data-Channel-Playground/wiki/Analyze-Data-Channel-traffic-with-Wireshark

The logs contain tests with both negotiated and non-negotiated datachannels, since I was advised that it might make a difference.

The unreliable datachannel in question is labeled "uu".

Comment 1

[Lennart Grahl [:lgrahl]]

Michael, could you take a look? There should also be verbose debug information in the logs.

Comment 2

[Michael Tüxen]

In which of the tracefiles do you see a retransmission? Which frame?

Comment 3

[Michael Tüxen]

Found retransmitted data chunks. Will look into it.

Comment 4

[Lennart Grahl [:lgrahl]]

We believe this is an API bug somewhere between JS-land and the data channel stack. If a channel is created via
createDataChannel('', { maxRetransmits: 0 });
a reliable (!) and ordered channel is created. However, if one does create a channel via
createDataChannel('', { maxRetransmits: 1 });
an unreliable (with one maximum retransmission) and ordered channel is created.

It also doesn't matter whether the channel is ordered or not.

Comment 5

[Michael Tüxen]

OK, Felix has a patch for it...

Comment 6

[Felix Weinrank]

Attachment: Allow "maxPacketLifeTime" and "maxRetransmits" to be zero

Comment 7

[Felix Weinrank]

How to progress? :)

Comment 8

[Nils Ohlmeier [:drno]]

(In reply to Felix Weinrank from comment #7)
> How to progress? :)

Awesome! Thank you.

The patch looks good. It would be super helpful to cover this with tests to prevent regressing again. But since there are apparently Web Platform Tests for it already I guess we only need to activate these https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/testing/web-platform/meta/webrtc/RTCPeerConnection-createDataChannel.html.ini#5

Comment 9

[Nils Ohlmeier [:drno]]

Dan, I'm actually not sure if my comment #8 makes sense, or if theses tests still won't pass because we still don't implement that attributes which allow checking of the values the WPT tests are looking for.

This looks like a good candidate for getting your "hands dirty" on data channels. Can I ask you to help Felix get this fixed and landed?

Comment 10

[Dan Minor [:dminor]]

Sure, I'll have a look this morning.

Comment 11

[Dan Minor [:dminor]]

Enabling the tests mentioned in comment #8 does require adding a couple of attributes to the datachannel webidl. That work is almost done, I'll put Felix's and my patches up for review after a bit more testing.

Comment 12

[Dan Minor [:dminor]]

Attachment: Bug 1464917 - Allow maxPacketLifeTime and maxRetransmits to be zero;

Review commit: https://reviewboard.mozilla.org/r/248496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248496/

Comment 13

[Dan Minor [:dminor]]

Attachment: Bug 1464917 - Add maxPacketLifeTime and maxRetransmits to RTCDatachannel.webidl;

Review commit: https://reviewboard.mozilla.org/r/248498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248498/

Comment 14

[Dan Minor [:dminor]]

Attachment: Bug 1464917 - Add GetMaxPacketLifeTime and GetMaxRetransmits to DataChannel;

Review commit: https://reviewboard.mozilla.org/r/248500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248500/

Comment 15

[Dan Minor [:dminor]]

Attachment: Bug 1464917 - Update DataChannel web-platform test expectations;

Review commit: https://reviewboard.mozilla.org/r/248502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248502/

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 8982561 [details]
Bug 1464917 - Add maxPacketLifeTime and maxRetransmits to RTCDatachannel.webidl;

https://reviewboard.mozilla.org/r/248498/#review254762

Comment 17

[Nils Ohlmeier [:drno]]

Comment on attachment 8982560 [details]
Bug 1464917 - Allow maxPacketLifeTime and maxRetransmits to be zero;

https://reviewboard.mozilla.org/r/248496/#review254810

Comment 18

[Nils Ohlmeier [:drno]]

Comment on attachment 8982562 [details]
Bug 1464917 - Add GetMaxPacketLifeTime and GetMaxRetransmits to DataChannel;

https://reviewboard.mozilla.org/r/248500/#review254812

Comment 19

[Nils Ohlmeier [:drno]]

Comment on attachment 8982563 [details]
Bug 1464917 - Update DataChannel web-platform test expectations;

https://reviewboard.mozilla.org/r/248502/#review254814

::: testing/web-platform/meta/webrtc/RTCPeerConnection-createDataChannel.html.ini:5
(Diff revision 1)
>    [createDataChannel with maxPacketLifeTime 0]
> -    expected: FAIL

I think instead of just removing the 'expected: FAIL' you should remove the whole test itself as well. This ini file is basically a black list and the list of tests to be executed like in the mochitest case.

::: testing/web-platform/meta/webrtc/RTCPeerConnection-createDataChannel.html.ini:9
(Diff revision 1)
>    [createDataChannel with both maxPacketLifeTime and maxRetransmits]
>      expected: FAIL

Can you check while you are on it why this test is not passing?
I think I saw code in PeerConnection.js which tries to cover this case.

Comment 20

[Dan Minor [:dminor]]

Comment on attachment 8982563 [details]
Bug 1464917 - Update DataChannel web-platform test expectations;

https://reviewboard.mozilla.org/r/248502/#review254814

> Can you check while you are on it why this test is not passing?
> I think I saw code in PeerConnection.js which tries to cover this case.

We're throwing an InvalidParameterError but the test expects TypeError. TypeError is correct according to the spec.

Comment 21

[Dan Minor [:dminor]]

Comment on attachment 8982563 [details]
Bug 1464917 - Update DataChannel web-platform test expectations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248502/diff/1-2/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2782fc090894
Allow maxPacketLifeTime and maxRetransmits to be zero; r=drno
https://hg.mozilla.org/integration/autoland/rev/160f7211a241
Add maxPacketLifeTime and maxRetransmits to RTCDatachannel.webidl; r=baku
https://hg.mozilla.org/integration/autoland/rev/aadb40090d20
Add GetMaxPacketLifeTime and GetMaxRetransmits to DataChannel; r=drno
https://hg.mozilla.org/integration/autoland/rev/2481a7bf81ea
Update DataChannel web-platform test expectations; r=drno

Comment 23

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2782fc090894
https://hg.mozilla.org/mozilla-central/rev/160f7211a241
https://hg.mozilla.org/mozilla-central/rev/aadb40090d20
https://hg.mozilla.org/mozilla-central/rev/2481a7bf81ea