Interrupted XmlHttpRequest returns completed/200 status and no data

RESOLVED FIXED in Firefox 67

Status

()

defect
P2
normal
RESOLVED FIXED
6 months ago
2 months ago

People

(Reporter: sebaks, Assigned: twisniewski)

Tracking

({regression})

63 Branch
mozilla67
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

Details

(Whiteboard: [necko-triaged][wptsync upstream])

Attachments

(2 attachments)

(Reporter)

Description

6 months ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.102 Safari/537.36

Steps to reproduce:

Interrupt/navigate away from a page while a XHR request is in progress.

To reproduce:
Sample app at https://my-awesome-project-2bb19.firebaseapp.com/ fetches a data file to estimate latency, then fetches it again asynchronously and reloads itself before the data is fully read. Open link in a browser with caching disables, look for "uncaught exception: Invalid response length for code 200" messages in the console.

Started happening in FF63.

Possibly linked to https://bugzilla.mozilla.org/show_bug.cgi?id=1007109


Actual results:

XHR changes ready state to Completed, status 200. However the response test is empty.



Expected results:

XHR should either have the complete data in state Completed/200 or it should indicate status != 200.
(Reporter)

Comment 1

6 months ago
Actual user agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:63.0) Gecko/20100101 Firefox/63.0
Using the sample app. I've tested on:

65.0a1          20181122220059
64.0b11         20181119162153
63.0.3 	        20181114214635

Also tested on osx 10.13.6 for:
65.0a1  2018-11-22
63.0.3 	2018-11-14


I can see the 2nd XHR request getting 200 ok status on all the versions above (cache disabled). The "uncaught exception: Invalid response length for code 200" is displayed for 2nd xhr call.

I've also looked at 
60.3.0esr       20181017185317
61.0.2          20180807170231
62.0.3   	20181001155545
and the 2nd XHR request is getting code 200ok as well, but the  The "uncaught exception: Invalid response length for code 200" is not displayed anymore.

AIUI, the XHR request code 200 is not a regression and the console error seems to me like an improved error logging, therefore I will pass the option to set "regression" for this bug's whiteboard until further investigation is being done.
Component: Untriaged → DOM
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 4

6 months ago
Thanks for taking a look!

A small clarification - the "uncaught exception: Invalid response length for code 200" is actually generated by the Javascript in the sample app (source uploaded, since the demo server is running out of quota). It's thrown when the XHR ends up in a strange state:

...
 xhr.onreadystatechange = function onreadystatechange() {
    if (xhr.readyState == 4 && xhr.status == 200 && xhr.responseText.length == 0) {
      console.log(xhr.readyState + ' ' + xhr.status + ' RESPONSE ' + xhr.responseText.length);
      throw "Invalid response length for code 200";
    }
  };
...

According to https://xhr.spec.whatwg.org/#the-responsetext-attribute, XHR.responseText in a done state should return the response body text. As the exception shows, we can get an XHR in DONE state, result code 200 OK, but no content.

Since FF 60-62 do not present this exception this might be a regression.
Is there a chance you can use https://mozilla.github.io/mozregression/ to figure out when it changed?

I'll CC some people who've made recent changes to our XHR code.
Flags: needinfo?(sebaks)
Priority: -- → P2
Component: DOM → DOM: Networking
Thanks, Sebastian!

Thomas, were your changes in bug 1362354 expected to have the effect that's been reported here?
Blocks: 1362354
Flags: needinfo?(twisniewski)
(Assignee)

Comment 8

6 months ago
Yes, the fix in that bug will detect an aborted XHR and handle it like an aborted XHR, calling the RequestErrorSteps of the XHR spec, which resets the response to a network error: https://xhr.spec.whatwg.org/#request-error-steps

I do this because the spec says to do it in the "handle errors for response" steps:

> Otherwise, if response’s aborted flag is set, then run the request error steps for event abort and exception "AbortError" DOMException.

But perhaps I shouldn't just be calling the request error steps for that specific case?

Anne, did I misinterpret the text for this specific case, where the user navigates away while an XHR is ongoing? Was the intent for that case to not reset the response to a network error, but keep whatever response body has been downloaded so far? Either way, we will probably want to change the abort-after-stop() test so that it confirms that the response is indeed what we expect it to be after the abort events are sent.
Flags: needinfo?(twisniewski) → needinfo?(annevk)

Comment 9

6 months ago
It's generally unclear to me how "navigating away" works and how interoperable it is across browsers. Aborting seems good, but it really depends on web compatibility.

It does seem like a bug though that status still returns 200 if the internal response is set to a network error. status should return 0 in that case.
Flags: needinfo?(annevk)
Whiteboard: [necko-triaged]
Hey Thomas - is this something you can tackle fixing?
Flags: needinfo?(twisniewski)
Likely too late to fix in 64 even in a dot release but let release management know if you land a patch and want to uplift.
(Assignee)

Comment 12

4 months ago

Ouch, sorry for letting your needinfo slip under the radar, Selena!

Yes I should be capable of fixing this, but (clearly) I haven't had any time to do so.

Flags: needinfo?(twisniewski)

Thomas, checking in to see if we can get this fixed in time for 67 soft freeze (Mar 11)?

Flags: needinfo?(twisniewski)
(Assignee)

Comment 14

2 months ago

I don't suspect that I'll have the time to personally get it done by then, but if no one else is available to do so, then I'll see what I can do.

(Assignee)

Comment 15

2 months ago

It looks as though the error here is that the code I added in bug 1362354 is not updating the XHR's state appropriately.

Weirdly, I can't reproduce the error at all on my Linux system, but it does reproduce for me on Windows.

I'm doing a try-run now to verify a candidate fix which will fix that (ensuring that the status is correctly set to 0). I'll update this bug ASAP.

Flags: needinfo?(twisniewski)
(Assignee)

Comment 16

2 months ago

Alright, a try-run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0eaff9393cf5be7b8105a9beaf52ce5e934f519

I'll post my patch up for review in a moment.

(Assignee)

Comment 17

2 months ago

properly update XHR status when one is aborted because of an NS_BINDING_ABORTED confition such as window.stop()

Comment 18

2 months ago
Pushed by twisniewski@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c46b805faeb
properly update XHR status when one is aborted because of an NS_BINDING_ABORTED confition such as window.stop(); r=baku

Comment 19

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Assignee: nobody → twisniewski
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15850 for changes under testing/web-platform/tests
Whiteboard: [necko-triaged] → [necko-triaged][wptsync upstream]
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.