Unexpected retransmissions on a completely unreliable data channel

RESOLVED FIXED in Firefox 62

Status

()

defect
P2
normal
Rank:
15
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: useruser, Assigned: weinrank)

Tracking

60 Branch
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(6 attachments)

Reporter

Description

a year ago
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

a year ago
Michael, could you take a look? There should also be verbose debug information in the logs.
Flags: needinfo?(tuexen)

Comment 2

a year ago
In which of the tracefiles do you see a retransmission? Which frame?
Flags: needinfo?(tuexen)

Updated

a year ago
Flags: needinfo?(lennart.grahl)

Comment 3

a year ago
Found retransmitted data chunks. Will look into it.
Flags: needinfo?(lennart.grahl)

Updated

a year ago
Component: Untriaged → WebRTC: Networking
Product: Firefox → Core
Summary: Unexpected retransmissions on a completely unreliable channel → Unexpected retransmissions on a completely unreliable data channel

Comment 4

a year ago
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

a year ago
OK, Felix has a patch for it...
Flags: needinfo?(weinrank)

Updated

a year ago
Assignee: nobody → weinrank
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee

Comment 6

a year ago
Flags: needinfo?(weinrank)
Assignee

Comment 7

a year ago
How to progress? :)
Flags: needinfo?(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
Flags: needinfo?(drno)
Rank: 15
Priority: -- → P2
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?
Flags: needinfo?(dminor)
Sure, I'll have a look this morning.
Flags: needinfo?(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 hidden (mozreview-request)

Comment 16

a year ago
mozreview-review
Comment on attachment 8982561 [details]
Bug 1464917 - Add maxPacketLifeTime and maxRetransmits to RTCDatachannel.webidl;

https://reviewboard.mozilla.org/r/248498/#review254762
Attachment #8982561 - Flags: review?(amarchesini) → review+

Comment 17

a year ago
mozreview-review
Comment on attachment 8982560 [details]
Bug 1464917 - Allow maxPacketLifeTime and maxRetransmits to be zero;

https://reviewboard.mozilla.org/r/248496/#review254810
Attachment #8982560 - Flags: review?(drno) → review+

Comment 18

a year ago
mozreview-review
Comment on attachment 8982562 [details]
Bug 1464917 - Add GetMaxPacketLifeTime and GetMaxRetransmits to DataChannel;

https://reviewboard.mozilla.org/r/248500/#review254812
Attachment #8982562 - Flags: review?(drno) → review+

Comment 19

a year ago
mozreview-review
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.
Attachment #8982563 - Flags: review?(drno) → review+

Comment 20

a year ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 22

a year ago
Pushed by dminor@mozilla.com:
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

Updated

5 months ago
Duplicate of this bug: 881532

Updated

5 months ago
Duplicate of this bug: 1278384
You need to log in before you can comment on or make changes to this bug.