1508377 - Interrupted XmlHttpRequest returns completed/200 status and no data

Status: RESOLVED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[Sebastian Liusnea]

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.

Comment 1

[Sebastian Liusnea]

Actual user agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:63.0) Gecko/20100101 Firefox/63.0

Comment 2

[Adrian Florinescu [:aflorinescu]]

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.

Comment 3

[Sebastian Liusnea]

Attachment: index.html - source of the sample app

Comment 4

[Sebastian Liusnea]

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.

Comment 5

[Andrew Overholt [:overholt]]

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.

Comment 6

[Sebastian Liusnea]

mozregression identified one change, seems pertinent:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=23889c258042e274783b442e2ed570e836978a9e&tochange=8ee7533d1e8e338f3400eaf0001e1e414d792754

Comment 7

[Andrew Overholt [:overholt]]

Thanks, Sebastian!

Thomas, were your changes in bug 1362354 expected to have the effect that's been reported here?

Comment 8

[Thomas Wisniewski [:twisniewski]]

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.

Comment 9

[Anne (:annevk)]

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.

Comment 10

[Selena Deckelmann :selenamarie :selena]

Hey Thomas - is this something you can tackle fixing?

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 12

[Thomas Wisniewski [:twisniewski]]

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.

Comment 13

[Neha Kochar [:neha]]

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

Comment 14

[Thomas Wisniewski [:twisniewski]]

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.

Comment 15

[Thomas Wisniewski [:twisniewski]]

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.

Comment 16

[Thomas Wisniewski [:twisniewski]]

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.

Comment 17

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1508377 - properly update XHR status when one is aborted because of an NS_BINDING_ABORTED confition such as window.stop(); r?baku

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9c46b805faeb

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15850 for changes under testing/web-platform/tests

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged