Accessing localDescription, remoteDescription, etc. after setTimeout raises EXC_BAD_A...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Jul 2017 22:31:45 +0000 (22:31 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Jul 2017 22:31:45 +0000 (22:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=174323
<rdar://problem/33267876>

Patch by Youenn Fablet <youenn@apple.com> on 2017-07-12
Reviewed by Eric Carlson.

Test: webrtc/calling-peerconnection-once-closed.html

In case the libwebrtc backend is null, we should not use it to get description from it.
Return null in that case.

Adding ASSERT to other calls where the layer above LibWebRTCMediaEndpoint should protect
from calling a function on a null libwebrtc backend.

* Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:
(WebCore::LibWebRTCMediaEndpoint::currentLocalDescription):
(WebCore::LibWebRTCMediaEndpoint::currentRemoteDescription):
(WebCore::LibWebRTCMediaEndpoint::pendingLocalDescription):
(WebCore::LibWebRTCMediaEndpoint::pendingRemoteDescription):
(WebCore::LibWebRTCMediaEndpoint::localDescription):
(WebCore::LibWebRTCMediaEndpoint::remoteDescription):
(WebCore::LibWebRTCMediaEndpoint::doSetLocalDescription):
(WebCore::LibWebRTCMediaEndpoint::doSetRemoteDescription):
(WebCore::LibWebRTCMediaEndpoint::addTrack):
(WebCore::LibWebRTCMediaEndpoint::removeTrack):
(WebCore::LibWebRTCMediaEndpoint::doCreateOffer):
(WebCore::LibWebRTCMediaEndpoint::doCreateAnswer):
(WebCore::LibWebRTCMediaEndpoint::createDataChannel):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219428 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/webrtc/calling-peerconnection-once-closed-expected.txt [new file with mode: 0644]
LayoutTests/webrtc/calling-peerconnection-once-closed.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp

diff --git a/LayoutTests/webrtc/calling-peerconnection-once-closed-expected.txt b/LayoutTests/webrtc/calling-peerconnection-once-closed-expected.txt
new file mode 100644 (file)
index 0000000..5765d23
--- /dev/null
@@ -0,0 +1,10 @@
+
+PASS Ensuring closed connection getters do not crash 
+PASS Ensuring closed connection createOffer does not crash 
+PASS Ensuring closed connection createDataChannel does not crash 
+PASS Ensuring closed connection addTransceiver does not crash 
+PASS Ensuring closed connection addTrack does not crash 
+PASS Ensuring closed connection setRemoteDescription does not crash 
+PASS Ensuring closed connection setLocalDescription does not crash 
+PASS Ensuring closed connection createAnswer does not crash 
+
diff --git a/LayoutTests/webrtc/calling-peerconnection-once-closed.html b/LayoutTests/webrtc/calling-peerconnection-once-closed.html
new file mode 100644 (file)
index 0000000..c72d938
--- /dev/null
@@ -0,0 +1,96 @@
+<!doctype html>
+<html>
+    <head>
+        <meta charset="utf-8">
+        <title>Testing description getters after connection is closed</title>
+        <script src="../resources/testharness.js"></script>
+        <script src="../resources/testharnessreport.js"></script>
+        <script src="routines.js"></script>
+    </head>
+    <body>
+        <script>
+function closedConnection()
+{
+    return new Promise((resolve, reject) => {
+        var localConnection = new RTCPeerConnection();
+        localConnection.close();
+        setTimeout(() => resolve(localConnection), 100);
+    });
+}
+
+promise_test(() => {
+    return closedConnection().then((localConnection) => {
+        assert_equals(localConnection.currentLocalDescription, localConnection.localDescription);
+        assert_equals(localConnection.pendingLocalDescription, localConnection.localDescription);
+
+        assert_equals(localConnection.currentRemoteDescription, localConnection.remoteDescription);
+        assert_equals(localConnection.pendingRemoteDescription, localConnection.remoteDescription);
+    });
+}, "Ensuring closed connection getters do not crash");
+
+promise_test((test) => {
+    return closedConnection().then((connection) => {
+        return promise_rejects(test, 'InvalidStateError', connection.createOffer());
+    });
+}, "Ensuring closed connection createOffer does not crash");
+
+promise_test(() => {
+    return closedConnection().then((connection) => {
+        assert_throws("InvalidStateError", () => {  connection.createDataChannel("test"); });
+    });
+}, "Ensuring closed connection createDataChannel does not crash");
+
+promise_test(() => {
+    return closedConnection().then((connection) => {
+        connection.addTransceiver("video");
+    });
+}, "Ensuring closed connection addTransceiver does not crash");
+
+promise_test(() => {
+    return closedConnection().then((connection) => {
+        return navigator.mediaDevices.getUserMedia({video: true}).then((stream) => {
+            assert_throws("InvalidStateError", () => { connection.addTrack(stream.getVideoTracks()[0], stream); });
+        });
+    });
+}, "Ensuring closed connection addTrack does not crash");
+
+promise_test((test) => {
+    var connection;
+    return closedConnection().then((pc) => {
+        connection = pc;
+        var pc2 = new RTCPeerConnection();
+        pc2.createDataChannel("test");
+        return pc2.createOffer();
+    }).then((sdp) => {
+        return promise_rejects(test, 'InvalidStateError', connection.setRemoteDescription(sdp));
+    });
+}, "Ensuring closed connection setRemoteDescription does not crash");
+
+promise_test((test) => {
+    var localConnection = new RTCPeerConnection();
+    return localConnection.createOffer().then((sdp) => {
+        localConnection.close();
+        return new Promise((resolve, reject) => {
+            setTimeout(() => resolve(sdp), 100);
+        });
+    }).then((sdp) => {
+        return promise_rejects(test, 'InvalidStateError', localConnection.setLocalDescription(sdp));
+    });
+}, "Ensuring closed connection setLocalDescription does not crash");
+
+promise_test((test) => {
+    var localConnection = new RTCPeerConnection();
+    var remoteConnection = new RTCPeerConnection();
+    remoteConnection.createDataChannel("test");
+    return remoteConnection.createOffer().then((sdp) => {
+        return localConnection.setRemoteDescription(sdp);
+    }).then(() => {
+        localConnection.close();
+    }).then(() => {
+        return promise_rejects(test, 'InvalidStateError', localConnection.createAnswer());
+    });
+}, "Ensuring closed connection createAnswer does not crash");
+
+        </script>
+    </body>
+</html>
index 9bf3827..a69bcaa 100644 (file)
@@ -1,3 +1,34 @@
+2017-07-12  Youenn Fablet  <youenn@apple.com>
+
+        Accessing localDescription, remoteDescription, etc. after setTimeout raises EXC_BAD_ACCESS
+        https://bugs.webkit.org/show_bug.cgi?id=174323
+        <rdar://problem/33267876>
+
+        Reviewed by Eric Carlson.
+
+        Test: webrtc/calling-peerconnection-once-closed.html
+
+        In case the libwebrtc backend is null, we should not use it to get description from it.
+        Return null in that case.
+
+        Adding ASSERT to other calls where the layer above LibWebRTCMediaEndpoint should protect
+        from calling a function on a null libwebrtc backend.
+
+        * Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:
+        (WebCore::LibWebRTCMediaEndpoint::currentLocalDescription):
+        (WebCore::LibWebRTCMediaEndpoint::currentRemoteDescription):
+        (WebCore::LibWebRTCMediaEndpoint::pendingLocalDescription):
+        (WebCore::LibWebRTCMediaEndpoint::pendingRemoteDescription):
+        (WebCore::LibWebRTCMediaEndpoint::localDescription):
+        (WebCore::LibWebRTCMediaEndpoint::remoteDescription):
+        (WebCore::LibWebRTCMediaEndpoint::doSetLocalDescription):
+        (WebCore::LibWebRTCMediaEndpoint::doSetRemoteDescription):
+        (WebCore::LibWebRTCMediaEndpoint::addTrack):
+        (WebCore::LibWebRTCMediaEndpoint::removeTrack):
+        (WebCore::LibWebRTCMediaEndpoint::doCreateOffer):
+        (WebCore::LibWebRTCMediaEndpoint::doCreateAnswer):
+        (WebCore::LibWebRTCMediaEndpoint::createDataChannel):
+
 2017-07-12  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r219176.
index 1ceebfc..8b2d3df 100644 (file)
@@ -117,37 +117,38 @@ static inline RefPtr<RTCSessionDescription> fromSessionDescription(const webrtc:
 // FIXME: We might want to create a new object only if the session actually changed for all description getters.
 RefPtr<RTCSessionDescription> LibWebRTCMediaEndpoint::currentLocalDescription() const
 {
-    return fromSessionDescription(m_backend->current_local_description());
+    return m_backend ? fromSessionDescription(m_backend->current_local_description()) : nullptr;
 }
 
 RefPtr<RTCSessionDescription> LibWebRTCMediaEndpoint::currentRemoteDescription() const
 {
-    return fromSessionDescription(m_backend->current_remote_description());
+    return m_backend ? fromSessionDescription(m_backend->current_remote_description()) : nullptr;
 }
 
 RefPtr<RTCSessionDescription> LibWebRTCMediaEndpoint::pendingLocalDescription() const
 {
-    return fromSessionDescription(m_backend->pending_local_description());
+    return m_backend ? fromSessionDescription(m_backend->pending_local_description()) : nullptr;
 }
 
 RefPtr<RTCSessionDescription> LibWebRTCMediaEndpoint::pendingRemoteDescription() const
 {
-    return fromSessionDescription(m_backend->pending_remote_description());
+    return m_backend ? fromSessionDescription(m_backend->pending_remote_description()) : nullptr;
 }
 
 RefPtr<RTCSessionDescription> LibWebRTCMediaEndpoint::localDescription() const
 {
-    return fromSessionDescription(m_backend->local_description());
+    return m_backend ? fromSessionDescription(m_backend->local_description()) : nullptr;
 }
 
 RefPtr<RTCSessionDescription> LibWebRTCMediaEndpoint::remoteDescription() const
 {
-    // FIXME: We might want to create a new object only if the session actually changed.
-    return fromSessionDescription(m_backend->remote_description());
+    return m_backend ? fromSessionDescription(m_backend->remote_description()) : nullptr;
 }
 
 void LibWebRTCMediaEndpoint::doSetLocalDescription(RTCSessionDescription& description)
 {
+    ASSERT(m_backend);
+
     webrtc::SdpParseError error;
     std::unique_ptr<webrtc::SessionDescriptionInterface> sessionDescription(webrtc::CreateSessionDescription(sessionDescriptionType(description.type()), description.sdp().utf8().data(), &error));
 
@@ -168,6 +169,8 @@ void LibWebRTCMediaEndpoint::doSetLocalDescription(RTCSessionDescription& descri
 
 void LibWebRTCMediaEndpoint::doSetRemoteDescription(RTCSessionDescription& description)
 {
+    ASSERT(m_backend);
+
     webrtc::SdpParseError error;
     std::unique_ptr<webrtc::SessionDescriptionInterface> sessionDescription(webrtc::CreateSessionDescription(sessionDescriptionType(description.type()), description.sdp().utf8().data(), &error));
     if (!sessionDescription) {
@@ -182,6 +185,8 @@ void LibWebRTCMediaEndpoint::doSetRemoteDescription(RTCSessionDescription& descr
 
 void LibWebRTCMediaEndpoint::addTrack(RTCRtpSender& sender, MediaStreamTrack& track, const Vector<String>& mediaStreamIds)
 {
+    ASSERT(m_backend);
+
     std::vector<webrtc::MediaStreamInterface*> mediaStreams;
     rtc::scoped_refptr<webrtc::MediaStreamInterface> mediaStream = nullptr;
     if (mediaStreamIds.size()) {
@@ -212,6 +217,8 @@ void LibWebRTCMediaEndpoint::addTrack(RTCRtpSender& sender, MediaStreamTrack& tr
 
 void LibWebRTCMediaEndpoint::removeTrack(RTCRtpSender& sender)
 {
+    ASSERT(m_backend);
+
     auto rtcSender = m_senders.get(&sender);
     if (!rtcSender)
         return;
@@ -250,6 +257,8 @@ bool LibWebRTCMediaEndpoint::shouldOfferAllowToReceiveVideo() const
 
 void LibWebRTCMediaEndpoint::doCreateOffer(const RTCOfferOptions& options)
 {
+    ASSERT(m_backend);
+
     m_isInitiator = true;
     webrtc::PeerConnectionInterface::RTCOfferAnswerOptions rtcOptions;
     rtcOptions.ice_restart = options.iceRestart;
@@ -264,6 +273,8 @@ void LibWebRTCMediaEndpoint::doCreateOffer(const RTCOfferOptions& options)
 
 void LibWebRTCMediaEndpoint::doCreateAnswer()
 {
+    ASSERT(m_backend);
+
     m_isInitiator = false;
     m_backend->CreateAnswer(&m_createSessionDescriptionObserver, nullptr);
 }
@@ -714,6 +725,8 @@ void LibWebRTCMediaEndpoint::OnAddTrack(rtc::scoped_refptr<webrtc::RtpReceiverIn
 
 std::unique_ptr<RTCDataChannelHandler> LibWebRTCMediaEndpoint::createDataChannel(const String& label, const RTCDataChannelInit& options)
 {
+    ASSERT(m_backend);
+
     webrtc::DataChannelInit init;
     if (options.ordered)
         init.ordered = *options.ordered;