Closed Bug 1315470 Opened 5 years ago Closed 5 years ago
Channel .send() never throws exceptions
No description provided.
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 firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/63be72bf8bbd booleans never turn negative. r=jesup
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
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...
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.
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.
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
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 email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/31b6c03c69f1 booleans never turn negative. r=jesup
You need to log in before you can comment on or make changes to this bug.