- Revision
- 290563
- Author
- [email protected]
- Date
- 2022-02-27 08:33:02 -0800 (Sun, 27 Feb 2022)
Log Message
Call WKNavigationDelegate.didFailProvisionalNavigation even after a cross-origin navigation with COOP
https://bugs.webkit.org/show_bug.cgi?id=237071
<rdar://problem/89354367>
Reviewed by Darin Adler.
Source/WebKit:
When doing a process swap on navigation (PSON), we start a new provisional load in the new provisional
process and ask the committed process to stop all loads. Since we swap in decidePolicyForNavigationAction,
no provisional load has started in the committed process yet. If the provisional process sends us a
didFailProvisionalLoad though, we know the provisional load has failed and we take this into consideration
and notify the client app.
When doing a process swap on resource response (due to COOP), we were behaving differently and it was
causing some confusion. Since we swap on resource response, the provisional load has started in the
committed process by the time we process-swap (unlike PSON). We were also not asking for the committed
process to stop/cancel this provisional load. As a result, a provisional load would still be going on
in the committed process while another provisional load starts happening in the provisional page / process.
Then, if the provisional process would send us a didFailProvisional, we would not pass it along to the
client app. Instead we would destroy the ProvisionalPageProxy, which would tell the committed process to
stop loading and send its own didFailProvisionalLoad, and the client app would eventually get notified.
Even though the difference in behavior was confusing, it was working in most cases. There was however
an edge case where it didn't work and we would fail to tell the client app that the provisional load
had failed. In particular, we would run into trouble when navigating cross-site to a site that adopted
the COOP header. We would first do a process-swap on navigation, stop the provisional load in the committed
process A, then start a provisional load in process B. We would then get the COOP header and we would
start a new provisional load in process C. Process B would go away since we cannot have 2 provisional
pages / processes at the same time for the same WebPageProxy. Then if the provisional load fails in
process C, we would ignore the didFailProvisionalLoad from process C and destroy the provisional page.
We would tell the committed process A to stop loading, expecting it to send its own didFailProvisionalLoad
but it wouldn't happen since PSON had already stopped all loading in process A earlier when swapping to
process B.
To address the issue, we align COOP process swap with PSON process swap. We now has the committed process
to stop all loads when process swapping on resource response due to COOP. As a result, when the provisional
load fails in the provisional process, we can simply pass it along to the client, no matter if the process
swap occurred due to PSON or COOP.
Covered by new API test.
* UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::~ProvisionalPageProxy):
We no longer need to tell the committed process to stop loading when the provisional page gets destroyed
and this was a process swap on resource response. The reason is that when process-swapping on navigation
response, we now drop the provisional load in the committed process right away, like in the PSON case.
(WebKit::ProvisionalPageProxy::cancel):
When cancelling a provisional load due to PSON or COOP, we need to notify the client that the provisional
load failed. We used to only to it in the PSON case. We now do it for both PSON and COOP since they
behave the same.
(WebKit::ProvisionalPageProxy::didFailProvisionalLoadForFrame):
Similarly as above, now that PSON and COOP behave the same, we need to pass the didFailProvisionalLoad
from the provisional process to the client app for both PSON and COOP (Not just PSON).
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::triggerBrowsingContextGroupSwitchForNavigation):
When triggering a process swap on resource response (due to COOP), we now stop the provisional load going
on in the committed process to be consistent with what we do in the PSON case. This way there is only a
single provisional load going on and it is happening in the provisional page / process.
Tools:
Add API test coverage (Test was written by Alex Christensen).
* TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm:
(TEST):
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (290562 => 290563)
--- trunk/Source/WebKit/ChangeLog 2022-02-27 16:26:51 UTC (rev 290562)
+++ trunk/Source/WebKit/ChangeLog 2022-02-27 16:33:02 UTC (rev 290563)
@@ -1,3 +1,67 @@
+2022-02-27 Chris Dumez <[email protected]>
+
+ Call WKNavigationDelegate.didFailProvisionalNavigation even after a cross-origin navigation with COOP
+ https://bugs.webkit.org/show_bug.cgi?id=237071
+ <rdar://problem/89354367>
+
+ Reviewed by Darin Adler.
+
+ When doing a process swap on navigation (PSON), we start a new provisional load in the new provisional
+ process and ask the committed process to stop all loads. Since we swap in decidePolicyForNavigationAction,
+ no provisional load has started in the committed process yet. If the provisional process sends us a
+ didFailProvisionalLoad though, we know the provisional load has failed and we take this into consideration
+ and notify the client app.
+
+ When doing a process swap on resource response (due to COOP), we were behaving differently and it was
+ causing some confusion. Since we swap on resource response, the provisional load has started in the
+ committed process by the time we process-swap (unlike PSON). We were also not asking for the committed
+ process to stop/cancel this provisional load. As a result, a provisional load would still be going on
+ in the committed process while another provisional load starts happening in the provisional page / process.
+ Then, if the provisional process would send us a didFailProvisional, we would not pass it along to the
+ client app. Instead we would destroy the ProvisionalPageProxy, which would tell the committed process to
+ stop loading and send its own didFailProvisionalLoad, and the client app would eventually get notified.
+
+ Even though the difference in behavior was confusing, it was working in most cases. There was however
+ an edge case where it didn't work and we would fail to tell the client app that the provisional load
+ had failed. In particular, we would run into trouble when navigating cross-site to a site that adopted
+ the COOP header. We would first do a process-swap on navigation, stop the provisional load in the committed
+ process A, then start a provisional load in process B. We would then get the COOP header and we would
+ start a new provisional load in process C. Process B would go away since we cannot have 2 provisional
+ pages / processes at the same time for the same WebPageProxy. Then if the provisional load fails in
+ process C, we would ignore the didFailProvisionalLoad from process C and destroy the provisional page.
+ We would tell the committed process A to stop loading, expecting it to send its own didFailProvisionalLoad
+ but it wouldn't happen since PSON had already stopped all loading in process A earlier when swapping to
+ process B.
+
+ To address the issue, we align COOP process swap with PSON process swap. We now has the committed process
+ to stop all loads when process swapping on resource response due to COOP. As a result, when the provisional
+ load fails in the provisional process, we can simply pass it along to the client, no matter if the process
+ swap occurred due to PSON or COOP.
+
+ Covered by new API test.
+
+ * UIProcess/ProvisionalPageProxy.cpp:
+ (WebKit::ProvisionalPageProxy::~ProvisionalPageProxy):
+ We no longer need to tell the committed process to stop loading when the provisional page gets destroyed
+ and this was a process swap on resource response. The reason is that when process-swapping on navigation
+ response, we now drop the provisional load in the committed process right away, like in the PSON case.
+
+ (WebKit::ProvisionalPageProxy::cancel):
+ When cancelling a provisional load due to PSON or COOP, we need to notify the client that the provisional
+ load failed. We used to only to it in the PSON case. We now do it for both PSON and COOP since they
+ behave the same.
+
+ (WebKit::ProvisionalPageProxy::didFailProvisionalLoadForFrame):
+ Similarly as above, now that PSON and COOP behave the same, we need to pass the didFailProvisionalLoad
+ from the provisional process to the client app for both PSON and COOP (Not just PSON).
+
+ * UIProcess/WebPageProxy.cpp:
+ (WebKit::WebPageProxy::triggerBrowsingContextGroupSwitchForNavigation):
+ When triggering a process swap on resource response (due to COOP), we now stop the provisional load going
+ on in the committed process to be consistent with what we do in the PSON case. This way there is only a
+ single provisional load going on and it is happening in the provisional page / process.
+
+
2022-02-27 Youenn Fablet <[email protected]>
Exposing RemoteVideoFrameProxy::write is unneeded
Modified: trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp (290562 => 290563)
--- trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp 2022-02-27 16:26:51 UTC (rev 290562)
+++ trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp 2022-02-27 16:33:02 UTC (rev 290563)
@@ -118,12 +118,6 @@
m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_webPageID);
send(Messages::WebPage::Close());
m_process->removeVisitedLinkStoreUser(m_page.visitedLinkStore(), m_page.identifier());
-
- // If we were process-swapping on navigation response then there is still a provisional load going on in the previous process
- // and its layer tree is frozen. Since we didn't end up committing the provisional process, we need to stop the load in the
- // previous process so that it cancels its navigation and unfreezes its layer tree.
- if (isProcessSwappingOnNavigationResponse())
- m_page.send(Messages::WebPage::StopLoading());
}
m_process->removeProvisionalPageProxy(*this);
@@ -143,7 +137,7 @@
void ProvisionalPageProxy::cancel()
{
// If the provisional load started, then indicate that it failed due to cancellation by calling didFailProvisionalLoadForFrame().
- if (m_provisionalLoadURL.isEmpty() || m_isProcessSwappingOnNavigationResponse)
+ if (m_provisionalLoadURL.isEmpty() || !m_mainFrame)
return;
ASSERT(m_process->state() == WebProcessProxy::State::Running);
@@ -297,15 +291,6 @@
ASSERT(!m_provisionalLoadURL.isNull());
m_provisionalLoadURL = { };
- if (m_isProcessSwappingOnNavigationResponse) {
- // If the provisional load fails and we were process-swapping on navigation response, then we simply destroy ourselves.
- // In this case, the provisional load is still ongoing in the committed process and the ProvisionalPageProxy destructor
- // will stop it and cause the committed process to send its own DidFailProvisionalLoadForFrame IPC.
- ASSERT(m_page.provisionalPageProxy() == this);
- m_page.destroyProvisionalPage();
- return;
- }
-
// Make sure the Page's main frame's expectedURL gets cleared since we updated it in didStartProvisionalLoad.
if (auto* pageMainFrame = m_page.mainFrame())
pageMainFrame->didFailProvisionalLoad();
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (290562 => 290563)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2022-02-27 16:26:51 UTC (rev 290562)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2022-02-27 16:33:02 UTC (rev 290563)
@@ -5754,6 +5754,9 @@
else
processForNavigation = m_process->processPool().processForRegistrableDomain(websiteDataStore(), responseDomain, m_process->captivePortalMode());
+ // Tell committed process to stop loading since we're going to do the provisional load in a provisional page now.
+ if (!m_provisionalPage)
+ send(Messages::WebPage::StopLoadingDueToProcessSwap());
continueNavigationInNewProcess(*navigation, nullptr, processForNavigation.releaseNonNull(), ProcessSwapRequestedByClient::No, ShouldTreatAsContinuingLoad::YesAfterProvisionalLoadStarted, nullptr, existingNetworkResourceLoadIdentifierToResume);
completionHandler(true);
}
Modified: trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp (290562 => 290563)
--- trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp 2022-02-27 16:26:51 UTC (rev 290562)
+++ trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp 2022-02-27 16:33:02 UTC (rev 290563)
@@ -587,6 +587,12 @@
if (!webPage)
return;
+ // When stopping the provisional load due to a process swap on resource response (due to COOP), the provisional load
+ // will simply keep going in a new process. As a result, we don't want to tell the injected bundle or the UIProcess client
+ // that the provisional load failed.
+ if (webPage->isStoppingLoadingDueToProcessSwap())
+ return;
+
WEBFRAMELOADERCLIENT_RELEASE_LOG(Network, "dispatchDidFailProvisionalLoad:");
RefPtr<API::Object> userData;
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp (290562 => 290563)
--- trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp 2022-02-27 16:26:51 UTC (rev 290562)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp 2022-02-27 16:33:02 UTC (rev 290563)
@@ -1834,6 +1834,12 @@
coreFrame->loader().completePageTransitionIfNeeded();
}
+void WebPage::stopLoadingDueToProcessSwap()
+{
+ SetForScope<bool> isStoppingLoadingDueToProcessSwap(m_isStoppingLoadingDueToProcessSwap, true);
+ stopLoading();
+}
+
bool WebPage::defersLoading() const
{
return m_page->defersLoading();
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebPage.h (290562 => 290563)
--- trunk/Source/WebKit/WebProcess/WebPage/WebPage.h 2022-02-27 16:26:51 UTC (rev 290562)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebPage.h 2022-02-27 16:33:02 UTC (rev 290563)
@@ -643,6 +643,7 @@
void stopLoading();
void stopLoadingFrame(WebCore::FrameIdentifier);
+ void stopLoadingDueToProcessSwap();
bool defersLoading() const;
void setDefersLoading(bool deferLoading);
@@ -991,6 +992,8 @@
void performDictionaryLookupForSelection(WebCore::Frame&, const WebCore::VisibleSelection&, WebCore::TextIndicatorPresentationTransition);
#endif
+ bool isStoppingLoadingDueToProcessSwap() const { return m_isStoppingLoadingDueToProcessSwap; }
+
bool isSmartInsertDeleteEnabled();
void setSmartInsertDeleteEnabled(bool);
@@ -2413,6 +2416,7 @@
bool m_canUseCredentialStorage { true };
bool m_didUpdateRenderingAfterCommittingLoad { false };
+ bool m_isStoppingLoadingDueToProcessSwap { false };
#if ENABLE(ARKIT_INLINE_PREVIEW)
bool m_useARKitForModel { false };
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebPage.messages.in (290562 => 290563)
--- trunk/Source/WebKit/WebProcess/WebPage/WebPage.messages.in 2022-02-27 16:26:51 UTC (rev 290562)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebPage.messages.in 2022-02-27 16:33:02 UTC (rev 290563)
@@ -194,6 +194,7 @@
Reload(uint64_t navigationID, uint32_t reloadOptions, WebKit::SandboxExtension::Handle sandboxExtensionHandle)
StopLoading()
+ StopLoadingDueToProcessSwap()
StopLoadingFrame(WebCore::FrameIdentifier frameID)
Modified: trunk/Tools/ChangeLog (290562 => 290563)
--- trunk/Tools/ChangeLog 2022-02-27 16:26:51 UTC (rev 290562)
+++ trunk/Tools/ChangeLog 2022-02-27 16:33:02 UTC (rev 290563)
@@ -1,3 +1,16 @@
+2022-02-27 Chris Dumez <[email protected]>
+
+ Call WKNavigationDelegate.didFailProvisionalNavigation even after a cross-origin navigation with COOP
+ https://bugs.webkit.org/show_bug.cgi?id=237071
+ <rdar://problem/89354367>
+
+ Reviewed by Darin Adler.
+
+ Add API test coverage (Test was written by Alex Christensen).
+
+ * TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm:
+ (TEST):
+
2022-02-25 Sihui Liu <[email protected]>
[macOS] TestWebKitAPI.WebKit.MigrateLocalStorageDataToGeneralStorageDirectory is a flaky failure
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm (290562 => 290563)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm 2022-02-27 16:26:51 UTC (rev 290562)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm 2022-02-27 16:33:02 UTC (rev 290563)
@@ -38,6 +38,8 @@
#import <WebKit/WKNavigationPrivate.h>
#import <WebKit/WKWebView.h>
#import <WebKit/WKWebViewConfigurationPrivate.h>
+#import <WebKit/WKWebsiteDataStorePrivate.h>
+#import <WebKit/_WKWebsiteDataStoreConfiguration.h>
#import <wtf/RetainPtr.h>
#import <wtf/Vector.h>
@@ -1010,3 +1012,54 @@
EXPECT_FALSE(didTryToLoadRadarURL);
}
+
+TEST(WKNavigation, CrossOriginCOOPCancelResponseFailProvisionalNavigationCallback)
+{
+ using namespace TestWebKitAPI;
+ HTTPServer server({
+ { "/path1", { "hi" } },
+ { "/path2", { "hi" } },
+ { "/path3", { { { "Cross-Origin-Opener-Policy", "same-origin" } }, "hi" } }
+ }, HTTPServer::Protocol::HttpsProxy);
+
+ auto storeConfiguration = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration]);
+ [storeConfiguration setProxyConfiguration:@{
+ (NSString *)kCFStreamPropertyHTTPSProxyHost: @"127.0.0.1",
+ (NSString *)kCFStreamPropertyHTTPSProxyPort: @(server.port())
+ }];
+ auto dataStore = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration:storeConfiguration.get()]);
+ auto configuration = adoptNS([WKWebViewConfiguration new]);
+ [configuration setWebsiteDataStore:dataStore.get()];
+ auto webView = adoptNS([[WKWebView alloc] initWithFrame:CGRectZero configuration:configuration.get()]);
+
+ __block Vector<bool> finishedSuccessfullyCallbacks;
+ auto loadWithResponsePolicy = ^(WKWebView *webView, NSString *url, WKNavigationResponsePolicy responsePolicy) {
+ auto callbacksSizeBefore = finishedSuccessfullyCallbacks.size();
+
+ auto delegate = adoptNS([TestNavigationDelegate new]);
+ delegate.get().decidePolicyForNavigationResponse = ^(WKNavigationResponse *response, void (^decisionHandler)(WKNavigationResponsePolicy)) {
+ decisionHandler(responsePolicy);
+ };
+
+ delegate.get().didReceiveAuthenticationChallenge = ^(WKWebView *, NSURLAuthenticationChallenge *challenge, void (^completionHandler)(NSURLSessionAuthChallengeDisposition, NSURLCredential *)) {
+ completionHandler(NSURLSessionAuthChallengeUseCredential, [NSURLCredential credentialForTrust:challenge.protectionSpace.serverTrust]);
+ };
+ delegate.get().didFailProvisionalNavigation = ^(WKWebView *, WKNavigation *, NSError *) {
+ finishedSuccessfullyCallbacks.append(false);
+ };
+ delegate.get().didFinishNavigation = ^(WKWebView *, WKNavigation *) {
+ finishedSuccessfullyCallbacks.append(true);
+ };
+ [webView setNavigationDelegate:delegate.get()];
+ [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:url]]];
+ while (finishedSuccessfullyCallbacks.size() == callbacksSizeBefore)
+ TestWebKitAPI::Util::spinRunLoop(10);
+ };
+
+ loadWithResponsePolicy(webView.get(), @"https://webkit.org/path1", WKNavigationResponsePolicyAllow);
+ loadWithResponsePolicy(webView.get(), @"https://webkit.org/path2", WKNavigationResponsePolicyCancel);
+ loadWithResponsePolicy(webView.get(), @"https://example.com/path3", WKNavigationResponsePolicyCancel);
+
+ Vector<bool> expectedCallbacks { true, false, false };
+ EXPECT_EQ(finishedSuccessfullyCallbacks, expectedCallbacks);
+}
