Protect WebRTC network monitoring to wait forever in edge cases
https://bugs.webkit.org/show_bug.cgi?id=204846
Source/WebKit:
Reviewed by Eric Carlson.
We were limiting the number of IPC message sent to network process by only sending the start monitoring event for the first client.
The issue is that, if network process crashes for instance while having not yet given the list of networks, all clients will be hanging
waiting for the completion of network list.
We are now sending an IPC message for every client and the network process will ignore the ones that are not useful.
In addition, in case of network process crash, we send a signal that network list has changed to make sure clients will never hang.
They might still fail connecting, which is ok since network process crashed.
Test: webrtc/datachannel/gather-candidates-networkprocess-crash.html
* NetworkProcess/webrtc/NetworkRTCMonitor.cpp:
(WebKit::NetworkRTCMonitor::startUpdatingIfNeeded):
* NetworkProcess/webrtc/NetworkRTCMonitor.h:
* NetworkProcess/webrtc/NetworkRTCMonitor.messages.in:
* WebProcess/Network/webrtc/LibWebRTCNetwork.h:
(WebKit::LibWebRTCNetwork::networkProcessCrashed):
* WebProcess/Network/webrtc/WebRTCMonitor.cpp:
(WebKit::WebRTCMonitor::StartUpdating):
(WebKit::WebRTCMonitor::StopUpdating):
(WebKit::WebRTCMonitor::networksChanged):
(WebKit::WebRTCMonitor::networkProcessCrashed):
* WebProcess/Network/webrtc/WebRTCMonitor.h:
* WebProcess/WebProcess.cpp:
(WebKit::WebProcess::networkProcessConnectionClosed):
LayoutTests:
<rdar://problem/57618773>
Reviewed by Eric Carlson.
* webrtc/datachannel/gather-candidates-networkprocess-crash-expected.txt: Added.
* webrtc/datachannel/gather-candidates-networkprocess-crash.html: Added.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@253203 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 57d076d..aec00ca 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,14 @@
+2019-12-06 youenn fablet <youenn@apple.com>
+
+ Protect WebRTC network monitoring to wait forever in edge cases
+ https://bugs.webkit.org/show_bug.cgi?id=204846
+ <rdar://problem/57618773>
+
+ Reviewed by Eric Carlson.
+
+ * webrtc/datachannel/gather-candidates-networkprocess-crash-expected.txt: Added.
+ * webrtc/datachannel/gather-candidates-networkprocess-crash.html: Added.
+
2019-12-05 Yury Semikhatsky <yurys@chromium.org>
Web Inspector: http/tests/inspector/target/pause-on-inline-debugger-statement.html is crashing in debug
diff --git a/LayoutTests/webrtc/datachannel/gather-candidates-networkprocess-crash-expected.txt b/LayoutTests/webrtc/datachannel/gather-candidates-networkprocess-crash-expected.txt
new file mode 100644
index 0000000..9cfd9cd
--- /dev/null
+++ b/LayoutTests/webrtc/datachannel/gather-candidates-networkprocess-crash-expected.txt
@@ -0,0 +1,3 @@
+
+PASS Gathering ICE candidates from a data channel while network process is crashing
+
diff --git a/LayoutTests/webrtc/datachannel/gather-candidates-networkprocess-crash.html b/LayoutTests/webrtc/datachannel/gather-candidates-networkprocess-crash.html
new file mode 100644
index 0000000..3f3e3d6
--- /dev/null
+++ b/LayoutTests/webrtc/datachannel/gather-candidates-networkprocess-crash.html
@@ -0,0 +1,28 @@
+<!doctype html>
+<html>
+ <head>
+ <meta charset="utf-8">
+ <title>Testing ICE candidate filtering when using data channel</title>
+ <script src="../../resources/testharness.js"></script>
+ <script src="../../resources/testharnessreport.js"></script>
+ </head>
+ <body>
+ <script>
+promise_test(async (test) => {
+ var counter = 0;
+ var pc = new RTCPeerConnection();
+ pc.createDataChannel('sendDataChannel');
+
+ const iceCandidatePromise = new Promise(r => pc.onicecandidate = r);
+
+ const offer = await pc.createOffer();
+ pc.setLocalDescription(offer);
+ // This test is a bit racy, we are trying to have network process crash between the time it receives a monitor start request and the time it answers that request.
+ if (window.testRunner && testRunner.terminateNetworkProcess)
+ setTimeout(() => testRunner.terminateNetworkProcess(), 0);
+
+ await iceCandidatePromise;
+}, "Gathering ICE candidates from a data channel while network process is crashing");
+ </script>
+ </body>
+</html>
diff --git a/Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.cpp b/Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.cpp
index b7fba0d..e48da18 100644
--- a/Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.cpp
+++ b/Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.cpp
@@ -217,6 +217,11 @@
delete data;
}
+bool LibWebRTCProvider::hasWebRTCThreads()
+{
+ return !!staticFactoryAndThreads().networkThread;
+}
+
void LibWebRTCProvider::callOnWebRTCNetworkThread(Function<void()>&& callback)
{
PeerConnectionFactoryAndThreads& threads = staticFactoryAndThreads();
diff --git a/Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h b/Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h
index e5722d3..e8f99c7 100644
--- a/Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h
+++ b/Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h
@@ -98,6 +98,7 @@
// FIXME: Make these methods not static.
static void callOnWebRTCNetworkThread(Function<void()>&&);
static void callOnWebRTCSignalingThread(Function<void()>&&);
+ static bool hasWebRTCThreads();
// Used for mock testing
void setPeerConnectionFactory(rtc::scoped_refptr<webrtc::PeerConnectionFactoryInterface>&&);
diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
index 6dd03db..9fa658d 100644
--- a/Source/WebKit/ChangeLog
+++ b/Source/WebKit/ChangeLog
@@ -1,5 +1,36 @@
2019-12-06 youenn fablet <youenn@apple.com>
+ Protect WebRTC network monitoring to wait forever in edge cases
+ https://bugs.webkit.org/show_bug.cgi?id=204846
+
+ Reviewed by Eric Carlson.
+
+ We were limiting the number of IPC message sent to network process by only sending the start monitoring event for the first client.
+ The issue is that, if network process crashes for instance while having not yet given the list of networks, all clients will be hanging
+ waiting for the completion of network list.
+ We are now sending an IPC message for every client and the network process will ignore the ones that are not useful.
+ In addition, in case of network process crash, we send a signal that network list has changed to make sure clients will never hang.
+ They might still fail connecting, which is ok since network process crashed.
+
+ Test: webrtc/datachannel/gather-candidates-networkprocess-crash.html
+
+ * NetworkProcess/webrtc/NetworkRTCMonitor.cpp:
+ (WebKit::NetworkRTCMonitor::startUpdatingIfNeeded):
+ * NetworkProcess/webrtc/NetworkRTCMonitor.h:
+ * NetworkProcess/webrtc/NetworkRTCMonitor.messages.in:
+ * WebProcess/Network/webrtc/LibWebRTCNetwork.h:
+ (WebKit::LibWebRTCNetwork::networkProcessCrashed):
+ * WebProcess/Network/webrtc/WebRTCMonitor.cpp:
+ (WebKit::WebRTCMonitor::StartUpdating):
+ (WebKit::WebRTCMonitor::StopUpdating):
+ (WebKit::WebRTCMonitor::networksChanged):
+ (WebKit::WebRTCMonitor::networkProcessCrashed):
+ * WebProcess/Network/webrtc/WebRTCMonitor.h:
+ * WebProcess/WebProcess.cpp:
+ (WebKit::WebProcess::networkProcessConnectionClosed):
+
+2019-12-06 youenn fablet <youenn@apple.com>
+
Output libwebrtc logging from Network Process as release logging
https://bugs.webkit.org/show_bug.cgi?id=204853
diff --git a/Source/WebKit/NetworkProcess/webrtc/NetworkRTCMonitor.cpp b/Source/WebKit/NetworkProcess/webrtc/NetworkRTCMonitor.cpp
index ab706dc..35d9278 100644
--- a/Source/WebKit/NetworkProcess/webrtc/NetworkRTCMonitor.cpp
+++ b/Source/WebKit/NetworkProcess/webrtc/NetworkRTCMonitor.cpp
@@ -40,8 +40,11 @@
ASSERT(!m_manager);
}
-void NetworkRTCMonitor::startUpdating()
+void NetworkRTCMonitor::startUpdatingIfNeeded()
{
+ if (m_isStarted)
+ return;
+
m_isStarted = true;
m_rtcProvider.callOnRTCNetworkThread([this]() {
m_manager = makeUniqueWithoutFastMallocCheck<rtc::BasicNetworkManager>();
diff --git a/Source/WebKit/NetworkProcess/webrtc/NetworkRTCMonitor.h b/Source/WebKit/NetworkProcess/webrtc/NetworkRTCMonitor.h
index d834def..d5c1725 100644
--- a/Source/WebKit/NetworkProcess/webrtc/NetworkRTCMonitor.h
+++ b/Source/WebKit/NetworkProcess/webrtc/NetworkRTCMonitor.h
@@ -51,7 +51,7 @@
bool isStarted() const { return m_isStarted; }
private:
- void startUpdating();
+ void startUpdatingIfNeeded();
void onNetworksChanged();
diff --git a/Source/WebKit/NetworkProcess/webrtc/NetworkRTCMonitor.messages.in b/Source/WebKit/NetworkProcess/webrtc/NetworkRTCMonitor.messages.in
index 4408d82..472e51f1 100644
--- a/Source/WebKit/NetworkProcess/webrtc/NetworkRTCMonitor.messages.in
+++ b/Source/WebKit/NetworkProcess/webrtc/NetworkRTCMonitor.messages.in
@@ -23,7 +23,7 @@
#if USE(LIBWEBRTC)
messages -> NetworkRTCMonitor NotRefCounted {
- void StartUpdating()
+ void StartUpdatingIfNeeded()
void StopUpdating()
}
diff --git a/Source/WebKit/WebProcess/Network/webrtc/LibWebRTCNetwork.h b/Source/WebKit/WebProcess/Network/webrtc/LibWebRTCNetwork.h
index f7e21f7..086c97e 100644
--- a/Source/WebKit/WebProcess/Network/webrtc/LibWebRTCNetwork.h
+++ b/Source/WebKit/WebProcess/Network/webrtc/LibWebRTCNetwork.h
@@ -42,6 +42,8 @@
public:
LibWebRTCNetwork() = default;
+ void networkProcessCrashed();
+
#if USE(LIBWEBRTC)
WebRTCMonitor& monitor() { return m_webNetworkMonitor; }
LibWebRTCSocketFactory& socketFactory() { return m_socketFactory; }
@@ -66,4 +68,11 @@
#endif
};
+inline void LibWebRTCNetwork::networkProcessCrashed()
+{
+#if USE(LIBWEBRTC)
+ m_webNetworkMonitor.networkProcessCrashed();
+#endif
+}
+
} // namespace WebKit
diff --git a/Source/WebKit/WebProcess/Network/webrtc/WebRTCMonitor.cpp b/Source/WebKit/WebProcess/Network/webrtc/WebRTCMonitor.cpp
index 79502c7..6aa8a98 100644
--- a/Source/WebKit/WebProcess/Network/webrtc/WebRTCMonitor.cpp
+++ b/Source/WebKit/WebProcess/Network/webrtc/WebRTCMonitor.cpp
@@ -28,6 +28,7 @@
#if USE(LIBWEBRTC)
+#include "Logging.h"
#include "NetworkConnectionToWebProcessMessages.h"
#include "NetworkProcessConnection.h"
#include "NetworkRTCMonitorMessages.h"
@@ -47,35 +48,36 @@
void WebRTCMonitor::StartUpdating()
{
- if (m_clientCount) {
- // Need to signal new client that we already have the network list, let's do it asynchronously
- if (m_receivedNetworkList) {
- WebCore::LibWebRTCProvider::callOnWebRTCNetworkThread([this] {
- SignalNetworksChanged();
- });
- }
- } else {
- m_receivedNetworkList = false;
- sendOnMainThread([](IPC::Connection& connection) {
- connection.send(Messages::NetworkRTCMonitor::StartUpdating(), 0);
+ RELEASE_LOG(WebRTC, "WebRTCMonitor::StartUpdating");
+ if (m_receivedNetworkList) {
+ WebCore::LibWebRTCProvider::callOnWebRTCNetworkThread([this] {
+ SignalNetworksChanged();
});
}
+
+ sendOnMainThread([](IPC::Connection& connection) {
+ RELEASE_LOG(WebRTC, "WebRTCMonitor asks network process to start updating");
+ connection.send(Messages::NetworkRTCMonitor::StartUpdatingIfNeeded(), 0);
+ });
++m_clientCount;
}
void WebRTCMonitor::StopUpdating()
{
+ RELEASE_LOG(WebRTC, "WebRTCMonitor::StopUpdating");
ASSERT(m_clientCount);
if (--m_clientCount)
return;
sendOnMainThread([](IPC::Connection& connection) {
+ RELEASE_LOG(WebRTC, "WebRTCMonitor asks network process to stop updating");
connection.send(Messages::NetworkRTCMonitor::StopUpdating(), 0);
});
}
void WebRTCMonitor::networksChanged(const Vector<RTCNetwork>& networks, const RTCNetwork::IPAddress& ipv4, const RTCNetwork::IPAddress& ipv6)
{
+ RELEASE_LOG(WebRTC, "WebRTCMonitor::networksChanged");
// No need to protect 'this' as it has the lifetime of LibWebRTC which has the lifetime of the web process.
WebCore::LibWebRTCProvider::callOnWebRTCNetworkThread([this, networks, ipv4, ipv6] {
std::vector<rtc::Network*> networkList(networks.size());
@@ -94,6 +96,18 @@
});
}
+void WebRTCMonitor::networkProcessCrashed()
+{
+ m_receivedNetworkList = false;
+ if (!WebCore::LibWebRTCProvider::hasWebRTCThreads())
+ return;
+
+ // In case we have clients waiting for networksChanged, we call SignalNetworksChanged to make sure they do not wait for nothing.
+ WebCore::LibWebRTCProvider::callOnWebRTCNetworkThread([this] {
+ SignalNetworksChanged();
+ });
+}
+
} // namespace WebKit
#endif // USE(LIBWEBRTC)
diff --git a/Source/WebKit/WebProcess/Network/webrtc/WebRTCMonitor.h b/Source/WebKit/WebProcess/Network/webrtc/WebRTCMonitor.h
index 5143478..f814763 100644
--- a/Source/WebKit/WebProcess/Network/webrtc/WebRTCMonitor.h
+++ b/Source/WebKit/WebProcess/Network/webrtc/WebRTCMonitor.h
@@ -44,6 +44,7 @@
public:
WebRTCMonitor() = default;
+ void networkProcessCrashed();
void didReceiveMessage(IPC::Connection&, IPC::Decoder&);
private:
diff --git a/Source/WebKit/WebProcess/WebProcess.cpp b/Source/WebKit/WebProcess/WebProcess.cpp
index bc66e49..06b08d7 100644
--- a/Source/WebKit/WebProcess/WebProcess.cpp
+++ b/Source/WebKit/WebProcess/WebProcess.cpp
@@ -1244,6 +1244,9 @@
WebSocketStream::networkProcessCrashed();
m_webSocketChannelManager.networkProcessCrashed();
+ if (m_libWebRTCNetwork)
+ m_libWebRTCNetwork->networkProcessCrashed();
+
for (auto& page : m_pageMap.values()) {
page->stopAllURLSchemeTasks();
#if ENABLE(APPLE_PAY)
