Firefox v62 Sends Request Twice
Categories
(Core :: DOM: Core & HTML, defect, P3)
Tracking
()
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)
26.51 KB,
image/png
|
Details | |
90.45 KB,
text/plain
|
Details | |
90.45 KB,
image/png
|
Details | |
4.04 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
1.56 KB,
patch
|
nika
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•6 years ago
|
||
Not a security issue.
Comment 2•6 years ago
|
||
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
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•6 years ago
|
||
(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...
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•6 years ago
|
||
(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•6 years ago
|
||
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.
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•6 years ago
|
||
(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•6 years ago
|
||
Pinging Mike as well in case he has cycles to take a look.
Comment 11•6 years ago
|
||
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. :)
Updated•6 years ago
|
Updated•6 years ago
|
Comment 12•6 years ago
|
||
Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=133a13c44abedac2e448d315a32068ce1a5568f4&tochange=66f6b8d84e5d35d0de8d07d3e9de44a9cce913f2 Regressed by: Bug 1434553
Updated•6 years ago
|
Comment 13•6 years ago
|
||
Baku, can you take a look given you fixed the regressing bug? Thanks!
Assignee | ||
Comment 14•6 years ago
|
||
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.
Reporter | ||
Comment 15•6 years ago
|
||
Reporter | ||
Comment 16•6 years ago
|
||
(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)
Reporter | ||
Comment 17•6 years ago
|
||
Comment 18•6 years ago
|
||
(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•6 years ago
|
||
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.
Reporter | ||
Comment 20•6 years ago
|
||
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
Assignee | ||
Comment 21•6 years ago
|
||
Ok, I wrote a test. Soon a fix.
Updated•6 years ago
|
Assignee | ||
Comment 22•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 23•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Comment 24•6 years ago
|
||
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
Updated•6 years ago
|
Comment 25•6 years ago
|
||
Tom is my go to XHR person
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Updated•5 years ago
|
Comment 27•5 years ago
|
||
Masayuki, would you be comfortable taking this review from smaug? This has been sitting in the "feedback?" stage for too long now.
Comment 28•5 years ago
|
||
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•5 years ago
|
||
FWIW ,I think some docshell peer should look at the patch.
Comment 30•5 years ago
|
||
Hi Nika, I hear you are a new docshell peer (congrats!). Could you provide some feedback/review of the attached patch? Thanks!
Comment 31•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 33•5 years ago
|
||
Hi Andrea (or smb else could reply).
We continue having such issue in firefox64. Is this patch planned to be finalized/applied to release?
Assignee | ||
Comment 34•5 years ago
|
||
Comment 35•5 years ago
|
||
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•5 years ago
|
||
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
Comment 37•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7973453599bb
https://hg.mozilla.org/mozilla-central/rev/1d6b361c337a
Assignee | ||
Comment 38•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
(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•5 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/1790d083bb57
https://hg.mozilla.org/releases/mozilla-beta/rev/f69157ba349e
Comment 42•5 years ago
|
||
(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•5 years ago
|
||
(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•5 years ago
|
||
Thanks for the confirmation!
Reporter | ||
Comment 45•5 years ago
|
||
I've just reviewed the functionality in the new version, and I can't reproduce the issue.
Thanks for the correction!
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 46•5 years ago
|
||
Backed out on request based on bug 1552138
Backout: https://hg.mozilla.org/integration/autoland/rev/7aeb3ef01176fc25e7b51d6f32b4093661800084
Comment 47•5 years ago
|
||
Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.
Updated•5 years ago
|
Assignee | ||
Comment 48•5 years ago
|
||
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!
Assignee | ||
Updated•5 years ago
|
Reporter | ||
Comment 49•5 years ago
|
||
(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•5 years ago
|
||
Too late for a fix in 68 but we could still potentially take a patch for 69.
Comment 51•5 years ago
|
||
Is this still an issue? Thanks!
Comment 52•5 years ago
|
||
Too late for 70 at this point. Following up in email.
Comment 53•4 years ago
|
||
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•4 years ago
|
||
(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•4 years ago
|
||
Hi, if anyone is still interested in investigating this issue, working demo site is available under this URL: http://bug1495363.mooo.com:5080/RichFacesUploadTest/
Updated•4 years ago
|
Comment 57•4 years ago
|
||
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!
Updated•4 years ago
|
Reporter | ||
Comment 58•4 years ago
|
||
Hi!
It's going to be fixed in firefox74?
Thanks and good job!
Comment 60•4 years ago
|
||
I see no point in keeping test site up, so I'm shutting it down. Let me know if you need it online again.
Updated•2 years ago
|
Updated•1 year ago
|
Description
•