jQuery.ajax uncaught Exception for jsonp calls to 404 error page even though response header Content-Type: text/html is present

[codefactor]

Description

Upgrading from version 2 to version 3 of jQuery we see uncaught exceptions being thrown in unit test cases that cause certain unit tests to fail due to these uncaught exceptions. This causes is a backwards incompatibility to adopt version 3 we need to hack our unit test to ignore certain uncaught exceptions or otherwise detect the test environment to avoid requests to URLs that don't work in this environment. It would be good if we can fix that incompatibility since it seems easily avoidable.

Steps to Reproduce:

Run something like this:

jQuery.ajax({ url: "./someInvalidUrl", dataType: "jsonp" });

Assume the URL goes to some place which responds with 404 status and a Content-Type: text/html response header and responseText with HTML content.

Expected Behavior:

No uncaught exceptions should be observed.

Actual Behavior:

An uncaught exception is thrown attempting to evaluate the response text, which is not a script but HTML content.

Uncaught SyntaxError: Unexpected token '<'
    at b (jquery.min.js:2:866)
    at Function.globalEval (jquery.min.js:2:2905)
    at text script (jquery.min.js:2:83080)
    at jquery.min.js:2:79470
    at l (jquery.min.js:2:79587)
    at XMLHttpRequest.<anonymous> (jquery.min.js:2:82355)

Suggested Solution:

Do not attempt to DOMEval a 404 response during jsonp evaluation, an additional clue that this response is not a script is the content type response header. Either one should be sufficient.

Link to test case

Failure in jQuery 3.X: https://jsfiddle.net/5o94ujsL/
Same thin in jQuery 2.X: https://jsfiddle.net/m3dp2zbt/

In both cases the global error handler should not be invoked, but we see in jquery 3 that the global error handler is invoked because of the following stack trace:

Uncaught SyntaxError: Unexpected token '<'
    at b (jquery.min.js:2:866)
    at Function.globalEval (jquery.min.js:2:2905)
    at text script (jquery.min.js:2:83080)
    at jquery.min.js:2:79470
    at l (jquery.min.js:2:79587)
    at XMLHttpRequest.<anonymous> (jquery.min.js:2:82355)

[codefactor]

Looking at the code, I might suggest some changes to the following lines:

jquery/src/ajax.js

Lines 272 to 279 in 5d5ea01

	try {
	response = conv( response );
	} catch ( e ) {
	return {
	state: "parsererror",
	error: conv ? e : "No conversion from " + prev + " to " + current
	};
	}

At this place I think there is enough information in the response and the isSuccess (which is false) to avoid calling conv() which we know is only going to result in an exception, if you ignore the unlikely scenario that the server is returning Javascript with an incorrectly typed Content-Type header on an unsuccessful response (such as 404 or 500) which was meant to be interpretted as application/javascript and passed along to the failure callbacks. I think that if the server meant to do that it should be responsible to set the content type header correctly.

One other thought - I haven't looked too much into the jQuery 2.0 version, but it might be a good idea to check what was going on there to see what was the solution there that would prevent this kind of error from being uncaught.

[dmethvin]

This is reminiscent of #4655

[codefactor]

@dmethvin ,
Thanks for the link - it does look very similar to that one. In this case, however, the dataType is "jsonp" and not "script" - so I wonder if that is a key difference.

Also the other errors says that the errorHandler is NOT called, and I'm fairly sure that in this test case the error handler is called; however, the issue is that there is still an uncaught exception that will fail most default QUnit test cases that attempt.

[mgol]

@codefactor Thanks for the report. There's actually no difference in converter code; converters also run in jQuery 2.x. This is a result of one of fundamental assumptions of jQuery.ajax - if you pass dataType, you decide to ignore the content type the server returns and you decide to always parse the response using the provided dataType.

What's different is that in jQuery 2.x jQuery.globalEval only uses script tags for scripts with "use strict" being at the index 1 - this was needed due to differences in how indirect eval works in strict mode (see #1449 for more info) - and in other cases an indirect eval is used. That indirect eval then throws an error because the response is not a valid script and the code gets to the catch from your snippet.

In jQuery 3.0.0 we decided to simplify the logic and always use script injection, with the caveat errors can only be detected via window.onerror. Again, see #1449 for more info.

Interestingly, we didn't cover that in the upgrade guide: https://jquery.com/upgrade-guide/3.0/. We should address that part.

@dmethvin #4655 is tangential to this issue; see also the followup in #4773 (and also #4778 but that one will only apply to >=4.0.0).

[timmywil]

if you pass dataType, you decide to ignore the content type the server returns and you decide to always parse the response using the provided dataType.

I'm fine with this for successful responses, but it seems like this should not be the case for error responses. Problem is I don't yet know the implications of that opinion.

[mgol]

We'll re-evaluate this for 5.0.0. It has a potential of breaking existing code so it needs to land in a major version update and we're trying to not add more stuff to 4.0.0 as we want to release a beta in some predictable future.