Closed
Bug 989936
Opened 11 years ago
Closed 5 years ago
PeerConnection.onsignalingstatechange is not triggered after PC.close called
Categories
(Core :: WebRTC, defect, P5)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
backlog | webrtc/webaudio+ |
People
(Reporter: u459114, Assigned: drno)
References
Details
Attachments
(1 file)
(deleted),
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
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
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.
Comment 2•11 years ago
|
||
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)
Comment 4•11 years ago
|
||
(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)
Comment 5•11 years ago
|
||
I believe this is related to Bug 929977.
Assignee | ||
Comment 6•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: nobody → drno
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
Updated•11 years ago
|
Attachment #8407936 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 9•11 years ago
|
||
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.
Keywords: checkin-needed,
leave-open
Comment 10•11 years ago
|
||
Keywords: checkin-needed
Comment 11•11 years ago
|
||
Flags: in-testsuite+
Comment 12•11 years ago
|
||
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 ?
Comment 13•11 years ago
|
||
(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
Comment 14•11 years ago
|
||
Nils, i think this might be a bad patch, can you provide an answer to above ?
Flags: needinfo?(drno)
Updated•11 years ago
|
Flags: needinfo?(rjesup)
Comment 15•11 years ago
|
||
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)
Comment 16•11 years ago
|
||
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.
Assignee | ||
Comment 17•11 years ago
|
||
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)
Comment 18•11 years ago
|
||
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.
Comment 19•9 years ago
|
||
Since one patch landed, should this be retitled for what's left?
backlog: --- → webRTC+
Rank: 45
Priority: -- → P4
Comment 20•7 years ago
|
||
Mass change P4->P5 to align with new Mozilla triage process.
Priority: P4 → P5
Assignee | ||
Comment 21•6 years ago
|
||
I don't have the capacity any more to work on this any time soon.
Assignee: drno → nobody
Comment 22•6 years ago
|
||
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)
Comment 23•5 years ago
|
||
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)
Comment 24•5 years ago
|
||
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 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Assignee: nobody → drno
Updated•5 years ago
|
Keywords: leave-open
Assignee | ||
Updated•4 years ago
|
Flags: needinfo?(drno)
You need to log in
before you can comment on or make changes to this bug.
Description
•