Closed Bug 989936 Opened 6 years ago Closed 5 months ago

PeerConnection.onsignalingstatechange is not triggered after PC.close called

Categories

(Core :: WebRTC, defect, P5)

defect

Tracking

()

RESOLVED FIXED
Blocking Flags:

People

(Reporter: u459114, Assigned: drno, NeedInfo)

References

Details

Attachments

(1 file)

According to spec(http://dev.w3.org/2011/webrtc/editor/webrtc.html#widl-RTCPeerConnection-onsignalingstatechange), onsignalingstatechange should be triggered after peerconnection.close called.

Here is my testing environment 
1. WebRTC server - https://github.com/CJKu/nightly-gupshup. I did some UI change from origin version.
2. Create two webrtc client on different machines.
3. Start a video call between and click "Hang Up", which calls pc.close(), to hang up that call.
4. There is no "onsignalingstatechange triggered" log in webconsole
https://github.com/CJKu/nightly-gupshup/blob/master/static/js/chat.js#L114
OS: Linux → All
Hardware: x86_64 → All
onremovestream is not received after pc.removeStream called in another peer.

onremovestream of type EventHandler,
This event handler, of event handler event type removestream, MUST be fired by all objects implementing the RTCPeerConnection interface. It is called any time a MediaStream is removed by the remote peer. This will be fired only as a result of setRemoteDescription.
removeStream() isn't implemented in FF yet (it requires renegotiation).
yes, I notices undefined console log after post that comment.

Hi Randell, is there a way that a client is able to know another peer had close connection via API that we have? Except send a event to server and let server send serer event to notify another client(three way communication instead of peer-to-peer connection), I don't know how to finish end call scenario correctly.
Flags: needinfo?(rjesup)
(In reply to C.J. Ku[:CJKu] from comment #3)
> yes, I notices undefined console log after post that comment.
> 
> Hi Randell, is there a way that a client is able to know another peer had
> close connection via API that we have? Except send a event to server and let
> server send serer event to notify another client(three way communication
> instead of peer-to-peer connection), I don't know how to finish end call
> scenario correctly.

Noticing shutdown is tricky.  The peer may close the tab.  They may kill the browser.  They may navigate away.  The browser can crash.  The computer could be shutdown or go to sleep.  They could lose connectivity (when does this amount to a failed call?)  Their IP address could change.  etc...

You have to handle all these cases.  To do that, signalingstate/removestream/etc don't cut it.  At best, after LONG timeouts, we can decide the connection has failed.  Apps can watch incoming packets, and if they unexpectedly stop, ping the sender.  (DataChannel is the fastest/most direct way).  If they don't respond "quickly", assume they're dead *or* they've lost connectivity temporarily (i.e. normal WiFi problems).  Figuring out the difference is tough; basically you just have to wait and in the meantime tell the user "something", or decide you don't care about this case and just kill it fast.

There are more subtleties.  You can use a DataChannel (or the signaling server) to also send notification of "clean" call ends before tearing the PeerConnection down (send it, wait a short while for an Ack, then tear the call down).  You can try to send them on navigation, but you can't guarantee the request will get onto the wire before the shutdown is complete without some fancy footwork.  But that doesn't remove the need to handle non-clean ends.
Flags: needinfo?(rjesup)
I believe this is related to Bug 929977.
I think we should use this ticket to track and implement the missing onsignalingstatechange event after close() got called, like CJ initially reported.
To quote http://dev.w3.org/2011/webrtc/editor/webrtc.html#methods in the close section:

3. Set the object's RTCPeerConnection signalingState to closed.

I think that state transition pretty clearly should result in a callback.

Max, Randel: thanks for pointing out this related bug. I would suggest that we can discuss the bigger problem of trying to solve all the "corner cases" of temporary and permanent connection interruptions in Bug 929977.
Assignee: nobody → drno
This patch sends at least the signal on the local side once the C++ Impl has reached the closed state.
How to get to the closed state and send the signal if something happened on/to to the remote side is a separate topic.
Attachment #8407936 - Flags: review?(rjesup)
Attachment #8407936 - Flags: review?(rjesup) → review+
Requesting leave-open as this patch does not solve the remote side yet, but it is important to get this patch in for our mochitests.
Comment on attachment 8407936 [details] [diff] [review]
send_onsignalingstatechange_event.patch

what happens in the case the onsignalingstatechange is not triggered, won't the test still pass ?
(In reply to dclarke@mozilla.com  [:onecyrenus] from comment #12)
> Comment on attachment 8407936 [details] [diff] [review]
> send_onsignalingstatechange_event.patch
> 
> what happens in the case the onsignalingstatechange is not triggered, won't
> the test still pass ?

for further reference am referring to the patch for pc.js, where you are setting the onsignalingstatechange
Nils, i think this might be a bad patch, can you provide an answer to above ?
Flags: needinfo?(drno)
Flags: needinfo?(rjesup)
dclarke: I think the state changing causes check_next_test(), which checks EventFired && stateChanged.  Without deeper look at *all* the code I can't tell you definitively, but it looks like it should work.
Flags: needinfo?(rjesup)
ok well then i won't worry about it.  

*The one thing is in 95% of the cases close is called outside of the above mentioned command loop. *

teardown is not called as part of the template... so it just a check that will not throw an error.
Setting the onsignalingstatechange handler in the PCT_close close function (line 488 down) in pc.js is only to make all existing tests pass. I'm only verifying that I'm actually getting the expected event type 'closed' as an additional precaution. Without this the default event handler is unexpectedEventAndFinish which interrupts any test.
Flags: needinfo?(drno)
Nils: got it, i thought you were relying on it, but it is just an added precaution, i removed the closed event test from my patch.
Blocks: 965656
Since one patch landed, should this be retitled for what's left?
backlog: --- → webRTC+
Rank: 45
Priority: -- → P4
Mass change P4->P5 to align with new Mozilla triage process.
Priority: P4 → P5
I don't have the capacity any more to work on this any time soon.
Assignee: drno → nobody

The leave-open keyword is there and there is no activity for 6 months.
:drno, maybe it's time to close this bug?

Flags: needinfo?(drno)

The leave-open keyword is there and there is no activity for 6 months.
:drno, maybe it's time to close this bug?

Flags: needinfo?(drno)

There is nothing in the spec that says that closing a PC will result in the other PC closing too. Closing a PC that is streaming media should be causing a mute event to fire on the other end, but that's bug 1545855.

Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.