- Revision
- 203155
- Author
- [email protected]
- Date
- 2016-07-13 01:36:01 -0700 (Wed, 13 Jul 2016)
Log Message
[GTK] WebKitGtk+ uses too many file descriptors
https://bugs.webkit.org/show_bug.cgi?id=152316
Reviewed by Michael Catanzaro.
Source/WebKit2:
The problem is that we are keeping file descriptors open in SharedMemory objects. Those objects can be kept
alive for a long time, for example in case of cached resources sent from the network process as shareable
resources. The thing is that we keep the file descriptor in the SharedMemory object only to be able to create a
Handle, duplicating the file descriptor. However, we are also assuming that we create handles for SharedMemory
objects created by another handle which is wrong. SharedMemory objects are created to map a file or data and
then a handle is created to send it to another process, or to map an existing file or data for a given handle
received from another process. They can also be created to wrap another map, but in that case we don't own the
file descritor nor the mapped data. So, after mapping from a handle, we no longer need the file descriptor, and
it can be closed to release it, while the mapped memory data will still be alive until munmap() is called. This
drastically reduces the amount of file descriptors used by WebKitGTK+.
* Platform/IPC/unix/ConnectionUnix.cpp:
(IPC::readBytesFromSocket): Use setCloseOnExec().
(IPC::Connection::createPlatformConnection): Ditto.
* Platform/SharedMemory.h:
* Platform/unix/SharedMemoryUnix.cpp:
(WebKit::SharedMemory::map): Close the file descriptor right after mmap.
(WebKit::SharedMemory::~SharedMemory): Close the file descriptor only if it hasn't been closed yet.
(WebKit::SharedMemory::createHandle): Add an ASSERT to ensure we only try to create a handle for SharedMemory
objects having a valid file descriptor. Use dupCloseOnExec() to duplicate the handle and set the close on exec flag.
* UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:
(WebKit::ProcessLauncher::launchProcess): Use setCloseOnExec().
Source/WTF:
Add helper functions to duplicate a file descriptor setting close on exec flag, and also to set close on exec
flag to an existing file descriptor.
* wtf/UniStdExtras.h:
* wtf/UniStdExtras.cpp
(WTF::setCloseOnExec):
(WTF::dupCloseOnExec):
Modified Paths
Added Paths
Diff
Modified: trunk/Source/WTF/ChangeLog (203154 => 203155)
--- trunk/Source/WTF/ChangeLog 2016-07-13 07:21:37 UTC (rev 203154)
+++ trunk/Source/WTF/ChangeLog 2016-07-13 08:36:01 UTC (rev 203155)
@@ -1,3 +1,18 @@
+2016-07-13 Carlos Garcia Campos <[email protected]>
+
+ [GTK] WebKitGtk+ uses too many file descriptors
+ https://bugs.webkit.org/show_bug.cgi?id=152316
+
+ Reviewed by Michael Catanzaro.
+
+ Add helper functions to duplicate a file descriptor setting close on exec flag, and also to set close on exec
+ flag to an existing file descriptor.
+
+ * wtf/UniStdExtras.h:
+ * wtf/UniStdExtras.cpp
+ (WTF::setCloseOnExec):
+ (WTF::dupCloseOnExec):
+
2016-07-12 Benjamin Poulain <[email protected]>
[JSC] Array.prototype.join() fails some conformance tests
Modified: trunk/Source/WTF/wtf/PlatformEfl.cmake (203154 => 203155)
--- trunk/Source/WTF/wtf/PlatformEfl.cmake 2016-07-13 07:21:37 UTC (rev 203154)
+++ trunk/Source/WTF/wtf/PlatformEfl.cmake 2016-07-13 08:36:01 UTC (rev 203155)
@@ -1,5 +1,6 @@
list(APPEND WTF_SOURCES
PlatformUserPreferredLanguagesUnix.cpp
+ UniStdExtras.cpp
text/efl/TextBreakIteratorInternalICUEfl.cpp
Modified: trunk/Source/WTF/wtf/PlatformGTK.cmake (203154 => 203155)
--- trunk/Source/WTF/wtf/PlatformGTK.cmake 2016-07-13 07:21:37 UTC (rev 203154)
+++ trunk/Source/WTF/wtf/PlatformGTK.cmake 2016-07-13 08:36:01 UTC (rev 203155)
@@ -8,6 +8,7 @@
glib/MainThreadGLib.cpp
glib/RunLoopGLib.cpp
PlatformUserPreferredLanguagesUnix.cpp
+ UniStdExtras.cpp
text/gtk/TextBreakIteratorInternalICUGtk.cpp
)
Copied: trunk/Source/WTF/wtf/UniStdExtras.cpp (from rev 203154, trunk/Source/WTF/wtf/UniStdExtras.h) (0 => 203155)
--- trunk/Source/WTF/wtf/UniStdExtras.cpp (rev 0)
+++ trunk/Source/WTF/wtf/UniStdExtras.cpp 2016-07-13 08:36:01 UTC (rev 203155)
@@ -0,0 +1,67 @@
+/*
+ * Copyright (C) 2016 Igalia S.L.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+#include "UniStdExtras.h"
+
+#include <fcntl.h>
+
+namespace WTF {
+
+bool setCloseOnExec(int fileDescriptor)
+{
+ int returnValue = -1;
+ do {
+ int flags = fcntl(fileDescriptor, F_GETFD);
+ if (flags != -1)
+ returnValue = fcntl(fileDescriptor, F_SETFD, flags | FD_CLOEXEC);
+ } while (returnValue == -1 && errno == EINTR);
+
+ return returnValue != -1;
+}
+
+int dupCloseOnExec(int fileDescriptor)
+{
+ int duplicatedFileDescriptor = -1;
+#ifdef F_DUPFD_CLOEXEC
+ while ((duplicatedFileDescriptor = fcntl(fileDescriptor, F_DUPFD_CLOEXEC, 0)) == -1 && errno == EINTR) { }
+ if (duplicatedFileDescriptor != -1)
+ return duplicatedFileDescriptor;
+
+#endif
+
+ while ((duplicatedFileDescriptor = dup(fileDescriptor)) == -1 && errno == EINTR) { }
+ if (duplicatedFileDescriptor == -1)
+ return -1;
+
+ if (!setCloseOnExec(duplicatedFileDescriptor)) {
+ closeWithRetry(duplicatedFileDescriptor);
+ return -1;
+ }
+
+ return duplicatedFileDescriptor;
+}
+
+} // namespace WTF
Modified: trunk/Source/WTF/wtf/UniStdExtras.h (203154 => 203155)
--- trunk/Source/WTF/wtf/UniStdExtras.h 2016-07-13 07:21:37 UTC (rev 203154)
+++ trunk/Source/WTF/wtf/UniStdExtras.h 2016-07-13 08:36:01 UTC (rev 203155)
@@ -31,6 +31,9 @@
namespace WTF {
+bool setCloseOnExec(int fileDescriptor);
+int dupCloseOnExec(int fileDescriptor);
+
inline int closeWithRetry(int fileDescriptor)
{
int ret;
@@ -50,5 +53,7 @@
} // namespace WTF
using WTF::closeWithRetry;
+using WTF::setCloseOnExec;
+using WTF::dupCloseOnExec;
#endif // UniStdExtras_h
Modified: trunk/Source/WebKit2/ChangeLog (203154 => 203155)
--- trunk/Source/WebKit2/ChangeLog 2016-07-13 07:21:37 UTC (rev 203154)
+++ trunk/Source/WebKit2/ChangeLog 2016-07-13 08:36:01 UTC (rev 203155)
@@ -1,3 +1,33 @@
+2016-07-13 Carlos Garcia Campos <[email protected]>
+
+ [GTK] WebKitGtk+ uses too many file descriptors
+ https://bugs.webkit.org/show_bug.cgi?id=152316
+
+ Reviewed by Michael Catanzaro.
+
+ The problem is that we are keeping file descriptors open in SharedMemory objects. Those objects can be kept
+ alive for a long time, for example in case of cached resources sent from the network process as shareable
+ resources. The thing is that we keep the file descriptor in the SharedMemory object only to be able to create a
+ Handle, duplicating the file descriptor. However, we are also assuming that we create handles for SharedMemory
+ objects created by another handle which is wrong. SharedMemory objects are created to map a file or data and
+ then a handle is created to send it to another process, or to map an existing file or data for a given handle
+ received from another process. They can also be created to wrap another map, but in that case we don't own the
+ file descritor nor the mapped data. So, after mapping from a handle, we no longer need the file descriptor, and
+ it can be closed to release it, while the mapped memory data will still be alive until munmap() is called. This
+ drastically reduces the amount of file descriptors used by WebKitGTK+.
+
+ * Platform/IPC/unix/ConnectionUnix.cpp:
+ (IPC::readBytesFromSocket): Use setCloseOnExec().
+ (IPC::Connection::createPlatformConnection): Ditto.
+ * Platform/SharedMemory.h:
+ * Platform/unix/SharedMemoryUnix.cpp:
+ (WebKit::SharedMemory::map): Close the file descriptor right after mmap.
+ (WebKit::SharedMemory::~SharedMemory): Close the file descriptor only if it hasn't been closed yet.
+ (WebKit::SharedMemory::createHandle): Add an ASSERT to ensure we only try to create a handle for SharedMemory
+ objects having a valid file descriptor. Use dupCloseOnExec() to duplicate the handle and set the close on exec flag.
+ * UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:
+ (WebKit::ProcessLauncher::launchProcess): Use setCloseOnExec().
+
2016-07-12 Simon Fraser <[email protected]>
[iOS WK2] After zooming and when under memory pressure, page tiles sometimes don't redraw while panning
Modified: trunk/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp (203154 => 203155)
--- trunk/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp 2016-07-13 07:21:37 UTC (rev 203154)
+++ trunk/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp 2016-07-13 08:36:01 UTC (rev 203155)
@@ -308,11 +308,9 @@
memcpy(fileDescriptors.data() + previousFileDescriptorsSize, CMSG_DATA(controlMessage), sizeof(int) * fileDescriptorsCount);
for (size_t i = 0; i < fileDescriptorsCount; ++i) {
- while (fcntl(fileDescriptors[previousFileDescriptorsSize + i], F_SETFD, FD_CLOEXEC) == -1) {
- if (errno != EINTR) {
- ASSERT_NOT_REACHED();
- break;
- }
+ if (!setCloseOnExec(fileDescriptors[previousFileDescriptorsSize + i])) {
+ ASSERT_NOT_REACHED();
+ break;
}
}
break;
@@ -532,14 +530,14 @@
if (options & SetCloexecOnServer) {
// Don't expose the child socket to the parent process.
- while (fcntl(sockets[1], F_SETFD, FD_CLOEXEC) == -1)
- RELEASE_ASSERT(errno != EINTR);
+ if (!setCloseOnExec(sockets[1]))
+ RELEASE_ASSERT_NOT_REACHED();
}
if (options & SetCloexecOnClient) {
// Don't expose the parent socket to potential future children.
- while (fcntl(sockets[0], F_SETFD, FD_CLOEXEC) == -1)
- RELEASE_ASSERT(errno != EINTR);
+ if (!setCloseOnExec(sockets[0]))
+ RELEASE_ASSERT_NOT_REACHED();
}
SocketPair socketPair = { sockets[0], sockets[1] };
Modified: trunk/Source/WebKit2/Platform/SharedMemory.h (203154 => 203155)
--- trunk/Source/WebKit2/Platform/SharedMemory.h 2016-07-13 07:21:37 UTC (rev 203154)
+++ trunk/Source/WebKit2/Platform/SharedMemory.h 2016-07-13 08:36:01 UTC (rev 203155)
@@ -113,7 +113,7 @@
Protection m_protection;
#if USE(UNIX_DOMAIN_SOCKETS)
- int m_fileDescriptor;
+ Optional<int> m_fileDescriptor;
bool m_isWrappingMap { false };
#elif OS(DARWIN)
mach_port_t m_port;
Modified: trunk/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp (203154 => 203155)
--- trunk/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp 2016-07-13 07:21:37 UTC (rev 203154)
+++ trunk/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp 2016-07-13 08:36:01 UTC (rev 203155)
@@ -152,11 +152,14 @@
{
ASSERT(!handle.isNull());
- void* data = "" handle.m_attachment.size(), accessModeMMap(protection), MAP_SHARED, handle.m_attachment.fileDescriptor(), 0);
+ int fd = handle.m_attachment.releaseFileDescriptor();
+ void* data = "" handle.m_attachment.size(), accessModeMMap(protection), MAP_SHARED, fd, 0);
+ closeWithRetry(fd);
if (data == MAP_FAILED)
return nullptr;
- RefPtr<SharedMemory> instance = wrapMap(data, handle.m_attachment.size(), handle.m_attachment.releaseFileDescriptor());
+ RefPtr<SharedMemory> instance = wrapMap(data, handle.m_attachment.size(), -1);
+ instance->m_fileDescriptor = Nullopt;
instance->m_isWrappingMap = false;
return instance;
}
@@ -173,34 +176,27 @@
SharedMemory::~SharedMemory()
{
- if (!m_isWrappingMap) {
- munmap(m_data, m_size);
- closeWithRetry(m_fileDescriptor);
- }
+ if (m_isWrappingMap)
+ return;
+
+ munmap(m_data, m_size);
+ if (m_fileDescriptor)
+ closeWithRetry(m_fileDescriptor.value());
}
bool SharedMemory::createHandle(Handle& handle, Protection)
{
ASSERT_ARG(handle, handle.isNull());
+ ASSERT(m_fileDescriptor);
// FIXME: Handle the case where the passed Protection is ReadOnly.
// See https://bugs.webkit.org/show_bug.cgi?id=131542.
- int duplicatedHandle;
- while ((duplicatedHandle = dup(m_fileDescriptor)) == -1) {
- if (errno != EINTR) {
- ASSERT_NOT_REACHED();
- return false;
- }
+ int duplicatedHandle = dupCloseOnExec(m_fileDescriptor.value());
+ if (duplicatedHandle == -1) {
+ ASSERT_NOT_REACHED();
+ return false;
}
-
- while (fcntl(duplicatedHandle, F_SETFD, FD_CLOEXEC) == -1) {
- if (errno != EINTR) {
- ASSERT_NOT_REACHED();
- closeWithRetry(duplicatedHandle);
- return false;
- }
- }
handle.m_attachment = IPC::Attachment(duplicatedHandle, m_size);
return true;
}
Modified: trunk/Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp (203154 => 203155)
--- trunk/Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp 2016-07-13 07:21:37 UTC (rev 203154)
+++ trunk/Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp 2016-07-13 08:36:01 UTC (rev 203155)
@@ -38,6 +38,7 @@
#include <glib.h>
#include <locale.h>
#include <wtf/RunLoop.h>
+#include <wtf/UniStdExtras.h>
#include <wtf/glib/GLibUtilities.h>
#include <wtf/glib/GUniquePtr.h>
#include <wtf/text/CString.h>
@@ -125,8 +126,8 @@
}
// Don't expose the parent socket to potential future children.
- while (fcntl(socketPair.client, F_SETFD, FD_CLOEXEC) == -1)
- RELEASE_ASSERT(errno != EINTR);
+ if (!setCloseOnExec(socketPair.client))
+ RELEASE_ASSERT_NOT_REACHED();
close(socketPair.client);
m_processIdentifier = pid;
