Changeset 245601 in webkit

Timestamp:

May 21, 2019 4:44:16 PM (5 years ago)

Author:

Chris Dumez

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:

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/ProvisionalPageProxy.cpp (modified) (1 diff)
• Source/WebKit/UIProcess/SuspendedPageProxy.cpp (modified) (2 diffs)
• Source/WebKit/UIProcess/SuspendedPageProxy.h (modified) (1 diff)
• Source/WebKit/UIProcess/WebPageProxy.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (modified) (2 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+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 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  <pvollan@apple.com>
 

trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp

@@ -62 +62 @@
 #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);

trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp

@@ -167 +167 @@
     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;
 }
 
@@ -201 +206 @@
     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)

trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h

@@ -50 +50 @@
     uint64_t mainFrameID() const { return m_mainFrameID; }
 
-    bool failedToSuspend() const { return m_suspensionState == SuspensionState::FailedToSuspend; }
+    bool pageIsClosedOrClosing() const;
 
     void waitUntilReadyToUnsuspend(CompletionHandler<void(SuspendedPageProxy*)>&&);

trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -2827 +2827 @@
 
             auto suspendedPage = destinationSuspendedPage ? process().processPool().takeSuspendedPage(*destinationSuspendedPage) : nullptr;
-            if (suspendedPage && suspendedPage->failedToSuspend())
+            if (suspendedPage && suspendedPage->pageIsClosedOrClosing())
                 suspendedPage = nullptr;
 

trunk/Tools/ChangeLog

@@ +1 @@
+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 Alex Christensen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2019-05-21  Carlos Garcia Campos  <cgarcia@igalia.com>
 

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

@@ -4258 +4258 @@
         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;
@@ -4266 +4266 @@
     EXPECT_EQ(2u, seenPIDs.size());
     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);
 }