Title: [242755] trunk
Revision
242755
Author
[email protected]
Date
2019-03-11 16:31:23 -0700 (Mon, 11 Mar 2019)

Log Message

Assert in WebPageProxy::suspendCurrentPageIfPossible()
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:

Modified Paths

Diff

Modified: trunk/Source/WebKit/ChangeLog (242754 => 242755)


--- trunk/Source/WebKit/ChangeLog	2019-03-11 23:30:38 UTC (rev 242754)
+++ trunk/Source/WebKit/ChangeLog	2019-03-11 23:31:23 UTC (rev 242755)
@@ -1,5 +1,29 @@
 2019-03-11  Chris Dumez  <[email protected]>
 
+        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  <[email protected]>
+
         Update device orientation & motion permission native SPI as per latest proposal
         https://bugs.webkit.org/show_bug.cgi?id=195567
 

Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (242754 => 242755)


--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp	2019-03-11 23:30:38 UTC (rev 242754)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp	2019-03-11 23:31:23 UTC (rev 242755)
@@ -770,6 +770,11 @@
         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;

Modified: trunk/Tools/ChangeLog (242754 => 242755)


--- trunk/Tools/ChangeLog	2019-03-11 23:30:38 UTC (rev 242754)
+++ trunk/Tools/ChangeLog	2019-03-11 23:31:23 UTC (rev 242755)
@@ -1,3 +1,15 @@
+2019-03-11  Chris Dumez  <[email protected]>
+
+        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  <[email protected]>
 
         Adding myself to contributors.json

Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (242754 => 242755)


--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm	2019-03-11 23:30:38 UTC (rev 242754)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm	2019-03-11 23:31:23 UTC (rev 242755)
@@ -4709,6 +4709,48 @@
     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)
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to