Regression(PSON): Assertion hit under WebPageProxy::didNavigateWithNavigationData()
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Oct 2018 18:21:11 +0000 (18:21 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Oct 2018 18:21:11 +0000 (18:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=190418
<rdar://problem/45059769>

Reviewed by Geoffrey Garen.

Source/WebKit:

When process swapping and "suspending" the previous WebProcess in a SuspendedPageProxy,
we need to keep around the main frame's ID that still exists on in this process. This
is needed so that we can re-create a UI-side WebFrameProxy for the WebFrame that exists
in the WebProcess, if we ever swap back to this suspended process (see login in
WebPageProxy::swapToWebProcess()).

The bug was that the main frame ID was stored on the WebPageProxy via m_mainFrameID instead of the
SuspendedPageProxy. This means that m_mainFrameID would get overriden when navigating in the new
WebProcess with the value 1 (because WebFrame identifiers start at 1 and are per-WebProcess).
This would lead to us constructing a WebFrameProxy with the wrong frame identifier in
WebPageProxy::swapToWebProcess(), which would override an existing unrelated WebFrame in the
WebProcessProxy's HashMap of frames. This would lead to crashes later on as the WebFrame
would not be associated to the WebPageProxy we expect.

* UIProcess/SuspendedPageProxy.cpp:
(WebKit::SuspendedPageProxy::SuspendedPageProxy):
* UIProcess/SuspendedPageProxy.h:
(WebKit::SuspendedPageProxy::mainFrameID const):
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::maybeCreateSuspendedPage):
(WebKit::WebPageProxy::swapToWebProcess):
(WebKit::WebPageProxy::continueNavigationInNewProcess):
(WebKit::WebPageProxy::didCreateMainFrame):
* UIProcess/WebPageProxy.h:
* UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::suspendWebPageProxy):
* UIProcess/WebProcessProxy.h:

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

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

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

index b3c017f..7ef22d1 100644 (file)
@@ -1,3 +1,39 @@
+2018-10-10  Chris Dumez  <cdumez@apple.com>
+
+        Regression(PSON): Assertion hit under WebPageProxy::didNavigateWithNavigationData()
+        https://bugs.webkit.org/show_bug.cgi?id=190418
+        <rdar://problem/45059769>
+
+        Reviewed by Geoffrey Garen.
+
+        When process swapping and "suspending" the previous WebProcess in a SuspendedPageProxy,
+        we need to keep around the main frame's ID that still exists on in this process. This
+        is needed so that we can re-create a UI-side WebFrameProxy for the WebFrame that exists
+        in the WebProcess, if we ever swap back to this suspended process (see login in
+        WebPageProxy::swapToWebProcess()).
+
+        The bug was that the main frame ID was stored on the WebPageProxy via m_mainFrameID instead of the
+        SuspendedPageProxy. This means that m_mainFrameID would get overriden when navigating in the new
+        WebProcess with the value 1 (because WebFrame identifiers start at 1 and are per-WebProcess).
+        This would lead to us constructing a WebFrameProxy with the wrong frame identifier in
+        WebPageProxy::swapToWebProcess(), which would override an existing unrelated WebFrame in the
+        WebProcessProxy's HashMap of frames. This would lead to crashes later on as the WebFrame
+        would not be associated to the WebPageProxy we expect.
+
+        * UIProcess/SuspendedPageProxy.cpp:
+        (WebKit::SuspendedPageProxy::SuspendedPageProxy):
+        * UIProcess/SuspendedPageProxy.h:
+        (WebKit::SuspendedPageProxy::mainFrameID const):
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::maybeCreateSuspendedPage):
+        (WebKit::WebPageProxy::swapToWebProcess):
+        (WebKit::WebPageProxy::continueNavigationInNewProcess):
+        (WebKit::WebPageProxy::didCreateMainFrame):
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::suspendWebPageProxy):
+        * UIProcess/WebProcessProxy.h:
+
 2018-10-10  Antti Koivisto  <antti@apple.com>
 
         Do domain prewarming for processes for new tabs
index 8bc4a75..02ac661 100644 (file)
@@ -74,9 +74,10 @@ static const HashSet<IPC::StringReference>& messageNamesToIgnoreWhileSuspended()
 }
 #endif
 
-SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, Ref<WebProcessProxy>&& process, WebBackForwardListItem& item)
+SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, Ref<WebProcessProxy>&& process, WebBackForwardListItem& item, uint64_t mainFrameID)
     : m_page(page)
     , m_process(WTFMove(process))
+    , m_mainFrameID(mainFrameID)
     , m_origin(SecurityOriginData::fromURL({ { }, item.url() }))
 {
     item.setSuspendedPage(*this);
index 3631f9f..04c4ecf 100644 (file)
@@ -38,13 +38,14 @@ class WebProcessProxy;
 
 class SuspendedPageProxy : public CanMakeWeakPtr<SuspendedPageProxy> {
 public:
-    SuspendedPageProxy(WebPageProxy&, Ref<WebProcessProxy>&&, WebBackForwardListItem&);
+    SuspendedPageProxy(WebPageProxy&, Ref<WebProcessProxy>&&, WebBackForwardListItem&, uint64_t mainFrameID);
     ~SuspendedPageProxy();
 
     void didReceiveMessage(IPC::Connection&, IPC::Decoder&);
 
     WebPageProxy& page() const { return m_page; }
     WebProcessProxy* process() const { return m_process.get(); }
+    uint64_t mainFrameID() const { return m_mainFrameID; }
     const WebCore::SecurityOriginData& origin() const { return m_origin; }
 
     void webProcessDidClose(WebProcessProxy&);
@@ -60,6 +61,7 @@ private:
 
     WebPageProxy& m_page;
     RefPtr<WebProcessProxy> m_process;
+    uint64_t m_mainFrameID;
     WebCore::SecurityOriginData m_origin;
 
 #if !LOG_DISABLED
index 108c6fa..eacaf48 100644 (file)
@@ -710,7 +710,7 @@ void WebPageProxy::reattachToWebProcess()
     finishAttachingToWebProcess();
 }
 
-SuspendedPageProxy* WebPageProxy::maybeCreateSuspendedPage(WebProcessProxy& process, API::Navigation& navigation)
+SuspendedPageProxy* WebPageProxy::maybeCreateSuspendedPage(WebProcessProxy& process, API::Navigation& navigation, uint64_t mainFrameID)
 {
     ASSERT(!m_suspendedPage || m_suspendedPage->process() != &process);
 
@@ -720,7 +720,7 @@ SuspendedPageProxy* WebPageProxy::maybeCreateSuspendedPage(WebProcessProxy& proc
         return nullptr;
     }
 
-    m_suspendedPage = std::make_unique<SuspendedPageProxy>(*this, process, *currentItem);
+    m_suspendedPage = std::make_unique<SuspendedPageProxy>(*this, process, *currentItem, mainFrameID);
 
     LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " created suspended page %s for process pid %i, back/forward item %s" PRIu64, pageID(), m_suspendedPage->loggingString(), process.processIdentifier(), currentItem->itemID().logString());
 
@@ -733,7 +733,7 @@ void WebPageProxy::suspendedPageClosed(SuspendedPageProxy& page)
     m_suspendedPage = nullptr;
 }
 
-void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigation& navigation)
+void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigation& navigation, std::optional<uint64_t> mainFrameIDInPreviousProcess)
 {
     ASSERT(!m_isClosed);
     RELEASE_LOG_IF_ALLOWED(Process, "%p WebPageProxy::swapToWebProcess\n", this);
@@ -741,7 +741,8 @@ void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigat
     // If the process we're attaching to is kept alive solely by our current suspended page,
     // we need to maintain that by temporarily keeping the suspended page alive.
     auto currentSuspendedPage = WTFMove(m_suspendedPage);
-    m_process->suspendWebPageProxy(*this, navigation);
+    if (mainFrameIDInPreviousProcess)
+        m_process->suspendWebPageProxy(*this, navigation, *mainFrameIDInPreviousProcess);
 
     m_process = WTFMove(process);
     m_isValid = true;
@@ -752,9 +753,8 @@ void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigat
     // already exists and already has a main frame.
     if (currentSuspendedPage && currentSuspendedPage->process() == m_process.ptr()) {
         ASSERT(!m_mainFrame);
-        ASSERT(m_mainFrameID);
-        m_mainFrame = WebFrameProxy::create(this, *m_mainFrameID);
-        m_process->frameCreated(*m_mainFrameID, *m_mainFrame);
+        m_mainFrame = WebFrameProxy::create(this, currentSuspendedPage->mainFrameID());
+        m_process->frameCreated(currentSuspendedPage->mainFrameID(), *m_mainFrame);
     }
 
     finishAttachingToWebProcess();
@@ -2537,15 +2537,13 @@ void WebPageProxy::continueNavigationInNewProcess(API::Navigation& navigation, R
     LOG(Loading, "Continuing navigation %" PRIu64 " '%s' in a new web process", navigation.navigationID(), navigation.loggingString());
 
     Ref<WebProcessProxy> previousProcess = m_process.copyRef();
-    std::optional<uint64_t> navigatedFrameIdentifierInPreviousProcess;
-    if (m_mainFrame)
-        navigatedFrameIdentifierInPreviousProcess = m_mainFrame->frameID();
+    std::optional<uint64_t> mainFrameIDInPreviousProcess = m_mainFrame ? std::make_optional(m_mainFrame->frameID()) : std::nullopt;
 
     ASSERT(m_process.ptr() != process.ptr());
 
     processDidTerminate(ProcessTerminationReason::NavigationSwap);
 
-    swapToWebProcess(WTFMove(process), navigation);
+    swapToWebProcess(WTFMove(process), navigation, mainFrameIDInPreviousProcess);
 
     if (auto* item = navigation.targetItem()) {
         LOG(Loading, "WebPageProxy %p continueNavigationInNewProcess to back item URL %s", this, item->url().utf8().data());
@@ -2584,13 +2582,13 @@ void WebPageProxy::continueNavigationInNewProcess(API::Navigation& navigation, R
     }
 
     bool isInitialNavigationInNewWindow = openedByDOM() && !hasCommittedAnyProvisionalLoads();
-    if (!isInitialNavigationInNewWindow || !navigatedFrameIdentifierInPreviousProcess)
+    if (!isInitialNavigationInNewWindow || !mainFrameIDInPreviousProcess)
         return;
 
-    m_mainFrameWindowCreationHandler = [this, previousProcess = WTFMove(previousProcess), navigatedFrameIdentifierInPreviousProcess = *navigatedFrameIdentifierInPreviousProcess](const GlobalWindowIdentifier& windowIdentifier) {
+    m_mainFrameWindowCreationHandler = [this, previousProcess = WTFMove(previousProcess), mainFrameIDInPreviousProcess = *mainFrameIDInPreviousProcess](const GlobalWindowIdentifier& windowIdentifier) {
         ASSERT(m_mainFrame);
         GlobalFrameIdentifier navigatedFrameIdentifierInNewProcess { pageID(), m_mainFrame->frameID() };
-        previousProcess->send(Messages::WebPage::FrameBecameRemote(navigatedFrameIdentifierInPreviousProcess, navigatedFrameIdentifierInNewProcess, windowIdentifier), pageID());
+        previousProcess->send(Messages::WebPage::FrameBecameRemote(mainFrameIDInPreviousProcess, navigatedFrameIdentifierInNewProcess, windowIdentifier), pageID());
     };
 }
 
@@ -3415,7 +3413,6 @@ void WebPageProxy::didCreateMainFrame(uint64_t frameID)
     MESSAGE_CHECK(m_process->canCreateFrame(frameID));
 
     m_mainFrame = WebFrameProxy::create(this, frameID);
-    m_mainFrameID = frameID;
 
     // Add the frame to the process wide map.
     m_process->frameCreated(frameID, *m_mainFrame);
index 8e92632..92a43d6 100644 (file)
@@ -1354,7 +1354,7 @@ public:
 
     WebPreferencesStore preferencesStore() const;
 
-    SuspendedPageProxy* maybeCreateSuspendedPage(WebProcessProxy&, API::Navigation&);
+    SuspendedPageProxy* maybeCreateSuspendedPage(WebProcessProxy&, API::Navigation&, uint64_t mainFrameID);
     SuspendedPageProxy* suspendedPage() const { return m_suspendedPage.get(); }
     void suspendedPageClosed(SuspendedPageProxy&);
 
@@ -1525,7 +1525,7 @@ private:
     void setCanShortCircuitHorizontalWheelEvents(bool canShortCircuitHorizontalWheelEvents) { m_canShortCircuitHorizontalWheelEvents = canShortCircuitHorizontalWheelEvents; }
 
     void reattachToWebProcess();
-    void swapToWebProcess(Ref<WebProcessProxy>&&, API::Navigation&);
+    void swapToWebProcess(Ref<WebProcessProxy>&&, API::Navigation&, std::optional<uint64_t> mainFrameIDInPreviousProcess);
     void finishAttachingToWebProcess();
 
     RefPtr<API::Navigation> reattachToWebProcessForReload();
@@ -1894,7 +1894,6 @@ private:
     Ref<WebsiteDataStore> m_websiteDataStore;
 
     RefPtr<WebFrameProxy> m_mainFrame;
-    std::optional<uint64_t> m_mainFrameID;
     Function<void()> m_mainFrameCreationHandler;
     Function<void(const WebCore::GlobalWindowIdentifier&)> m_mainFrameWindowCreationHandler;
 
index d671d6a..c9947db 100644 (file)
@@ -438,9 +438,9 @@ void WebProcessProxy::addExistingWebPage(WebPageProxy& webPage, uint64_t pageID)
     updateBackgroundResponsivenessTimer();
 }
 
-void WebProcessProxy::suspendWebPageProxy(WebPageProxy& webPage, API::Navigation& navigation)
+void WebProcessProxy::suspendWebPageProxy(WebPageProxy& webPage, API::Navigation& navigation, uint64_t mainFrameID)
 {
-    if (auto* suspendedPage = webPage.maybeCreateSuspendedPage(*this, navigation)) {
+    if (auto* suspendedPage = webPage.maybeCreateSuspendedPage(*this, navigation, mainFrameID)) {
         LOG(ProcessSwapping, "WebProcessProxy pid %i added suspended page %s", processIdentifier(), suspendedPage->loggingString());
         m_suspendedPageMap.set(webPage.pageID(), suspendedPage);
     }
index 325acbf..33274df 100644 (file)
@@ -208,7 +208,7 @@ public:
     void didCommitProvisionalLoad() { m_hasCommittedAnyProvisionalLoads = true; }
     bool hasCommittedAnyProvisionalLoads() const { return m_hasCommittedAnyProvisionalLoads; }
 
-    void suspendWebPageProxy(WebPageProxy&, API::Navigation&);
+    void suspendWebPageProxy(WebPageProxy&, API::Navigation&, uint64_t mainFrameID);
     void suspendedPageWasDestroyed(SuspendedPageProxy&);
 
 #if PLATFORM(WATCHOS)
index 214cf40..bbdb53e 100644 (file)
@@ -1,3 +1,15 @@
+2018-10-10  Chris Dumez  <cdumez@apple.com>
+
+        Regression(PSON): Assertion hit under WebPageProxy::didNavigateWithNavigationData()
+        https://bugs.webkit.org/show_bug.cgi?id=190418
+        <rdar://problem/45059769>
+
+        Reviewed by Geoffrey Garen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2018-10-10  Guillaume Emont  <guijemont@igalia.com>
 
         [JSCOnly Add an armv7 JSCOnly EWS that runs tests
index 4d16dac..7615aee 100644 (file)
@@ -307,6 +307,16 @@ window.onload = function() {
 </script>
 )PSONRESOURCE";
 
+static const char* linkToAppleTestBytes = R"PSONRESOURCE(
+<script>
+window.addEventListener('pageshow', function(event) {
+    if (event.persisted)
+        window.webkit.messageHandlers.pson.postMessage("Was persisted");
+});
+</script>
+<a id="testLink" href="pson://www.apple.com/main.html">Navigate</a>
+)PSONRESOURCE";
+
 #endif // PLATFORM(MAC)
 
 TEST(ProcessSwap, Basic)
@@ -1927,4 +1937,80 @@ TEST(ProcessSwap, TerminatedSuspendedPageProcess)
     EXPECT_NE(pid3, pid4);
 }
 
+#if PLATFORM(MAC)
+
+TEST(ProcessSwap, GoBackToSuspendedPageWithMainFrameIDThatIsNotOne)
+{
+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    [processPoolConfiguration setProcessSwapsOnNavigation:YES];
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webViewConfiguration setProcessPool:processPool.get()];
+    auto handler = adoptNS([[PSONScheme alloc] init]);
+    [handler addMappingFromURLString:@"pson://www.webkit.org/main1.html" toData:targetBlankSameSiteNoOpenerTestBytes];
+    [handler addMappingFromURLString:@"pson://www.webkit.org/main2.html" toData:linkToAppleTestBytes];
+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+    auto messageHandler = adoptNS([[PSONMessageHandler alloc] init]);
+    [[webViewConfiguration userContentController] addScriptMessageHandler:messageHandler.get() name:@"pson"];
+
+    auto webView1 = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    auto navigationDelegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+    [webView1 setNavigationDelegate:navigationDelegate.get()];
+    auto uiDelegate = adoptNS([[PSONUIDelegate alloc] initWithNavigationDelegate:navigationDelegate.get()]);
+    [webView1 setUIDelegate:uiDelegate.get()];
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main1.html"]];
+
+    [webView1 loadRequest:request];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    EXPECT_WK_STREQ(@"pson://www.webkit.org/main1.html", [[webView1 URL] absoluteString]);
+    auto pid1 = [webView1 _webProcessIdentifier];
+
+    TestWebKitAPI::Util::run(&didCreateWebView);
+    didCreateWebView = false;
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    // New WKWebView has now navigated to webkit.org.
+    EXPECT_WK_STREQ(@"pson://www.webkit.org/main2.html", [[createdWebView URL] absoluteString]);
+    auto pid2 = [createdWebView _webProcessIdentifier];
+    EXPECT_EQ(pid1, pid2);
+
+    // Click link in new WKWebView so that it navigates cross-site to apple.com.
+    [createdWebView evaluateJavaScript:@"testLink.click()" completionHandler:nil];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    // New WKWebView has now navigated to apple.com.
+    EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[createdWebView URL] absoluteString]);
+    auto pid3 = [createdWebView _webProcessIdentifier];
+    EXPECT_NE(pid1, pid3); // Should have process-swapped.
+
+    // Navigate back to the suspended page (should use the page cache).
+    [createdWebView goBack];
+    TestWebKitAPI::Util::run(&receivedMessage);
+    receivedMessage = false;
+
+    EXPECT_WK_STREQ(@"pson://www.webkit.org/main2.html", [[createdWebView URL] absoluteString]);
+    auto pid4 = [createdWebView _webProcessIdentifier];
+    EXPECT_EQ(pid1, pid4); // Should have process-swapped to the original "suspended" process.
+
+    // Do a fragment navigation in the original WKWebView and make sure this does not crash.
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main1.html#testLink"]];
+    [webView1 loadRequest:request];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    EXPECT_WK_STREQ(@"pson://www.webkit.org/main1.html#testLink", [[webView1 URL] absoluteString]);
+    auto pid5 = [createdWebView _webProcessIdentifier];
+    EXPECT_EQ(pid1, pid5);
+}
+
+#endif // PLATFORM(MAC)
+
 #endif // WK_API_ENABLED