Diff
Modified: trunk/Source/WebKit/ChangeLog (237007 => 237008)
--- trunk/Source/WebKit/ChangeLog 2018-10-10 18:20:08 UTC (rev 237007)
+++ trunk/Source/WebKit/ChangeLog 2018-10-10 18:21:11 UTC (rev 237008)
@@ -1,3 +1,39 @@
+2018-10-10 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
Do domain prewarming for processes for new tabs
Modified: trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp (237007 => 237008)
--- trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp 2018-10-10 18:20:08 UTC (rev 237007)
+++ trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp 2018-10-10 18:21:11 UTC (rev 237008)
@@ -74,9 +74,10 @@
}
#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);
Modified: trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h (237007 => 237008)
--- trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h 2018-10-10 18:20:08 UTC (rev 237007)
+++ trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h 2018-10-10 18:21:11 UTC (rev 237008)
@@ -38,7 +38,7 @@
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&);
@@ -45,6 +45,7 @@
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 @@
WebPageProxy& m_page;
RefPtr<WebProcessProxy> m_process;
+ uint64_t m_mainFrameID;
WebCore::SecurityOriginData m_origin;
#if !LOG_DISABLED
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (237007 => 237008)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2018-10-10 18:20:08 UTC (rev 237007)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2018-10-10 18:21:11 UTC (rev 237008)
@@ -710,7 +710,7 @@
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 @@
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 @@
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 @@
// 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 @@
// 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 @@
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 @@
}
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 @@
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);
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.h (237007 => 237008)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.h 2018-10-10 18:20:08 UTC (rev 237007)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.h 2018-10-10 18:21:11 UTC (rev 237008)
@@ -1354,7 +1354,7 @@
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 @@
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 @@
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;
Modified: trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp (237007 => 237008)
--- trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp 2018-10-10 18:20:08 UTC (rev 237007)
+++ trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp 2018-10-10 18:21:11 UTC (rev 237008)
@@ -438,9 +438,9 @@
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);
}
Modified: trunk/Source/WebKit/UIProcess/WebProcessProxy.h (237007 => 237008)
--- trunk/Source/WebKit/UIProcess/WebProcessProxy.h 2018-10-10 18:20:08 UTC (rev 237007)
+++ trunk/Source/WebKit/UIProcess/WebProcessProxy.h 2018-10-10 18:21:11 UTC (rev 237008)
@@ -208,7 +208,7 @@
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)
Modified: trunk/Tools/ChangeLog (237007 => 237008)
--- trunk/Tools/ChangeLog 2018-10-10 18:20:08 UTC (rev 237007)
+++ trunk/Tools/ChangeLog 2018-10-10 18:21:11 UTC (rev 237008)
@@ -1,3 +1,15 @@
+2018-10-10 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
[JSCOnly Add an armv7 JSCOnly EWS that runs tests
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (237007 => 237008)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2018-10-10 18:20:08 UTC (rev 237007)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2018-10-10 18:21:11 UTC (rev 237008)
@@ -307,6 +307,16 @@
</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=""
+)PSONRESOURCE";
+
#endif // PLATFORM(MAC)
TEST(ProcessSwap, Basic)
@@ -1927,4 +1937,80 @@
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
