Changeset 203155 in webkit

Timestamp:

Jul 13, 2016 1:36:01 AM (8 years ago)

Author:

Carlos Garcia Campos

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):

Location:

trunk/Source

Files:

• WTF/ChangeLog (modified) (1 diff)
• WTF/wtf/PlatformEfl.cmake (modified) (1 diff)
• WTF/wtf/PlatformGTK.cmake (modified) (1 diff)
• WTF/wtf/UniStdExtras.cpp (copied) (copied from trunk/Source/WTF/wtf/UniStdExtras.h) (2 diffs)
• WTF/wtf/UniStdExtras.h (modified) (2 diffs)
• WebKit2/ChangeLog (modified) (1 diff)
• WebKit2/Platform/IPC/unix/ConnectionUnix.cpp (modified) (2 diffs)
• WebKit2/Platform/SharedMemory.h (modified) (1 diff)
• WebKit2/Platform/unix/SharedMemoryUnix.cpp (modified) (3 diffs)
• WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp (modified) (2 diffs)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2016-07-13  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [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  <bpoulain@apple.com>
 

trunk/Source/WTF/wtf/PlatformEfl.cmake

@@ -1 +1 @@
 list(APPEND WTF_SOURCES
     PlatformUserPreferredLanguagesUnix.cpp
+    UniStdExtras.cpp
 
     text/efl/TextBreakIteratorInternalICUEfl.cpp

trunk/Source/WTF/wtf/PlatformGTK.cmake

@@ -9 +9 @@
     glib/RunLoopGLib.cpp
     PlatformUserPreferredLanguagesUnix.cpp
+    UniStdExtras.cpp
 
     text/gtk/TextBreakIteratorInternalICUGtk.cpp

trunk/Source/WTF/wtf/UniStdExtras.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies).
+ * Copyright (C) 2016 Igalia S.L.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -24 +24 @@
  */
 
-#ifndef UniStdExtras_h
-#define UniStdExtras_h
+#include "config.h"
+#include "UniStdExtras.h"
 
-#include <errno.h>
-#include <unistd.h>
+#include <fcntl.h>
 
 namespace WTF {
 
-inline int closeWithRetry(int fileDescriptor)
+bool setCloseOnExec(int fileDescriptor)
 {
-    int ret;
-#if OS(LINUX)
-    // Workaround for the Linux behavior of closing the descriptor
-    // unconditionally, even if the close() call is interrupted.
-    // See https://bugs.webkit.org/show_bug.cgi?id=117266 for more
-    // details.
-    if ((ret = close(fileDescriptor)) == -1 && errno == EINTR)
-        return 0;
-#else
-    while ((ret = close(fileDescriptor)) == -1 && errno == EINTR) { }
+    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
-    return ret;
+
+    while ((duplicatedFileDescriptor = dup(fileDescriptor)) == -1 && errno == EINTR) { }
+    if (duplicatedFileDescriptor == -1)
+        return -1;
+
+    if (!setCloseOnExec(duplicatedFileDescriptor)) {
+        closeWithRetry(duplicatedFileDescriptor);
+        return -1;
+    }
+
+    return duplicatedFileDescriptor;
 }
 
 } // namespace WTF
-
-using WTF::closeWithRetry;
-
-#endif // UniStdExtras_h

trunk/Source/WTF/wtf/UniStdExtras.h

@@ -32 +32 @@
 namespace WTF {
 
+bool setCloseOnExec(int fileDescriptor);
+int dupCloseOnExec(int fileDescriptor);
+
 inline int closeWithRetry(int fileDescriptor)
 {
@@ -51 +54 @@
 
 using WTF::closeWithRetry;
+using WTF::setCloseOnExec;
+using WTF::dupCloseOnExec;
 
 #endif // UniStdExtras_h

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2016-07-13  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [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  <simon.fraser@apple.com>
 

trunk/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp

@@ -309 +309 @@
 
                 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;
                     }
                 }
@@ -533 +531 @@
     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();
     }
 

trunk/Source/WebKit2/Platform/SharedMemory.h

@@ -114 +114 @@
 
 #if USE(UNIX_DOMAIN_SOCKETS)
-    int m_fileDescriptor;
+    Optional<int> m_fileDescriptor;
     bool m_isWrappingMap { false };
 #elif OS(DARWIN)

trunk/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp

@@ -153 +153 @@
     ASSERT(!handle.isNull());
 
-    void* data = mmap(0, handle.m_attachment.size(), accessModeMMap(protection), MAP_SHARED, handle.m_attachment.fileDescriptor(), 0);
+    int fd = handle.m_attachment.releaseFileDescriptor();
+    void* data = mmap(0, 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;
@@ -174 +177 @@
 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());
 }
 
@@ -183 +188 @@
 {
     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;
-        }
-    }
-
-    while (fcntl(duplicatedHandle, F_SETFD, FD_CLOEXEC) == -1) {
-        if (errno != EINTR) {
-            ASSERT_NOT_REACHED();
-            closeWithRetry(duplicatedHandle);
-            return false;
-        }
+    int duplicatedHandle = dupCloseOnExec(m_fileDescriptor.value());
+    if (duplicatedHandle == -1) {
+        ASSERT_NOT_REACHED();
+        return false;
     }
     handle.m_attachment = IPC::Attachment(duplicatedHandle, m_size);

trunk/Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp

@@ -39 +39 @@
 #include <locale.h>
 #include <wtf/RunLoop.h>
+#include <wtf/UniStdExtras.h>
 #include <wtf/glib/GLibUtilities.h>
 #include <wtf/glib/GUniquePtr.h>
@@ -126 +127 @@
 
     // 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);