Closed
Bug 1315470
Opened 8 years ago
Closed 8 years ago
DataChannel.send() never throws exceptions
Categories
(Core :: WebRTC: Networking, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla52
backlog | webrtc/webaudio+ |
People
(Reporter: drno, Assigned: drno)
References
Details
Attachments
(1 file, 1 obsolete file)
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
backlog: --- → webrtc/webaudio+
Rank: 21
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox-esr45:
--- → affected
Comment 3•8 years ago
|
||
mozreview-review |
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
Comment 5•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/5dfff5a5ff3eb97ef601e5f8f685bd82797dfad2 because passing datachannel-emptystring.html relies on not throwing when !sent, https://treeherder.mozilla.org/logviewer.html#?job_id=6216505&repo=autoland
Assignee | ||
Comment 6•8 years ago
|
||
Interesting. Not sure how this ever worked before... (famous last words)
Assignee | ||
Comment 7•8 years ago
|
||
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...
Assignee | ||
Comment 8•8 years ago
|
||
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.
Comment 9•8 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8807907 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•8 years ago
|
||
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 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8808411 [details]
Bug 1315470: booleans never turn negative.
https://reviewboard.mozilla.org/r/91224/#review92540
Attachment #8808411 -
Flags: review?(rjesup) → review+
Comment 15•8 years ago
|
||
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31b6c03c69f1
booleans never turn negative. r=jesup
Comment 16•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•