Bug 174314 - RTCPeerConnection's close method should update signalingState
Summary: RTCPeerConnection's close method should update signalingState
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-10 10:37 PDT by Mark Roberts
Modified: 2018-03-12 14:16 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.51 KB, patch)
2018-03-12 10:52 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Roberts 2017-07-10 10:37:50 PDT
Run the following code:

    (async () => {
      console.log('1. Constructing an RTCPeerConnection')
      const pc = new RTCPeerConnection()
      const signalingStateChangeEvent = new Promise(resolve => pc.onsignalingstatechange = resolve)
      console.log('2. Closing the RTCPeerConnection')
      pc.close()
      if (pc.signalingState !== 'closed') {
        console.error(`Expected the RTCPeerConnection's signaling state to be "closed", not "${pc.signalingState}"`)
      }
      console.log('3. Awaiting a "signalingstatechange" event')
      await signalingStateChangeEvent
      console.log('Success!')
    })()

Expected output (Chrome, Firefox):

    1. Constructing an RTCPeerConnection
    2. Closing the RTCPeerConnection
    3. Awaiting a "signalingstatechange" event
    Success!

Actual output (Safari):

    1. Constructing an RTCPeerConnection
    2. Closing the RTCPeerConnection
    Expected the RTCPeerConnection's signaling state to be "closed", not "stable"
    3. Awaiting a "signalingstatechange" event

In the actual output, "Success!" never prints because `signalingStateChangeEvent` never resolves. I guess the behavior around this was only recently specified here?

    https://github.com/w3c/webrtc-pc/pull/1371
Comment 1 Mark Roberts 2017-07-10 11:01:35 PDT
RTCPeerConnection's `close` method should also update `iceConnectionState`. Perhaps this issue could be tracked here, too, instead of a separate ticket.

Run the following code:

    (async () => {
      console.log('1. Constructing an RTCPeerConnection')
      const pc = new RTCPeerConnection()
      const iceConnectionStateChangeEvent = new Promise(resolve => pc.oniceconnectionstatechange = resolve)
      console.log('2. Closing the RTCPeerConnection')
      pc.close()
      if (pc.iceConnectionState !== 'closed') {
        console.error(`Expected the RTCPeerConnection's ICE connection state to be "closed", not "${pc.iceConnectionState}"`)
      }
      console.log('3. Awaiting a "iceconnectionstatechange" event')
      await iceConnectionStateChangeEvent
      console.log('Success!')
    })()

Expected output (Firefox):

    1. Constructing an RTCPeerConnection
    2. Closing the RTCPeerConnection
    Expected the RTCPeerConnection's ICE connection state to be "closed", not "new"
    3. Awaiting a "iceconnectionstatechange" event
    Success!

Actual output (Safari):

    1. Constructing an RTCPeerConnection
    2. Closing the RTCPeerConnection
    Expected the RTCPeerConnection's ICE connection state to be "closed", not "new"
    3. Awaiting a "iceconnectionstatechange" event

In the actual output, "Success!" never prints because `iceConnectionStateChangeEvent` never resolves. This behavior seems specified in the Step 4 of `close` and the description of "closed" in RTCIceConnectionState:

    * http://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-close
    * http://w3c.github.io/webrtc-pc/#rtciceconnectionstate-enum

(Note: In Chrome, the error prints, too, I guess because `iceConnectionState` isn't updated synchronously?)
Comment 2 Radar WebKit Bug Importer 2017-07-12 10:40:53 PDT
<rdar://problem/33267977>
Comment 3 youenn fablet 2018-03-12 10:49:58 PDT
Thanks Mark, it is true we are missing the closed state.
I will add it. Looking at the spec, it does not seem to mandate firing an event though.
Maybe this is my reading of the spec.
Comment 4 youenn fablet 2018-03-12 10:50:16 PDT
Testing further, you are right that Chrome and Firefox are firing this event.
Comment 5 youenn fablet 2018-03-12 10:52:07 PDT
Created attachment 335605 [details]
Patch
Comment 6 youenn fablet 2018-03-12 10:57:51 PDT
Filed https://github.com/w3c/webrtc-pc/issues/1799 for clarification about the event part of the bug.
Comment 7 WebKit Commit Bot 2018-03-12 14:16:39 PDT
Comment on attachment 335605 [details]
Patch

Clearing flags on attachment: 335605

Committed r229548: <https://trac.webkit.org/changeset/229548>
Comment 8 WebKit Commit Bot 2018-03-12 14:16:40 PDT
All reviewed patches have been landed.  Closing bug.