- Revision
- 281445
- Author
- [email protected]
- Date
- 2021-08-23 08:02:30 -0700 (Mon, 23 Aug 2021)
Log Message
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:
Modified Paths
Removed Paths
- trunk/LayoutTests/platform/ios-wk1/imported/w3c/web-platform-tests/html/browsers/windows/
- trunk/LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/html/browsers/windows/
Diff
Modified: trunk/LayoutTests/ChangeLog (281444 => 281445)
--- trunk/LayoutTests/ChangeLog 2021-08-23 14:53:04 UTC (rev 281444)
+++ trunk/LayoutTests/ChangeLog 2021-08-23 15:02:30 UTC (rev 281445)
@@ -1,3 +1,16 @@
+2021-08-23 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
Create a RenderLineBreak when BR element has unsupported content data style
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (281444 => 281445)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2021-08-23 14:53:04 UTC (rev 281444)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2021-08-23 15:02:30 UTC (rev 281445)
@@ -1,5 +1,16 @@
2021-08-23 Chris Dumez <[email protected]>
+ 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 <[email protected]>
+
HTMLStyleElement should be able to fire the load event more than once
https://bugs.webkit.org/show_bug.cgi?id=228975
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt (281444 => 281445)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt 2021-08-23 14:53:04 UTC (rev 281444)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt 2021-08-23 15:02:30 UTC (rev 281445)
@@ -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
Modified: trunk/LayoutTests/platform/wk2/TestExpectations (281444 => 281445)
--- trunk/LayoutTests/platform/wk2/TestExpectations 2021-08-23 14:53:04 UTC (rev 281444)
+++ trunk/LayoutTests/platform/wk2/TestExpectations 2021-08-23 15:02:30 UTC (rev 281445)
@@ -846,12 +846,6 @@
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 ]
Modified: trunk/Source/WebKit/ChangeLog (281444 => 281445)
--- trunk/Source/WebKit/ChangeLog 2021-08-23 14:53:04 UTC (rev 281444)
+++ trunk/Source/WebKit/ChangeLog 2021-08-23 15:02:30 UTC (rev 281445)
@@ -1,3 +1,32 @@
+2021-08-23 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
Report correct blocked URI in CSP violation report
Modified: trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp (281444 => 281445)
--- trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp 2021-08-23 14:53:04 UTC (rev 281444)
+++ trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp 2021-08-23 15:02:30 UTC (rev 281445)
@@ -1036,7 +1036,7 @@
void WebFrameLoaderClient::cancelPolicyCheck()
{
- m_frame->invalidatePolicyListener();
+ m_frame->invalidatePolicyListeners();
}
void WebFrameLoaderClient::dispatchUnableToImplementPolicy(const ResourceError& error)
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebFrame.cpp (281444 => 281445)
--- trunk/Source/WebKit/WebProcess/WebPage/WebFrame.cpp 2021-08-23 14:53:04 UTC (rev 281444)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebFrame.cpp 2021-08-23 15:02:30 UTC (rev 281445)
@@ -150,7 +150,7 @@
{
ASSERT(!m_coreFrame);
- auto willSubmitFormCompletionHandlers = WTFMove(m_willSubmitFormCompletionHandlers);
+ auto willSubmitFormCompletionHandlers = std::exchange(m_willSubmitFormCompletionHandlers, { });
for (auto& completionHandler : willSubmitFormCompletionHandlers.values())
completionHandler();
@@ -208,21 +208,19 @@
uint64_t WebFrame::setUpPolicyListener(WebCore::PolicyCheckIdentifier identifier, WebCore::FramePolicyFunction&& policyFunction, ForNavigationAction forNavigationAction)
{
- // FIXME: <rdar://5634381> We need to support multiple active policy listeners.
+ auto policyListenerID = generateListenerID();
+ m_pendingPolicyChecks.add(policyListenerID, PolicyCheck {
+ identifier,
+ forNavigationAction,
+ WTFMove(policyFunction)
+ });
- invalidatePolicyListener();
-
- 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,22 +230,16 @@
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())
completionHandler();
@@ -255,16 +247,17 @@
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());
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebFrame.h (281444 => 281445)
--- trunk/Source/WebKit/WebProcess/WebPage/WebFrame.h 2021-08-23 14:53:04 UTC (rev 281444)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebFrame.h 2021-08-23 15:02:30 UTC (rev 281445)
@@ -86,7 +86,7 @@
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 @@
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;
