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

VERIFIED FIXED in Firefox 57

Status

()

defect
VERIFIED FIXED
2 years ago
3 months ago

People

(Reporter: michael.skree, Assigned: bytesized)

Tracking

({regression, site-compat})

55 Branch
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 wontfix, firefox56+ wontfix, firefox57+ fixed, firefox58+ verified)

Details

Attachments

(3 attachments)

Reporter

Description

2 years ago
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.
Component: Untriaged → HTML: Form Submission
Product: Firefox → Core
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.
Flags: needinfo?(ksteuber)
That patch was not intended to have this effect. I'll look into it.
Assignee: nobody → ksteuber
Flags: needinfo?(ksteuber)
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.
Posted file 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.
[Tracking Requested - why for this release]: Web compat regression.
Blocks: 1365092
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
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 hidden (mozreview-request)
Attachment #8913765 - Flags: review?(bzbarsky)

Comment 8

2 years ago
mozreview-review
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).
Attachment #8913765 - Flags: review?(bzbarsky) → review+
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.
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

2 years ago
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/32fa20aaa56c
Ensure form submission flush when unsetting relevant attributes r=bz
(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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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
Attachment #8913765 - Flags: approval-mozilla-beta?
(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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/32fa20aaa56c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Hi Michael, could you please verify this issue is fixed as expected on a latest nightly build? Thanks!
Flags: needinfo?(michael.skree)
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+
Attachment #8913765 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reporter

Comment 19

2 years ago
(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!
Flags: needinfo?(michael.skree)
(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.
Flags: qe-verify-
Status: RESOLVED → VERIFIED
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.
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.