We should have a test case which verifies the functionality around RTCPeerConnection.close()

Note: this implementation is known to fail on slower system as the ICEGatheringState does not switch over fast enough.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=9215972a84a6

Try run: https://tbpl.mozilla.org/?tree=Try&rev=d4b371956401

Comment on attachment 8402122 [details] [diff] [review] A new test case for PeerConnection.close() Review of attachment 8402122 [details] [diff] [review]: ----------------------------------------------------------------- r- but easily modified ::: dom/media/tests/mochitest/test_peerConnection_close.html @@ +65,5 @@ > + pc = null; > + > + SimpleTest.finish(); > + }, 1000); > + }, 1000); These timeouts are likely too short, if they're needed at all (see similar issues with other tests recently, especially on B2G emulator debug). Either it will never happen (so either make it a good percentage of mochitest timeout time, or let the test time out), or it will happen in which case we don't really care in this context how long it took. If we care a little about timeliness, we can log if it's longer than N when it happens.

(In reply to Randell Jesup [:jesup] from comment #5) > These timeouts are likely too short, if they're needed at all (see similar > issues with other tests recently, especially on B2G emulator debug). Either > it will never happen (so either make it a good percentage of mochitest > timeout time, or let the test time out), or it will happen in which case we > don't really care in this context how long it took. If we care a little > about timeliness, we can log if it's longer than N when it happens. As you can see from this line in the test todo(signalstatechanged, "Closing the connection should create a callback"); the callback right now never happen. Which is also documented in Bug 989936. So if I have a callback sitting their waiting and then just another timer to ensure the callback fires in X time that is one thing. As the callback right now never fires. We only have three options: 1) Wait for the callback to get implemented properly 2) Have a single long timeout (and sure I can make the timeout longer - but then the test would run that long every single time) 3) Implement a interval based polling (to save time on the faster platforms) And obviously we could and should replace #2 or #3 with #1 once the callback is properly implemented. In case of the second timer I actually wait because I do not expected to get a callback (as not state transition should happen). So there I can only wait for X time. If you think 1s is not long enough in that case I'll be happy to increase the value, but again each test would then run that long.

Comment on attachment 8402122 [details] [diff] [review] A new test case for PeerConnection.close() Can we really implement this mochitest reliably without fixing bug 989936? It feels like the implementation of this test is blocked on the resolution of bug 989936, as I'm not sure if timeouts are going to be reliable here.

Note: this patch depends on landing the patch in 989936. It only tests the locally generated close event for now.

Comment on attachment 8407959 [details] [diff] [review] Patch for testing the PC.close() behavior Review of attachment 8407959 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/tests/mochitest/test_peerConnection_close.html @@ +84,5 @@ > + // This is only a shortcut to prevent a mochitest timeout in case the event does not fire > + var eTimeout = setTimeout(function() { > + ok(signalstatechanged, "Failed to receive expected onsignalingstatechange event in 30s"); > + SimpleTest.finish(); > + }, 30000); let's use at least 60s. Sometimes the testers can be very slow (b2g emulator), and all we really care about is not hitting the global timeout in the unlikely case of an error.

Comment on attachment 8407959 [details] [diff] [review] Patch for testing the PC.close() behavior Review of attachment 8407959 [details] [diff] [review]: ----------------------------------------------------------------- review- mainly for the intermittent failure risk. ::: dom/media/tests/mochitest/pc.js @@ +492,5 @@ > */ > PeerConnectionTest.prototype.close = function PCT_close(onSuccess) { > info("Closing peer connections. Connection state=" + this.connected); > > + signalingstatechangeClose = function (state) { Nit - this patch needs to use camel case throughout each file for variable naming. Also, we need a "var" before this. @@ +594,5 @@ > } > } > > + peer.onsignalingstatechange = function (state) { > + //info(peer + ": 'onsignalingstatechange' event registered, signalingState: " + peer.signalingState); Nit - we should remove this dead code ::: dom/media/tests/mochitest/test_peerConnection_close.html @@ +26,5 @@ > + pc.onsignalingstatechange = function(aEvent) { > + signalstatechanged = true; > + is(aEvent, "closed", "onsignalingstatechange event is 'closed'"); > + is(pc.signalingState, "closed", "Event callback signalingState is 'closed'"); > + is(pc.iceConnectionState, "closed", "Event callback iceConnectionState is 'closed'"); Nit - signalingState & iceConnectionState would have already been checked at this point after close() is called outside of this event handler. I think we only need one check here - so we can remove the other one. @@ +95,5 @@ > + is(pc.signalingState, "closed", "Final signalingState is 'closed'"); > + is(pc.iceConnectionState, "closed", "Final iceConnectionState is 'closed'"); > + is(exception, null, "closing the connection raises no exception"); > + > + if (signalstatechanged) { This might lead to intermittent failures because events can choose to fire arbitrarily sometime in the future (i.e. we don't have direct have control over this). I think what we should do here instead is move this into the onsignalingstatechange event handler itself to ensure that this runs more deterministically. @@ +102,5 @@ > + > + // destroy the PC object to be safe > + pc = null; > + > + SimpleTest.finish(); I think we need to do these cleanup steps (i.e. pc = null & this) at the end of the event handler, as it's currently possible that this test could finish without completing the event handler.

This version can handle closure from outside the event callback (current behavior) as well as within the event callback (in case our implementation behavior should change in the future). Try run: https://tbpl.mozilla.org/?tree=Try&rev=9da326f362ef

Comment on attachment 8423242 [details] [diff] [review] Patch for testing the PC.close() behavior Review of attachment 8423242 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/tests/mochitest/test_peerConnection_close.html @@ +8,5 @@ > +<body> > +<pre id="test"> > +<script type="application/javascript"> > + createHTML({ > + bug: "834153", Nit - bug # should be 991877 @@ +14,5 @@ > + }); > + > + runTest(function () { > + var pc = new mozRTCPeerConnection(); > + var signalstatechanged = false; Nit - camel case @@ +85,5 @@ > + SimpleTest.finish(); > + } > + } > + > + // This is only a shortcut to prevent a mochitest timeout in case the Nit - trailing whitespace @@ +102,5 @@ > + pc.close(); > + } catch (e) { > + exception = e; > + } > + is(exception, null, "closing the connection raises no exception"); Is anything below this point of code really needed? We could just call clearTimeout right after the event handler fires (right after line 26).

Addressed NIT's and added clearTimeout() in the signal handler. Carry forward r+=jsmith.

Comment on attachment 8423463 [details] [diff] [review] Patch for testing the PC.close() behavior Review of attachment 8423463 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/tests/mochitest/test_peerConnection_close.html @@ +40,5 @@ > + is(pc.signalingState, "closed", "Final signalingState stays at 'closed'"); > + is(pc.iceConnectionState, "closed", "Final iceConnectionState stays at 'closed'"); > + > + // TODO: according to the webrtc spec all of these should throw InvalidStateError's > + // instead they seem to throw simple Error's File a bug on this please and reference here

Opened bug 1011586 for the wrong exception on closed connections.