Changeset 250329 in webkit

Timestamp:

Sep 24, 2019 6:56:50 PM (5 years ago)

Author:

Chris Dumez

Message:

[iOS] Regression(r249703) frequent 'kill() returned unexpected error' log messages
​https://bugs.webkit.org/show_bug.cgi?id=202173

Reviewed by Geoffrey Garen.

The kill(pid, 0) command actually fails with an EPERM error when there is a process
running with the given pid, and this is causing us to log a lot of errors. The good
news is that we merely want to know that there is no process with the given PID and
we correctly get a ESRCH error in this case. I renamed the function from
isRunningProcessPID() to wasTerminated() and only check for ESRCH error now. I no
longer log any error otherwise since this is expected.

Also, for performance reason, I no longer call kill(pid, 0) from inside
AuxiliaryProcessProxy::state() as it gets called a lot. I instead only call it from
AuxiliaryProcessProxy::wasTerminated() and call it from
WebProcessPool::tryTakePrewarmedProcess().

• UIProcess/AuxiliaryProcessProxy.cpp:

(WebKit::AuxiliaryProcessProxy::state const):
(WebKit::AuxiliaryProcessProxy::wasTerminated const):
(WebKit::AuxiliaryProcessProxy::isRunningProcessPID): Deleted.

• UIProcess/AuxiliaryProcessProxy.h:
• UIProcess/WebProcessPool.cpp:

(WebKit::WebProcessPool::tryTakePrewarmedProcess):

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/AuxiliaryProcessProxy.cpp (modified) (2 diffs)
• UIProcess/AuxiliaryProcessProxy.h (modified) (2 diffs)
• UIProcess/WebProcessPool.cpp (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-09-24  Chris Dumez  <cdumez@apple.com>
+
+        [iOS] Regression(r249703) frequent 'kill() returned unexpected error' log messages
+        https://bugs.webkit.org/show_bug.cgi?id=202173
+
+        Reviewed by Geoffrey Garen.
+
+        The kill(pid, 0) command actually fails with an EPERM error when there is a process
+        running with the given pid, and this is causing us to log a lot of errors. The good
+        news is that we merely want to know that there is no process with the given PID and
+        we correctly get a ESRCH error in this case. I renamed the function from
+        isRunningProcessPID() to wasTerminated() and only check for ESRCH error now. I no
+        longer log any error otherwise since this is expected.
+
+        Also, for performance reason, I no longer call kill(pid, 0) from inside
+        AuxiliaryProcessProxy::state() as it gets called a lot. I instead only call it from
+        AuxiliaryProcessProxy::wasTerminated() and call it from
+        WebProcessPool::tryTakePrewarmedProcess().
+
+        * UIProcess/AuxiliaryProcessProxy.cpp:
+        (WebKit::AuxiliaryProcessProxy::state const):
+        (WebKit::AuxiliaryProcessProxy::wasTerminated const):
+        (WebKit::AuxiliaryProcessProxy::isRunningProcessPID): Deleted.
+        * UIProcess/AuxiliaryProcessProxy.h:
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::tryTakePrewarmedProcess):
+
 2019-09-24  Christopher Reid  <chris.reid@sony.com>
 

trunk/Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp

@@ -112 +112 @@
         return AuxiliaryProcessProxy::State::Launching;
 
-    // There is sometimes a delay until we get the notification from mach about the connection getting closed.
-    // To help detect terminated process earlier, we also check that the PID is for a valid running process.
-    if (!m_connection || !isRunningProcessPID(processIdentifier()))
+    if (!m_connection)
         return AuxiliaryProcessProxy::State::Terminated;
 
@@ -120 +118 @@
 }
 
-bool AuxiliaryProcessProxy::isRunningProcessPID(ProcessID pid)
-{
+bool AuxiliaryProcessProxy::wasTerminated() const
+{
+    switch (state()) {
+    case AuxiliaryProcessProxy::State::Launching:
+        return false;
+    case AuxiliaryProcessProxy::State::Terminated:
+        return true;
+    case AuxiliaryProcessProxy::State::Running:
+        break;
+    }
+
+    auto pid = processIdentifier();
     if (!pid)
-        return false;
+        return true;
 
 #if PLATFORM(COCOA)
-    // Use kill() with a signal of 0 to check if there is actually still a process with the given PID.
-    if (!kill(pid, 0))
-        return true;
-
-    if (errno == ESRCH) {
-        // No process can be found corresponding to that specified by pid.
-        return false;
-    }
-
-    RELEASE_LOG_ERROR(Process, "kill() returned unexpected error %d", errno);
-    return true;
+    // Use kill() with a signal of 0 to make sure there is indeed still a process with the given PID.
+    // This is needed because it sometimes takes a little bit of time for us to get notified that a process
+    // was terminated.
+    return kill(pid, 0) && errno == ESRCH;
 #else
-    UNUSED_PARAM(pid);
-    return true;
+    return false;
 #endif
 }

trunk/Source/WebKit/UIProcess/AuxiliaryProcessProxy.h

@@ -100 +100 @@
     State state() const;
     bool isLaunching() const { return state() == State::Launching; }
+    bool wasTerminated() const;
 
     ProcessID processIdentifier() const { return m_processLauncher ? m_processLauncher->processIdentifier() : 0; }
@@ -125 +126 @@
     virtual void connectionWillOpen(IPC::Connection&);
     virtual void processWillShutDown(IPC::Connection&) = 0;
-    static bool isRunningProcessPID(ProcessID);
 
     struct PendingMessage {

trunk/Source/WebKit/UIProcess/WebProcessPool.cpp

@@ -808 +808 @@
     // There is sometimes a delay until we get notified that a prewarmed process has been terminated (e.g. after resuming
     // from suspension) so make sure the process is still running here before deciding to use it.
-    if (m_prewarmedProcess->state() == AuxiliaryProcessProxy::State::Terminated) {
+    if (m_prewarmedProcess->wasTerminated()) {
         RELEASE_LOG_ERROR(Process, "Not using prewarmed process %d because it has been terminated", m_prewarmedProcess->processIdentifier());
         m_prewarmedProcess = nullptr;