Skip to content
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

JSONP and parsing responseText upon 409 Conflict #4771

Closed
fras2560 opened this issue Aug 18, 2020 · 4 comments · Fixed by #4773
Closed

JSONP and parsing responseText upon 409 Conflict #4771

fras2560 opened this issue Aug 18, 2020 · 4 comments · Fixed by #4773

Comments

@fras2560
Copy link
Contributor

Bug Reports:
Unable to parse responseText of JSONP in a error handler.

  • What do you expect to happen? Able to parse the JSON
  • What actually happens? Get a parsing error
  • Which browsers are affected - Chrome/Firefox?

Currently using Backbone and JQuery. Recently have upgraded from 3.3.1 to 3.5.1. Essentially, backbone is using a jsonp request to delete a model but expecting a 409 due to the model being unable to be deleted. It works as expected and end up in the error handler but the responseJson is not present. I have walked through the stack trace difference between 3.3.1 & 3.5.1. The reason seems to be a parsing error:

error: Error: jQuery35109401972421271656_1597775421415 was not called at Function.error (3.5.1/jquery.js:334:9) at s.converters.script json (3.5.1/jquery.js:10232:12) at ajaxConvert (jquery/3.5.1/jquery.js:9260:19) at done (3.5.1/jquery.js:9736:15) at XMLHttpRequest.<anonymous> (3.5.1/jquery.js:10047:9) message: "jQuery35109401972421271656_1597775421415 was not called" stack: "Error: jQuery35109401972421271656_1597775421415 was not called state: "parsererror"

From walking through the stacktrace it seems related to - 50871a5

@mgol
Copy link
Member

mgol commented Aug 18, 2020

Thanks for the report.

We considered #4379 to be a bug fix but JSONP is a bit special, indeed - under the hood it's a script but it simulates JSON responses in an environment without a CORS setup and sending JSON payloads on error responses is quite typical there. Maybe we need to revisit the JSONP case.

It'd help if you could provide a minimal test case or - even better - a test case for the jQuery test suite, something like

jquery/test/unit/ajax.js

Lines 809 to 922 in 82b87f6

ajaxTest( "jQuery.ajax() - do not execute scripts from unsuccessful responses (gh-4250)", 11, function( assert ) {
var globalEval = jQuery.globalEval;
var failConverters = {
"text script": function() {
assert.ok( false, "No converter for unsuccessful response" );
}
};
function request( title, options ) {
var testMsg = title + ": expected file missing status";
return jQuery.extend( {
beforeSend: function() {
jQuery.globalEval = function() {
assert.ok( false, "Should not eval" );
};
},
complete: function() {
jQuery.globalEval = globalEval;
},
// error is the significant assertion
error: function( xhr ) {
assert.strictEqual( xhr.status, 404, testMsg );
},
success: function() {
assert.ok( false, "Unanticipated success" );
}
}, options );
}
return [
request(
"HTML reply",
{
url: url( "404.txt" )
}
),
request(
"HTML reply with dataType",
{
dataType: "script",
url: url( "404.txt" )
}
),
request(
"script reply",
{
url: url( "mock.php?action=errorWithScript&withScriptContentType" )
}
),
request(
"non-script reply",
{
url: url( "mock.php?action=errorWithScript" )
}
),
request(
"script reply with dataType",
{
dataType: "script",
url: url( "mock.php?action=errorWithScript&withScriptContentType" )
}
),
request(
"non-script reply with dataType",
{
dataType: "script",
url: url( "mock.php?action=errorWithScript" )
}
),
request(
"script reply with converter",
{
converters: failConverters,
url: url( "mock.php?action=errorWithScript&withScriptContentType" )
}
),
request(
"non-script reply with converter",
{
converters: failConverters,
url: url( "mock.php?action=errorWithScript" )
}
),
request(
"script reply with converter and dataType",
{
converters: failConverters,
dataType: "script",
url: url( "mock.php?action=errorWithScript&withScriptContentType" )
}
),
request(
"non-script reply with converter and dataType",
{
converters: failConverters,
dataType: "script",
url: url( "mock.php?action=errorWithScript" )
}
),
request(
"JSONP reply with dataType",
{
dataType: "jsonp",
url: url( "mock.php?action=errorWithScript" ),
beforeSend: function() {
jQuery.globalEval = function( response ) {
assert.ok( /"status": 404, "msg": "Not Found"/.test( response ), "Error object returned" );
};
}
}
)
];
} );
but for JSONP & a different expected result. Would you want to try that?

@mgol mgol added Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. Ajax labels Aug 18, 2020
@fras2560
Copy link
Contributor Author

@mgol Thanks for the quick response. I will give it a try to make a test case or at the very least a minimal test case. I hope I correctly described the situation but will confirm for sure (bit complicated with Backbone, Server, and JQuery).

@mgol
Copy link
Member

mgol commented Aug 18, 2020 via email

@fras2560
Copy link
Contributor Author

@mgol I think I have fix for the issue and wrote up a simple test. However, I am not too familar with PHP so I might need some help with this. I am not even sure how to run it locally. Also, need some help with response body

#4773

@mgol mgol added this to the 3.6.0 milestone Aug 22, 2020
@timmywil timmywil removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Aug 24, 2020
mgol pushed a commit that referenced this issue Aug 25, 2020
Issue gh-4379 was meant to be a bug fix but the JSONP case is a bit special:
under the hood it's a script but it simulates JSON responses in an environment
without a CORS setup and sending JSON payloads on error responses is quite
typical there.

This commit makes JSONP error responses still execute the payload. The regular
script error responses continue to be skipped.

Fixes gh-4771
Closes gh-4773
mgol pushed a commit that referenced this issue Aug 25, 2020
Issue gh-4379 was meant to be a bug fix but the JSONP case is a bit special:
under the hood it's a script but it simulates JSON responses in an environment
without a CORS setup and sending JSON payloads on error responses is quite
typical there.

This commit makes JSONP error responses still execute the payload. The regular
script error responses continue to be skipped.

Fixes gh-4771
Closes gh-4773

(cherry picked from commit a1e619b)
mgol added a commit to mgol/jquery that referenced this issue Aug 26, 2020
Don't use a script tag for JSONP requests unless for cross-domain requests
or if scriptAttrs are provided. This makes the `responseJSON` property available
in JSONP error callbacks.

This fixes a regression from jQuery 3.5.0 introduced in jquerygh-4379 which made
erroneous script responses to not be executed to follow native behavior.

The 3.x-stable branch doesn't need this fix as it doesn't use script tags for
regular async requests.

Ref jquerygh-4771
Ref jquerygh-4773
Ref jquerygh-4379
mgol added a commit that referenced this issue Aug 31, 2020
Don't use a script tag for JSONP requests unless for cross-domain requests
or if scriptAttrs are provided. This makes the `responseJSON` property available
in JSONP error callbacks.

This fixes a regression from jQuery 3.5.0 introduced in gh-4379 which made
erroneous script responses to not be executed to follow native behavior.

The 3.x-stable branch doesn't need this fix as it doesn't use script tags for
regular async requests.

Closes gh-4778
Ref gh-4771
Ref gh-4773
Ref gh-4379
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants