- Revision
- 290841
- Author
- [email protected]
- Date
- 2022-03-04 12:35:01 -0800 (Fri, 04 Mar 2022)
Log Message
Load event never firing after form is submitted
https://bugs.webkit.org/show_bug.cgi?id=235407
<rdar://problem/87831049>
Reviewed by Geoffrey Garen.
LayoutTests/imported/w3c:
Rebaseline WPT tests that are no longer timing out.
* web-platform-tests/html/semantics/forms/form-submission-target/rel-base-target-expected.txt:
* web-platform-tests/html/semantics/forms/form-submission-target/rel-button-target-expected.txt:
* web-platform-tests/html/semantics/forms/form-submission-target/rel-form-target-expected.txt:
* web-platform-tests/html/semantics/forms/form-submission-target/rel-input-target-expected.txt:
Source/WebCore:
In Document::implicitClose(), we early return (and thus don't fire the load
event) if there is a location change pending. To determine if there is a
location change pending, we rely on NavigationScheduler::locationChangePending()
which checks if there is a schedule navigation or not. This usually works fine.
However, when a form gets submitted with a target that is "_blank",
FrameLoader::submitForm() is not able to find the target frame (since we'll need
to create one) and it ends up using the current frame's scheduler. The idea is
that once the navigation actually triggers, FrameLoader::loadFrameRequest() will
check the target and create the new Frame.
The issue is that as a result of this, NavigationScheduler::locationChangePending()
returns true for the submitter's frame while such form submission is scheduled,
even though the navigation will actually happen in another (new) frame. To address
the issue, I updated NavigationScheduler::locationChangePending() to check that
the pending navigation is actually for the current frame.
Test: http/tests/loading/form-submission-no-load-event.html
* loader/NavigationScheduler.cpp:
(WebCore::ScheduledNavigation::targetIsCurrentFrame const):
(WebCore::NavigationScheduler::locationChangePending):
(WebCore::ScheduledFormSubmission::ScheduledFormSubmission): Deleted.
LayoutTests:
Add layout test coverage (Based on reduction from Sam Sneddon).
* http/tests/loading/form-submission-no-load-event-expected.txt: Added.
* http/tests/loading/form-submission-no-load-event.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (290840 => 290841)
--- trunk/LayoutTests/ChangeLog 2022-03-04 20:33:12 UTC (rev 290840)
+++ trunk/LayoutTests/ChangeLog 2022-03-04 20:35:01 UTC (rev 290841)
@@ -1,3 +1,16 @@
+2022-03-04 Chris Dumez <[email protected]>
+
+ Load event never firing after form is submitted
+ https://bugs.webkit.org/show_bug.cgi?id=235407
+ <rdar://problem/87831049>
+
+ Reviewed by Geoffrey Garen.
+
+ Add layout test coverage (Based on reduction from Sam Sneddon).
+
+ * http/tests/loading/form-submission-no-load-event-expected.txt: Added.
+ * http/tests/loading/form-submission-no-load-event.html: Added.
+
2022-03-04 Said Abou-Hallawa <[email protected]>
[GPU Process] Canvas compositing buffer should be created through its GraphicsContext
Added: trunk/LayoutTests/http/tests/loading/form-submission-no-load-event-expected.txt (0 => 290841)
--- trunk/LayoutTests/http/tests/loading/form-submission-no-load-event-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/loading/form-submission-no-load-event-expected.txt 2022-03-04 20:35:01 UTC (rev 290841)
@@ -0,0 +1,10 @@
+main frame - didStartProvisionalLoadForFrame
+main frame - didCommitLoadForFrame
+main frame - willPerformClientRedirectToURL: about:blank?input=
+main frame - didFinishDocumentLoadForFrame
+main frame - didHandleOnloadEventsForFrame
+main frame - didFinishLoadForFrame
+Tests that the load event fires if a form submission to a new window happens during the load.
+
+PASS: The load event was fired
+
Added: trunk/LayoutTests/http/tests/loading/form-submission-no-load-event.html (0 => 290841)
--- trunk/LayoutTests/http/tests/loading/form-submission-no-load-event.html (rev 0)
+++ trunk/LayoutTests/http/tests/loading/form-submission-no-load-event.html 2022-03-04 20:35:01 UTC (rev 290841)
@@ -0,0 +1,25 @@
+<p>Tests that the load event fires if a form submission to a new window happens during the load.</p>
+<div id=log></div>
+<form action="" target=_blank><input type=hidden name=input></form>
+<script>
+ if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+ }
+
+ timeoutHandle = setTimeout(() => {
+ document.querySelector("#log").textContent = `FAIL: The load event did not fire`;
+ if (window.testRunner)
+ testRunner.notifyDone();
+ }, 5000);
+
+ const submitter = document.querySelector("form");
+ submitter.submit();
+
+ window._onload_ = () => {
+ document.querySelector("#log").textContent = `PASS: The load event was fired`;
+ clearTimeout(timeoutHandle);
+ if (window.testRunner)
+ testRunner.notifyDone();
+ }
+</script>
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (290840 => 290841)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2022-03-04 20:33:12 UTC (rev 290840)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2022-03-04 20:35:01 UTC (rev 290841)
@@ -1,3 +1,18 @@
+2022-03-04 Chris Dumez <[email protected]>
+
+ Load event never firing after form is submitted
+ https://bugs.webkit.org/show_bug.cgi?id=235407
+ <rdar://problem/87831049>
+
+ Reviewed by Geoffrey Garen.
+
+ Rebaseline WPT tests that are no longer timing out.
+
+ * web-platform-tests/html/semantics/forms/form-submission-target/rel-base-target-expected.txt:
+ * web-platform-tests/html/semantics/forms/form-submission-target/rel-button-target-expected.txt:
+ * web-platform-tests/html/semantics/forms/form-submission-target/rel-form-target-expected.txt:
+ * web-platform-tests/html/semantics/forms/form-submission-target/rel-input-target-expected.txt:
+
2022-03-04 Antoine Quint <[email protected]>
[web-animations] "inherit" values should trigger keyframe recomputation if any previous effect has changed that property
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-base-target-expected.txt (290840 => 290841)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-base-target-expected.txt 2022-03-04 20:33:12 UTC (rev 290840)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-base-target-expected.txt 2022-03-04 20:35:01 UTC (rev 290841)
@@ -1,6 +1,4 @@
-Harness Error (TIMEOUT), message = null
-
PASS <form rel=""> with <base target>
PASS <form rel="noopener"> with <base target>
PASS <form rel="noreferrer"> with <base target>
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-button-target-expected.txt (290840 => 290841)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-button-target-expected.txt 2022-03-04 20:33:12 UTC (rev 290840)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-button-target-expected.txt 2022-03-04 20:35:01 UTC (rev 290841)
@@ -1,6 +1,4 @@
-Harness Error (TIMEOUT), message = null
-
PASS <form rel=""> with <button formtarget>
PASS <form rel="noopener"> with <button formtarget>
PASS <form rel="noreferrer"> with <button formtarget>
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-form-target-expected.txt (290840 => 290841)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-form-target-expected.txt 2022-03-04 20:33:12 UTC (rev 290840)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-form-target-expected.txt 2022-03-04 20:35:01 UTC (rev 290841)
@@ -1,6 +1,4 @@
-Harness Error (TIMEOUT), message = null
-
PASS <form rel=""> with <form target>
PASS <form rel="noopener"> with <form target>
PASS <form rel="noreferrer"> with <form target>
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-input-target-expected.txt (290840 => 290841)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-input-target-expected.txt 2022-03-04 20:33:12 UTC (rev 290840)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-input-target-expected.txt 2022-03-04 20:35:01 UTC (rev 290841)
@@ -1,6 +1,4 @@
-Harness Error (TIMEOUT), message = null
-
PASS <form rel=""> with <input formtarget>
PASS <form rel="noopener"> with <input formtarget>
PASS <form rel="noreferrer"> with <input formtarget>
Modified: trunk/Source/WebCore/ChangeLog (290840 => 290841)
--- trunk/Source/WebCore/ChangeLog 2022-03-04 20:33:12 UTC (rev 290840)
+++ trunk/Source/WebCore/ChangeLog 2022-03-04 20:35:01 UTC (rev 290841)
@@ -1,3 +1,35 @@
+2022-03-04 Chris Dumez <[email protected]>
+
+ Load event never firing after form is submitted
+ https://bugs.webkit.org/show_bug.cgi?id=235407
+ <rdar://problem/87831049>
+
+ Reviewed by Geoffrey Garen.
+
+ In Document::implicitClose(), we early return (and thus don't fire the load
+ event) if there is a location change pending. To determine if there is a
+ location change pending, we rely on NavigationScheduler::locationChangePending()
+ which checks if there is a schedule navigation or not. This usually works fine.
+
+ However, when a form gets submitted with a target that is "_blank",
+ FrameLoader::submitForm() is not able to find the target frame (since we'll need
+ to create one) and it ends up using the current frame's scheduler. The idea is
+ that once the navigation actually triggers, FrameLoader::loadFrameRequest() will
+ check the target and create the new Frame.
+
+ The issue is that as a result of this, NavigationScheduler::locationChangePending()
+ returns true for the submitter's frame while such form submission is scheduled,
+ even though the navigation will actually happen in another (new) frame. To address
+ the issue, I updated NavigationScheduler::locationChangePending() to check that
+ the pending navigation is actually for the current frame.
+
+ Test: http/tests/loading/form-submission-no-load-event.html
+
+ * loader/NavigationScheduler.cpp:
+ (WebCore::ScheduledNavigation::targetIsCurrentFrame const):
+ (WebCore::NavigationScheduler::locationChangePending):
+ (WebCore::ScheduledFormSubmission::ScheduledFormSubmission): Deleted.
+
2022-03-04 Said Abou-Hallawa <[email protected]>
[GPU Process] Canvas compositing buffer should be created through its GraphicsContext
Modified: trunk/Source/WebCore/loader/NavigationScheduler.cpp (290840 => 290841)
--- trunk/Source/WebCore/loader/NavigationScheduler.cpp 2022-03-04 20:33:12 UTC (rev 290840)
+++ trunk/Source/WebCore/loader/NavigationScheduler.cpp 2022-03-04 20:35:01 UTC (rev 290841)
@@ -92,6 +92,7 @@
virtual bool shouldStartTimer(Frame&) { return true; }
virtual void didStartTimer(Frame&, Timer&) { }
virtual void didStopTimer(Frame&, NewLoadInProgress) { }
+ virtual bool targetIsCurrentFrame() const { return true; }
double delay() const { return m_delay; }
LockHistory lockHistory() const { return m_lockHistory; }
@@ -290,7 +291,7 @@
int m_historySteps;
};
-class ScheduledFormSubmission : public ScheduledNavigation {
+class ScheduledFormSubmission final : public ScheduledNavigation {
public:
ScheduledFormSubmission(Ref<FormSubmission>&& submission, LockBackForwardList lockBackForwardList, bool duringLoad)
: ScheduledNavigation(0, submission->lockHistory(), lockBackForwardList, duringLoad, true, submission->state().sourceDocument().shouldOpenExternalURLsPolicyToPropagate())
@@ -298,7 +299,7 @@
{
}
- void fire(Frame& frame) override
+ void fire(Frame& frame) final
{
if (m_submission->wasCancelled())
return;
@@ -323,7 +324,7 @@
frame.loader().loadFrameRequest(WTFMove(frameLoadRequest), m_submission->event(), m_submission->takeState());
}
- void didStartTimer(Frame& frame, Timer& timer) override
+ void didStartTimer(Frame& frame, Timer& timer) final
{
if (m_haveToldClient)
return;
@@ -333,7 +334,7 @@
frame.loader().clientRedirected(m_submission->requestURL(), delay(), WallTime::now() + timer.nextFireInterval(), lockBackForwardList());
}
- void didStopTimer(Frame& frame, NewLoadInProgress newLoadInProgress) override
+ void didStopTimer(Frame& frame, NewLoadInProgress newLoadInProgress) final
{
if (!m_haveToldClient)
return;
@@ -347,6 +348,14 @@
frame.loader().clientRedirectCancelledOrFinished(newLoadInProgress);
}
+ bool targetIsCurrentFrame() const final
+ {
+ // For form submissions, we normally resolve the target frame before scheduling the submission on the
+ // NavigationScheduler. However, if the target is _blank, we schedule the submission on the submitter's
+ // frame and only create the new frame when actually starting the navigation.
+ return !isBlankTargetFrameName(m_submission->target());
+ }
+
private:
Ref<FormSubmission> m_submission;
bool m_haveToldClient { false };
@@ -395,7 +404,7 @@
bool NavigationScheduler::locationChangePending()
{
- return m_redirect && m_redirect->isLocationChange();
+ return m_redirect && m_redirect->isLocationChange() && m_redirect->targetIsCurrentFrame();
}
void NavigationScheduler::clear()
