Changeset 283307 in webkit

Timestamp:

Sep 30, 2021, 4:20:49 AM (5 years ago)

Author:

youenn@apple.com

Message:

Layout Test imported/w3c/web-platform-tests/webrtc/RTCPeerConnection-iceConnectionState.https.html is a flaky failure
​https://bugs.webkit.org/show_bug.cgi?id=203256
<rdar://problem/56506063>

Reviewed by Eric Carlson.

Source/WebCore:

Test was flaky for a few reasons:

• Setting local/remote descriptions may change the ICE transports in use so we need to update the ice connection state when setting local/remote descriptions.
• We start observing ICE transport backend state asynchronously and we might miss the checking state which can happen very quickly. Synthesize it if needed.
• We were using the ICE connection state value from the backend as well as computing it from our RTCIceTransport objects. We now fully compute it from RTCIceTransport objects. This ensures we have consistent state between RTCIceTransport and RTCPeerConnection objects with regards to ICE state.

Covered by no longer flaky test.

• Modules/mediastream/PeerConnectionBackend.cpp:

(WebCore::PeerConnectionBackend::setLocalDescriptionSucceeded):
(WebCore::PeerConnectionBackend::setRemoteDescriptionSucceeded):

• Modules/mediastream/RTCPeerConnection.cpp:

(WebCore::RTCPeerConnection::updateIceGatheringState):
(WebCore::RTCPeerConnection::updateIceConnectionState):
(WebCore::RTCPeerConnection::computeIceConnectionStateFromIceTransports):

• Modules/mediastream/RTCPeerConnection.h:
• Modules/mediastream/libwebrtc/LibWebRTCIceTransportBackend.cpp:

(WebCore::LibWebRTCIceTransportBackendObserver::start):

LayoutTests:

• TestExpectations:

Mark webrtc/connection-state.html as flaky as it is probably too restrictive and should be reworked.

• platform/mac-wk2/TestExpectations:

Unflake test.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (1 diff)
• LayoutTests/platform/mac-wk2/TestExpectations (modified) (2 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp (modified) (2 diffs)
• Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp (modified) (2 diffs)
• Source/WebCore/Modules/mediastream/RTCPeerConnection.h (modified) (1 diff)
• Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCIceTransportBackend.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-09-30  Youenn Fablet  <youenn@apple.com>
+
+        Layout Test imported/w3c/web-platform-tests/webrtc/RTCPeerConnection-iceConnectionState.https.html is a flaky failure
+        https://bugs.webkit.org/show_bug.cgi?id=203256
+        <rdar://problem/56506063>
+
+        Reviewed by Eric Carlson.
+
+        * TestExpectations:
+        Mark webrtc/connection-state.html as flaky as it is probably too restrictive and should be reworked.
+        * platform/mac-wk2/TestExpectations:
+        Unflake test.
+
 2021-09-29  Youenn Fablet  <youenn@apple.com>
 

trunk/LayoutTests/TestExpectations

@@ -2073 +2073 @@
 webrtc/h264-baseline.html [ Slow ]
 webrtc/h264-high.html [ Slow ]
+
+webkit.org/b/207798 webrtc/connection-state.html [ Pass Failure ]
 
 imported/w3c/web-platform-tests/webrtc/RTCRtpReceiver-getSynchronizationSources.https.html [ Pass Failure Slow ]

trunk/LayoutTests/platform/mac-wk2/TestExpectations

@@ -988 +988 @@
 webkit.org/b/199089 [ Debug ] plugins/window-open.html [ Skip ]
 
-# <rdar://problem/56506063> Layout Test imported/w3c/web-platform-tests/webrtc/RTCPeerConnection-iceConnectionState.https.html is a flaky failure (203256)
-webkit.org/b/203256 [ Debug ] imported/w3c/web-platform-tests/webrtc/RTCPeerConnection-iceConnectionState.https.html [ Pass Failure ]
-
 webkit.org/b/205808 fast/text/international/unicode-bidi-other-neutrals.html [ Pass Failure ]
 
@@ -1033 +1030 @@
 
 webkit.org/b/207796 [ Release ] fast/box-shadow/hidpi-box-shadow.html [ Pass ImageOnlyFailure ]
-
-webkit.org/b/207798 webrtc/connection-state.html [ Pass Failure ]
 
 webkit.org/b/207938 http/wpt/crypto/derive-hmac-key-crash.any.html [ Pass Crash ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-09-30  Youenn Fablet  <youenn@apple.com>
+
+        Layout Test imported/w3c/web-platform-tests/webrtc/RTCPeerConnection-iceConnectionState.https.html is a flaky failure
+        https://bugs.webkit.org/show_bug.cgi?id=203256
+        <rdar://problem/56506063>
+
+        Reviewed by Eric Carlson.
+
+        Test was flaky for a few reasons:
+        - Setting local/remote descriptions may change the ICE transports in use so we need to update the ice connection state when setting local/remote descriptions.
+        - We start observing ICE transport backend state asynchronously and we might miss the checking state which can happen very quickly. Synthesize it if needed.
+        - We were using the ICE connection state value from the backend as well as computing it from our RTCIceTransport objects. We now fully compute it from RTCIceTransport objects.
+          This ensures we have consistent state between RTCIceTransport and RTCPeerConnection objects with regards to ICE state.
+
+        Covered by no longer flaky test.
+
+        * Modules/mediastream/PeerConnectionBackend.cpp:
+        (WebCore::PeerConnectionBackend::setLocalDescriptionSucceeded):
+        (WebCore::PeerConnectionBackend::setRemoteDescriptionSucceeded):
+        * Modules/mediastream/RTCPeerConnection.cpp:
+        (WebCore::RTCPeerConnection::updateIceGatheringState):
+        (WebCore::RTCPeerConnection::updateIceConnectionState):
+        (WebCore::RTCPeerConnection::computeIceConnectionStateFromIceTransports):
+        * Modules/mediastream/RTCPeerConnection.h:
+        * Modules/mediastream/libwebrtc/LibWebRTCIceTransportBackend.cpp:
+        (WebCore::LibWebRTCIceTransportBackendObserver::start):
+
 2021-09-30  Carlos Garcia Campos  <cgarcia@igalia.com>
 

trunk/Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp

@@ -178 +178 @@
         m_peerConnection.updateTransceiversAfterSuccessfulLocalDescription();
         m_peerConnection.updateSctpBackend(WTFMove(sctpBackend));
-
+        m_peerConnection.processIceTransportChanges();
         callback({ });
     });
@@ -236 +236 @@
         m_peerConnection.updateTransceiversAfterSuccessfulRemoteDescription();
         m_peerConnection.updateSctpBackend(WTFMove(sctpBackend));
+        m_peerConnection.processIceTransportChanges();
         callback({ });
     });

trunk/Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp

@@ -671 +671 @@
 }
 
-void RTCPeerConnection::updateIceConnectionState(RTCIceConnectionState newState)
-{
-    ALWAYS_LOG(LOGIDENTIFIER, newState);
-
-    queueTaskKeepingObjectAlive(*this, TaskSource::Networking, [this, newState] {
-        if (isClosed() || m_iceConnectionState == newState)
+void RTCPeerConnection::updateIceConnectionState(RTCIceConnectionState)
+{
+    queueTaskKeepingObjectAlive(*this, TaskSource::Networking, [this] {
+        if (isClosed())
+            return;
+        auto newState = computeIceConnectionStateFromIceTransports();
+        if (m_iceConnectionState == newState)
             return;
 
@@ -792 +793 @@
 }
 
+void RTCPeerConnection::processIceTransportChanges()
+{
+    auto newIceConnectionState = computeIceConnectionStateFromIceTransports();
+    bool iceConnectionStateChanged = m_iceConnectionState != newIceConnectionState;
+    m_iceConnectionState = newIceConnectionState;
+
+    if (iceConnectionStateChanged && !isClosed())
+        dispatchEvent(Event::create(eventNames().iceconnectionstatechangeEvent, Event::CanBubble::No, Event::IsCancelable::No));
+}
+
 void RTCPeerConnection::updateNegotiationNeededFlag(std::optional<uint32_t> eventId)
 {

trunk/Source/WebCore/Modules/mediastream/RTCPeerConnection.h

@@ -193 +193 @@
 
     void processIceTransportStateChange(RTCIceTransport&);
+    void processIceTransportChanges();
 
     RTCSctpTransport* sctp() { return m_sctpTransport.get(); }

trunk/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCIceTransportBackend.cpp

@@ -109 +109 @@
         internal->SignalIceTransportStateChanged.connect(this, &LibWebRTCIceTransportBackendObserver::onIceTransportStateChanged);
         internal->SignalGatheringState.connect(this, &LibWebRTCIceTransportBackendObserver::onGatheringStateChanged);
-        callOnMainThread([protectedThis = Ref { *this }, transportState = internal->GetIceTransportState(), gatheringState = internal->gathering_state()] {
+        auto transportState = internal->GetIceTransportState();
+        // We start observing a bit late and might miss the checking state. Synthesize it as needed.
+        if (transportState > webrtc::IceTransportState::kChecking && transportState != webrtc::IceTransportState::kClosed) {
+            callOnMainThread([protectedThis = Ref { *this }] {
+                if (protectedThis->m_client)
+                    protectedThis->m_client->onStateChanged(RTCIceTransportState::Checking);
+            });
+        }
+        callOnMainThread([protectedThis = Ref { *this }, transportState, gatheringState = internal->gathering_state()] {
             if (!protectedThis->m_client)
                 return;