Closed Bug 943898 Opened 6 years ago Closed 6 years ago

Return value 'undefined' is not a valid value for enumeration RTCIceConnectionState.

Categories

(Core :: WebRTC, defect, blocker)

27 Branch
defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: standard8, Assigned: bwc)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Seen on today's nightly 2013-11-27, but not on yesterdays.

Our code is monitoring the ice connection state along the lines of:

this.trigger('ice:' + this.pc.iceConnectionState);

in an oniceconnectionstatechange listener.

This is now failing. On the console we're seeing:

Return value 'undefined' is not a valid value for enumeration RTCIceConnectionState.

This would seem to be a regression from bug 935723, for us it currently stops Talkilla working (as we're relying on the ice state to know when the call has connected) and breaks our tests.
Keywords: regression
Inspecting "this.pc.iceConnectionState" with the debugger shows that pc doesn't have an iceConnectionState until the connection is closed when the call aborts (as it doesn't think it has completed the connection).
Attached patch Possible fix (obsolete) — Splinter Review
This fixes the issue for me locally. Not sure if we should/can have tests for this - whilst I've done this, I won't be able to work on the tests for a few days.
Yep. I will have a slightly more thorough patch for this in a sec.
Replace iceState with iceConnectionState in a few places.
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Attachment #8339347 - Attachment is obsolete: true
Comment on attachment 8339385 [details] [diff] [review]
Fixed an important iceState -> iceConnectionState replacement, and a few less-important ones.

Review of attachment 8339385 [details] [diff] [review]:
-----------------------------------------------------------------

Does this work for you?
Attachment #8339385 - Flags: review?(mbanner)
Comment on attachment 8339385 [details] [diff] [review]
Fixed an important iceState -> iceConnectionState replacement, and a few less-important ones.

Looks good to me. Not sure I can review it, maybe ekr or abr can.
Attachment #8339385 - Flags: review?(mbanner)
Attachment #8339385 - Flags: review?(ekr)
Attachment #8339385 - Flags: review?(adam)
Attachment #8339385 - Flags: feedback+
Comment on attachment 8339385 [details] [diff] [review]
Fixed an important iceState -> iceConnectionState replacement, and a few less-important ones.

Review of attachment 8339385 [details] [diff] [review]:
-----------------------------------------------------------------

This needs a mochitest.
Comment on attachment 8339385 [details] [diff] [review]
Fixed an important iceState -> iceConnectionState replacement, and a few less-important ones.

Review of attachment 8339385 [details] [diff] [review]:
-----------------------------------------------------------------

This seems OK to me, but it needs a test. Please add one or modify the existing tests
and then ask for re-review
Attachment #8339385 - Flags: review?(ekr)
Attachment #8339385 - Flags: review?(adam)
I know we've just had a long weekend, but any chance we can either get an updated patch for this or backout the original that caused the issue until we get this fixed?

I may be able to get someone to work on a test, but I'd want to make sure that Bryon hasn't already got a test in progress.
I do not have a test in progress at the moment, and I have not ramped up on the mochitest stuff yet. So, not sure how much time this will take.
Attached patch Mochitest (obsolete) — Splinter Review
Attachment #8341751 - Flags: review?(ekr)
Is there a reason this mochitest can't use the existing PC test framework?
(In reply to Eric Rescorla (:ekr) from comment #12)
> Is there a reason this mochitest can't use the existing PC test framework?

I started from http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_peerConnection_bug834153.html?force=1 which looked simple enough.
Comment on attachment 8341751 [details] [diff] [review]
Mochitest

Review of attachment 8341751 [details] [diff] [review]:
-----------------------------------------------------------------

After discussing this with Florian on IRC, I believe that instead of
having a new patch you should just add state checks to the overall
framework and not create a new test.
Attachment #8341751 - Flags: review?(ekr) → review-
Reproduced this issue while testing the Social allow multiple workers feature also on Win 7 64-bit and Ubuntu 64-bit.
When calling using two Nightly builds (2013-11-28), the person who initiate the call doesn't see that the callee answers, it still hears the ring tone; after the callee answers, it can be heard in the background along with the ring tone until the call is stopped because of no answer.
OS: Mac OS X → All
Comment on attachment 8342612 [details] [diff] [review]
Add ice state checking to webrtc mochitest framework

Verified this fails without Bryon's patch (JS error about undefined not being a valid return for the icestate), and succeeds with his patch (all dom/tests/mochitest tests).
Attachment #8342612 - Flags: review?(jsmith)
Attachment #8342612 - Flags: review?(ekr)
Comment on attachment 8342612 [details] [diff] [review]
Add ice state checking to webrtc mochitest framework

Review of attachment 8342612 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #8342612 - Flags: review?(ekr) → review+
Attachment #8339385 - Flags: review?(docfaraday)
Comment on attachment 8339385 [details] [diff] [review]
Fixed an important iceState -> iceConnectionState replacement, and a few less-important ones.

Review of attachment 8339385 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, this was Byron's patch
Attachment #8339385 - Flags: review?(docfaraday) → review?(adam)
Attachment #8341751 - Attachment is obsolete: true
Comment on attachment 8339385 [details] [diff] [review]
Fixed an important iceState -> iceConnectionState replacement, and a few less-important ones.

Review of attachment 8339385 [details] [diff] [review]:
-----------------------------------------------------------------

Looks correct. Thanks for cleaning up the other instances of misnaming in here.
Attachment #8339385 - Flags: review?(adam) → review+
Comment on attachment 8342612 [details] [diff] [review]
Add ice state checking to webrtc mochitest framework

Review of attachment 8342612 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

We might want to file a followup bug for making use of the next_ice_state attribute to do more deeper checking of the iceConnectionState.
Attachment #8342612 - Flags: review?(jsmith) → review+
I guess this is ready to go.
Whiteboard: [checkin-needed]
https://hg.mozilla.org/mozilla-central/rev/57833ea1a0d2
https://hg.mozilla.org/mozilla-central/rev/68cea8e73f42
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.