Changeset 249703 in webkit

Timestamp:

Sep 9, 2019 9:24:23 PM (5 years ago)

Author:

Chris Dumez

Message:

[iOS] We sometimes attempt to use a terminated prewarmed WebContent process
​https://bugs.webkit.org/show_bug.cgi?id=201614
<rdar://problem/54714507>

Reviewed by Geoffrey Garen.

On iOS, it is possible for our processes to get terminated (e.g. jetsammed) while the UIProcess
is suspended. Upon resuming, it takes a little while for the UIProcess to get the notification
that the mac connection to its child process has been severed and the UIProcess may try to use
it for a load. This is especially problematic for prewarmed process because the client will end
up showing a crash banner and reloading when we could have used a new process rather the prewarmed
one if we had known it was dead.

This patch makes 2 improvements:

1. It makes AuxiliaryProcessProxy::state() return Terminated if we still have a connection but the PID is not the PID of a running process. I also added a check in tryTakePrewarmedProcess() to not use the prewarmed process if it state() is Terminated.
2. When the UIProcess is about to get suspended, have the process pools terminate their non-critical processes (i.e. prewarmed + the ones used for PageCache). This makes WebKit friendlier with other apps on the system when suspended with regards to memory. Also, it makes it less likely useful WebContent processes will get jetsammed.

• UIProcess/AuxiliaryProcessProxy.cpp:

(WebKit::AuxiliaryProcessProxy::state const):
(WebKit::AuxiliaryProcessProxy::isRunningProcessPID):

• UIProcess/AuxiliaryProcessProxy.h:
• UIProcess/Cocoa/WebProcessPoolCocoa.mm:

(WebKit::WebProcessPool::applicationIsAboutToSuspend):

• UIProcess/WebProcessPool.cpp:

(WebKit::WebProcessPool::tryTakePrewarmedProcess):

• UIProcess/WebProcessPool.h:
• UIProcess/ios/ProcessAssertionIOS.mm:

(-[WKProcessAssertionBackgroundTaskManager init]):
(-[WKProcessAssertionBackgroundTaskManager _releaseBackgroundTask]):

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/AuxiliaryProcessProxy.cpp (modified) (2 diffs)
• UIProcess/AuxiliaryProcessProxy.h (modified) (1 diff)
• UIProcess/Cocoa/WebProcessPoolCocoa.mm (modified) (2 diffs)
• UIProcess/WebProcessPool.cpp (modified) (1 diff)
• UIProcess/WebProcessPool.h (modified) (1 diff)
• UIProcess/ios/ProcessAssertionIOS.mm (modified) (3 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-09-09  Chris Dumez  <cdumez@apple.com>
+
+        [iOS] We sometimes attempt to use a terminated prewarmed WebContent process
+        https://bugs.webkit.org/show_bug.cgi?id=201614
+        <rdar://problem/54714507>
+
+        Reviewed by Geoffrey Garen.
+
+        On iOS, it is possible for our processes to get terminated (e.g. jetsammed) while the UIProcess
+        is suspended. Upon resuming, it takes a little while for the UIProcess to get the notification
+        that the mac connection to its child process has been severed and the UIProcess may try to use
+        it for a load. This is especially problematic for prewarmed process because the client will end
+        up showing a crash banner and reloading when we could have used a new process rather the prewarmed
+        one if we had known it was dead.
+
+        This patch makes 2 improvements:
+        1. It makes AuxiliaryProcessProxy::state() return Terminated if we still have a connection but
+           the PID is not the PID of a running process. I also added a check in tryTakePrewarmedProcess()
+           to not use the prewarmed process if it state() is Terminated.
+        2. When the UIProcess is about to get suspended, have the process pools terminate their non-critical
+           processes (i.e. prewarmed + the ones used for PageCache). This makes WebKit friendlier with
+           other apps on the system when suspended with regards to memory. Also, it makes it less likely
+           useful WebContent processes will get jetsammed.
+
+        * UIProcess/AuxiliaryProcessProxy.cpp:
+        (WebKit::AuxiliaryProcessProxy::state const):
+        (WebKit::AuxiliaryProcessProxy::isRunningProcessPID):
+        * UIProcess/AuxiliaryProcessProxy.h:
+        * UIProcess/Cocoa/WebProcessPoolCocoa.mm:
+        (WebKit::WebProcessPool::applicationIsAboutToSuspend):
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::tryTakePrewarmedProcess):
+        * UIProcess/WebProcessPool.h:
+        * UIProcess/ios/ProcessAssertionIOS.mm:
+        (-[WKProcessAssertionBackgroundTaskManager init]):
+        (-[WKProcessAssertionBackgroundTaskManager _releaseBackgroundTask]):
+
 2019-09-09  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp

@@ -29 +29 @@
 #include "AuxiliaryProcessMessages.h"
 #include "LoadParameters.h"
+#include "Logging.h"
 #include "WebPageMessages.h"
 #include <wtf/RunLoop.h>
@@ -109 +110 @@
         return AuxiliaryProcessProxy::State::Launching;
 
-    if (!m_connection)
+    // 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()))
         return AuxiliaryProcessProxy::State::Terminated;
 
     return AuxiliaryProcessProxy::State::Running;
+}
+
+bool AuxiliaryProcessProxy::isRunningProcessPID(ProcessID pid)
+{
+    if (!pid)
+        return false;
+
+#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;
+#else
+    UNUSED_PARAM(pid);
+    return true;
+#endif
 }
 

trunk/Source/WebKit/UIProcess/AuxiliaryProcessProxy.h

@@ -124 +124 @@
     virtual void connectionWillOpen(IPC::Connection&);
     virtual void processWillShutDown(IPC::Connection&) = 0;
+    static bool isRunningProcessPID(ProcessID);
 
     Vector<std::pair<std::unique_ptr<IPC::Encoder>, OptionSet<IPC::SendOption>>> m_pendingMessages;

trunk/Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm

@@ -30 +30 @@
 #import "CookieStorageUtilsCF.h"
 #import "LegacyCustomProtocolManagerClient.h"
+#import "Logging.h"
 #import "NetworkProcessCreationParameters.h"
 #import "NetworkProcessMessages.h"
@@ -581 +582 @@
 }
 
+#if PLATFORM(IOS_FAMILY)
+void WebProcessPool::applicationIsAboutToSuspend()
+{
+    RELEASE_LOG(ProcessSuspension, "Application is about to suspend so we simulate memory pressure to terminate non-critical processes");
+    // Simulate memory pressure handling so free as much memory as possible before suspending.
+    // In particular, this will terminate prewarmed and PageCache processes.
+    for (auto* processPool : allProcessPools())
+        processPool->handleMemoryPressureWarning(Critical::Yes);
+}
+#endif
+
 } // namespace WebKit

trunk/Source/WebKit/UIProcess/WebProcessPool.cpp

@@ -826 +826 @@
     if (!m_prewarmedProcess)
         return nullptr;
+    
+    // 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) {
+        RELEASE_LOG_ERROR(Process, "Not using prewarmed process %d because it has been terminated", m_prewarmedProcess->processIdentifier());
+        m_prewarmedProcess = nullptr;
+        return nullptr;
+    }
 
 #if PLATFORM(GTK) || PLATFORM(WPE)

trunk/Source/WebKit/UIProcess/WebProcessPool.h

@@ -223 +223 @@
     void populateVisitedLinks();
 
+#if PLATFORM(IOS_FAMILY)
+    static void applicationIsAboutToSuspend();
+#endif
+
     void handleMemoryPressureWarning(Critical);
 

trunk/Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm

@@ -31 +31 @@
 #import "AssertionServicesSPI.h"
 #import "Logging.h"
+#import "WebProcessPool.h"
 #import <UIKit/UIApplication.h>
 #import <wtf/HashMap.h>
@@ -82 +83 @@
     [[NSNotificationCenter defaultCenter] addObserverForName:UIApplicationDidEnterBackgroundNotification object:[UIApplication sharedApplication] queue:nil usingBlock:^(NSNotification *) {
         _applicationIsBackgrounded = YES;
+        
+        if (_backgroundTask == UIBackgroundTaskInvalid)
+            WebKit::WebProcessPool::applicationIsAboutToSuspend();
     }];
 
@@ -184 +188 @@
 
     RELEASE_LOG(ProcessSuspension, "%p - WKProcessAssertionBackgroundTaskManager - endBackgroundTask", self);
+    if (_applicationIsBackgrounded)
+        WebKit::WebProcessPool::applicationIsAboutToSuspend();
     [[UIApplication sharedApplication] endBackgroundTask:_backgroundTask];
     _backgroundTask = UIBackgroundTaskInvalid;