REGRESSION: (r251677) imported/w3c/web-platform-tests/html/semantics/forms/form-submi...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Dec 2019 00:28:57 +0000 (00:28 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Dec 2019 00:28:57 +0000 (00:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=205164
<rdar://problem/57879042>

Reviewed by Ryosuke Niwa.

LayoutTests/imported/w3c:

Rebaseline tests that are now passing.

* web-platform-tests/html/semantics/forms/form-submission-0/form-double-submit-3-expected.txt:
* web-platform-tests/html/semantics/forms/form-submission-0/form-double-submit-expected.txt:

Source/WebCore:

Submitting a form should cancel any pending navigation scheduled by a previous submission of this form:
- https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-algorithm (step 22.3)

No new tests, rebaselined existing tests.

Test: fast/forms/form-double-submission.html

* html/HTMLFormElement.cpp:
(WebCore::HTMLFormElement::submit):
* html/HTMLFormElement.h:
* loader/FormSubmission.h:
(WebCore::FormSubmission::cancel):
(WebCore::FormSubmission::wasCancelled const):

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::submitForm):
Drop previous non-standard compliant logic to avoid double-form submission.

* loader/NavigationScheduler.cpp:

LayoutTests:

* fast/forms/form-double-submission-expected.txt: Added.
* fast/forms/form-double-submission.html: Added.
* fast/forms/resources/form-double-submission-frame.html: Added.
Add layout test for the regression that was introduced the first time this patch landed.

* http/tests/misc/multiple-submit-expected.txt:
Rebaseline test due to behavior change. I have verified that our new behavior on this test is
aligned with Firefox 71 and Chrome 79.

* platform/mac/TestExpectations:
Unskip tests that are no longer flaky.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@253799 268f45cc-cd09-0410-ab3c-d52691b4dbfc

15 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/forms/form-double-submission-expected.txt [new file with mode: 0644]
LayoutTests/fast/forms/form-double-submission.html [new file with mode: 0644]
LayoutTests/fast/forms/resources/form-double-submission-frame.html [new file with mode: 0644]
LayoutTests/http/tests/misc/multiple-submit-expected.txt
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-0/form-double-submit-3-expected.txt
LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-0/form-double-submit-expected.txt
LayoutTests/platform/mac/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLFormElement.cpp
Source/WebCore/html/HTMLFormElement.h
Source/WebCore/loader/FormSubmission.h
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/NavigationScheduler.cpp

index 0b7819a..fb61caa 100644 (file)
@@ -1,5 +1,25 @@
 2019-12-19  Chris Dumez  <cdumez@apple.com>
 
+        REGRESSION: (r251677) imported/w3c/web-platform-tests/html/semantics/forms/form-submission-0/form-double-submit-3.html is a flaky failure
+        https://bugs.webkit.org/show_bug.cgi?id=205164
+        <rdar://problem/57879042>
+
+        Reviewed by Ryosuke Niwa.
+
+        * fast/forms/form-double-submission-expected.txt: Added.
+        * fast/forms/form-double-submission.html: Added.
+        * fast/forms/resources/form-double-submission-frame.html: Added.
+        Add layout test for the regression that was introduced the first time this patch landed.
+
+        * http/tests/misc/multiple-submit-expected.txt:
+        Rebaseline test due to behavior change. I have verified that our new behavior on this test is
+        aligned with Firefox 71 and Chrome 79.
+
+        * platform/mac/TestExpectations:
+        Unskip tests that are no longer flaky.
+
+2019-12-19  Chris Dumez  <cdumez@apple.com>
+
         Unreviewed, land missing iOS baselines from r253791.
 
         * platform/ios-wk2/imported/w3c/web-platform-tests/html/browsers/sandboxing/sandbox-parse-noscript-expected.txt: Added.
diff --git a/LayoutTests/fast/forms/form-double-submission-expected.txt b/LayoutTests/fast/forms/form-double-submission-expected.txt
new file mode 100644 (file)
index 0000000..f8fc5aa
--- /dev/null
@@ -0,0 +1,10 @@
+Makes sure that the form submission causes a navigation, even if the script attempts to submit the form twice.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Frame was navigated
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/forms/form-double-submission.html b/LayoutTests/fast/forms/form-double-submission.html
new file mode 100644 (file)
index 0000000..bc29521
--- /dev/null
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test.js"></script>
+<script>
+description("Makes sure that the form submission causes a navigation, even if the script attempts to submit the form twice.");
+jsTestIsAsync = true;
+
+onload = () => {
+    frame1.form1.addEventListener('click', () => {
+        frame1.form1.submit();
+    });
+    document.getElementById("frame1").onload = () => {
+        clearInterval(handle);
+        testPassed("Frame was navigated");
+        finishJSTest();
+    }
+    handle = setTimeout(() => {
+        testFailed("Frame was not navigated");
+        finishJSTest();
+    }, 10000);
+    frame1.submitbutton.click();
+}
+</script>
+<iframe name="frame1" id="frame1" src="resources/form-double-submission-frame.html"></iframe>
+</body>
+</html>
diff --git a/LayoutTests/fast/forms/resources/form-double-submission-frame.html b/LayoutTests/fast/forms/resources/form-double-submission-frame.html
new file mode 100644 (file)
index 0000000..794e83d
--- /dev/null
@@ -0,0 +1,9 @@
+<!DOCTYPE html>
+<html>
+<body>
+<form id="form1" action="../../dom/resources/top.html">
+  <input type=hidden name=navigated value=1>
+  <input id=submitbutton type=submit>
+</form>
+</body>
+</html>
index ac30357..2afbe4c 100644 (file)
@@ -1,3 +1,16 @@
+2019-12-19  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION: (r251677) imported/w3c/web-platform-tests/html/semantics/forms/form-submission-0/form-double-submit-3.html is a flaky failure
+        https://bugs.webkit.org/show_bug.cgi?id=205164
+        <rdar://problem/57879042>
+
+        Reviewed by Ryosuke Niwa.
+
+        Rebaseline tests that are now passing.
+
+        * web-platform-tests/html/semantics/forms/form-submission-0/form-double-submit-3-expected.txt:
+        * web-platform-tests/html/semantics/forms/form-submission-0/form-double-submit-expected.txt:
+
 2019-12-19  Jonathan Bedard  <jbedard@apple.com>
 
         Resync web-platform-tests/WebIDL tests from upstream
index 10c21ee..2a3ea66 100644 (file)
@@ -4,5 +4,5 @@ This frame should navigate (to 404)
 
 submit
 
-FAIL <button> should have the same double-submit protection as <input type=submit> assert_unreached: Frame1 should not get navigated by this test. Reached unreachable code
+PASS <button> should have the same double-submit protection as <input type=submit> 
 
index 02317ac..9a2bcb0 100644 (file)
@@ -4,5 +4,5 @@ This frame should navigate (to 404)
 
 
 
-FAIL default submit action should supersede onclick submit() assert_unreached: Frame1 should not get navigated by this test. Reached unreachable code
+PASS default submit action should supersede onclick submit() 
 
index aab5127..bb79b1f 100644 (file)
@@ -1963,7 +1963,4 @@ webkit.org/b/205216 imported/w3c/web-platform-tests/content-security-policy/repo
 
 webkit.org/b/205361 [ Release ] crypto/workers/subtle/aes-indexeddb.html [ Pass Timeout ]
 
-webkit.org/b/205164 imported/w3c/web-platform-tests/html/semantics/forms/form-submission-0/form-double-submit-3.html [ Pass Failure ]
-webkit.org/b/205164 imported/w3c/web-platform-tests/html/semantics/forms/form-submission-0/form-double-submit.html [ Pass Failure ]
-
 webkit.org/b/205403 accessibility/presentation-role-iframe.html [ Pass Failure ]
index 315b6a2..1e87310 100644 (file)
@@ -1,3 +1,31 @@
+2019-12-19  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION: (r251677) imported/w3c/web-platform-tests/html/semantics/forms/form-submission-0/form-double-submit-3.html is a flaky failure
+        https://bugs.webkit.org/show_bug.cgi?id=205164
+        <rdar://problem/57879042>
+
+        Reviewed by Ryosuke Niwa.
+
+        Submitting a form should cancel any pending navigation scheduled by a previous submission of this form:
+        - https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-algorithm (step 22.3)
+
+        No new tests, rebaselined existing tests.
+
+        Test: fast/forms/form-double-submission.html
+
+        * html/HTMLFormElement.cpp:
+        (WebCore::HTMLFormElement::submit):
+        * html/HTMLFormElement.h:
+        * loader/FormSubmission.h:
+        (WebCore::FormSubmission::cancel):
+        (WebCore::FormSubmission::wasCancelled const):
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::submitForm):
+        Drop previous non-standard compliant logic to avoid double-form submission.
+
+        * loader/NavigationScheduler.cpp:
+
 2019-12-19  Ryosuke Niwa  <rniwa@webkit.org>
 
         Make ShadowRoot.delegateFocus work in iOS
index 435d951..15ad57f 100644 (file)
@@ -358,7 +358,11 @@ void HTMLFormElement::submit(Event* event, bool activateSubmitButton, bool proce
     auto protectedThis = makeRef(*this); // Form submission can execute arbitary JavaScript.
 
     auto shouldLockHistory = processingUserGesture ? LockHistory::No : LockHistory::Yes;
-    frame->loader().submitForm(FormSubmission::create(*this, m_attributes, event, shouldLockHistory, formSubmissionTrigger));
+    auto formSubmission = FormSubmission::create(*this, m_attributes, event, shouldLockHistory, formSubmissionTrigger);
+    if (m_plannedFormSubmission)
+        m_plannedFormSubmission->cancel();
+    m_plannedFormSubmission = makeWeakPtr(formSubmission.get());
+    frame->loader().submitForm(WTFMove(formSubmission));
 
     if (needButtonActivation && firstSuccessfulSubmitButton)
         firstSuccessfulSubmitButton->setActivatedSubmit(false);
index ebcdc0a..b2171d7 100644 (file)
@@ -181,6 +181,7 @@ private:
     Vector<FormAssociatedElement*> m_associatedElements;
     Vector<WeakPtr<HTMLImageElement>> m_imageElements;
     WeakHashSet<HTMLFormControlElement> m_invalidAssociatedFormControls;
+    WeakPtr<FormSubmission> m_plannedFormSubmission;
 
     bool m_wasUserSubmitted { false };
     bool m_isSubmittingOrPreparingForSubmission { false };
index f9accf1..3c17662 100644 (file)
@@ -33,6 +33,7 @@
 #include "FormState.h"
 #include "FrameLoaderTypes.h"
 #include <wtf/URL.h>
+#include <wtf/WeakPtr.h>
 
 namespace WebCore {
 
@@ -40,9 +41,9 @@ class Event;
 class FormData;
 class FrameLoadRequest;
 
-class FormSubmission : public RefCounted<FormSubmission> {
+class FormSubmission : public RefCounted<FormSubmission>, public CanMakeWeakPtr<FormSubmission> {
 public:
-    enum class Method { Get, Post };
+    enum class Method : bool { Get, Post };
 
     class Attributes {
     public:
@@ -96,11 +97,15 @@ public:
     void setReferrer(const String& referrer) { m_referrer = referrer; }
     void setOrigin(const String& origin) { m_origin = origin; }
 
+    void cancel() { m_wasCancelled = true; }
+    bool wasCancelled() const { return m_wasCancelled; }
+
 private:
     FormSubmission(Method, const URL& action, const String& target, const String& contentType, Ref<FormState>&&, Ref<FormData>&&, const String& boundary, LockHistory, Event*);
 
     // FIXME: Hold an instance of Attributes instead of individual members.
     Method m_method;
+    bool m_wasCancelled { false };
     URL m_action;
     String m_target;
     String m_contentType;
index 5310d35..d8c6a7f 100644 (file)
@@ -485,23 +485,8 @@ void FrameLoader::submitForm(Ref<FormSubmission>&& submission)
     if (!targetFrame->page())
         return;
 
-    // FIXME: We'd like to remove this altogether and fix the multiple form submission issue another way.
-
-    // We do not want to submit more than one form from the same page, nor do we want to submit a single
-    // form more than once. This flag prevents these from happening; not sure how other browsers prevent this.
-    // The flag is reset in each time we start dispatching a new mouse or key down event, and
-    // also in setView since this part may get reused for a page from the back/forward cache.
-    // The form multi-submit logic here is only needed when we are submitting a form that affects this frame.
-
-    // FIXME: Frame targeting is only one of the ways the submission could end up doing something other
-    // than replacing this frame's content, so this check is flawed. On the other hand, the check is hardly
-    // needed any more now that we reset m_submittedFormURL on each mouse or key down event.
-
-    if (m_frame.tree().isDescendantOf(targetFrame)) {
-        if (m_submittedFormURL == submission->requestURL())
-            return;
+    if (m_frame.tree().isDescendantOf(targetFrame))
         m_submittedFormURL = submission->requestURL();
-    }
 
     submission->setReferrer(outgoingReferrer());
     submission->setOrigin(outgoingOrigin());
index 3c2c252..80ded79 100644 (file)
@@ -277,6 +277,9 @@ public:
 
     void fire(Frame& frame) override
     {
+        if (m_submission->wasCancelled())
+            return;
+
         UserGestureIndicator gestureIndicator(userGestureToForward());
 
         // The submitForm function will find a target frame before using the redirection timer.