Closed Bug 1658082 Opened 4 years ago Closed 4 years ago

FF79 now performs multiple navigations when calling `form.dispatchEvent(submit)` + `form.submit()`

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
79 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
82 Branch
Tracking Flags:
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox79 --- wontfix
firefox80 --- wontfix
firefox81 --- verified
firefox82 --- verified

People

(Reporter: bakkot, Assigned: nika)

References

(Regression)

Details

(Keywords: regression)

Attachments

(5 files)

Example from Comment 1
465 bytes, text/html
Details
Bug 1658082 - Part 2: Remove duplicate information from InternalLoad IPC messages,
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
Details | Review
Bug 1658082 - Part 3: Set TargetBrowsingContext for non-retargeted loads,
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
Details | Review
Bug 1658082 - Part 4: Add test for duplicate form submission with non-retargeted loads,
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
Details | Review
Bug 1658082 - Part 1: Correctly set mTargetBrowsingContext from IPC,
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
Details | Review

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/83.0.4103.116 Safari/537.36

Steps to reproduce:

Here's a demo page:

<!doctype html>
<head><title>Submit Demo</title></head>
<body>
<form id="f" action="http://example.com/" method="POST">
<input name="foo">
</form>

<input type="button" id="b" value="submit">

<script>
addEventListener('beforeunload', () => {
  console.log('beforeunload f');
});
b.addEventListener('click', () => {
  f.dispatchEvent(new Event('submit'));
  for (let i = 0; i < 2**30; ++i) {
    // spin
    i;
  }
  f.submit();
});
</script>

Actual results:

Clicking the button results in two distinct navigation events - this is observable with in the console as two "Navigation occurred" and "beforeunload fired" lines, or in the network tab as two POSTs.

On my machine, if you include the spin part, you can actually get two complete network requests. This still reproduces without the spin loop, but the first network request is more likely to complete (depending on your network and CPU speed).

This breaks several of my customers, though I don't have permission to name them right now.

Expected results:

Only a single navigation should occur, as in Firefox 78, in Chrome, and in Safari.

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core

Could be a regression of bug 1590762?

farre: could you take a look?

Flags: needinfo?(afarre)

I ran mozregression on my demo page, and (assuming that I did it right) it points to https://phabricator.services.mozilla.com/D79821 as the culprit, which seems plausible.

And yeah, the revision I identified was in response to bug 159076, and indeed I see that one of the first comments on the bug is "This is needed to prevent repeat submission of forms". Looks like the patch landed and now I'm getting repeat submissions of forms.

That should be bug 1590762, sorry (and sorry for the multiple messages; I don't see a way to edit).

Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Unspecified → All
Regressed by: 1590762
Hardware: Unspecified → All
Has Regression Range: --- → yes

Set release status flags based on info from the regressing bug 1590762

status-firefox79: --- → affected
status-firefox80: --- → affected
status-firefox81: --- → affected
status-firefox-esr68: --- → unaffected
status-firefox-esr78: --- → unaffected

I can't reproduce this using the attached test case, but Bug 1590762 did indeed change how we mitigate double submits so it's very possible that that is the issue. bakkot could you supply more information on how to use the test?

Flags: needinfo?(afarre) → needinfo?(bakkot)

Neha, does anyone need to look at this while I'm on PTO?

Flags: needinfo?(nkochar)
Flags: needinfo?(afarre)

I didn't do anything special to use the test case. Just open it as a page and click the button. Or server it from a local server, if you're worried about file:// interactions.

Here, I recorded it: https://www.youtube.com/watch?v=tbtfwy5q2lA

This is with a clean profile on Firefox 79.0 on Mac OS. At 13 seconds you can see that the POST occurs twice in the network tab. At 15 seconds you can see that the console records the "unload" event occurring twice and that the navigation occurs twice. The server also observes two POST requests. (I can record that too if you like.)

Flags: needinfo?(bakkot)

Nika can you look into this while farre is on pto?

Flags: needinfo?(nkochar) → needinfo?(nika)
Assignee: nobody → nika
Flags: needinfo?(nika)

Previously, this field was incorrectly being initialized to
SourceBrowsingContext instead of TargetBrowsingContext.

Attachment #9170731 - Attachment description: Bug 1658082 - Part 1: Remove duplicate information from InternalLoad IPC messages, → Bug 1658082 - Part 2: Remove duplicate information from InternalLoad IPC messages,
Attachment #9170732 - Attachment description: Bug 1658082 - Part 2: Set TargetBrowsingContext for non-retargeted loads, → Bug 1658082 - Part 3: Set TargetBrowsingContext for non-retargeted loads,
Attachment #9170733 - Attachment description: Bug 1658082 - Part 3: Add test for duplicate form submission with non-retargeted loads, → Bug 1658082 - Part 4: Add test for duplicate form submission with non-retargeted loads,
Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/695bda5827f3
Part 1: Correctly set mTargetBrowsingContext from IPC, r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/a44ec47dc7c1
Part 2: Remove duplicate information from InternalLoad IPC messages, r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/31fd963f33d4
Part 3: Set TargetBrowsingContext for non-retargeted loads, r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/59aaa4311b3a
Part 4: Add test for duplicate form submission with non-retargeted loads, r=mattwoodrow

Is this something we should consider uplifting to Beta? Please nominate if so.

Flags: needinfo?(nika)

Given we're pretty early in the beta cycle, I think this should be OK to uplift. There's some risk from the changes in part 2, which are not all necessary to fix this bug, but it may be riskier to pull those small changes out separately.

Flags: needinfo?(nika)
Flags: needinfo?(afarre)

Comment on attachment 9172239 [details]
Bug 1658082 - Part 1: Correctly set mTargetBrowsingContext from IPC,

Beta/Release Uplift Approval Request

  • User impact if declined: Firefox may allow multiple non-retargeting form submissions in some edge-cases
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: none
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Fairly straightforward changes. There is some mild risk in part 2 due to extra added assertions, and general refactoring cleanups, but rebasing and testing the other patches without those changes may not be worth it/introduce other bugs, as they're still pretty small.

As far as I am aware, there aren't any crashes caused by the assertions added in this bug on nightly since it was landed.

  • String changes made/needed: none
Attachment #9172239 - Flags: approval-mozilla-beta?
Attachment #9170731 - Flags: approval-mozilla-beta?
Attachment #9170732 - Flags: approval-mozilla-beta?
Attachment #9170733 - Flags: approval-mozilla-beta?

Comment on attachment 9170731 [details]
Bug 1658082 - Part 2: Remove duplicate information from InternalLoad IPC messages,

Approved for 81.0b5. Thanks for including new automated tests.

Attachment #9170731 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9170732 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9170733 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9172239 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Hello!

In order to verify this bug I tried reproducing it with the demo page provided in the description using Fx 79, 80.0 and 81.0b1 on MacOS 10.15 and Windows 10. In order to verify this issue I have created a localhost server with the demo page on both windows and Mac but the output of the webconsole was only one POST after submitting.

Is there some step that I might have missed in reproducing this issue and can somebody point me in the right direction?

Flags: needinfo?(bakkot)

Did you ensure that "Persist logs" is checked in the console (under the gear icon)? When it is not checked the console clears when navigation occurs, so you can't ever observe more than one navigation event.

Flags: needinfo?(bakkot)

Hello! Thank you bakkot for the extra step provided in the latest comment.

I have managed to reproduce the issue using Fx 80.0b1 on macOS 10.15. I can confirm that the issue is fixed on Fx 81.0b7 and 82.0a1(2020-09-07) on all OS's.

Status: RESOLVED → VERIFIED
status-firefox81: fixed → verified
status-firefox82: fixed → verified
Flags: qe-verify+
See Also: → 1666505
See Also: → 1803805
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: