Attachment 370347 Details for Bug 198006 – Patch

[patch] Patch

bug-198006-20190521152734.patch (text/plain), 10.52 KB, created by Chris Dumez on 2019-05-21 15:27:37 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-05-21 15:27:37 PDT

Size: 10.52 KB

patch

obsolete

>Subversion Revision: 245538
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index efeb55cb9dfdb9285f49803219f25e319ffd9e6f..78c5fb2d9b52fc6d86de886d9800f7765363f520 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,35 @@
>+2019-05-21  Chris Dumez  <cdumez@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 NOBODY (OOPS!).
>+
>+        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-20  Ross Kirsling  <ross.kirsling@sony.com>
> 
>         [WinCairo] Implement Remote Web Inspector Client.
>diff --git a/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp b/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp
>index f89008673726db5794a175a94a3baa0398f0c240..45a1f0f5b5cb80ccc9a0cfba1613e44c874a5607 100644
>--- a/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp
>@@ -61,6 +61,8 @@ ProvisionalPageProxy::ProvisionalPageProxy(WebPageProxy& page, Ref<WebProcessPro
>     , 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);
> 
>diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>index 4d6c85dc893811e2007ff8f54c8e719ff546e65a..1346fcc05c5a29cdb086f72ef496782cb2b005ae 100644
>--- a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>@@ -166,12 +166,17 @@ void SuspendedPageProxy::pageEnteredAcceleratedCompositingMode()
> {
>     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 @@ void SuspendedPageProxy::didProcessRequestToSuspend(SuspensionState newSuspensio
> 
>     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);
>diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.h b/Source/WebKit/UIProcess/SuspendedPageProxy.h
>index 8fe1c12a708396785b33db836f807741ad79d50e..0dad054b272a819d6e0b06c77ced812a263d1340 100644
>--- a/Source/WebKit/UIProcess/SuspendedPageProxy.h
>+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.h
>@@ -49,7 +49,7 @@ public:
>     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();
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 010c4863115856d36d86ec6d340955072dcd8c3d..43f8e18868d23ae5720e7e176b20c3dbb6c7d6c1 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -2826,7 +2826,7 @@ void WebPageProxy::receivedNavigationPolicyDecision(PolicyAction policyAction, A
>             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));
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index dbef1cc36c1db01e7a35196bbbbe16207e1af24a..2186ab2b1c43bd5c538862631f035de0b352915c 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-05-21  Chris Dumez  <cdumez@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 NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
> 2019-05-20  Keith Rollin  <krollin@apple.com>
> 
>         generate-xcfilelists is stranding temporary files
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index 4c3ca9fde9766ebd956285af13a729e27fb54a5d..4e9973c153004c8b33ccb37f57c29a60226e6207 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>@@ -4257,7 +4257,7 @@ TEST(ProcessSwap, APIControlledProcessSwapping)
>     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 @@ TEST(ProcessSwap, APIControlledProcessSwapping)
>     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) {

Actions: View | Formatted Diff | Diff

Attachments on bug 198006: 370347