Changeset 268097 in webkit

Timestamp:

Oct 6, 2020 5:21:02 PM (4 years ago)

Author:

Chris Dumez

Message:

Reloading a view in its processTerminationHandler does not work reliably when using related views
​https://bugs.webkit.org/show_bug.cgi?id=217407

Reviewed by Geoff Garen.

Source/WebKit:

Related web views share the same WebContent process. When this process crashes, we iterate over
the list of WebPageProxy objects sharing this process and let them know that their process has
crashed. This causes the WebPageProxy to reset its state (so that it is aware it no longer has
a running process) and to notify the client application. Because we were notifying the client
application synchronously, the client could trigger a load in the view synchronously while we
are still iterating over the WebPageProxy objects. When triggering a load in a web view that
has no running process, we normally relaunch one. However, in the case of related web views,
the view uses its related webview's process, relaunching it if necessary. The 'relaunching
if necessary' part was not reliably happening here because the related web view may not have
been notified yet that its WebProcess has crashed (since we are still iterating over the
pages to notify them).

To address the issue, we now notify the client asynchronously of the process termination.

• UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::dispatchProcessDidTerminate):

Tools:

Add API test coverage.

• TestWebKitAPI/Tests/WebKitCocoa/WebContentProcessDidTerminate.mm:

(-[NavigationDelegateWithCrashHandlerThatLoadsAgain _webView:webContentProcessDidTerminateWithReason:]):
(-[NavigationDelegateWithCrashHandlerThatLoadsAgain webView:didFinishNavigation:]):
(TEST):

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/WebPageProxy.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/WebContentProcessDidTerminate.mm (modified) (3 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2020-10-06  Chris Dumez  <cdumez@apple.com>
+
+        Reloading a view in its processTerminationHandler does not work reliably when using related views
+        https://bugs.webkit.org/show_bug.cgi?id=217407
+
+        Reviewed by Geoff Garen.
+
+        Related web views share the same WebContent process. When this process crashes, we iterate over
+        the list of WebPageProxy objects sharing this process and let them know that their process has
+        crashed. This causes the WebPageProxy to reset its state (so that it is aware it no longer has
+        a running process) and to notify the client application. Because we were notifying the client
+        application synchronously, the client could trigger a load in the view synchronously while we
+        are still iterating over the WebPageProxy objects. When triggering a load in a web view that
+        has no running process, we normally relaunch one. However, in the case of related web views,
+        the view uses its related webview's process, relaunching it if necessary. The 'relaunching
+        if necessary' part was not reliably happening here because the related web view may not have
+        been notified yet that its WebProcess has crashed (since we are still iterating over the
+        pages to notify them).
+
+        To address the issue, we now notify the client asynchronously of the process termination.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::dispatchProcessDidTerminate):
+
 2020-10-06  Peng Liu  <peng.liu6@apple.com>
 

trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -7465 +7465 @@
     RELEASE_LOG_ERROR_IF_ALLOWED(Loading, "dispatchProcessDidTerminate: reason = %d", reason);
 
-    bool handledByClient = false;
-    if (m_loaderClient)
-        handledByClient = reason != ProcessTerminationReason::RequestedByClient && m_loaderClient->processDidCrash(*this);
-    else
-        handledByClient = m_navigationClient->processDidTerminate(*this, reason);
-
-    if (!handledByClient && shouldReloadAfterProcessTermination(reason))
-        tryReloadAfterProcessTermination();
+    // We notify the client asynchronously because several pages may share the same process
+    // and we want to make sure all pages are aware their process has crashed before the
+    // the client reacts to the process termination.
+    RunLoop::main().dispatch([this, weakThis = makeWeakPtr(*this), reason] {
+        if (!weakThis)
+            return;
+
+        bool handledByClient = false;
+        if (m_loaderClient)
+            handledByClient = reason != ProcessTerminationReason::RequestedByClient && m_loaderClient->processDidCrash(*this);
+        else
+            handledByClient = m_navigationClient->processDidTerminate(*this, reason);
+
+        if (!handledByClient && shouldReloadAfterProcessTermination(reason))
+            tryReloadAfterProcessTermination();
+    });
 }
 

trunk/Tools/ChangeLog

@@ +1 @@
+2020-10-06  Chris Dumez  <cdumez@apple.com>
+
+        Reloading a view in its processTerminationHandler does not work reliably when using related views
+        https://bugs.webkit.org/show_bug.cgi?id=217407
+
+        Reviewed by Geoff Garen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/WebContentProcessDidTerminate.mm:
+        (-[NavigationDelegateWithCrashHandlerThatLoadsAgain _webView:webContentProcessDidTerminateWithReason:]):
+        (-[NavigationDelegateWithCrashHandlerThatLoadsAgain webView:didFinishNavigation:]):
+        (TEST):
+
 2020-10-06  Devin Rousso  <drousso@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebContentProcessDidTerminate.mm

@@ -32 +32 @@
 #import <WebKit/WKProcessPoolPrivate.h>
 #import <WebKit/WKWebView.h>
+#import <WebKit/WKWebViewConfigurationPrivate.h>
 #import <WebKit/WKWebViewPrivate.h>
 #import <wtf/RetainPtr.h>
+#import <wtf/Vector.h>
 
 static bool didCrash;
@@ -43 +45 @@
 static bool calledAllCallbacks;
 static unsigned callbackCount;
+static unsigned crashHandlerCount;
+static unsigned loadCount;
+static unsigned expectedLoadCount;
 
 static NSString *testHTML = @"<script>window.webkit.messageHandlers.testHandler.postMessage('LOADED');</script>";
@@ -278 +283 @@
     EXPECT_EQ(6U, callbackCount);
 }
+
+@interface NavigationDelegateWithCrashHandlerThatLoadsAgain : NSObject <WKNavigationDelegate>
+@end
+
+@implementation NavigationDelegateWithCrashHandlerThatLoadsAgain
+
+- (void)_webView:(WKWebView *)webView webContentProcessDidTerminateWithReason:(_WKProcessTerminationReason)reason
+{
+    ++crashHandlerCount;
+
+    // Attempt the load again synchronously.
+    [webView loadHTMLString:@"foo" baseURL:nil];
+}
+
+- (void)webView:(WKWebView *)webView didFinishNavigation:(WKNavigation *)navigation
+{
+    if (++loadCount == expectedLoadCount)
+        finishedLoad = true;
+}
+
+@end
+
+TEST(WKNavigation, ReloadRelatedViewsInProcessDidTerminate)
+{
+    const unsigned numberOfViews = 20;
+    auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    auto webView1 = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 100, 100) configuration:configuration.get()]);
+
+    Vector<RetainPtr<WKWebView>> webViews;
+    webViews.append(webView1);
+
+    configuration.get()._relatedWebView = webView1.get();
+    for (unsigned i = 0; i < numberOfViews - 1; ++i) {
+        auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 100, 100) configuration:configuration.get()]);
+        webViews.append(webView);
+    }
+    auto delegate = adoptNS([[NavigationDelegateWithCrashHandlerThatLoadsAgain alloc] init]);
+    for (auto& webView : webViews)
+        [webView setNavigationDelegate:delegate.get()];
+
+    crashHandlerCount = 0;
+    loadCount = 0;
+    expectedLoadCount = numberOfViews;
+    finishedLoad = false;
+
+    for (auto& webView : webViews)
+        [webView loadHTMLString:@"foo" baseURL:nil];
+
+    TestWebKitAPI::Util::run(&finishedLoad);
+    EXPECT_EQ(0U, crashHandlerCount);
+
+    auto pidBefore = [webView1 _webProcessIdentifier];
+    EXPECT_TRUE(!!pidBefore);
+    for (auto& webView : webViews)
+        EXPECT_EQ(pidBefore, [webView _webProcessIdentifier]);
+
+    loadCount = 0;
+    finishedLoad = false;
+
+    // Kill the WebContent process. The crash handler should reload all views.
+    kill(pidBefore, 9);
+
+    TestWebKitAPI::Util::run(&finishedLoad);
+    EXPECT_EQ(numberOfViews, crashHandlerCount);
+
+    auto pidAfter = [webView1 _webProcessIdentifier];
+    EXPECT_TRUE(!!pidAfter);
+    for (auto& webView : webViews)
+        EXPECT_EQ(pidAfter, [webView _webProcessIdentifier]);
+}