Sebastian Liusnea Reporter
	Description • 6 years 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.

	Sebastian Liusnea Reporter
	Comment 1 • 6 years ago

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

	Adrian Florinescu [:aflorinescu]
	Comment 2 • 6 years ago

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.

	Sebastian Liusnea Reporter
	Comment 4 • 6 years 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.

	Andrew Overholt [:overholt]
	Comment 5 • 6 years ago

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.

	Sebastian Liusnea Reporter
	Comment 6 • 6 years ago

mozregression identified one change, seems pertinent:

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

	Andrew Overholt [:overholt]
	Comment 7 • 6 years ago

Thanks, Sebastian!

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

	Thomas Wisniewski [:twisniewski] Assignee
	Comment 8 • 6 years 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.

	Anne (:annevk)
	Comment 9 • 6 years 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.

	Selena Deckelmann :selenamarie :selena
	Comment 10 • 6 years ago

Hey Thomas - is this something you can tackle fixing?

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 11 • 6 years ago

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.

	Pulsebot
	Comment 18 • 5 years 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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 20 • 5 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 21 • 5 years ago

Upstream PR merged