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
+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
}
#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);
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&);
WebPageProxy& m_page;
RefPtr<WebProcessProxy> m_process;
+ uint64_t m_mainFrameID;
WebCore::SecurityOriginData m_origin;
#if !LOG_DISABLED
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);
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());
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);
// 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;
// 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();
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());
}
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());
};
}
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);
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&);
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();
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;
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);
}
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)
+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
</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)
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
git://git.webkit.org / WebKit-https.git / commitdiff