Assert in WebPageProxy::suspendCurrentPageIfPossible()
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 11 Mar 2019 23:31:23 +0000 (23:31 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 11 Mar 2019 23:31:23 +0000 (23:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195506
<rdar://problem/48733477>

Reviewed by Alex Christensen.

Source/WebKit:

The crash was caused by the top-hit preloading logic in MobileSafari which creates a new Web view that is *related*
to the previous one, restores the session state onto it and then triggers a load in this new Web view.

Initially, the 2 Web views use the same process as they are related. However, if the new load's URL is cross-site
with regards to the first view's URL, then we decide to process swap in the new view. This process swap makes
sense from a security standpoint. However, when we commit the load in the new process and call
suspendCurrentPageIfPossible() we end up in an unexpected state because we have a fromItem (due to the session
state having been restored on the new view) but we haven't committed any load yet in this new view.

To address the issue, suspendCurrentPageIfPossible() now returns early and does not attempt to suspend anything
if we have not committed any load yet.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::suspendCurrentPageIfPossible):
Do not attempt to suspend to current page if we have not committed any provisional load yet.

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242755 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/WebPageProxy.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

index beb0b4d..fb9a327 100644 (file)
@@ -1,5 +1,29 @@
 2019-03-11  Chris Dumez  <cdumez@apple.com>
 
+        Assert in WebPageProxy::suspendCurrentPageIfPossible()
+        https://bugs.webkit.org/show_bug.cgi?id=195506
+        <rdar://problem/48733477>
+
+        Reviewed by Alex Christensen.
+
+        The crash was caused by the top-hit preloading logic in MobileSafari which creates a new Web view that is *related*
+        to the previous one, restores the session state onto it and then triggers a load in this new Web view.
+
+        Initially, the 2 Web views use the same process as they are related. However, if the new load's URL is cross-site
+        with regards to the first view's URL, then we decide to process swap in the new view. This process swap makes
+        sense from a security standpoint. However, when we commit the load in the new process and call
+        suspendCurrentPageIfPossible() we end up in an unexpected state because we have a fromItem (due to the session
+        state having been restored on the new view) but we haven't committed any load yet in this new view.
+
+        To address the issue, suspendCurrentPageIfPossible() now returns early and does not attempt to suspend anything
+        if we have not committed any load yet.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::suspendCurrentPageIfPossible):
+        Do not attempt to suspend to current page if we have not committed any provisional load yet.
+
+2019-03-11  Chris Dumez  <cdumez@apple.com>
+
         Update device orientation & motion permission native SPI as per latest proposal
         https://bugs.webkit.org/show_bug.cgi?id=195567
 
index f14b8d6..65d38ae 100644 (file)
@@ -770,6 +770,11 @@ bool WebPageProxy::suspendCurrentPageIfPossible(API::Navigation& navigation, Opt
         return false;
     }
 
+    if (!hasCommittedAnyProvisionalLoads()) {
+        RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because has not committed any load yet", m_process->processIdentifier());
+        return false;
+    }
+
     if (isPageOpenedByDOMShowingInitialEmptyDocument()) {
         RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because it is showing the initial empty document", m_process->processIdentifier());
         return false;
index 6f9f22b..9fd2305 100644 (file)
@@ -1,3 +1,15 @@
+2019-03-11  Chris Dumez  <cdumez@apple.com>
+
+        Assert in WebPageProxy::suspendCurrentPageIfPossible()
+        https://bugs.webkit.org/show_bug.cgi?id=195506
+        <rdar://problem/48733477>
+
+        Reviewed by Alex Christensen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2019-03-11  Shawn Roberts  <sroberts@apple.com>
 
         Adding myself to contributors.json
index d786974..3c326d6 100644 (file)
@@ -4709,6 +4709,48 @@ TEST(ProcessSwap, UseSessionCookiesAfterProcessSwapInNonDefaultPersistentSession
     EXPECT_WK_STREQ(@"foo=bar", receivedMessages.get()[0]);
 }
 
+TEST(ProcessSwap, ProcessSwapInRelatedView)
+{
+    auto processPoolConfiguration = psonProcessPoolConfiguration();
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto webView1Configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webView1Configuration setProcessPool:processPool.get()];
+    auto handler = adoptNS([[PSONScheme alloc] init]);
+    [webView1Configuration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+    auto webView1 = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webView1Configuration.get()]);
+
+    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+    [webView1 setNavigationDelegate:delegate.get()];
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+    [webView1 loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto applePID = [webView1 _webProcessIdentifier];
+
+    auto webView2Configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webView2Configuration setProcessPool:processPool.get()];
+    [webView2Configuration _setRelatedWebView:webView1.get()];
+    [webView2Configuration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+    auto webView2 = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webView2Configuration.get()]);
+    [webView2 _restoreSessionState:webView1.get()._sessionState andNavigate:NO];
+
+    [webView2 setNavigationDelegate:delegate.get()];
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+    [webView2 loadRequest:request];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto webkitPID = [webView2 _webProcessIdentifier];
+    EXPECT_NE(applePID, webkitPID);
+}
+
 #if PLATFORM(MAC)
 
 TEST(ProcessSwap, TerminateProcessAfterProcessSwap)