Fix a bug that could allow duplicate form submissions

Previous to this CL, and after [1], if a form Submit button had an
onclick handler that also called form.submit() and did not call
event.preventDefault(), the form would get submitted twice. The second
request was eventually cancelled, but not before it hit the network.
This behavior is specified in [2], through the "plan to navigate"
mechanism. In the case of this bug, the "click" event occurs first,
and changes the "planned navigation". Then, because the click handler
does not preventDefault(), the default submit action is executed,
which changes the "planned navigation", replacing the navigation
queued by the onclick handler. Therefore, only the default submit
navigation is performed.

Note that there are other potential interactions which are less
clearly specified, and which are not addressed in this CL.
For example:

  <iframe id="test" name="test"></iframe>
  <form id=form1 target="test" action="click.html"></form>
  <a target="test" onclick="form1.submit()" href="href.html">Test</a>

In this case, clicking the <a> link first submits the form (to
click.html), and then queues a navigation to href.html. Because
the navigation to href.html is specified (in [3]) to "queue a
navigation", independently of the planned navigation specified
in [2], it is unclear when/whether the form submission should
take place. The spec ([4]) does have provisions for canceling
existing navigations, but that leaves room for the form to still
get to the network in this case, before getting canceled.

[1] https://chromium.googlesource.com/chromium/src/+/6931ab86f19aa79abbdd0c1062084e16b5c4f0f6
[2] https://www.w3.org/TR/html52/sec-forms.html#form-submission-algorithm
[3] https://html.spec.whatwg.org/#following-hyperlinks
[4] https://html.spec.whatwg.org/#navigating-across-documents

Bug: 977882
Change-Id: I693f3bdccb17c5e64df75c2e569fab589c02e88c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1850358
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706782}

third_party/blink/renderer/core/dom/events/event_dispatcher.cc

@@ -346,8 +346,9 @@ inline void EventDispatcher::DispatchEventPostProcess(
   // Call default event handlers. While the DOM does have a concept of
   // preventing default handling, the detail of which handlers are called is an
   // internal implementation detail and not part of the DOM.
-  if (!event_->defaultPrevented() && !event_->DefaultHandled() &&
-      is_trusted_or_click) {
+  if (event_->defaultPrevented()) {
+    node_->DidPreventDefault(*event_);
+  } else if (!event_->DefaultHandled() && is_trusted_or_click) {
     // Non-bubbling events call only one default event handler, the one for the
     // target.
     node_->WillCallDefaultEventHandler(*event_);

third_party/blink/renderer/core/dom/node.h

@@ -819,6 +819,10 @@ class CORE_EXPORT Node : public EventTarget {
   }
   virtual void PostDispatchEventHandler(Event&, EventDispatchHandlingState*) {}
 
+  // TODO(crbug.com/1013385): Remove DidPreventDefault. It is here as a
+  //   temporary fix for form double-submit.
+  virtual void DidPreventDefault(const Event&) {}
+
   void DispatchScopedEvent(Event&);
 
   virtual void HandleLocalEvents(Event&);

third_party/blink/renderer/core/html/forms/html_button_element.cc

@@ -27,6 +27,7 @@
 
 #include "third_party/blink/renderer/core/dom/attribute.h"
 #include "third_party/blink/renderer/core/events/keyboard_event.h"
+#include "third_party/blink/renderer/core/events/mouse_event.h"
 #include "third_party/blink/renderer/core/html/forms/form_data.h"
 #include "third_party/blink/renderer/core/html/forms/html_form_element.h"
 #include "third_party/blink/renderer/core/html_names.h"
@@ -109,6 +110,13 @@ void HTMLButtonElement::ParseAttribute(
 }
 
 void HTMLButtonElement::DefaultEventHandler(Event& event) {
+  DefaultEventHandlerInternal(event);
+
+  if (event.type() == event_type_names::kDOMActivate && formOwner())
+    formOwner()->DidActivateSubmitButton(this);
+}
+
+void HTMLButtonElement::DefaultEventHandlerInternal(Event& event) {
   if (event.type() == event_type_names::kDOMActivate &&
       !IsDisabledFormControl()) {
     if (Form() && type_ == SUBMIT) {
@@ -219,4 +227,16 @@ Node::InsertionNotificationRequest HTMLButtonElement::InsertedInto(
   return request;
 }
 
+EventDispatchHandlingState* HTMLButtonElement::PreDispatchEventHandler(
+    Event& event) {
+  if (Form() && CanBeSuccessfulSubmitButton())
+    Form()->WillActivateSubmitButton(this);
+  return nullptr;
+}
+
+void HTMLButtonElement::DidPreventDefault(const Event& event) {
+  if (auto* form = formOwner())
+    form->DidActivateSubmitButton(this);
+}
+
 }  // namespace blink

third_party/blink/renderer/core/html/forms/html_button_element.h

@@ -78,6 +78,13 @@ class HTMLButtonElement final : public HTMLFormControlElement {
   bool IsOptionalFormControl() const override { return true; }
   bool RecalcWillValidate() const override;
 
+  // TODO(crbug.com/1013385): Remove PreDispatchEventHandler, DidPreventDefault,
+  //   and DefaultEventHandlerInternal. They are here to temporarily fix form
+  //   double-submit.
+  EventDispatchHandlingState* PreDispatchEventHandler(Event&) override;
+  void DidPreventDefault(const Event&) final;
+  void DefaultEventHandlerInternal(Event&);
+
   Type type_;
   bool is_activated_submit_;
 };

third_party/blink/renderer/core/html/forms/html_form_element.cc

@@ -94,6 +94,7 @@ void HTMLFormElement::Trace(Visitor* visitor) {
   visitor->Trace(listed_elements_);
   visitor->Trace(image_elements_);
   visitor->Trace(planned_navigation_);
+  visitor->Trace(activated_submit_button_);
   HTMLElement::Trace(visitor);
 }
 
@@ -331,10 +332,27 @@ void HTMLFormElement::PrepareForSubmission(
     planned_navigation_ = nullptr;
     Submit(event, submit_button);
   }
+  if (!planned_navigation_ || activated_submit_button_)
+    return;
+  base::AutoReset<bool> submit_scope(&is_submitting_, true);
+  SubmitForm(planned_navigation_);
+  planned_navigation_ = nullptr;
+}
+
+void HTMLFormElement::WillActivateSubmitButton(
+    HTMLFormControlElement* element) {
+  if (!activated_submit_button_)
+    activated_submit_button_ = element;
+}
+
+void HTMLFormElement::DidActivateSubmitButton(HTMLFormControlElement* element) {
+  if (activated_submit_button_ != element)
+    return;
+  activated_submit_button_ = nullptr;
   if (!planned_navigation_)
     return;
   base::AutoReset<bool> submit_scope(&is_submitting_, true);
-  ScheduleFormSubmission(planned_navigation_);
+  SubmitForm(planned_navigation_);
   planned_navigation_ = nullptr;
 }
 
@@ -449,14 +467,14 @@ void HTMLFormElement::Submit(Event* event,
   }
   if (form_submission->Method() == FormSubmission::kDialogMethod) {
     SubmitDialog(form_submission);
-  } else if (in_user_js_submit_event_) {
+  } else if (in_user_js_submit_event_ || activated_submit_button_) {
     // Need to postpone the submission in order to make this cancelable by
     // another submission request.
     planned_navigation_ = form_submission;
   } else {
     // This runs JavaScript code if action attribute value is javascript:
     // protocol.
-    ScheduleFormSubmission(form_submission);
+    SubmitForm(form_submission);
   }
 }
 
@@ -489,13 +507,16 @@ FormData* HTMLFormElement::ConstructEntryList(
   return &form_data;
 }
 
-void HTMLFormElement::ScheduleFormSubmission(FormSubmission* submission) {
+// Actually submit the form - navigate now.
+void HTMLFormElement::SubmitForm(FormSubmission* submission) {
   DCHECK(submission->Method() == FormSubmission::kPostMethod ||
          submission->Method() == FormSubmission::kGetMethod);
   DCHECK(submission->Data());
   DCHECK(submission->Form());
   if (submission->Action().IsEmpty())
     return;
+  if (!GetDocument().IsActive())
+    return;
   if (GetDocument().IsSandboxed(WebSandboxFlags::kForms)) {
     // FIXME: This message should be moved off the console once a solution to
     // https://bugs.webkit.org/show_bug.cgi?id=103274 exists.
@@ -598,6 +619,11 @@ void HTMLFormElement::Disassociate(ListedElement& e) {
   listed_elements_are_dirty_ = true;
   listed_elements_.clear();
   RemoveFromPastNamesMap(e.ToHTMLElement());
+
+  if (activated_submit_button_ != &e)
+    return;
+  activated_submit_button_ = nullptr;
+  planned_navigation_ = nullptr;
 }
 
 bool HTMLFormElement::IsURLAttribute(const Attribute& attribute) const {

third_party/blink/renderer/core/html/forms/html_form_element.h

@@ -118,6 +118,12 @@ class CORE_EXPORT HTMLFormElement final : public HTMLElement {
 
   unsigned UniqueRendererFormId() const { return unique_renderer_form_id_; }
 
+  // TODO(crbug.com/1013385): Remove WillActivateSubmitButton,
+  //   DidActivateSubmitButton, and RemovedAssociatedControlElement. They are
+  //   here temporarily to fix form double-submit.
+  void WillActivateSubmitButton(HTMLFormControlElement* element);
+  void DidActivateSubmitButton(HTMLFormControlElement* element);
+
  private:
   InsertionNotificationRequest InsertedInto(ContainerNode&) override;
   void RemovedFrom(ContainerNode&) override;
@@ -136,7 +142,7 @@ class CORE_EXPORT HTMLFormElement final : public HTMLElement {
   void SubmitDialog(FormSubmission*);
   void Submit(Event*, HTMLFormControlElement* submit_button);
 
-  void ScheduleFormSubmission(FormSubmission*);
+  void SubmitForm(FormSubmission*);
 
   void CollectListedElements(Node& root, ListedElement::List&) const;
   void CollectImageElements(Node& root, HeapVector<Member<HTMLImageElement>>&);
@@ -182,6 +188,8 @@ class CORE_EXPORT HTMLFormElement final : public HTMLElement {
   bool has_elements_associated_by_form_attribute_ : 1;
   bool did_finish_parsing_children_ : 1;
   bool is_in_reset_function_ : 1;
+
+  Member<HTMLFormControlElement> activated_submit_button_;
 };
 
 }  // namespace blink

third_party/blink/renderer/core/html/forms/html_input_element.cc

@@ -1282,6 +1282,8 @@ EventDispatchHandlingState* HTMLInputElement::PreDispatchEventHandler(
       ToMouseEvent(event).button() !=
           static_cast<int16_t>(WebPointerProperties::Button::kLeft))
     return nullptr;
+  if (formOwner() && CanBeSuccessfulSubmitButton())
+    formOwner()->WillActivateSubmitButton(this);
   return input_type_view_->WillDispatchClick();
 }
 
@@ -1294,7 +1296,19 @@ void HTMLInputElement::PostDispatchEventHandler(
                                      *static_cast<ClickHandlingState*>(state));
 }
 
-void HTMLInputElement::DefaultEventHandler(Event& evt) {
+void HTMLInputElement::DidPreventDefault(const Event& event) {
+  if (auto* form = formOwner())
+    form->DidActivateSubmitButton(this);
+}
+
+void HTMLInputElement::DefaultEventHandler(Event& event) {
+  DefaultEventHandlerInternal(event);
+
+  if (event.type() == event_type_names::kDOMActivate && formOwner())
+    formOwner()->DidActivateSubmitButton(this);
+}
+
+void HTMLInputElement::DefaultEventHandlerInternal(Event& evt) {
   if (evt.IsMouseEvent() && evt.type() == event_type_names::kClick &&
       ToMouseEvent(evt).button() ==
           static_cast<int16_t>(WebPointerProperties::Button::kLeft)) {

third_party/blink/renderer/core/html/forms/html_input_element.h

@@ -372,6 +372,11 @@ class CORE_EXPORT HTMLInputElement
 
   EventDispatchHandlingState* PreDispatchEventHandler(Event&) final;
   void PostDispatchEventHandler(Event&, EventDispatchHandlingState*) final;
+  // TODO(crbug.com/1013385): Remove DidPreventDefault and
+  //   DefaultEventHandlerInternal. They are here as a temporary fix for form
+  //   double-submit.
+  void DidPreventDefault(const Event&) final;
+  void DefaultEventHandlerInternal(Event& evt);
 
   bool IsURLAttribute(const Attribute&) const final;
   bool HasLegalLinkAttribute(const QualifiedName&) const final;

third_party/blink/web_tests/external/wpt/html/semantics/forms/form-submission-0/form-double-submit-2.html

+<!DOCTYPE html>
+<meta charset="utf-8">
+<link rel="author" title="Mason Freed" href="mailto:masonfreed@chromium.org">
+<link rel="help" href="https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-algorithm">
+
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<!-- The onclick submit() should *not* get superseded in this case by the
+     default action submit(), because onclick here calls preventDefault().
+  -->
+
+
+
+
+<label for=frame1 style="display:block">This frame should stay blank</label>
+<iframe name=frame1 id=frame1></iframe>
+<label for=frame2 style="display:block">This frame should navigate (to 404)</label>
+<iframe name=frame2 id=frame2></iframe>
+<form id="form1" target="frame2" action="nonexistent.html">
+  <input type=hidden name=navigated value=1>
+  <input id=submitbutton type=submit>
+</form>
+
+<script>
+let frame1 = document.getElementById('frame1');
+let frame2 = document.getElementById('frame2');
+
+async_test(t => {
+  window.addEventListener('load', () => {
+    frame1.addEventListener('load', t.step_func_done(() => {
+      assert_unreached("Frame1 should not get navigated by this test.");
+    }));
+    frame2.addEventListener('load', t.step_func_done(() => {
+      let params = (new URL(frame2.contentWindow.location)).searchParams;
+      let wasNavigated = !!params.get("navigated");
+      assert_true(wasNavigated);
+    }));
+    form1.addEventListener('click', t.step_func(() => {
+      form1.submit();
+      form1.target='frame1';
+      event.preventDefault();  // Prevent default here
+    }));
+    submitbutton.click();
+  });
+}, 'preventDefault should allow onclick submit() to succeed');
+</script>

third_party/blink/web_tests/external/wpt/html/semantics/forms/form-submission-0/form-double-submit-3.html

+<!DOCTYPE html>
+<meta charset="utf-8">
+<link rel="author" title="Mason Freed" href="mailto:masonfreed@chromium.org">
+<link rel="help" href="https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-algorithm">
+
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<!-- <button> should have the same double-submit protection that
+     <input type=submit> has.
+  -->
+
+
+
+
+<label for=frame1 style="display:block">This frame should stay blank</label>
+<iframe name=frame1 id=frame1></iframe>
+<label for=frame2 style="display:block">This frame should navigate (to 404)</label>
+<iframe name=frame2 id=frame2></iframe>
+<form id="form1" target="frame1" action="nonexistent.html">
+  <input type=hidden name=navigated value=1>
+  <button id=submitbutton>submit</button>
+</form>
+
+<script>
+let frame1 = document.getElementById('frame1');
+let frame2 = document.getElementById('frame2');
+
+async_test(t => {
+  window.addEventListener('load', () => {
+    frame1.addEventListener('load', t.step_func_done(() => {
+      assert_unreached("Frame1 should not get navigated by this test.");
+    }));
+    frame2.addEventListener('load', t.step_func_done(() => {
+      let params = (new URL(frame2.contentWindow.location)).searchParams;
+      let wasNavigated = !!params.get("navigated");
+      assert_true(wasNavigated)
+    }));
+    form1.addEventListener('click', t.step_func(() => {
+      form1.submit();
+      form1.target='frame2';
+
+    }));
+    submitbutton.click();
+  });
+}, '<button> should have the same double-submit protection as <input type=submit>');
+
+</script>

third_party/blink/web_tests/external/wpt/html/semantics/forms/form-submission-0/form-double-submit.html

+<!DOCTYPE html>
+<meta charset="utf-8">
+<link rel="author" title="Mason Freed" href="mailto:masonfreed@chromium.org">
+<link rel="help" href="https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-algorithm">
+
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<!-- The onclick submit() should get superseded by the default
+     action submit(), which isn't preventDefaulted by onclick here.
+     This is per the Form Submission Algorithm [1], step 22.3, which
+     says that new planned navigations replace old planned navigations.
+    [1] https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-algorithm
+  -->
+
+<label for=frame1 style="display:block">This frame should stay blank</label>
+<iframe name=frame1 id=frame1></iframe>
+<label for=frame2 style="display:block">This frame should navigate (to 404)</label>
+<iframe name=frame2 id=frame2></iframe>
+<form id="form1" target="frame1" action="nonexistent.html">
+  <input type=hidden name=navigated value=1>
+  <input id=submitbutton type=submit>
+</form>
+
+<script>
+let frame1 = document.getElementById('frame1');
+let frame2 = document.getElementById('frame2');
+
+async_test(t => {
+  window.addEventListener('load', () => {
+    frame1.addEventListener('load', t.step_func_done(() => {
+      assert_unreached("Frame1 should not get navigated by this test.");
+    }));
+    frame2.addEventListener('load', t.step_func_done(() => {
+      let params = (new URL(frame2.contentWindow.location)).searchParams;
+      let wasNavigated = !!params.get("navigated");
+      assert_true(wasNavigated)
+    }));
+    form1.addEventListener('click', t.step_func(() => {
+      form1.submit();
+      form1.target='frame2';
+
+    }));
+    submitbutton.click();
+  });
+}, 'default submit action should supersede onclick submit()');
+
+</script>