1402025 - Removing form target after calling HTMLFormElement.submit() changes target of pending submission

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Michael Skree]

Attachment: HTML and PHP test files

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.91 Safari/537.36

Steps to reproduce:

1. Open the attached index.html file in an environment where PHP is enabled (ex. php -S localhost:1234)
2. Click "Submit"

We're using an old vendor library to perform asynchronous file uploads and it doesn't seem to work anymore in Firefox 55. The library works by dynamically creating an iframe, setting the form's target to the new iframe (in addition to modifying a few other attributes), calling HTMLFormElement.submit(), and then restoring the form's attributes to their original state. The attached index.html file contains an approximation of the relevant vendor code. In newer versions of Firefox (> 55), the submission response ends up being displayed in a new page instead of being sent to the iframe.

I'm not entirely sure what the "correct" behavior is in this case, but Firefox behaves differently than the other browsers I tested (Chrome 61, IE 11, Edge). I used the mozregression tool to track down the following regression range:

15:20.62 INFO: Narrowed inbound regression window from [e3608824, dcdff550] (3 builds) to [e3608824, f5da859b] (2 builds) (~1 steps left)
15:20.62 INFO: No more inbound revisions, bisection finished.
15:20.62 INFO: Last good revision: e36088242facca249d718e6d79a6cfa7007fd42b
15:20.62 INFO: First bad revision: f5da859b433fe40803847ad77cc4a573fbf8dc25
15:20.62 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e36088242facca249d718e6d79a6cfa7007fd42b&tochange=f5da859b433fe40803847ad77cc4a573fbf8dc25


Actual results:

Firefox navigates away from the page and displays the JSON response.


Expected results:

Firefox stays on the current page and displays the response in an iframe.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Kirk, bug 1365092 was got by the mozregression tool in comment 0. Could you please take a look and confirm the expected result? Is this a regression or a change by design? thanks.

Comment 2

[Robin Steuber (they/them) [:bytesized]]

That patch was not intended to have this effect. I'll look into it.

Comment 3

[Robin Steuber (they/them) [:bytesized]]

I have confirmed on Nightly that this issue occurs as described. I have also confirmed that current versions of Chrome and Edge exhibit the expected behavior stated in the bug description. I am now looking for the cause.

Comment 4

[Robin Steuber (they/them) [:bytesized]]

Attachment: Python testcase.zip

After testing Nightly with the PHP-based test file provided, I rewrote the PHP part of the test case using Python3 because I do not have PHP installed in my primary work environment.

The HTML component of my testcase is identical to the one provided with the PHP-based testcase. Since the problem is really an issue with the HTML component, this testcase should exhibit the problem in an identical fashion to the original testcase.

Comment 5

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Web compat regression.

Comment 6

[Robin Steuber (they/them) [:bytesized]]

With the help of :bz, I believe I have uncovered the root of the problem. When |form.submit| is called in the testcase html file, the form is not immediately submitted; the submission is deferred until the Javascript event handler finishes. However, changing attributes that affect form submission will cause the pending submission to be flushed [1].

This section of the testcase:
>    form.submit();
>    form.setAttribute("action", originalAction);
>    form.removeAttribute("target");
changes both the "action" and "target" attributes after form submission, either of which should cause the submission flush. Prior to Firefox 55, the submission flush would happen when |form.setAttribute("action", originalAction);| is called. Bug 1365092, however, changed this behavior. The testcase line is setting the "action" attribute to the value that it already has, so the flush isn't really necessary. Therefore the flush ought to instead happen when |form.removeAttribute("target");| is called.

Clearly, however, the flush does not happen when that line is called. It appears that setting an applicable attribute causes the flush but unsetting one does not. It seems that this is, in fact, a long-standing bug. In my patch for Bug 1365092 I actually preserved this behavior, not realizing that it was a bug.

I need to examine the spec to make sure that our behavior follows it, but assuming that the only change necessary is performing the same submission flush on attribute unset that we already perform on attribute set, this should be an easy fix.

[1] http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/dom/html/HTMLFormElement.cpp#199

Comment 7

[Robin Steuber (they/them) [:bytesized]]

Attachment: 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.

Review commit: https://reviewboard.mozilla.org/r/185154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185154/

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8913765 [details]
Bug 1402025 - Ensure form submission flush when unsetting relevant attributes

https://reviewboard.mozilla.org/r/185154/#review190186

Nice test!

::: dom/html/HTMLFormElement.cpp:192
(Diff revision 1)
>  {
>    if (aNamespaceID == kNameSpaceID_None) {
>      if (aName == nsGkAtoms::action || aName == nsGkAtoms::target) {
> -      // This check is mostly to preserve previous behavior.
> -      if (aValue) {
> -        if (mPendingSubmission) {
> +      if (mPendingSubmission) {

OK.  So this doesn't really match spec in various edge cases, and we may want a followup bug on that.  But also, spec doesn't so much match reality (at least in terms of <https://github.com/whatwg/html/issues/3083>, but there may be more there).

Comment 9

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Do we think this may be a widespread problem? If it is not, I would rather not try to fix this in 56. We still may be able to take a patch for 57 if the fix is verified in nightly.

Comment 10

[Robin Steuber (they/them) [:bytesized]]

I don't have any data on how widespread this problem is. The fact that it took so long to be reported suggests that does not affect many people, but I can't say for sure. The code that triggers this does seem like a bit of an edge case. The initial report says
> We're using an old vendor library to perform asynchronous file uploads
I don't know what vendor library that is, but if others use it too the problem could be more widespread.

I would like to uplift this to 57, if possible. After my try run [1] confirms that my test passes across platforms I will land my commit and request uplift to 57.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=05f959004092&group_state=expanded

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32fa20aaa56c
Ensure form submission flush when unsetting relevant attributes r=bz

Comment 12

[Robin Steuber (they/them) [:bytesized]]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #8)
> OK.  So this doesn't really match spec in various edge cases, and we may
> want a followup bug on that.
Could you elaborate so that I can file the followup bug? Do you mean that we ought to synchronously capture variable values?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yes, exactly.  The spec captures more things as part of the submission than we do right now.  Though again, how it should actually work in the spec is not clear given the issue I filed.

Comment 14

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8913765 [details]
Bug 1402025 - Ensure form submission flush when unsetting relevant attributes

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1365092

[User impact if declined]:
A form submission edge case will not be handled properly. The details are in this bug, but I will summarize them here.
For the bug to show up:
  1) Submit a form
  2) Within the event handler for the form submit event, call the submit method on the form.
  3) Remove the "action" and/or "target" attributes from the form.
Expected Effect:
  The form will be submitted as it would have before the attributes were unset.
Actual Effect:
  The form will be submitted as if the attributes were never set, causing the form to submit with the wrong action or target.

[Is this code covered by automated tests?]:
Test added in this patch.

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]:
No manual QA needed.

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
Minimal risk.

[Why is the change risky/not risky?]:
Changes the side effects of unsetting the "action" or "target" form attributes to match the side effects of setting those attributes. Those side effects are to flush any pending submission of the from and to clear data about any in-progress submission of the form.

[String changes made/needed]:
None

Comment 15

[Robin Steuber (they/them) [:bytesized]]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #8)
> OK.  So this doesn't really match spec in various edge cases, and we may
> want a followup bug on that.

Bug 1405075 filed

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/32fa20aaa56c

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Michael, could you please verify this issue is fixed as expected on a latest nightly build? Thanks!

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8913765 [details]
Bug 1402025 - Ensure form submission flush when unsetting relevant attributes

recent webcompat regression, still early in beta cycle so taking it, Beta57+

Comment 19

[Michael Skree]

(In reply to Ritu Kothari (:ritu) from comment #17)
> Hi Michael, could you please verify this issue is fixed as expected on a
> latest nightly build? Thanks!

I can verify that this issue is fixed in the current nightly build. Thanks for taking a look at this!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5e1c7f03e336

Comment 21

[Andrei Vaida [:avaida]]

(In reply to Kirk Steuber [:bytesized] from comment #14)
> [Is this code covered by automated tests?]:
> Test added in this patch.
> 
> [Has the fix been verified in Nightly?]:
> Yes
> 
> [Needs manual test from QE? If yes, steps to reproduce]:
> No manual QA needed.

Setting qe-verify- based on Kirk's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 22

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

If this is an edge case, I'd like to wait till 57 for the fix. 
Especially as it is not a new regression in 56, and I don't see a lot of duplicates/complaints.