Permalink
Browse files

Fix #11743: Don't mask script errors in jQuery.ajax, closes gh-795.

  • Loading branch information...
1 parent 2d37b6c commit 742872984e000ff8e13b9a23e74852d1b549f161 @gibson042 gibson042 committed with dmethvin May 31, 2012
Showing with 126 additions and 90 deletions.
  1. +85 −86 src/ajax.js
  2. +4 −3 src/manipulation.js
  3. +1 −1 test/data/badjson.js
  4. +22 −0 test/unit/ajax.js
  5. +14 −0 test/unit/manipulation.js
View
@@ -319,6 +319,7 @@ jQuery.extend({
username: null,
password: null,
cache: null,
+ throws: false,
traditional: false,
headers: {},
*/
@@ -480,6 +481,8 @@ jQuery.extend({
// It is defined here because jslint complains if it is declared
// at the end of the function (which would be more logical and readable)
function done( status, nativeStatusText, responses, headers ) {
+ var isSuccess, success, error, response, modified,
+ statusText = nativeStatusText;
// Called once
if ( state === 2 ) {
@@ -504,25 +507,24 @@ jQuery.extend({
// Set readyState
jqXHR.readyState = status > 0 ? 4 : 0;
- var isSuccess,
- success,
- error,
- statusText = nativeStatusText,
- response = responses ? ajaxHandleResponses( s, jqXHR, responses ) : undefined,
- lastModified,
- etag;
+ // Get response data
+ if ( responses ) {
+ response = ajaxHandleResponses( s, jqXHR, responses );
+ }
// If successful, handle type chaining
if ( status >= 200 && status < 300 || status === 304 ) {
// Set the If-Modified-Since and/or If-None-Match header, if in ifModified mode.
if ( s.ifModified ) {
- if ( ( lastModified = jqXHR.getResponseHeader( "Last-Modified" ) ) ) {
- jQuery.lastModified[ ifModifiedKey ] = lastModified;
+ modified = jqXHR.getResponseHeader("Last-Modified");
+ if ( modified ) {
+ jQuery.lastModified[ ifModifiedKey ] = modified;
}
- if ( ( etag = jqXHR.getResponseHeader( "Etag" ) ) ) {
- jQuery.etag[ ifModifiedKey ] = etag;
+ modified = jqXHR.getResponseHeader("Etag");
+ if ( modified ) {
+ jQuery.etag[ ifModifiedKey ] = modified;
}
}
@@ -535,15 +537,11 @@ jQuery.extend({
// If we have data
} else {
- try {
- success = ajaxConvert( s, response );
- statusText = "success";
- isSuccess = true;
- } catch(e) {
- // We have a parsererror
- statusText = "parsererror";
- error = e;
- }
+ isSuccess = ajaxConvert( s, response );
+ statusText = isSuccess.state;
+ success = isSuccess.data;
+ error = isSuccess.error;
+ isSuccess = !error;
}
} else {
// We extract error from statusText
@@ -913,86 +911,87 @@ function ajaxHandleResponses( s, jqXHR, responses ) {
// Chain conversions given the request and the original response
function ajaxConvert( s, response ) {
+ var conv, conv2, current, tmp,
+ // Work with a copy of dataTypes in case we need to modify it for conversion
+ dataTypes = s.dataTypes.slice(),
+ prev = dataTypes[ 0 ],
+ converters = {},
+ i = 0;
+
// Apply the dataFilter if provided
if ( s.dataFilter ) {
response = s.dataFilter( response, s.dataType );
}
- var dataTypes = s.dataTypes,
- converters = {},
- i,
- key,
- length = dataTypes.length,
- tmp,
- // Current and previous dataTypes
- current = dataTypes[ 0 ],
- prev,
- // Conversion expression
- conversion,
- // Conversion function
- conv,
- // Conversion functions (transitive conversion)
- conv1,
- conv2;
-
- // For each dataType in the chain
- for ( i = 1; i < length; i++ ) {
-
- // Create converters map
- // with lowercased keys
- if ( i === 1 ) {
- for ( key in s.converters ) {
- if ( typeof key === "string" ) {
- converters[ key.toLowerCase() ] = s.converters[ key ];
- }
- }
+ // Create converters map with lowercased keys
+ if ( dataTypes[ 1 ] ) {
+ for ( conv in s.converters ) {
+ converters[ conv.toLowerCase() ] = s.converters[ conv ];
}
+ }
+
+ // Convert to each sequential dataType, tolerating list modification
+ for ( ; (current = dataTypes[++i]); ) {
+
+ // There's only work to do if current dataType is non-auto
+ if ( current !== "*" ) {
+
+ // Convert response if prev dataType is non-auto and differs from current
+ if ( prev !== "*" && prev !== current ) {
+
+ // Seek a direct converter
+ conv = converters[ prev + " " + current ] || converters[ "* " + current ];
- // Get the dataTypes
- prev = current;
- current = dataTypes[ i ];
-
- // If current is auto dataType, update it to prev
- if ( current === "*" ) {
- current = prev;
- // If no auto and dataTypes are actually different
- } else if ( prev !== "*" && prev !== current ) {
-
- // Get the converter
- conversion = prev + " " + current;
- conv = converters[ conversion ] || converters[ "* " + current ];
-
- // If there is no direct converter, search transitively
- if ( !conv ) {
- conv2 = undefined;
- for ( conv1 in converters ) {
- tmp = conv1.split( " " );
- if ( tmp[ 0 ] === prev || tmp[ 0 ] === "*" ) {
- conv2 = converters[ tmp[1] + " " + current ];
- if ( conv2 ) {
- conv1 = converters[ conv1 ];
- if ( conv1 === true ) {
- conv = conv2;
- } else if ( conv2 === true ) {
- conv = conv1;
+ // If none found, seek a pair
+ if ( !conv ) {
+ for ( conv2 in converters ) {
+
+ // If conv2 outputs current
+ tmp = conv2.split(" ");
+ if ( tmp[ 1 ] === current ) {
+
+ // If prev can be converted to accepted input
+ conv = converters[ prev + " " + tmp[ 0 ] ] ||
+ converters[ "* " + tmp[ 0 ] ];
+ if ( conv ) {
+ // Condense equivalence converters
+ if ( conv === true ) {
+ conv = converters[ conv2 ];
+
+ // Otherwise, insert the intermediate dataType
+ } else if ( converters[ conv2 ] !== true ) {
+ current = tmp[ 0 ];
+ dataTypes.splice( i--, 0, current );
+ }
+
+ break;
}
- break;
+ }
+ }
+ }
+
+ // Apply converter (if not an equivalence)
+ if ( conv !== true ) {
+
+ // Unless errors are allowed to bubble, catch and return them
+ if ( conv && s.throws ) {
+ response = conv( response );
+ } else {
+ try {
+ response = conv( response );
+ } catch ( e ) {
+ return { state: "parsererror", error: conv ? e : "No conversion from " + prev + " to " + current };
}
}
}
}
- // If we found no converter, dispatch an error
- if ( !( conv || conv2 ) ) {
- jQuery.error( "No conversion from " + conversion.replace(" "," to ") );
- }
- // If found converter is not an equivalence
- if ( conv !== true ) {
- // Convert with 1 or 2 converters accordingly
- response = conv ? conv( response ) : conv2( conv1(response) );
- }
+
+ // Update prev for next iteration
+ prev = current;
}
}
- return response;
+
+ return { state: "success", data: response };
}
})( jQuery );
View
@@ -343,11 +343,12 @@ jQuery.fn.extend({
jQuery.each( scripts, function( i, elem ) {
if ( elem.src ) {
jQuery.ajax({
- type: "GET",
- global: false,
url: elem.src,
+ type: "GET",
+ dataType: "script",
async: false,
- dataType: "script"
+ global: false,
+ throws: true
});
} else {
jQuery.globalEval( ( elem.text || elem.textContent || elem.innerHTML || "" ).replace( rcleanScript, "" ) );
View
@@ -1 +1 @@
-{bad: 1}
+{bad: toTheBone}
View
@@ -1746,6 +1746,28 @@ test("jQuery.ajax() - malformed JSON", function() {
});
});
+test("jQuery.ajax() - script, throws exception (#11743)", function() {
+ expect(1);
+
+ raises(function() {
+ jQuery.ajax({
+ url: "data/badjson.js",
+ dataType: "script",
+ throws: true,
+ // TODO find a way to test this asynchronously, too
+ async: false,
+ // Global events get confused by the exception
+ global: false,
+ success: function() {
+ ok( false, "Success." );
+ },
+ error: function() {
+ ok( false, "Error." );
+ }
+ });
+ }, "exception bubbled" );
+});
+
test("jQuery.ajax() - script by content-type", function() {
expect(2);
View
@@ -1796,3 +1796,17 @@ test("Ensure oldIE creates a new set on appendTo (#8894)", function() {
strictEqual( jQuery("<bdi/>").clone().addClass("test").appendTo("<div/>").end().hasClass("test"), false, "Check jQuery.fn.appendTo after clone html5 element" );
strictEqual( jQuery("<p/>").appendTo("<div/>").end().length, jQuery("<p>test</p>").appendTo("<div/>").end().length, "Elements created with createElement and with createDocumentFragment should be treated alike" );
});
+
+test("html() - script exceptions bubble (#11743)", function() {
+ expect(2);
+
+ raises(function() {
+ jQuery("#qunit-fixture").html("<script>undefined(); ok( false, 'error not thrown' );</script>");
+ ok( false, "error ignored" );
+ }, "exception bubbled from inline script" );
+
+ raises(function() {
+ jQuery("#qunit-fixture").html("<script src='data/badjson.js'></script>");
+ ok( false, "error ignored" );
+ }, "exception bubbled from remote script" );
+});

3 comments on commit 7428729

Member

Krinkle replied Jun 3, 2012

As of this commit there is a new failure in IE9 in TestSwarm.

ajax:
ERROR: 'toTheBone' is undefined

manipulation: html() - script exceptions bubble (#11743)
- false: error ignored
- false: exception bubbled from remote script

ERROR: Object expected
Member

gibson042 replied Jun 3, 2012

It's either a problem with QUnit or the test environment, because this works for me in a live browser. Could raises be suffering from bad interaction with globalEval?

Member

gibson042 replied Jun 8, 2012

Looks like that was exactly the problem, but I have no idea how I saw success before. jquery/qunit#258

Please sign in to comment.