- Revision
- 245601
- Author
- cdu...@apple.com
- Date
- 2019-05-21 16:44:16 -0700 (Tue, 21 May 2019)
Log Message
[PSON] Assertion hit when navigating back after a process swap forced by the client
https://bugs.webkit.org/show_bug.cgi?id=198006
Reviewed by Alex Christensen.
Source/WebKit:
After r245198, we construct a SuspendedPageProxy when a process-swap is forced by the client
and we delay to closing of the WebPage in the old WebProcess until it is safe to do so without
flashing (by calling SuspendedPageProxy::closeWithoutFlashing()). The issue is that our logic
deciding if we should reuse a SuspendedPageProxy's WebPage relied on the SuspendedPageProxy's
m_suspensionState not being set to FailedToSuspend. In the case of a process-swap forced by the
client with delayed page closing, the suspended state may be suspended but is still not usable
because it is about to get closed. We would wrongly believe there is a WebPage to be reused so
the ProvisionalPageProxy would construct a proxy for the main frame in its constructor, we would
then hit the ASSERT(!m_mainFrame) assertion in ProvisionalPageProxy::didCreateMainFrame() when
the WebContent process would unexpectedly create a main frame.
To address the issue, stop relying on the suspended state to determine if we can reuse a WebPage
or not and introduce a new pageIsClosedOrClosing() getter on the SuspendedPageProxy instead
which indicates if the WebPage in the WebContent process has been closed or is about to be.
* UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::ProvisionalPageProxy):
* UIProcess/SuspendedPageProxy.cpp:
(WebKit::SuspendedPageProxy::pageEnteredAcceleratedCompositingMode):
(WebKit::SuspendedPageProxy::pageIsClosedOrClosing const):
(WebKit::SuspendedPageProxy::didProcessRequestToSuspend):
* UIProcess/SuspendedPageProxy.h:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::receivedNavigationPolicyDecision):
Tools:
Add API test coverage.
* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (245600 => 245601)
--- trunk/Source/WebKit/ChangeLog 2019-05-21 23:42:42 UTC (rev 245600)
+++ trunk/Source/WebKit/ChangeLog 2019-05-21 23:44:16 UTC (rev 245601)
@@ -1,3 +1,35 @@
+2019-05-21 Chris Dumez <cdu...@apple.com>
+
+ [PSON] Assertion hit when navigating back after a process swap forced by the client
+ https://bugs.webkit.org/show_bug.cgi?id=198006
+
+ Reviewed by Alex Christensen.
+
+ After r245198, we construct a SuspendedPageProxy when a process-swap is forced by the client
+ and we delay to closing of the WebPage in the old WebProcess until it is safe to do so without
+ flashing (by calling SuspendedPageProxy::closeWithoutFlashing()). The issue is that our logic
+ deciding if we should reuse a SuspendedPageProxy's WebPage relied on the SuspendedPageProxy's
+ m_suspensionState not being set to FailedToSuspend. In the case of a process-swap forced by the
+ client with delayed page closing, the suspended state may be suspended but is still not usable
+ because it is about to get closed. We would wrongly believe there is a WebPage to be reused so
+ the ProvisionalPageProxy would construct a proxy for the main frame in its constructor, we would
+ then hit the ASSERT(!m_mainFrame) assertion in ProvisionalPageProxy::didCreateMainFrame() when
+ the WebContent process would unexpectedly create a main frame.
+
+ To address the issue, stop relying on the suspended state to determine if we can reuse a WebPage
+ or not and introduce a new pageIsClosedOrClosing() getter on the SuspendedPageProxy instead
+ which indicates if the WebPage in the WebContent process has been closed or is about to be.
+
+ * UIProcess/ProvisionalPageProxy.cpp:
+ (WebKit::ProvisionalPageProxy::ProvisionalPageProxy):
+ * UIProcess/SuspendedPageProxy.cpp:
+ (WebKit::SuspendedPageProxy::pageEnteredAcceleratedCompositingMode):
+ (WebKit::SuspendedPageProxy::pageIsClosedOrClosing const):
+ (WebKit::SuspendedPageProxy::didProcessRequestToSuspend):
+ * UIProcess/SuspendedPageProxy.h:
+ * UIProcess/WebPageProxy.cpp:
+ (WebKit::WebPageProxy::receivedNavigationPolicyDecision):
+
2019-05-21 Per Arne Vollan <pvol...@apple.com>
Sandbox violation is making the WebContent process crash
Modified: trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp (245600 => 245601)
--- trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp 2019-05-21 23:42:42 UTC (rev 245600)
+++ trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp 2019-05-21 23:44:16 UTC (rev 245601)
@@ -61,6 +61,8 @@
, m_suspensionToken(m_process->throttler().foregroundActivityToken())
#endif
{
+ RELEASE_LOG_ERROR_IF_ALLOWED(ProcessSwapping, "ProvisionalPageProxy: pageID = %" PRIu64 " navigationID = %" PRIu64 " suspendedPage: %p", m_page.pageID(), navigationID, suspendedPage.get());
+
m_process->addMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID(), *this);
m_process->addProvisionalPageProxy(*this);
Modified: trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp (245600 => 245601)
--- trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp 2019-05-21 23:42:42 UTC (rev 245600)
+++ trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp 2019-05-21 23:44:16 UTC (rev 245601)
@@ -166,12 +166,17 @@
{
m_shouldDelayClosingUntilEnteringAcceleratedCompositingMode = ShouldDelayClosingUntilEnteringAcceleratedCompositingMode::No;
- if (m_suspensionState == SuspensionState::FailedToSuspend || m_shouldCloseWhenEnteringAcceleratedCompositingMode) {
+ if (m_shouldCloseWhenEnteringAcceleratedCompositingMode) {
// We needed the suspended page to stay alive to avoid flashing. Now we can get rid of it.
close();
}
}
+bool SuspendedPageProxy::pageIsClosedOrClosing() const
+{
+ return m_isClosed || m_shouldCloseWhenEnteringAcceleratedCompositingMode;
+}
+
void SuspendedPageProxy::closeWithoutFlashing()
{
RELEASE_LOG(ProcessSwapping, "%p - SuspendedPageProxy::closeWithoutFlashing() shouldDelayClosingUntilEnteringAcceleratedCompositingMode? %d", this, m_shouldDelayClosingUntilEnteringAcceleratedCompositingMode == ShouldDelayClosingUntilEnteringAcceleratedCompositingMode::Yes);
@@ -200,8 +205,8 @@
m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
- if (m_suspensionState == SuspensionState::FailedToSuspend && m_shouldDelayClosingUntilEnteringAcceleratedCompositingMode == ShouldDelayClosingUntilEnteringAcceleratedCompositingMode::No)
- close();
+ if (m_suspensionState == SuspensionState::FailedToSuspend)
+ closeWithoutFlashing();
if (m_readyToUnsuspendHandler)
m_readyToUnsuspendHandler(this);
Modified: trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h (245600 => 245601)
--- trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h 2019-05-21 23:42:42 UTC (rev 245600)
+++ trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h 2019-05-21 23:44:16 UTC (rev 245601)
@@ -49,7 +49,7 @@
WebProcessProxy& process() { return m_process.get(); }
uint64_t mainFrameID() const { return m_mainFrameID; }
- bool failedToSuspend() const { return m_suspensionState == SuspensionState::FailedToSuspend; }
+ bool pageIsClosedOrClosing() const;
void waitUntilReadyToUnsuspend(CompletionHandler<void(SuspendedPageProxy*)>&&);
void unsuspend();
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (245600 => 245601)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2019-05-21 23:42:42 UTC (rev 245600)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2019-05-21 23:44:16 UTC (rev 245601)
@@ -2826,7 +2826,7 @@
ASSERT(destinationSuspendedPage || !process().processPool().hasSuspendedPageFor(processForNavigation, *this));
auto suspendedPage = destinationSuspendedPage ? process().processPool().takeSuspendedPage(*destinationSuspendedPage) : nullptr;
- if (suspendedPage && suspendedPage->failedToSuspend())
+ if (suspendedPage && suspendedPage->pageIsClosedOrClosing())
suspendedPage = nullptr;
continueNavigationInNewProcess(navigation, WTFMove(suspendedPage), WTFMove(processForNavigation), processSwapRequestedByClient, WTFMove(data));
Modified: trunk/Tools/ChangeLog (245600 => 245601)
--- trunk/Tools/ChangeLog 2019-05-21 23:42:42 UTC (rev 245600)
+++ trunk/Tools/ChangeLog 2019-05-21 23:44:16 UTC (rev 245601)
@@ -1,3 +1,14 @@
+2019-05-21 Chris Dumez <cdu...@apple.com>
+
+ [PSON] Assertion hit when navigating back after a process swap forced by the client
+ https://bugs.webkit.org/show_bug.cgi?id=198006
+
+ Reviewed by Alex Christensen.
+
+ Add API test coverage.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
2019-05-21 Carlos Garcia Campos <cgar...@igalia.com>
Unreviewed. Fix the build with HAVE(ACCESSIBILITY) disabled
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (245600 => 245601)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-05-21 23:42:42 UTC (rev 245600)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-05-21 23:44:16 UTC (rev 245601)
@@ -4257,7 +4257,7 @@
navigationDelegate->decidePolicyForNavigationAction = ^(WKNavigationAction *, void (^decisionHandler)(WKNavigationActionPolicy)) {
decisionHandler(_WKNavigationActionPolicyAllowInNewProcess);
};
- [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webit.org/3"]]];
+ [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/3"]]];
TestWebKitAPI::Util::run(&done);
done = false;
auto pid3 = [webView _webProcessIdentifier];
@@ -4267,6 +4267,56 @@
EXPECT_NE(pid1, pid3);
}
+enum class WithDelay : bool { No, Yes };
+static void runAPIControlledProcessSwappingThenBackTest(WithDelay withDelay)
+{
+ auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+ auto handler = adoptNS([[PSONScheme alloc] initWithBytes:"Hello World!"]);
+ [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+ auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+ auto navigationDelegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+ [webView setNavigationDelegate:navigationDelegate.get()];
+
+ [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org"]]];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto pid1 = [webView _webProcessIdentifier];
+
+ navigationDelegate->decidePolicyForNavigationAction = ^(WKNavigationAction *, void (^decisionHandler)(WKNavigationActionPolicy)) {
+ decisionHandler(_WKNavigationActionPolicyAllowInNewProcess);
+ };
+
+ [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com"]]];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto pid2 = [webView _webProcessIdentifier];
+ EXPECT_NE(pid1, pid2);
+
+ // Give time to the suspended WebPage to close.
+ if (withDelay == WithDelay::Yes)
+ TestWebKitAPI::Util::sleep(0.1);
+
+ navigationDelegate->decidePolicyForNavigationAction = nil;
+ [webView goBack];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ EXPECT_EQ(pid1, [webView _webProcessIdentifier]);
+}
+
+TEST(ProcessSwap, APIControlledProcessSwappingThenBackWithDelay)
+{
+ runAPIControlledProcessSwappingThenBackTest(WithDelay::Yes);
+}
+
+TEST(ProcessSwap, APIControlledProcessSwappingThenBackWithoutDelay)
+{
+ runAPIControlledProcessSwappingThenBackTest(WithDelay::No);
+}
+
static const char* navigateToCrossSiteThenBackFromJSBytes = R"PSONRESOURCE(
<script>
_onpageshow_ = function(event) {
