Skip to content
Permalink
Browse files
WebKit2 can only have one active navigation policy check for a given …
…frame

https://bugs.webkit.org/show_bug.cgi?id=229012

Reviewed by Youenn Fablet.

LayoutTests/imported/w3c:

Rebaseline test that is now passing one more check.

* web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt:

Source/WebKit:

WebKit2 could only have one active navigation policy check for a given frame
and there was a FIXME comment about this in the code. This was causing some
WPT tests to timeout in WebKit2 only because those tests would trigger
several navigations (e.g. in new windows) and only the last one would proceed
(earlier ones would get cancelled).

This patch updates the policy checking logic in WebFrame so that we can support
several concurrent policy checks.

No new tests, unskipped / rebaselined existing tests.

* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::cancelPolicyCheck):
* WebProcess/WebPage/WebFrame.cpp:
(WebKit::WebFrame::~WebFrame):
(WebKit::WebFrame::setUpPolicyListener):
(WebKit::WebFrame::setUpWillSubmitFormListener):
(WebKit::WebFrame::continueWillSubmitForm):
(WebKit::WebFrame::invalidatePolicyListeners):
(WebKit::WebFrame::didReceivePolicyDecision):
* WebProcess/WebPage/WebFrame.h:

LayoutTests:

Unskip a couple of tests that are no longer timing out in WebKit2.

* platform/ios-wk1/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt: Removed.
* platform/mac-wk1/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt: Removed.
* platform/wk2/TestExpectations:

Canonical link: https://commits.webkit.org/240828@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@281445 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
cdumez committed Aug 23, 2021
1 parent 0e86f37 commit ff71e5a6e93da6669ebdf86e7dfa2ce41bfcb2fa
Showing 10 changed files with 83 additions and 50 deletions.
@@ -1,3 +1,16 @@
2021-08-23 Chris Dumez <cdumez@apple.com>

WebKit2 can only have one active navigation policy check for a given frame
https://bugs.webkit.org/show_bug.cgi?id=229012

Reviewed by Youenn Fablet.

Unskip a couple of tests that are no longer timing out in WebKit2.

* platform/ios-wk1/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt: Removed.
* platform/mac-wk1/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt: Removed.
* platform/wk2/TestExpectations:

2021-08-23 Carlos Garcia Campos <cgarcia@igalia.com>

Create a RenderLineBreak when BR element has unsupported content data style
@@ -1,3 +1,14 @@
2021-08-23 Chris Dumez <cdumez@apple.com>

WebKit2 can only have one active navigation policy check for a given frame
https://bugs.webkit.org/show_bug.cgi?id=229012

Reviewed by Youenn Fablet.

Rebaseline test that is now passing one more check.

* web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt:

2021-08-23 Chris Dumez <cdumez@apple.com>

HTMLStyleElement should be able to fire the load event more than once
@@ -1,7 +1,7 @@

Harness Error (TIMEOUT), message = null

TIMEOUT Following a noreferrer link with a named target should not cause creation of a window that can be targeted by another noreferrer link with the same named target Test timed out
PASS Following a noreferrer link with a named target should not cause creation of a window that can be targeted by another noreferrer link with the same named target
PASS Targeting a rel=noreferrer link at an existing named subframe should work
TIMEOUT Targeting a rel=noreferrer link at an existing named window should work Test timed out

This file was deleted.

This file was deleted.

@@ -846,12 +846,6 @@ webkit.org/b/186406 compositing/iframes/display-none-subframe.html [ Pass Failur

webkit.org/b/187766 http/wpt/service-workers/update-service-worker.https.html [ Pass Failure Timeout ]

# These 2 tests have been failing on WK2 only since adding support for BroadcastChannel. The test triggers several navigations with
# target=_blank and it seems only the last one proceeds. I suspect this is an async policy decision issue and previous navigations
# are getting cancelled when triggering a new one.
imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/target_blank_implicit_noopener.html [ Skip ]
imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/target_blank_implicit_noopener_base.html [ Skip ]

# Newly imported test that is flaky on WebKit2 only.
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/no_window_open_when_term_nesting_level_nonzero.window.html [ Pass Failure ]

@@ -1,3 +1,32 @@
2021-08-23 Chris Dumez <cdumez@apple.com>

WebKit2 can only have one active navigation policy check for a given frame
https://bugs.webkit.org/show_bug.cgi?id=229012

Reviewed by Youenn Fablet.

WebKit2 could only have one active navigation policy check for a given frame
and there was a FIXME comment about this in the code. This was causing some
WPT tests to timeout in WebKit2 only because those tests would trigger
several navigations (e.g. in new windows) and only the last one would proceed
(earlier ones would get cancelled).

This patch updates the policy checking logic in WebFrame so that we can support
several concurrent policy checks.

No new tests, unskipped / rebaselined existing tests.

* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::cancelPolicyCheck):
* WebProcess/WebPage/WebFrame.cpp:
(WebKit::WebFrame::~WebFrame):
(WebKit::WebFrame::setUpPolicyListener):
(WebKit::WebFrame::setUpWillSubmitFormListener):
(WebKit::WebFrame::continueWillSubmitForm):
(WebKit::WebFrame::invalidatePolicyListeners):
(WebKit::WebFrame::didReceivePolicyDecision):
* WebProcess/WebPage/WebFrame.h:

2021-08-22 Kate Cheney <katherine_cheney@apple.com>

Report correct blocked URI in CSP violation report
@@ -1036,7 +1036,7 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const Navigat

void WebFrameLoaderClient::cancelPolicyCheck()
{
m_frame->invalidatePolicyListener();
m_frame->invalidatePolicyListeners();
}

void WebFrameLoaderClient::dispatchUnableToImplementPolicy(const ResourceError& error)
@@ -150,7 +150,7 @@ WebFrame::~WebFrame()
{
ASSERT(!m_coreFrame);

auto willSubmitFormCompletionHandlers = WTFMove(m_willSubmitFormCompletionHandlers);
auto willSubmitFormCompletionHandlers = std::exchange(m_willSubmitFormCompletionHandlers, { });
for (auto& completionHandler : willSubmitFormCompletionHandlers.values())
completionHandler();

@@ -208,21 +208,19 @@ void WebFrame::invalidate()

uint64_t WebFrame::setUpPolicyListener(WebCore::PolicyCheckIdentifier identifier, WebCore::FramePolicyFunction&& policyFunction, ForNavigationAction forNavigationAction)
{
// FIXME: <rdar://5634381> We need to support multiple active policy listeners.

invalidatePolicyListener();
auto policyListenerID = generateListenerID();
m_pendingPolicyChecks.add(policyListenerID, PolicyCheck {
identifier,
forNavigationAction,
WTFMove(policyFunction)
});

m_policyIdentifier = identifier;
m_policyListenerID = generateListenerID();
m_policyFunction = WTFMove(policyFunction);
m_policyFunctionForNavigationAction = forNavigationAction;
return m_policyListenerID;
return policyListenerID;
}

uint64_t WebFrame::setUpWillSubmitFormListener(CompletionHandler<void()>&& completionHandler)
{
uint64_t identifier = generateListenerID();
invalidatePolicyListener();
m_willSubmitFormCompletionHandlers.set(identifier, WTFMove(completionHandler));
return identifier;
}
@@ -232,21 +230,15 @@ void WebFrame::continueWillSubmitForm(uint64_t listenerID)
Ref<WebFrame> protectedThis(*this);
if (auto completionHandler = m_willSubmitFormCompletionHandlers.take(listenerID))
completionHandler();
invalidatePolicyListener();
}

void WebFrame::invalidatePolicyListener()
void WebFrame::invalidatePolicyListeners()
{
if (!m_policyListenerID)
return;

m_policyDownloadID = { };
m_policyListenerID = 0;
auto identifier = m_policyIdentifier;
m_policyIdentifier = std::nullopt;
if (auto function = std::exchange(m_policyFunction, nullptr))
function(PolicyAction::Ignore, *identifier);
m_policyFunctionForNavigationAction = ForNavigationAction::No;

auto pendingPolicyChecks = std::exchange(m_pendingPolicyChecks, { });
for (auto& policyCheck : pendingPolicyChecks.values())
policyCheck.policyFunction(PolicyAction::Ignore, policyCheck.corePolicyIdentifier);

auto willSubmitFormCompletionHandlers = WTFMove(m_willSubmitFormCompletionHandlers);
for (auto& completionHandler : willSubmitFormCompletionHandlers.values())
@@ -255,16 +247,17 @@ void WebFrame::invalidatePolicyListener()

void WebFrame::didReceivePolicyDecision(uint64_t listenerID, PolicyDecision&& policyDecision)
{
if (!m_coreFrame || !m_policyListenerID || listenerID != m_policyListenerID || !m_policyFunction)
if (!m_coreFrame)
return;

ASSERT(policyDecision.identifier == m_policyIdentifier);
m_policyIdentifier = std::nullopt;
auto policyCheck = m_pendingPolicyChecks.take(listenerID);
if (!policyCheck.policyFunction)
return;

FramePolicyFunction function = WTFMove(m_policyFunction);
bool forNavigationAction = m_policyFunctionForNavigationAction == ForNavigationAction::Yes;
ASSERT(policyDecision.identifier == policyCheck.corePolicyIdentifier);

invalidatePolicyListener();
FramePolicyFunction function = WTFMove(policyCheck.policyFunction);
bool forNavigationAction = policyCheck.forNavigationAction == ForNavigationAction::Yes;

if (forNavigationAction && frameLoaderClient() && policyDecision.websitePoliciesData) {
ASSERT(page());
@@ -86,7 +86,7 @@ class WebFrame : public API::ObjectImpl<API::Object::Type::BundleFrame>, public

enum class ForNavigationAction { No, Yes };
uint64_t setUpPolicyListener(WebCore::PolicyCheckIdentifier, WebCore::FramePolicyFunction&&, ForNavigationAction);
void invalidatePolicyListener();
void invalidatePolicyListeners();
void didReceivePolicyDecision(uint64_t listenerID, PolicyDecision&&);

uint64_t setUpWillSubmitFormListener(CompletionHandler<void()>&&);
@@ -200,10 +200,13 @@ class WebFrame : public API::ObjectImpl<API::Object::Type::BundleFrame>, public

WeakPtr<WebCore::Frame> m_coreFrame;

uint64_t m_policyListenerID { 0 };
std::optional<WebCore::PolicyCheckIdentifier> m_policyIdentifier;
WebCore::FramePolicyFunction m_policyFunction;
ForNavigationAction m_policyFunctionForNavigationAction { ForNavigationAction::No };
struct PolicyCheck {
WebCore::PolicyCheckIdentifier corePolicyIdentifier;
ForNavigationAction forNavigationAction { ForNavigationAction::No };
WebCore::FramePolicyFunction policyFunction;
};
HashMap<uint64_t, PolicyCheck> m_pendingPolicyChecks;

HashMap<uint64_t, CompletionHandler<void()>> m_willSubmitFormCompletionHandlers;
std::optional<DownloadID> m_policyDownloadID;

0 comments on commit ff71e5a

Please sign in to comment.