From f6a8ae285db16d48de9cb35e23c49820bf1b5fb3 Mon Sep 17 00:00:00 2001 From: David Rowe Date: Sun, 4 Jul 2021 15:50:52 +1200 Subject: [PATCH] Fix WebRTC peer connection not being closed properly --- .../src/webrtc/WebRTCDataChannels.cpp | 94 +++++++++++++++---- .../src/webrtc/WebRTCDataChannels.h | 31 +++++- 2 files changed, 100 insertions(+), 25 deletions(-) diff --git a/libraries/networking/src/webrtc/WebRTCDataChannels.cpp b/libraries/networking/src/webrtc/WebRTCDataChannels.cpp index 497ecf9a55..48ec1b7b9b 100644 --- a/libraries/networking/src/webrtc/WebRTCDataChannels.cpp +++ b/libraries/networking/src/webrtc/WebRTCDataChannels.cpp @@ -22,7 +22,7 @@ const std::string ICE_SERVER_URI = "stun://ice.vircadia.com:7337"; -#define WEBRTC_DEBUG +// #define WEBRTC_DEBUG void WDCSetSessionDescriptionObserver::OnSuccess() { @@ -102,6 +102,10 @@ void WDCPeerConnectionObserver::OnDataChannel(rtc::scoped_refptronPeerConnectionStateChanged(newState); } @@ -253,6 +257,20 @@ void WDCConnection::sendIceCandidate(const IceCandidateInterface* candidate) { _parent->sendSignalingMessage(jsonObject); } +void WDCConnection::onPeerConnectionStateChanged(PeerConnectionInterface::PeerConnectionState state) { +#ifdef WEBRTC_DEBUG + const char* STATES[] = { + "New", + "Connecting", + "Connected", + "Disconnected", + "Failed", + "Closed" + }; + qCDebug(networking_webrtc) << "WDCConnection::onPeerConnectionStateChanged() :" << (int)state << STATES[(int)state]; +#endif +} + void WDCConnection::onDataChannelOpened(rtc::scoped_refptr dataChannel) { #ifdef WEBRTC_DEBUG qCDebug(networking_webrtc) << "WDCConnection::onDataChannelOpened() :" @@ -276,16 +294,19 @@ void WDCConnection::onDataChannelOpened(rtc::scoped_refptr void WDCConnection::onDataChannelStateChanged() { auto state = _dataChannel->state(); #ifdef WEBRTC_DEBUG - qCDebug(networking_webrtc) << "WDCConnection::dataChannelStateChanged() :" << (int)state + qCDebug(networking_webrtc) << "WDCConnection::onDataChannelStateChanged() :" << (int)state << DataChannelInterface::DataStateString(state); #endif if (state == DataChannelInterface::kClosed) { - _dataChannel->Close(); + // Close data channel. + _dataChannel->UnregisterObserver(); + _dataChannelObserver = nullptr; _dataChannel = nullptr; - // WEBRTC FIXME: The following line causes the _peerConnectionFactory to fail. - //_peerConnection->Close(); - //_peerConnection = nullptr; - _parent->onDataChannelClosed(this, _dataChannelID); +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "Disposed of data channel"; +#endif + // Close peer connection. + _parent->closePeerConnection(this); } } @@ -320,6 +341,18 @@ bool WDCConnection::sendDataMessage(const DataBuffer& buffer) { return _dataChannel->Send(buffer); } +void WDCConnection::closePeerConnection() { +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "WDCConnection::closePeerConnection()"; +#endif + _peerConnection->Close(); + _peerConnection = nullptr; + _peerConnectionObserver = nullptr; +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "Disposed of peer connection"; +#endif +} + WebRTCDataChannels::WebRTCDataChannels(NodeType_t nodeType, QObject* parent) : _nodeType(nodeType), @@ -348,6 +381,9 @@ WebRTCDataChannels::WebRTCDataChannels(NodeType_t nodeType, QObject* parent) : if (!_peerConnectionFactory) { qCWarning(networking_webrtc) << "Failed to create WebRTC peer connection factory"; } + + // Set up mechanism for closing peer connections. + connect(this, &WebRTCDataChannels::closePeerConnectionSoon, this, &WebRTCDataChannels::closePeerConnectionNow); } WebRTCDataChannels::~WebRTCDataChannels() { @@ -384,18 +420,6 @@ void WebRTCDataChannels::onDataChannelOpened(WDCConnection* connection, quint16 _connectionsByDataChannel.insert(dataChannelID, connection); } -void WebRTCDataChannels::onDataChannelClosed(WDCConnection* connection, quint16 dataChannelID) { -#ifdef WEBRTC_DEBUG - qCDebug(networking_webrtc) << "WebRTCDataChannels::onDataChannelClosed() :" << dataChannelID; -#endif - - // Delete WDCConnection. - _connectionsByWebSocket.remove(connection->getWebSocketID()); - _connectionsByDataChannel.remove(dataChannelID); - // WEBRTC FIXME: The following line causes the _peerConnectionFactory to fail. - //delete connection; -} - void WebRTCDataChannels::onSignalingMessage(const QJsonObject& message) { #ifdef WEBRTC_DEBUG qCDebug(networking_webrtc) << "WebRTCDataChannel::onSignalingMessage()" << message; @@ -481,12 +505,42 @@ rtc::scoped_refptr WebRTCDataChannels::createPeerConnec configuration.servers.push_back(iceServer); #ifdef WEBRTC_DEBUG - qCDebug(networking_webrtc) << "2. Create a new PeerConnection"; + qCDebug(networking_webrtc) << "2. Create a new peer connection"; #endif PeerConnectionDependencies dependencies(peerConnectionObserver.get()); auto result = _peerConnectionFactory->CreatePeerConnection(configuration, std::move(dependencies)); +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "Created peer connection"; +#endif return result; } +void WebRTCDataChannels::closePeerConnection(WDCConnection* connection) { +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "WebRTCDataChannels::closePeerConnection()"; +#endif + // Use Qt's signals/slots mechanism to close the peer connection on its own call stack, separate from the DataChannel + // callback that initiated the peer connection. + // https://bugs.chromium.org/p/webrtc/issues/detail?id=3721 + emit closePeerConnectionSoon(connection); +} + + +void WebRTCDataChannels::closePeerConnectionNow(WDCConnection* connection) { +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "WebRTCDataChannels::closePeerConnectionNow()"; +#endif + // Close the peer connection. + connection->closePeerConnection(); + + // Delete the WDCConnection. + _connectionsByWebSocket.remove(connection->getWebSocketID()); + _connectionsByDataChannel.remove(connection->getDataChannelID()); + delete connection; +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "Disposed of connection"; +#endif +} + #endif // WEBRTC_DATA_CHANNELS diff --git a/libraries/networking/src/webrtc/WebRTCDataChannels.h b/libraries/networking/src/webrtc/WebRTCDataChannels.h index d9bb213a1d..0b8caf80b5 100644 --- a/libraries/networking/src/webrtc/WebRTCDataChannels.h +++ b/libraries/networking/src/webrtc/WebRTCDataChannels.h @@ -48,6 +48,7 @@ public: /// @brief A WebRTC create session description observer. class WDCCreateSessionDescriptionObserver : public CreateSessionDescriptionObserver { public: + WDCCreateSessionDescriptionObserver(WDCConnection* parent); /// @brief The call to CreateAnswer succeeded. @@ -118,6 +119,7 @@ private: class WDCConnection { public: + /// @brief Constructs a new WDCConnection and opens a WebRTC data connection. /// @param webSocketID The signaling channel that initiated the opening of the WebRTC data channel. /// @param parent The parent WebRTCDataChannels object. @@ -155,6 +157,10 @@ public: /// @param candidate The ICE candidate. void sendIceCandidate(const IceCandidateInterface* candidate); + /// @brief Monitors the peer connection state. + /// @param state The new peer connection state. + void onPeerConnectionStateChanged(PeerConnectionInterface::PeerConnectionState state); + /// @brief Handles the WebRTC data channel being opened. /// @param dataChannel The WebRTC data channel. void onDataChannelOpened(rtc::scoped_refptr dataChannel); @@ -171,6 +177,9 @@ public: /// @param buffer The message to send. /// @return `true` if the message was sent, otherwise `false`. bool sendDataMessage(const DataBuffer& buffer); + + /// @brief Closes the WebRTC peer connection. + void closePeerConnection(); private: WebRTCDataChannels* _parent; @@ -228,11 +237,6 @@ public: /// @param dataChannelID The WebRTC data channel ID. void onDataChannelOpened(WDCConnection* connection, quint16 dataChannelID); - /// @brief Handles a WebRTC data channel closing. - /// @param connection The WebRTC data channel connection. - /// @param dataChannelID The WebRTC data channel ID. - void onDataChannelClosed(WDCConnection* connection, quint16 dataChannelID); - /// @brief Emits a signalingMessage to be sent to the Interface client. /// @param message The WebRTC signaling message to send. void sendSignalingMessage(const QJsonObject& message); @@ -254,12 +258,24 @@ public: rtc::scoped_refptr createPeerConnection( const std::shared_ptr peerConnectionObserver); + /// @brief Initiates closing the peer connection for a WebRTC data channel. + /// @details Emits a {@link WebRTCDataChannels.closePeerConnectionSoon} signal which is connected to + /// {@link WebRTCDataChannels.closePeerConnectionNow} in order to close the peer connection on a new call stack. This is + /// necessary to work around a WebRTC library limitation. + /// @param connection The WebRTC data channel connection. + void closePeerConnection(WDCConnection* connection); + public slots: /// @brief Handles a WebRTC signaling message received from the Interface client. /// @param message The WebRTC signaling message. void onSignalingMessage(const QJsonObject& message); + /// @brief Closes the peer connection for a WebRTC data channel. + /// @details Used by {@link WebRTCDataChannels.closePeerConnection}. + /// @param connection The WebRTC data channel connection. + void closePeerConnectionNow(WDCConnection* connection); + signals: /// @brief A WebRTC signaling message to be sent to the Interface client. @@ -273,6 +289,11 @@ signals: /// @param byteArray The Vircadia protocol message. void dataMessage(int dataChannelID, const QByteArray& byteArray); + /// @brief Signals that the peer connection for a WebRTC data channel should be closed. + /// @details Used by {@link WebRTCDataChannels.closePeerConnection}. + /// @param connection The WebRTC data channel connection. + void closePeerConnectionSoon(WDCConnection* connection); + private: QObject* _parent;