1495363 - Firefox v62 Sends Request Twice

Status: REOPENED

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

Description

[amontanor]

Attachment: bug62.png

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

Steps to reproduce:

When I try to upload a file using a richfaces component, in the new version of firefox (v62) the request is sent twice. I have seen this behavior in my own website: https://desarrollo.sercae.com/sercae/pages/test/fileupload.richfaces.jsf. This page is only an example. This behavior was previously managed with the Community Support, but we can't find a solution: https://support.mozilla.org/es/questions/1235249


Actual results:

The request was sent twice, and the result was an "Transfer error occurred" message


Expected results:

The request should be sent only once. In the previous version, 61 (or Chrome, IE, Edge...) the result is correct, only one request and a "Done" mesagge.

Comment 1

[:Gijs (he/him)]

Not a security issue.

Comment 2

[:Gijs (he/him)]

Pretty hard to see what's going on here without a more minimalistic testcase. The other issue here is that all your JS is compressed (whitespace removed) which makes it really hard to debug. Can you try to slim down the testcase, or at least un-shrinkwrap the JS?

Off-hand, it looks to me like there's an XHR in addition to a "normal" form submit. There's also a page refresh, which might give a clue - perhaps you need to call event.preventDefault() on the submit event if you're handling it from JS or something?

Did this not happen with earlier versions of Firefox (e.g. v60esr) or with newer versions (e.g. https://nightly.mozilla.org/ - make sure to use a separate firefox profile for testing nightly! https://support.mozilla.org/kb/profile-manager-create-and-remove-firefox-profiles

Comment 3

[Daniel_A[:daschilean] [Please contact - [e-mail]]]

User Agent:  Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0
Build ID: 20181001220118   
I manage to reproduce this on Windows 10 x64 with the latest Firefox Nightly 64.0a1 (2018-10-01) (64-bit).

Comment 4

[:Gijs (he/him)]

(In reply to Daniel_A[:daschilean] from comment #3)
> User Agent:  Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0)
> Gecko/20100101 Firefox/64.0
> Build ID: 20181001220118   
> I manage to reproduce this on Windows 10 x64 with the latest Firefox Nightly
> 64.0a1 (2018-10-01) (64-bit).

This isn't a File Handling issue - if it's valid, it's a gecko issue and belongs somewhere in Core - but without more details it's not clear whether that's going to be DOM, Form Controls, Networking, or something else -- and Core doesn't have an "Untriaged" component...

Comment 5

[amontanor]

Thank you for your response!
I have also reviewed the steps in the previous version (v61), and it works perfectly.

@Gijs, what testcase can I do? The point is there is only an upload component in the page (and the RichFaces libraries)

Comment 6

[:Gijs (he/him)]

(In reply to amontanor from comment #5)
> Thank you for your response!
> I have also reviewed the steps in the previous version (v61), and it works
> perfectly.
> 
> @Gijs, what testcase can I do? The point is there is only an upload
> component in the page (and the RichFaces libraries)

Can you use a non-minified version of the library in the testcase?

Comment 7

[:Gijs (he/him)]

Dennis, can you ask some of the webcompat folks to have a look? Figuring this out isn't my area of expertise, and the testcase is complex but seems to work in Chrome but not in Firefox.

Comment 8

[amontanor]

We have changed the page, and (In reply to :Gijs (he/him) from comment #6)
> (In reply to amontanor from comment #5)
> > Thank you for your response!
> > I have also reviewed the steps in the previous version (v61), and it works
> > perfectly.
> > 
> > @Gijs, what testcase can I do? The point is there is only an upload
> > component in the page (and the RichFaces libraries)
> 
> Can you use a non-minified version of the library in the testcase?

Done! https://desarrollo.sercae.com/sercae/pages/test/fileupload.richfaces.jsf

Comment 9

[:Gijs (he/him)]

(In reply to amontanor from comment #8)
> We have changed the page, and (In reply to :Gijs (he/him) from comment #6)
> > (In reply to amontanor from comment #5)
> > > Thank you for your response!
> > > I have also reviewed the steps in the previous version (v61), and it works
> > > perfectly.
> > > 
> > > @Gijs, what testcase can I do? The point is there is only an upload
> > > component in the page (and the RichFaces libraries)
> > 
> > Can you use a non-minified version of the library in the testcase?
> 
> Done!
> https://desarrollo.sercae.com/sercae/pages/test/fileupload.richfaces.jsf

Thanks for this - our web compat team will hopefully look at this bug further, I'm not able to do so myself at this time - I took a quick look but wasn't quickly able to determine the source of the problem. Dennis is in Germany, where yesterday was a public holiday. Perhaps they'll have time to take a look today or tomorrow.

If you're interested in helping us further here, one thing you could do is determine exactly when things broke (ie get a nightly channel regression window between Fx61 and Fx62), by using mozregression ( https://mozilla.github.io/mozregression/ ).

Comment 10

[:Gijs (he/him)]

Pinging Mike as well in case he has cycles to take a look.

Comment 11

[Dennis Schubert [:denschub]]

Thanks so much for the reduced testcase!

I'm a bit busy with GoFaster things right now, but I'll leave my ni? in place so I can look at that hopefully at the beginning of next week. :)

Comment 12

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=133a13c44abedac2e448d315a32068ce1a5568f4&tochange=66f6b8d84e5d35d0de8d07d3e9de44a9cce913f2

Regressed by: Bug 1434553

Comment 13

[:Gijs (he/him)]

Baku, can you take a look given you fixed the regressing bug? Thanks!

Comment 14

[Andrea Marchesini [:baku]]

Actually I'm able to reproduce the double post in firefox 60 as well. It seems that 1 post is done via XHR, the second one is done via form subsumption.

Comment 15

[amontanor]

Attachment: firefox60.png

Comment 16

[amontanor]

(In reply to Andrea Marchesini [:baku] from comment #14)
> Actually I'm able to reproduce the double post in firefox 60 as well. It
> seems that 1 post is done via XHR, the second one is done via form
> subsumption.

Thank you Andrea.
I can't reproduce this behavior. Attached you can see the image "firefox60.png", with the result using Firefox 60. Indeed, I only can see this duplicated post using firefox 62 (or newer)

Comment 17

[amontanor]

Attachment: firefox60.png

Comment 18

[:Gijs (he/him)]

(In reply to amontanor from comment #16)
> (In reply to Andrea Marchesini [:baku] from comment #14)
> > Actually I'm able to reproduce the double post in firefox 60 as well. It
> > seems that 1 post is done via XHR, the second one is done via form
> > subsumption.
> 
> Thank you Andrea.
> I can't reproduce this behavior. Attached you can see the image
> "firefox60.png", with the result using Firefox 60. Indeed, I only can see
> this duplicated post using firefox 62 (or newer)

There is both an `xhr` and a `subdocument` POST. The XHR is underneath the highlighted subdocument one in your screenshot...

Comment 19

[:Gijs (he/him)]

Note that when this fails, I see 1 xhr and 2 'subdocument' POSTs. I assume the double subdocument POST is the cause of the issue here.

Comment 20

[amontanor]

Exactly, only when the (In reply to :Gijs (he/him) from comment #19)
> Note that when this fails, I see 1 xhr and 2 'subdocument' POSTs. I assume
> the double subdocument POST is the cause of the issue here.

I think so, too.
That's the point, the result is always an error if there are 2 'subdocument' POSTs, but no if there is only one

Comment 21

[Andrea Marchesini [:baku]]

Attachment: test

Ok, I wrote a test. Soon a fix.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: fix

Comment 23

[Andrea Marchesini [:baku]]

The reason why I'm proposing this patch is because of this: https://html.spec.whatwg.org/#planned-navigation
We don't have (it seems) a concept of task where we abort the previous form submission. Here I abort the previous loading if a new is coming.

Comment 24

[Olli Pettay [:smaug]]

Comment on attachment 9015478 [details] [diff] [review]
test


>--- /dev/null
>+++ b/dom/html/test/forms/file_bug1495363.sjs
>@@ -0,0 +1,14 @@
>+function handleRequest(aRequest, aResponse) {
>+  aResponse.setStatusLine(aRequest.httpVersion, 200);
>+
>+  if (aRequest.queryString.includes("result")) {
>+    aResponse.write(getState("hints") || 0);
>+    setState("hints", "0");
>+  } else {
>+    let hints = parseInt(getState("hints") || 0) + 1;
>+    setState("hints", hints.toString());
>+
>+    aResponse.setHeader("Content-Type", "text/html", false);
>+    aResponse.write("Hello World!");
>+ }
This could use a comment. The 'if' is about the last check and 'else' is for submission

Comment 25

[Mike Taylor [:miketaylr]]

Tom is my go to XHR person

Comment 26

[amontanor]

Hi Andrea, any update on this issue?

Comment 27

[(parental leave 10/24-mid-February) Jared Wein [:jaws] (please needinfo? me)]

Masayuki, would you be comfortable taking this review from smaug? This has been sitting in the "feedback?" stage for too long now.

Comment 28

[Olli Pettay [:smaug]]

It is not in my review queue. And I process reviews before feedbacks, and my review queue hasn't been empty for weeks.
Sorry about the delay.

Comment 29

[Olli Pettay [:smaug]]

FWIW ,I think some docshell peer should look at the patch.

Comment 30

[(parental leave 10/24-mid-February) Jared Wein [:jaws] (please needinfo? me)]

Hi Nika, I hear you are a new docshell peer (congrats!). Could you provide some feedback/review of the attached patch? Thanks!

Comment 31

[:Nika Layzell (on leave)]

Comment on attachment 9015511 [details] [diff] [review]
fix

Review of attachment 9015511 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/test/forms/test_bug1495363.html
@@ +38,5 @@
>    SimpleTest.executeSoon(() => {
>      fetch("file_bug1495363.sjs?result").then(r => r.text()).then(text => {
> +      let parts = text.split("-");
> +      is(parts[0], "1", "We have 1 request only");
> +      is(parts[1], "TIMEOUT", "The request comes from the timer");

This situation feels a bit racey to me - theoretically if everything was executing sufficiently quickly the timeout wouldn't have a chance to cause the original load to be cancelled, no?

I suppose that situation is unlikely to come up on infra.

@@ +43,3 @@
>        SimpleTest.finish();
>      });
>    }); 

nit: trailing whitespace

::: uriloader/base/nsDocLoader.cpp
@@ +432,5 @@
>        //
>        // Make sure that the document channel is null at this point...
>        // (unless its been redirected)
>        //
> +      if (!(loadFlags & nsIChannel::LOAD_REPLACE) && mDocumentRequest) {

I think this definitely needs a better updated comment on it explaining why we want to cancel the existing request, and what situations we'd end up here. It'd probably also be good to note why we don't want to cancel the old channel during a replace load.

Comment 32

[Julien Cristau [:jcristau]]

Andrea, any update on this patch?

Comment 33

[Alex]

Hi Andrea (or smb else could reply).

We continue having such issue in firefox64. Is this patch planned to be finalized/applied to release?

Comment 34

[Andrea Marchesini [:baku]]

Attachment: form_fix.patch

Comment 35

[:Nika Layzell (on leave)]

Comment on attachment 9036992 [details] [diff] [review]
form_fix.patch

Review of attachment 9036992 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK. IIRC in the redirect case our original channel should have been cancelled at this point. I wonder if we can assert that here.

I think it'd actually be fine to call `Cancel` on the old channel in the redirect case as well, so I wonder if we could just have it check if there's an existing channel, and cancel it if there is. No need to do that in this bug though.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7973453599bb
Abort the previous request, if a form is submitted twice, r=nike
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6b361c337a
Abort the previous request, if a form is submitted twice - tests, r=smaug

Comment 37

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/7973453599bb
https://hg.mozilla.org/mozilla-central/rev/1d6b361c337a

Comment 38

[Andrea Marchesini [:baku]]

Comment on attachment 9036992 [details] [diff] [review]
form_fix.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: n/a

User impact if declined: Double form submission can trigger double requests sent. We should abort the pending one when a second form submission is triggered.

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): Instead of having an assertion, the patch cancels the pending request and lets the new one to proceed.
The code is extremely simple and it follows what the spec says.

String changes made/needed: none

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9036992 [details] [diff] [review]
form_fix.patch

[Triage Comment]
Simple fix with tests which makes us more spec-compliant and fixes real-world site issues. Approved for 65.0b12.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

(In reply to Alex from comment #33)

Hi Andrea (or smb else could reply).

We continue having such issue in firefox64. Is this patch planned to be
finalized/applied to release?

This patch should be in the Firefox 65b12 build shipping tomorrow. If you could try it out to verify that things are working better for you, we'd be very grateful!

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1790d083bb57
https://hg.mozilla.org/releases/mozilla-beta/rev/f69157ba349e

Comment 42

[sobmar]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #40)

This patch should be in the Firefox 65b12 build shipping tomorrow. If you could try it out to verify that things are working better for you, we'd be very grateful!

Hi
I've tested build 65b12 and it's working fine in my environment, only one request is sent. Thanks for fixing this issue.

Comment 43

[Alex]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #40)

(In reply to Alex from comment #33)

Hi Andrea (or smb else could reply).

We continue having such issue in firefox64. Is this patch planned to be
finalized/applied to release?

This patch should be in the Firefox 65b12 build shipping tomorrow. If you could try it out to verify that things are working better for you, we'd be very grateful!

Hi,
I've also tested 65b12 and the issue is not reproduced anymore. Thanks for the fix.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Thanks for the confirmation!

Comment 45

[amontanor]

I've just reviewed the functionality in the new version, and I can't reproduce the issue.
Thanks for the correction!

Comment 46

[Andreea Pavel [:apavel]]

Backed out on request based on bug 1552138

Backout: https://hg.mozilla.org/integration/autoland/rev/7aeb3ef01176fc25e7b51d6f32b4093661800084

Comment 47

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 48

[Andrea Marchesini [:baku]]

amontanor, sorry to bother you again, but the test is down and I need it check this issue again.
Do you think it's possible to have this URL up again: https://desarrollo.sercae.com/sercae/pages/test/fileupload.richfaces.jsf ? Thanks!

Comment 49

[amontanor]

(In reply to Andrea Marchesini [:baku] from comment #48)

amontanor, sorry to bother you again, but the test is down and I need it check this issue again.
Do you think it's possible to have this URL up again: https://desarrollo.sercae.com/sercae/pages/test/fileupload.richfaces.jsf ? Thanks!
Hi Andres, let me see if I can speak to the developer team and I'll give you feedback ASAP

Comment 50

[Liz Henry (:lizzard)]

Too late for a fix in 68 but we could still potentially take a patch for 69.

Comment 51

[Liz Henry (:lizzard)]

Is this still an issue? Thanks!

Comment 52

[Liz Henry (:lizzard)]

Too late for 70 at this point. Following up in email.

Comment 53

[Pascal Chevrel:pascalc]

This bug hasn't seen any activity over the last 6 months, at this point it is too too late for 71. Adjusting flags.
Hsin-Yi, is that still a P2?

Comment 54

[Hsin-Yi Tsai [:hsinyi]]

(In reply to Pascal Chevrel:pascalc from comment #53)

This bug hasn't seen any activity over the last 6 months, at this point it is too too late for 71. Adjusting flags.
Hsin-Yi, is that still a P2?

Good question! It seems we're still waiting for response from comment 49, not actionable at this moment. Moving to P3 for now.

Comment 55

[sobmar]

Hi, if anyone is still interested in investigating this issue, working demo site is available under this URL: http://bug1495363.mooo.com:5080/RichFacesUploadTest/