Bug 1402025 - Ensure form submission flush when unsetting relevant attributes draft
authorKirk Steuber <ksteuber@mozilla.com>
Thu, 28 Sep 2017 12:09:56 -0700
changeset 672723 9d05f978ea0fb08fc3330235cb9a0f2940fd141f
parent 671558 69e3f89816455e567f1a20b694fd6afd549c82c7
child 733895 312a00be88ffdbf66bee04f2031edb4b6a8a7add
push id82346
push userksteuber@mozilla.com
push dateFri, 29 Sep 2017 17:09:44 +0000
bugs1402025
milestone58.0a1
Bug 1402025 - Ensure form submission flush when unsetting relevant attributes It seems that we were flushing any pending submission when changing the action or target attributes of a form, but not when unsetting those attributes. MozReview-Commit-ID: E6aUnokg54k
dom/html/HTMLFormElement.cpp
dom/html/test/browser.ini
dom/html/test/browser_submission_flush.js
dom/html/test/post_action_page.html
dom/html/test/submission_flush.html
--- a/dom/html/HTMLFormElement.cpp
+++ b/dom/html/HTMLFormElement.cpp
@@ -184,31 +184,28 @@ HTMLFormElement::GetElements(nsIDOMHTMLC
 }
 
 nsresult
 HTMLFormElement::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
                                const nsAttrValueOrString* aValue, bool aNotify)
 {
   if (aNamespaceID == kNameSpaceID_None) {
     if (aName == nsGkAtoms::action || aName == nsGkAtoms::target) {
-      // This check is mostly to preserve previous behavior.
-      if (aValue) {
-        if (mPendingSubmission) {
-          // aha, there is a pending submission that means we're in
-          // the script and we need to flush it. let's tell it
-          // that the event was ignored to force the flush.
-          // the second argument is not playing a role at all.
-          FlushPendingSubmission();
-        }
-        // Don't forget we've notified the password manager already if the
-        // page sets the action/target in the during submit. (bug 343182)
-        bool notifiedObservers = mNotifiedObservers;
-        ForgetCurrentSubmission();
-        mNotifiedObservers = notifiedObservers;
+      if (mPendingSubmission) {
+        // aha, there is a pending submission that means we're in
+        // the script and we need to flush it. let's tell it
+        // that the event was ignored to force the flush.
+        // the second argument is not playing a role at all.
+        FlushPendingSubmission();
       }
+      // Don't forget we've notified the password manager already if the
+      // page sets the action/target in the during submit. (bug 343182)
+      bool notifiedObservers = mNotifiedObservers;
+      ForgetCurrentSubmission();
+      mNotifiedObservers = notifiedObservers;
     }
   }
 
   return nsGenericHTMLElement::BeforeSetAttr(aNamespaceID, aName, aValue,
                                              aNotify);
 }
 
 nsresult
--- a/dom/html/test/browser.ini
+++ b/dom/html/test/browser.ini
@@ -1,16 +1,18 @@
 [DEFAULT]
 support-files =
   bug592641_img.jpg
   dummy_page.html
   file_bug649778.html
   file_bug649778.html^headers^
   file_fullscreen-api-keys.html
   file_content_contextmenu.html
+  submission_flush.html
+  post_action_page.html
   form_data_file.bin
   form_data_file.txt
   form_submit_server.sjs
   head.js
 
 [browser_bug592641.js]
 [browser_bug649778.js]
 skip-if = e10s # Bug 1271025
@@ -22,8 +24,9 @@ support-files =
   file_bug1108547-3.html
 [browser_content_contextmenu_userinput.js]
 [browser_DOMDocElementInserted.js]
 [browser_form_post_from_file_to_http.js]
 [browser_fullscreen-api-keys.js]
 tags = fullscreen
 [browser_fullscreen-contextmenu-esc.js]
 tags = fullscreen
+[browser_submission_flush.js]
new file mode 100644
--- /dev/null
+++ b/dom/html/test/browser_submission_flush.js
@@ -0,0 +1,85 @@
+"use strict";
+// Form submissions triggered by the Javascript 'submit' event listener are
+// deferred until the event listener finishes. However, changes to specific
+// attributes ("action" and "target" attributes) need to cause an immediate
+// flush of any pending submission to prevent the form submission from using the
+// wrong action or target. This test ensures that such flushes happen properly.
+
+const kTestPage = "http://example.org/browser/dom/html/test/submission_flush.html";
+// This is the page pointed to by the form action in the test HTML page.
+const kPostActionPage = "http://example.org/browser/dom/html/test/post_action_page.html";
+
+const kFormId = "test_form"
+const kFrameId = "test_frame"
+const kSubmitButtonId = "submit_button"
+
+// Take in a variety of actions (in the form of setting and unsetting form
+// attributes). Then submit the form in the submit event listener to cause a
+// deferred form submission. Then perform the test actions and ensure that the
+// form used the correct attribute values rather than the changed ones.
+async function runTest(aTestActions) {
+  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, kTestPage);
+  registerCleanupFunction(() => BrowserTestUtils.removeTab(tab));
+
+  let frame_url = await ContentTask.spawn(gBrowser.selectedBrowser,
+                          {kFormId, kFrameId, kSubmitButtonId, aTestActions},
+                          async function({kFormId, kFrameId, kSubmitButtonId,
+                                          aTestActions}) {
+    let form = content.document.getElementById(kFormId);
+
+    form.addEventListener("submit", (event) => {
+      // Need to trigger the deferred submission by submitting in the submit
+      // event handler. To prevent the form from being submitted twice, the
+      // <form> tag contains the attribute |onsubmit="return false;"| to cancel
+      // the original submission.
+      form.submit();
+
+      if (aTestActions.setattr) {
+        for (let {attr, value} of aTestActions.setattr) {
+          form.setAttribute(attr, value);
+        }
+      }
+      if (aTestActions.unsetattr) {
+        for (let attr of aTestActions.unsetattr) {
+          form.removeAttribute(attr);
+        }
+      }
+    }, {capture: true, once: true});
+
+    // Trigger the above event listener
+    content.document.getElementById(kSubmitButtonId).click();
+
+    // Test that the form was submitted to the correct target (the frame) with
+    // the correct action (kPostActionPage).
+    let frame = content.document.getElementById(kFrameId);
+    await new Promise(resolve => {
+      frame.addEventListener("load", resolve, {once: true});
+    });
+    return frame.contentWindow.location.href;
+  });
+  is(frame_url, kPostActionPage,
+     "Form should have submitted with correct target and action");
+}
+
+add_task(async function() {
+  info("Changing action should flush pending submissions");
+  await runTest({setattr: [{attr: "action", value: "about:blank"}]});
+});
+
+add_task(async function() {
+  info("Changing target should flush pending submissions");
+  await runTest({setattr: [{attr: "target", value: "_blank"}]});
+});
+
+add_task(async function() {
+  info("Unsetting action should flush pending submissions");
+  await runTest({unsetattr: ["action"]});
+});
+
+// On failure, this test will time out rather than failing an assert. When the
+// target attribute is not set, the form will submit the active page, navigating
+// it away and preventing the wait for iframe load from ever finishing.
+add_task(async function() {
+  info("Unsetting target should flush pending submissions");
+  await runTest({unsetattr: ["target"]});
+});
new file mode 100644
--- /dev/null
+++ b/dom/html/test/post_action_page.html
@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<html lang="en">
+  <head>
+    <meta charset="utf-8"/>
+    <title>Submission Flush Test Post Action Page</title>
+  </head>
+  <body>
+    <h1>Post Action Page</h1>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/dom/html/test/submission_flush.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html lang="en">
+  <head>
+    <meta charset="utf-8"/>
+    <title>Submission Flush Test</title>
+  </head>
+  <body>
+    <form id="test_form" action="post_action_page.html" target="form_target" method="POST" onsubmit="return false;">
+      <button type="submit" id="submit_button">Submit</button>
+    </form>
+    <iframe name="form_target" id="test_frame"></iframe>
+  </body>
+</html>