New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid inaccurately inferring xhr abort. #1612

Merged
merged 4 commits into from Feb 18, 2017

Conversation

Projects
None yet
5 participants
@bruce-one
Contributor

bruce-one commented Feb 10, 2017

The xhr.status can equal zero in non-abort scenarios, eg timeout or CORS failure.

The current handling means that the request promise doesn't reject for a CORS failure or for a timeout.

Rather than inferring that xhr.status === 0 means abort, track the invocation of the abort function instead.

Mostly making this PR as a RFC... (And to check I didn't miss something logical :-) )

Avoid inaccurately inferring xhr abort.
xhr.status can equal zero in non-abort scenarios, eg timeout or CORS
failure.

Instead use a variable to track aborts.
@isiahmeadows

This comment has been minimized.

Show comment
Hide comment
@isiahmeadows

isiahmeadows Feb 11, 2017

Collaborator

Could you add a test case to your patch so we can verify it stays that way?

Collaborator

isiahmeadows commented Feb 11, 2017

Could you add a test case to your patch so we can verify it stays that way?

@bruce-one

This comment has been minimized.

Show comment
Hide comment
@bruce-one

bruce-one Feb 12, 2017

Contributor

Test added :-)

The test fails without the patch, but passes with; but I'm not hugely familiar with the mocks, so... It might be rubbish :-p

(I didn't add an ontimeout-type test because I couldn't figure out the best way to add it? But the "cors-like" test is essentially covering the same thing, I think?)

Contributor

bruce-one commented Feb 12, 2017

Test added :-)

The test fails without the patch, but passes with; but I'm not hugely familiar with the mocks, so... It might be rubbish :-p

(I didn't add an ontimeout-type test because I couldn't figure out the best way to add it? But the "cors-like" test is essentially covering the same thing, I think?)

@simov

This comment has been minimized.

Show comment
Hide comment
@simov

simov Feb 13, 2017

Hey, @tivac pointed me out this PR about the regression I'm experiencing in #1624

I wanted to point out that requesting file:/// URLs with XMLHttpRequest is definitely possible, when for example making request from a Chrome extension, or Electron app, but both file and ftp protocols does not use the same status codes as HTTP, so in these cases xhr.status is set to 0. Also in my personal tests it looks like the xhr.readyState is set to 2 instead of 4, and yet the response body is still available.

simov commented Feb 13, 2017

Hey, @tivac pointed me out this PR about the regression I'm experiencing in #1624

I wanted to point out that requesting file:/// URLs with XMLHttpRequest is definitely possible, when for example making request from a Chrome extension, or Electron app, but both file and ftp protocols does not use the same status codes as HTTP, so in these cases xhr.status is set to 0. Also in my personal tests it looks like the xhr.readyState is set to 2 instead of 4, and yet the response body is still available.

@isiahmeadows

This comment has been minimized.

Show comment
Hide comment
@isiahmeadows

isiahmeadows Feb 14, 2017

Collaborator

@simov 😕 I hate oddities like that...

For what it's worth, I rarely find it useful to even make an XHR in Electron, unless I'm specifically seeking a remote resource. Now Chrome doing it in extensions is entirely unhelpful, and almost seems worth filing a bug over there to fix that unhelpful oddity (although it may get rejected for compatibility reasons).

@bruce-one Mind adding a special case for xhr.status === 2 + a truthy response body due to the above? Sorry to trouble you more about it. Edit: Never mind.

Collaborator

isiahmeadows commented Feb 14, 2017

@simov 😕 I hate oddities like that...

For what it's worth, I rarely find it useful to even make an XHR in Electron, unless I'm specifically seeking a remote resource. Now Chrome doing it in extensions is entirely unhelpful, and almost seems worth filing a bug over there to fix that unhelpful oddity (although it may get rejected for compatibility reasons).

@bruce-one Mind adding a special case for xhr.status === 2 + a truthy response body due to the above? Sorry to trouble you more about it. Edit: Never mind.

@simov

This comment has been minimized.

Show comment
Hide comment
@simov

simov Feb 14, 2017

@simov 😕 I hate oddities like that...

Well that's HTTP 😁

The problem is with xhr.readyState being 2 instead of 4 and I couldn't find any information about why it is that way.

xhr.status is 0 and that is somewhat documented - I found it in the Spanish version of MDN and used Google Translate, so my definition from above is pretty accurate :)

I'll try to dig some more information today.

simov commented Feb 14, 2017

@simov 😕 I hate oddities like that...

Well that's HTTP 😁

The problem is with xhr.readyState being 2 instead of 4 and I couldn't find any information about why it is that way.

xhr.status is 0 and that is somewhat documented - I found it in the Spanish version of MDN and used Google Translate, so my definition from above is pretty accurate :)

I'll try to dig some more information today.

@simov

This comment has been minimized.

Show comment
Hide comment
@simov

simov Feb 14, 2017

@bruce-one @isiahmeadows my bad, xhr.readyState is 2 only the first time it enters the callback, obviously after that it updates to 3 and 4.

So removing xhr.status && should allow file:// URLs to work again! The only problem remaining is that the response body is returned inside the catch error handler because if ((xhr.status >= 200 && xhr.status < 300) || xhr.status === 304) fails.

Again xhr.status is always 0 for file:// URLs.

Probably additional condition to check for the requested URL's protocol would do the trick:
if (/^file:\/\//.test(args.url) || (xhr.status >= 200 && xhr.status < 300) || xhr.status === 304)

simov commented Feb 14, 2017

@bruce-one @isiahmeadows my bad, xhr.readyState is 2 only the first time it enters the callback, obviously after that it updates to 3 and 4.

So removing xhr.status && should allow file:// URLs to work again! The only problem remaining is that the response body is returned inside the catch error handler because if ((xhr.status >= 200 && xhr.status < 300) || xhr.status === 304) fails.

Again xhr.status is always 0 for file:// URLs.

Probably additional condition to check for the requested URL's protocol would do the trick:
if (/^file:\/\//.test(args.url) || (xhr.status >= 200 && xhr.status < 300) || xhr.status === 304)

@simov

This comment has been minimized.

Show comment
Hide comment
@simov

simov Feb 14, 2017

This works:

- if (xhr.status && xhr.readyState === 4) {
+ if (xhr.readyState === 4) {
    try {
      var response = (args.extract !== extract) ? args.extract(xhr, args) : args.deserialize(args.extract(xhr, args))
-     if ((xhr.status >= 200 && xhr.status < 300) || xhr.status === 304) {
+     if (/^file:\/\//.test(args.url) || (xhr.status >= 200 && xhr.status < 300) || xhr.status === 304) {

though I'm not sure if your mock library supports the file:// protocol.

simov commented Feb 14, 2017

This works:

- if (xhr.status && xhr.readyState === 4) {
+ if (xhr.readyState === 4) {
    try {
      var response = (args.extract !== extract) ? args.extract(xhr, args) : args.deserialize(args.extract(xhr, args))
-     if ((xhr.status >= 200 && xhr.status < 300) || xhr.status === 304) {
+     if (/^file:\/\//.test(args.url) || (xhr.status >= 200 && xhr.status < 300) || xhr.status === 304) {

though I'm not sure if your mock library supports the file:// protocol.

@simov

This comment has been minimized.

Show comment
Hide comment
@simov

simov commented on 27881af Feb 14, 2017

Thanks @bruce-one !

Fix xhr abort test.
Need to be more creative about ensuring that onreadystatechange is
called, but that the test conditions execute after that the function
(that does nothing...) is called.

@isiahmeadows isiahmeadows requested a review from lhorie Feb 15, 2017

@lhorie lhorie merged commit d31e0a8 into MithrilJS:master Feb 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pygy

This comment has been minimized.

Show comment
Hide comment
@pygy

pygy Feb 21, 2017

Member

@bruce-one this was targeted agains the wrong branch (master), could you re-do this against next? (without the #1610-related commits)?

Member

pygy commented Feb 21, 2017

@bruce-one this was targeted agains the wrong branch (master), could you re-do this against next? (without the #1610-related commits)?

@@ -2,6 +2,8 @@
var buildQueryString = require("../querystring/build")
var FILE_PROTOCOL_REGEX = new RegExp('^file://', 'i')

This comment has been minimized.

@simov

simov Mar 10, 2017

btw, I forgot to mention that a literal regex /file:\/\//ishould be used whenever possible:

Literal regular expressions are evaluated only once at evaluation time while the constructor is evaluated at runtime, thus unless you need to dynamically create a regular expression use the literal syntax.

@simov

simov Mar 10, 2017

btw, I forgot to mention that a literal regex /file:\/\//ishould be used whenever possible:

Literal regular expressions are evaluated only once at evaluation time while the constructor is evaluated at runtime, thus unless you need to dynamically create a regular expression use the literal syntax.

This comment has been minimized.

@pygy

pygy Mar 10, 2017

Member

That line only runs once, at load time, so it does not matter for perf.

However, a literal /file:\/\//i would shave a few bytes which is always nice.

@pygy

pygy Mar 10, 2017

Member

That line only runs once, at load time, so it does not matter for perf.

However, a literal /file:\/\//i would shave a few bytes which is always nice.

This comment has been minimized.

@bruce-one

bruce-one Mar 10, 2017

Contributor

If saving bytes is considered worth a change; I realised afterwards that the following patch is simpler (and smaller, hence mentioning it):


diff --git a/request/request.js b/request/request.js
index 552fa51..58d04a2 100644
--- a/request/request.js
+++ b/request/request.js
@@ -56,13 +56,13 @@ module.exports = function($window, Promise) {
 			else args.url = assemble(args.url, args.data)
 
 			var xhr = new $window.XMLHttpRequest(),
-				aborted = false,
 				_abort = xhr.abort
 
 
 			xhr.abort = function abort() {
-				aborted = true
+				// Don't throw errors on xhr.abort().
+				xhr.onreadystatechange = null
 				_abort.call(xhr)
 			}
 
 			xhr.open(args.method, args.url, typeof args.async === "boolean" ? args.async : true, typeof args.user === "string" ? args.user : undefined, typeof args.password === "string" ? args.password : undefined)
@@ -82,9 +82,6 @@ module.exports = function($window, Promise) {
 			if (typeof args.config === "function") xhr = args.config(xhr, args) || xhr
 
 			xhr.onreadystatechange = function() {
-				// Don't throw errors on xhr.abort().
-				if(aborted) return
-
 				if (xhr.readyState === 4) {
 					try {
 						var response = (args.extract !== extract) ? args.extract(xhr, args) : args.deserialize(args.extract(xhr, args))

(Patch is approximate, could turn into a PR with associated test fix if desired)

@bruce-one

bruce-one Mar 10, 2017

Contributor

If saving bytes is considered worth a change; I realised afterwards that the following patch is simpler (and smaller, hence mentioning it):


diff --git a/request/request.js b/request/request.js
index 552fa51..58d04a2 100644
--- a/request/request.js
+++ b/request/request.js
@@ -56,13 +56,13 @@ module.exports = function($window, Promise) {
 			else args.url = assemble(args.url, args.data)
 
 			var xhr = new $window.XMLHttpRequest(),
-				aborted = false,
 				_abort = xhr.abort
 
 
 			xhr.abort = function abort() {
-				aborted = true
+				// Don't throw errors on xhr.abort().
+				xhr.onreadystatechange = null
 				_abort.call(xhr)
 			}
 
 			xhr.open(args.method, args.url, typeof args.async === "boolean" ? args.async : true, typeof args.user === "string" ? args.user : undefined, typeof args.password === "string" ? args.password : undefined)
@@ -82,9 +82,6 @@ module.exports = function($window, Promise) {
 			if (typeof args.config === "function") xhr = args.config(xhr, args) || xhr
 
 			xhr.onreadystatechange = function() {
-				// Don't throw errors on xhr.abort().
-				if(aborted) return
-
 				if (xhr.readyState === 4) {
 					try {
 						var response = (args.extract !== extract) ? args.extract(xhr, args) : args.deserialize(args.extract(xhr, args))

(Patch is approximate, could turn into a PR with associated test fix if desired)

This comment has been minimized.

@simov

simov Mar 10, 2017

@pygy right, I was looking at the diff only so I wasn't aware of the actual code position.
I usually don't define such simple regular expressions as separate variables, but that might be a style preference. Also having a good syntax highlighting in your code editor would highlight the literal regular expression versus the string which will be highlighted as .. well as string :)
My initial example from few comments above was using inline literal regexp inside the event handler, so that would have made a performance difference .. I guess.
Also not having the regex as a separate variable would definitely save up some bytes, though that wasn't my point :)

@simov

simov Mar 10, 2017

@pygy right, I was looking at the diff only so I wasn't aware of the actual code position.
I usually don't define such simple regular expressions as separate variables, but that might be a style preference. Also having a good syntax highlighting in your code editor would highlight the literal regular expression versus the string which will be highlighted as .. well as string :)
My initial example from few comments above was using inline literal regexp inside the event handler, so that would have made a performance difference .. I guess.
Also not having the regex as a separate variable would definitely save up some bytes, though that wasn't my point :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment