Closed Bug 1495363 Opened 6 years ago Closed 1 year ago

Firefox v62 Sends Request Twice

Categories

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

62 Branch
Unspecified
Windows 10
defect

Tracking

()

RESOLVED INCOMPLETE
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- verified
firefox66 --- verified
firefox67 --- fixed
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 --- wontfix

People

(Reporter: amontanor, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: regression, site-compat, Whiteboard: [webcompat][needsdiagnosis])

Attachments

(5 files, 1 obsolete file)

Attached image 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.
Component: Untriaged → File Handling
Not a security issue.
Group: firefox-core-security
Component: File Handling → Untriaged
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
Flags: needinfo?(amontanor)
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).
Status: UNCONFIRMED → NEW
Component: Untriaged → File Handling
Ever confirmed: true
OS: Unspecified → Windows 10
(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...
Component: File Handling → Untriaged
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)
Flags: needinfo?(amontanor)
(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?
Flags: needinfo?(amontanor)
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.
Flags: needinfo?(dschubert)
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
Flags: needinfo?(amontanor)
Flags: needinfo?(gijskruitbosch+bugs)
(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/ ).
Flags: needinfo?(gijskruitbosch+bugs)
Pinging Mike as well in case he has cycles to take a look.
Flags: needinfo?(miket)
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. :)
Whiteboard: [webcompat][needsdiagnosis]
QA Contact: past
QA Contact: past
Baku, can you take a look given you fixed the regressing bug? Thanks!
Flags: needinfo?(miket)
Flags: needinfo?(dschubert)
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini) → needinfo?(amontanor)
Attached file firefox60.png —
(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)
Flags: needinfo?(amontanor) → needinfo?(amarchesini)
Attached image firefox60.png —
(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...
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.
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
Flags: needinfo?(amarchesini)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(amarchesini)
Attached patch test — — Splinter Review
Ok, I wrote a test. Soon a fix.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Flags: needinfo?(gijskruitbosch+bugs)
Attached patch fix (obsolete) — — Splinter Review
Attachment #9015511 - Flags: review?(bugs)
Attachment #9015478 - Flags: review?(bugs)
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.
Attachment #9015511 - Flags: review?(bugs) → feedback?(bugs)
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
Attachment #9015478 - Flags: review?(bugs) → review+
Priority: -- → P2
Tom is my go to XHR person
Keywords: site-compat
Hi Andrea, any update on this issue?
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini) → needinfo?(bugs)
Masayuki, would you be comfortable taking this review from smaug? This has been sitting in the "feedback?" stage for too long now.
Flags: needinfo?(masayuki)
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.
FWIW ,I think some docshell peer should look at the patch.
Hi Nika, I hear you are a new docshell peer (congrats!). Could you provide some feedback/review of the attached patch? Thanks!
Flags: needinfo?(masayuki) → needinfo?(nika)
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.
Attachment #9015511 - Flags: feedback?(bugs) → feedback+
Flags: needinfo?(nika)
Flags: needinfo?(bugs)
Andrea, any update on this patch?
Flags: needinfo?(amarchesini)

Hi Andrea (or smb else could reply).

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

Attached patch form_fix.patch — — Splinter Review
Attachment #9015511 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #9036992 - Flags: review?(nika)
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.
Attachment #9036992 - Flags: review?(nika) → review+
Pushed by amarchesini@mozilla.com:
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
Blocks: 1520674
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

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

Attachment #9036992 - Flags: approval-mozilla-beta?
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.
Attachment #9036992 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

(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!

Flags: needinfo?(alexander.tka4uk)

(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.

(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.

Thanks for the confirmation!

Status: RESOLVED → VERIFIED
Flags: needinfo?(alexander.tka4uk)

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

Depends on: 1528457
Component: HTML: Form Submission → DOM: Core & HTML
Depends on: 1542912
No longer depends on: 1542912
Regressions: 1542912
No longer depends on: 1528457
Regressions: 1528457
Regressions: 1529846
Status: VERIFIED → REOPENED
Flags: needinfo?(amarchesini)
Resolution: FIXED → ---
Target Milestone: mozilla66 → ---

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

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!

Flags: needinfo?(amarchesini)
Flags: needinfo?(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

Flags: needinfo?(amontanor)

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

Is this still an issue? Thanks!

Flags: needinfo?(amarchesini)

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

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?

(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.

Flags: needinfo?(htsai)
Priority: P2 → P3

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

Baku or Hsin-Yi, does the site in comment 55 help?

Flags: needinfo?(htsai)

Thanks for the testing url. I'll leave it to baku to provide feedback or questions. Please let me know if you need more help from me or my team, baku. Thanks!

Flags: needinfo?(htsai)

Hi!
It's going to be fixed in firefox74?
Thanks and good job!

I see no point in keeping test site up, so I'm shutting it down. Let me know if you need it online again.

Severity: normal → S3
Status: REOPENED → RESOLVED
Closed: 5 years ago1 year ago
Flags: needinfo?(amarchesini)
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: