Closed Bug 1315470 Opened 3 years ago Closed 3 years ago

DataChannel.send() never throws exceptions

Categories

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

52 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- affected
firefox-esr45 --- affected
firefox50 --- affected
firefox51 --- affected
firefox52 --- fixed
Blocking Flags:

People

(Reporter: drno, Assigned: drno)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
backlog: --- → webrtc/webaudio+
Rank: 21
Comment on attachment 8807907 [details]
Bug 1315470: booleans never turn negative.

https://reviewboard.mozilla.org/r/90886/#review90600
Attachment #8807907 - Flags: review?(rjesup) → review+
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/63be72bf8bbd
booleans never turn negative. r=jesup
Interesting. Not sure how this ever worked before... (famous last words)
So even without my patch the SCTP stack returns -1 when sending this empty string, but that error message so far got always turned into a success message.

Makes one wonder how this web platform test successfully receives the empty string on the receiving end...
See Also: → 1315747
So it turn out that the datachannel-emptystring web platform test never actually passed. See bug 1315747.
Apparently the test failure just goes unnoticed by our framework.
Ah, right: https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/webrtc/datachannel-emptystring.html.ini#4 says that we expect the test to fail, because even before your patch we weren't actually sending the empty string, but what your patch did was change the test from failing like we expected to just timing out.

The failure isn't going unnoticed, it's just annotated as being what we expect to have happen until we "properly" send an empty string. So you and jesup just have to decide whether you're okay with the fact that this patch changes our behavior in the case of someone attempting to send an empty string from "just don't send it at all" to "throw when they try", and if you're okay with that then either rewrite the test to notice the throw and treat it as a failure, or change the annotation from expecting FAILURE to expecting TIMEOUT.
See Also: → 1315782
Attachment #8807907 - Attachment is obsolete: true
Radell I put a workaround into the new patch set which is suppose to maintain the current behavior when sending an empty string.

The alternative as Phil described it would be to let it throw and adjust the web platform test. My only concern is that this is a change in behavior of Firefox which people could potentially be unhappy about (although it technically is probably better to point out via the exception that their code is trying to do something which is working like they might expect).

Let me know which option you prefer.
Flags: needinfo?(rjesup)
Talked to Randell on IRC and agreed on letting the exception in place and adapt the platform test accordingly instead of keeping the previous broken behavior.

As it turned out that the reason for the test timing out is actually a bug in the platform test itself I submitted the fix for the test in this PR https://github.com/w3c/web-platform-tests/pull/4202
Flags: needinfo?(rjesup)
Comment on attachment 8808411 [details]
Bug 1315470: booleans never turn negative.

https://reviewboard.mozilla.org/r/91224/#review92540
Attachment #8808411 - Flags: review?(rjesup) → review+
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31b6c03c69f1
booleans never turn negative. r=jesup
https://hg.mozilla.org/mozilla-central/rev/31b6c03c69f1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.