Skip to content

Commit

Permalink
Inform client if intercepted navigation failed
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=260638
rdar://105809792

Reviewed by Brent Fulgham and J Pascoe.

A client may allow a navigation, but that navigation is ignored anyway. In this
situation, the client may never learn that the navigation was canceled. In the
specific case for SOAuthentication, the client may allow the navigation but
then that decision is overridden by SOAuthentication and the request is
cancelled. When this happens, WebKit doesn't dispatch a failure because we
never reach the provisional load stage.

This change addresses this issue by identifying when a policy decision was
intercepted and the resulting decision is PolicyAction::Ignore. In this case,
we dispatch didFailProvisionalNavigation. We make this call in
WebPageProxy::receivedNavigationPolicyDecision because if we call it earlier
then WKWebView::isLoading returns true and this is wrong and confuses clients.
Most of this patch is just plumbing.

This change extends the existing test and resolves the FIXME.

* Source/WebKit/UIProcess/Cocoa/NavigationState.mm:
(WebKit::NavigationState::NavigationClient::decidePolicyForNavigationAction):
* Source/WebKit/UIProcess/WebFramePolicyListenerProxy.cpp:
(WebKit::WebFramePolicyListenerProxy::didReceiveAppBoundDomainResult):
(WebKit::WebFramePolicyListenerProxy::didReceiveSafeBrowsingResults):
(WebKit::WebFramePolicyListenerProxy::didReceiveInitialLinkDecorationFilteringData):
(WebKit::WebFramePolicyListenerProxy::use):
(WebKit::WebFramePolicyListenerProxy::download):
(WebKit::WebFramePolicyListenerProxy::ignore):
* Source/WebKit/UIProcess/WebFramePolicyListenerProxy.h:
* Source/WebKit/UIProcess/WebFrameProxy.cpp:
(WebKit::WebFrameProxy::setUpPolicyListenerProxy):
* Source/WebKit/UIProcess/WebFrameProxy.h:
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::receivedNavigationPolicyDecision):
(WebKit::WebPageProxy::decidePolicyForNavigationAction):
(WebKit::WebPageProxy::decidePolicyForNewWindowAction):
(WebKit::WebPageProxy::decidePolicyForResponseShared):
* Source/WebKit/UIProcess/WebPageProxy.h:
* Tools/TestWebKitAPI/Tests/WebKitCocoa/SOAuthorizationTests.mm:
(-[TestSOAuthorizationDelegate webView:didFailProvisionalNavigation:withError:]):
(resetState):
(TestWebKitAPI::TEST):

Canonical link: https://commits.webkit.org/267360@main
  • Loading branch information
@sysrqb
sysrqb authored and Matthew Finkel committed Aug 28, 2023
1 parent 9a27947 commit d417f61
Show file tree
Hide file tree
Showing 8 changed files with 52 additions and 26 deletions.
6 changes: 3 additions & 3 deletions Source/WebKit/UIProcess/Cocoa/NavigationState.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ static void tryInterceptNavigation(Ref<API::NavigationAction>&& navigationAction
&& !m_navigationState->m_navigationDelegateMethods.webViewDecidePolicyForNavigationActionWithPreferencesDecisionHandler)) {
auto completionHandler = [webPage = Ref { webPageProxy }, listener = WTFMove(listener), navigationAction, defaultWebsitePolicies] (bool interceptedNavigation) {
if (interceptedNavigation) {
listener->ignore();
listener->ignore(WasNavigationIntercepted::Yes);
return;
}

Expand Down Expand Up @@ -516,7 +516,7 @@ static void tryInterceptNavigation(Ref<API::NavigationAction>&& navigationAction
case _WKNavigationActionPolicyAllowInNewProcess:
tryInterceptNavigation(WTFMove(navigationAction), webPageProxy, [actionPolicy, localListener = WTFMove(localListener), websitePolicies = WTFMove(apiWebsitePolicies)](bool interceptedNavigation) mutable {
if (interceptedNavigation) {
localListener->ignore();
localListener->ignore(WasNavigationIntercepted::Yes);
return;
}

Expand All @@ -535,7 +535,7 @@ static void tryInterceptNavigation(Ref<API::NavigationAction>&& navigationAction
case _WKNavigationActionPolicyAllowWithoutTryingAppLink:
trySOAuthorization(WTFMove(navigationAction), webPageProxy, [localListener = WTFMove(localListener), websitePolicies = WTFMove(apiWebsitePolicies)] (bool optimizedLoad) {
if (optimizedLoad) {
localListener->ignore();
localListener->ignore(WasNavigationIntercepted::Yes);
return;
}

Expand Down
14 changes: 7 additions & 7 deletions Source/WebKit/UIProcess/WebFramePolicyListenerProxy.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ void WebFramePolicyListenerProxy::didReceiveAppBoundDomainResult(std::optional<N

if (m_policyResult && m_safeBrowsingWarning && m_doneWaitingForLinkDecorationFilteringData) {
if (m_reply)
m_reply(WebCore::PolicyAction::Use, m_policyResult->first.get(), m_policyResult->second, WTFMove(*m_safeBrowsingWarning), isNavigatingToAppBoundDomain);
m_reply(WebCore::PolicyAction::Use, m_policyResult->first.get(), m_policyResult->second, WTFMove(*m_safeBrowsingWarning), isNavigatingToAppBoundDomain, WasNavigationIntercepted::No);
} else
m_isNavigatingToAppBoundDomain = isNavigatingToAppBoundDomain;
}
Expand All @@ -65,7 +65,7 @@ void WebFramePolicyListenerProxy::didReceiveSafeBrowsingResults(RefPtr<SafeBrows
ASSERT(!m_safeBrowsingWarning);
if (m_policyResult && m_isNavigatingToAppBoundDomain && m_doneWaitingForLinkDecorationFilteringData) {
if (m_reply)
m_reply(WebCore::PolicyAction::Use, m_policyResult->first.get(), m_policyResult->second, WTFMove(safeBrowsingWarning), *m_isNavigatingToAppBoundDomain);
m_reply(WebCore::PolicyAction::Use, m_policyResult->first.get(), m_policyResult->second, WTFMove(safeBrowsingWarning), *m_isNavigatingToAppBoundDomain, WasNavigationIntercepted::No);
} else
m_safeBrowsingWarning = WTFMove(safeBrowsingWarning);
}
Expand All @@ -77,7 +77,7 @@ void WebFramePolicyListenerProxy::didReceiveInitialLinkDecorationFilteringData()

if (m_policyResult && m_isNavigatingToAppBoundDomain && m_safeBrowsingWarning) {
if (m_reply)
m_reply(WebCore::PolicyAction::Use, m_policyResult->first.get(), m_policyResult->second, WTFMove(*m_safeBrowsingWarning), *m_isNavigatingToAppBoundDomain);
m_reply(WebCore::PolicyAction::Use, m_policyResult->first.get(), m_policyResult->second, WTFMove(*m_safeBrowsingWarning), *m_isNavigatingToAppBoundDomain, WasNavigationIntercepted::No);
return;
}

Expand All @@ -88,21 +88,21 @@ void WebFramePolicyListenerProxy::use(API::WebsitePolicies* policies, ProcessSwa
{
if (m_safeBrowsingWarning && m_isNavigatingToAppBoundDomain && m_doneWaitingForLinkDecorationFilteringData) {
if (m_reply)
m_reply(WebCore::PolicyAction::Use, policies, processSwapRequestedByClient, WTFMove(*m_safeBrowsingWarning), *m_isNavigatingToAppBoundDomain);
m_reply(WebCore::PolicyAction::Use, policies, processSwapRequestedByClient, WTFMove(*m_safeBrowsingWarning), *m_isNavigatingToAppBoundDomain, WasNavigationIntercepted::No);
} else if (!m_policyResult)
m_policyResult = {{ policies, processSwapRequestedByClient }};
}

void WebFramePolicyListenerProxy::download()
{
if (m_reply)
m_reply(WebCore::PolicyAction::Download, nullptr, ProcessSwapRequestedByClient::No, { }, { });
m_reply(WebCore::PolicyAction::Download, nullptr, ProcessSwapRequestedByClient::No, { }, { }, WasNavigationIntercepted::No);
}

void WebFramePolicyListenerProxy::ignore()
void WebFramePolicyListenerProxy::ignore(WasNavigationIntercepted wasNavigationIntercepted)
{
if (m_reply)
m_reply(WebCore::PolicyAction::Ignore, nullptr, ProcessSwapRequestedByClient::No, { }, { });
m_reply(WebCore::PolicyAction::Ignore, nullptr, ProcessSwapRequestedByClient::No, { }, { }, wasNavigationIntercepted);
}

} // namespace WebKit
5 changes: 3 additions & 2 deletions Source/WebKit/UIProcess/WebFramePolicyListenerProxy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@ enum class ProcessSwapRequestedByClient : bool { No, Yes };
enum class ShouldExpectSafeBrowsingResult : bool { No, Yes };
enum class ShouldExpectAppBoundDomainResult : bool { No, Yes };
enum class ShouldWaitForInitialLinkDecorationFilteringData : bool { No, Yes };
enum class WasNavigationIntercepted : bool { No, Yes };

class WebFramePolicyListenerProxy : public API::ObjectImpl<API::Object::Type::FramePolicyListener> {
public:

using Reply = CompletionHandler<void(WebCore::PolicyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient, RefPtr<SafeBrowsingWarning>&&, std::optional<NavigatingToAppBoundDomain>)>;
using Reply = CompletionHandler<void(WebCore::PolicyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient, RefPtr<SafeBrowsingWarning>&&, std::optional<NavigatingToAppBoundDomain>, WasNavigationIntercepted)>;
static Ref<WebFramePolicyListenerProxy> create(Reply&& reply, ShouldExpectSafeBrowsingResult expectSafeBrowsingResult, ShouldExpectAppBoundDomainResult expectAppBoundDomainResult, ShouldWaitForInitialLinkDecorationFilteringData shouldWaitForInitialLinkDecorationFilteringData)
{
return adoptRef(*new WebFramePolicyListenerProxy(WTFMove(reply), expectSafeBrowsingResult, expectAppBoundDomainResult, shouldWaitForInitialLinkDecorationFilteringData));
Expand All @@ -56,7 +57,7 @@ class WebFramePolicyListenerProxy : public API::ObjectImpl<API::Object::Type::Fr

void use(API::WebsitePolicies* = nullptr, ProcessSwapRequestedByClient = ProcessSwapRequestedByClient::No);
void download();
void ignore();
void ignore(WasNavigationIntercepted = WasNavigationIntercepted::No);

void didReceiveSafeBrowsingResults(RefPtr<SafeBrowsingWarning>&&);
void didReceiveAppBoundDomainResult(std::optional<NavigatingToAppBoundDomain>);
Expand Down
6 changes: 3 additions & 3 deletions Source/WebKit/UIProcess/WebFrameProxy.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -281,15 +281,15 @@ void WebFrameProxy::didChangeTitle(const String& title)
m_title = title;
}

WebFramePolicyListenerProxy& WebFrameProxy::setUpPolicyListenerProxy(CompletionHandler<void(PolicyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient, RefPtr<SafeBrowsingWarning>&&, std::optional<NavigatingToAppBoundDomain>)>&& completionHandler, ShouldExpectSafeBrowsingResult expectSafeBrowsingResult, ShouldExpectAppBoundDomainResult expectAppBoundDomainResult, ShouldWaitForInitialLinkDecorationFilteringData shouldWaitForInitialLinkDecorationFilteringData)
WebFramePolicyListenerProxy& WebFrameProxy::setUpPolicyListenerProxy(CompletionHandler<void(PolicyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient, RefPtr<SafeBrowsingWarning>&&, std::optional<NavigatingToAppBoundDomain>, WasNavigationIntercepted)>&& completionHandler, ShouldExpectSafeBrowsingResult expectSafeBrowsingResult, ShouldExpectAppBoundDomainResult expectAppBoundDomainResult, ShouldWaitForInitialLinkDecorationFilteringData shouldWaitForInitialLinkDecorationFilteringData)
{
if (m_activeListener)
m_activeListener->ignore();
m_activeListener = WebFramePolicyListenerProxy::create([this, protectedThis = Ref { *this }, completionHandler = WTFMove(completionHandler)] (PolicyAction action, API::WebsitePolicies* policies, ProcessSwapRequestedByClient processSwapRequestedByClient, RefPtr<SafeBrowsingWarning>&& safeBrowsingWarning, std::optional<NavigatingToAppBoundDomain> isNavigatingToAppBoundDomain) mutable {
m_activeListener = WebFramePolicyListenerProxy::create([this, protectedThis = Ref { *this }, completionHandler = WTFMove(completionHandler)] (PolicyAction action, API::WebsitePolicies* policies, ProcessSwapRequestedByClient processSwapRequestedByClient, RefPtr<SafeBrowsingWarning>&& safeBrowsingWarning, std::optional<NavigatingToAppBoundDomain> isNavigatingToAppBoundDomain, WasNavigationIntercepted wasNavigationIntercepted) mutable {
if (action != PolicyAction::Use && m_navigateCallback)
m_navigateCallback(pageIdentifier(), frameID());

completionHandler(action, policies, processSwapRequestedByClient, WTFMove(safeBrowsingWarning), isNavigatingToAppBoundDomain);
completionHandler(action, policies, processSwapRequestedByClient, WTFMove(safeBrowsingWarning), isNavigatingToAppBoundDomain, wasNavigationIntercepted);
m_activeListener = nullptr;
}, expectSafeBrowsingResult, expectAppBoundDomainResult, shouldWaitForInitialLinkDecorationFilteringData);
return *m_activeListener;
Expand Down
2 changes: 1 addition & 1 deletion Source/WebKit/UIProcess/WebFrameProxy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ class WebFrameProxy : public API::ObjectImpl<API::Object::Type::Frame>, public C
void didSameDocumentNavigation(const URL&); // eg. anchor navigation, session state change.
void didChangeTitle(const String&);

WebFramePolicyListenerProxy& setUpPolicyListenerProxy(CompletionHandler<void(WebCore::PolicyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient, RefPtr<SafeBrowsingWarning>&&, std::optional<NavigatingToAppBoundDomain>)>&&, ShouldExpectSafeBrowsingResult, ShouldExpectAppBoundDomainResult, ShouldWaitForInitialLinkDecorationFilteringData);
WebFramePolicyListenerProxy& setUpPolicyListenerProxy(CompletionHandler<void(WebCore::PolicyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient, RefPtr<SafeBrowsingWarning>&&, std::optional<NavigatingToAppBoundDomain>, WasNavigationIntercepted)>&&, ShouldExpectSafeBrowsingResult, ShouldExpectAppBoundDomainResult, ShouldWaitForInitialLinkDecorationFilteringData);

#if ENABLE(CONTENT_FILTERING)
void contentFilterDidBlockLoad(WebCore::ContentFilterUnblockHandler contentFilterUnblockHandler) { m_contentFilterUnblockHandler = WTFMove(contentFilterUnblockHandler); }
Expand Down
Loading

0 comments on commit d417f61

Please sign in to comment.