Title: [268097] trunk
- Revision
- 268097
- Author
- cdu...@apple.com
- Date
- 2020-10-06 17:21:02 -0700 (Tue, 06 Oct 2020)
Log 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):
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (268096 => 268097)
--- trunk/Source/WebKit/ChangeLog 2020-10-07 00:19:37 UTC (rev 268096)
+++ trunk/Source/WebKit/ChangeLog 2020-10-07 00:21:02 UTC (rev 268097)
@@ -1,3 +1,27 @@
+2020-10-06 Chris Dumez <cdu...@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.l...@apple.com>
[Media in GPU Process] The seekable attribute of HTMLMediaElement has an incorrect value
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (268096 => 268097)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2020-10-07 00:19:37 UTC (rev 268096)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2020-10-07 00:21:02 UTC (rev 268097)
@@ -7464,14 +7464,22 @@
{
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);
+ // 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;
- if (!handledByClient && shouldReloadAfterProcessTermination(reason))
- tryReloadAfterProcessTermination();
+ 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();
+ });
}
void WebPageProxy::tryReloadAfterProcessTermination()
Modified: trunk/Tools/ChangeLog (268096 => 268097)
--- trunk/Tools/ChangeLog 2020-10-07 00:19:37 UTC (rev 268096)
+++ trunk/Tools/ChangeLog 2020-10-07 00:21:02 UTC (rev 268097)
@@ -1,3 +1,17 @@
+2020-10-06 Chris Dumez <cdu...@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 <drou...@apple.com>
CSS hover and "pointer: fine" media queries do not evaluate to true with iOS 13.4 mouse support
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebContentProcessDidTerminate.mm (268096 => 268097)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebContentProcessDidTerminate.mm 2020-10-07 00:19:37 UTC (rev 268096)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebContentProcessDidTerminate.mm 2020-10-07 00:21:02 UTC (rev 268097)
@@ -31,8 +31,10 @@
#import <WebKit/WKNavigationPrivate.h>
#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;
static _WKProcessTerminationReason expectedCrashReason;
@@ -42,6 +44,9 @@
static bool receivedScriptMessage;
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>";
@@ -277,3 +282,73 @@
TestWebKitAPI::Util::sleep(0.5);
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]);
+}
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
