Skip to main content

Bug Tracker

Side navigation

#9887 closed bug (fixed)

Opened July 22, 2011 01:07AM UTC

Closed July 23, 2011 12:10AM UTC

Last modified March 09, 2012 12:40AM UTC

core jQuery.ajaxSetup function may cause unnecessary memory usage and possible Out of stack space error

Reported by: rtumaykin Owned by: jaubourg
Priority: low Milestone: 1.6.3
Component: ajax Version: 1.6.2
Keywords: Cc:
Blocked by: Blocking:
Description

The bug is found in the function jQuery.ajaxSetup, jquery-1.6.2.js - lines 6901 - 6919

Before v 1.5 function jQuery.ajaxSetup used to merge custom settings for each call with the standard jQuery.ajaxSettings using a shallow copy, for example:

jQuery.extend( jQuery.ajaxSettings, settings );

Beginning with v 1.5 function jQuery.ajaxSetup started using deep copy (recursive) flag in it, for example:

jQuery.extend( true, target, jQuery.ajaxSettings, settings );

There are 2 problems with this approach.

First - when context is supplied within the settings, then if for example it is a "body" node of the document, then the deep copy would spend time making copy of an entire document into the target. And it is unnecessary, because one of the next steps in the original code goes and replaces the context and url with the references to the original objects. But until the copied data is not picked up by the garbage collector, it consumes memory, not even speaking of some extra resources spent for each ajax request.

Second - it is possible that as a context, an object containing circular references could be passed. That will lead to out of stack space errors.

I've created a simple jsfiddle case to demo the error.

http://jsfiddle.net/MBwUx/3/

It produces errors on at least IE8, Firefox 5, Chrome12. I have not tested on other browsers but in my opinion the problem will 100% reproduce in any browser because it is not a browser-specific issue.

In my jQuery copy I rewrote the function in the following way. Sorry I did not use GitHub:

/*  Creates a full fledged settings object into target
    with both ajaxSettings and settings fields.
    If target is omitted, writes into ajaxSettings.
*/
ajaxSetup: function ( target, settings ) {
    /*  Create an empty object to hold an independent copy of 
        jQuery.ajaxSettings if target is provided
    */
    
    var _ajaxSettings = {};

    /* no target provided --> assign settings and make jQuery.ajaxSettings a target */
    
    if (!settings) {
        settings = target;
        target = jQuery.ajaxSettings;
    }
    /*  else create a copy of jQuery.ajaxSettings so context and url 
        could be deleted from it without modifying jQuery.ajaxSettings itself
    */

    else {
        jQuery.extend(_ajaxSettings, jQuery.ajaxSettings);
    }
    /* Flatten fields we don't want deep extended */

    for (var field in { context: 1, url: 1 }) {
        if (field in settings) {
            target[field] = settings[field];
        } else if (field in _ajaxSettings) {
            target[field] = _ajaxSettings[field];
        }
        /* drop properties so they don't get involved in the jQuery.extend step */

        delete settings[field];
        delete _ajaxSettings[field];
    }

    jQuery.extend(true, target, _ajaxSettings, settings);

    return target;
},

Thank you.

Roman Tumaykin

Attachments (0)
Change History (9)

Changed July 22, 2011 01:43AM UTC by jaubourg comment:1

component: unfiledajax
description: The bug is found in the function jQuery.ajaxSetup, jquery-1.6.2.js - lines 6901 - 6919 \ \ Before v 1.5 function jQuery.ajaxSetup used to merge custom settings for each call with the standard jQuery.ajaxSettings using a shallow copy, for example: \ \ jQuery.extend( jQuery.ajaxSettings, settings ); \ \ Beginning with v 1.5 function jQuery.ajaxSetup started using deep copy (recursive) flag in it, for example: \ \ jQuery.extend( true, target, jQuery.ajaxSettings, settings ); \ \ There are 2 problems with this approach. \ \ First - when context is supplied within the settings, then if for example it is a "body" node of the document, then the deep copy would spend time making copy of an entire document into the target. And it is unnecessary, because one of the next steps in the original code goes and replaces the context and url with the references to the original objects. But until the copied data is not picked up by the garbage collector, it consumes memory, not even speaking of some extra resources spent for each ajax request. \ \ Second - it is possible that as a context, an object containing circular references could be passed. That will lead to out of stack space errors. \ \ I've created a simple jsfiddle case to demo the error. \ \ http://jsfiddle.net/MBwUx/3/ \ \ It produces errors on at least IE8, Firefox 5, Chrome12. I have not tested on other browsers but in my opinion the problem will 100% reproduce in any browser because it is not a browser-specific issue. \ \ In my jQuery copy I rewrote the function in the following way. Sorry I did not use GitHub: \ \ /* Creates a full fledged settings object into target \ with both ajaxSettings and settings fields. \ If target is omitted, writes into ajaxSettings. \ */ \ ajaxSetup: function ( target, settings ) { \ /* Create an empty object to hold an independent copy of \ jQuery.ajaxSettings if target is provided \ */ \ \ var _ajaxSettings = {}; \ \ /* no target provided --> assign settings and make jQuery.ajaxSettings a target */ \ \ if (!settings) { \ settings = target; \ target = jQuery.ajaxSettings; \ } \ /* else create a copy of jQuery.ajaxSettings so context and url \ could be deleted from it without modifying jQuery.ajaxSettings itself \ */ \ \ else { \ jQuery.extend(_ajaxSettings, jQuery.ajaxSettings); \ } \ /* Flatten fields we don't want deep extended */ \ \ for (var field in { context: 1, url: 1 }) { \ if (field in settings) { \ target[field] = settings[field]; \ } else if (field in _ajaxSettings) { \ target[field] = _ajaxSettings[field]; \ } \ /* drop properties so they don't get involved in the jQuery.extend step */ \ \ delete settings[field]; \ delete _ajaxSettings[field]; \ } \ \ jQuery.extend(true, target, _ajaxSettings, settings); \ \ return target; \ }, \ \ Thank you. \ \ Roman TumaykinThe bug is found in the function jQuery.ajaxSetup, jquery-1.6.2.js - lines 6901 - 6919 \ \ Before v 1.5 function jQuery.ajaxSetup used to merge custom settings for each call with the standard jQuery.ajaxSettings using a shallow copy, for example: \ \ jQuery.extend( jQuery.ajaxSettings, settings ); \ \ Beginning with v 1.5 function jQuery.ajaxSetup started using deep copy (recursive) flag in it, for example: \ \ jQuery.extend( true, target, jQuery.ajaxSettings, settings ); \ \ There are 2 problems with this approach. \ \ First - when context is supplied within the settings, then if for example it is a "body" node of the document, then the deep copy would spend time making copy of an entire document into the target. And it is unnecessary, because one of the next steps in the original code goes and replaces the context and url with the references to the original objects. But until the copied data is not picked up by the garbage collector, it consumes memory, not even speaking of some extra resources spent for each ajax request. \ \ Second - it is possible that as a context, an object containing circular references could be passed. That will lead to out of stack space errors. \ \ I've created a simple jsfiddle case to demo the error. \ \ http://jsfiddle.net/MBwUx/3/ \ \ It produces errors on at least IE8, Firefox 5, Chrome12. I have not tested on other browsers but in my opinion the problem will 100% reproduce in any browser because it is not a browser-specific issue. \ \ In my jQuery copy I rewrote the function in the following way. Sorry I did not use GitHub: \ {{{#!js \ /* Creates a full fledged settings object into target \ with both ajaxSettings and settings fields. \ If target is omitted, writes into ajaxSettings. \ */ \ ajaxSetup: function ( target, settings ) { \ /* Create an empty object to hold an independent copy of \ jQuery.ajaxSettings if target is provided \ */ \ \ var _ajaxSettings = {}; \ \ /* no target provided --> assign settings and make jQuery.ajaxSettings a target */ \ \ if (!settings) { \ settings = target; \ target = jQuery.ajaxSettings; \ } \ /* else create a copy of jQuery.ajaxSettings so context and url \ could be deleted from it without modifying jQuery.ajaxSettings itself \ */ \ \ else { \ jQuery.extend(_ajaxSettings, jQuery.ajaxSettings); \ } \ /* Flatten fields we don't want deep extended */ \ \ for (var field in { context: 1, url: 1 }) { \ if (field in settings) { \ target[field] = settings[field]; \ } else if (field in _ajaxSettings) { \ target[field] = _ajaxSettings[field]; \ } \ /* drop properties so they don't get involved in the jQuery.extend step */ \ \ delete settings[field]; \ delete _ajaxSettings[field]; \ } \ \ jQuery.extend(true, target, _ajaxSettings, settings); \ \ return target; \ }, \ }}} \ \ Thank you. \ \ Roman Tumaykin
milestone: None1.next
owner: → jaubourg
priority: undecidedlow
status: newassigned
type: bugenhancement

Please make a pull request with this on github :)

Changed July 22, 2011 01:45AM UTC by jaubourg comment:2

description: The bug is found in the function jQuery.ajaxSetup, jquery-1.6.2.js - lines 6901 - 6919 \ \ Before v 1.5 function jQuery.ajaxSetup used to merge custom settings for each call with the standard jQuery.ajaxSettings using a shallow copy, for example: \ \ jQuery.extend( jQuery.ajaxSettings, settings ); \ \ Beginning with v 1.5 function jQuery.ajaxSetup started using deep copy (recursive) flag in it, for example: \ \ jQuery.extend( true, target, jQuery.ajaxSettings, settings ); \ \ There are 2 problems with this approach. \ \ First - when context is supplied within the settings, then if for example it is a "body" node of the document, then the deep copy would spend time making copy of an entire document into the target. And it is unnecessary, because one of the next steps in the original code goes and replaces the context and url with the references to the original objects. But until the copied data is not picked up by the garbage collector, it consumes memory, not even speaking of some extra resources spent for each ajax request. \ \ Second - it is possible that as a context, an object containing circular references could be passed. That will lead to out of stack space errors. \ \ I've created a simple jsfiddle case to demo the error. \ \ http://jsfiddle.net/MBwUx/3/ \ \ It produces errors on at least IE8, Firefox 5, Chrome12. I have not tested on other browsers but in my opinion the problem will 100% reproduce in any browser because it is not a browser-specific issue. \ \ In my jQuery copy I rewrote the function in the following way. Sorry I did not use GitHub: \ {{{#!js \ /* Creates a full fledged settings object into target \ with both ajaxSettings and settings fields. \ If target is omitted, writes into ajaxSettings. \ */ \ ajaxSetup: function ( target, settings ) { \ /* Create an empty object to hold an independent copy of \ jQuery.ajaxSettings if target is provided \ */ \ \ var _ajaxSettings = {}; \ \ /* no target provided --> assign settings and make jQuery.ajaxSettings a target */ \ \ if (!settings) { \ settings = target; \ target = jQuery.ajaxSettings; \ } \ /* else create a copy of jQuery.ajaxSettings so context and url \ could be deleted from it without modifying jQuery.ajaxSettings itself \ */ \ \ else { \ jQuery.extend(_ajaxSettings, jQuery.ajaxSettings); \ } \ /* Flatten fields we don't want deep extended */ \ \ for (var field in { context: 1, url: 1 }) { \ if (field in settings) { \ target[field] = settings[field]; \ } else if (field in _ajaxSettings) { \ target[field] = _ajaxSettings[field]; \ } \ /* drop properties so they don't get involved in the jQuery.extend step */ \ \ delete settings[field]; \ delete _ajaxSettings[field]; \ } \ \ jQuery.extend(true, target, _ajaxSettings, settings); \ \ return target; \ }, \ }}} \ \ Thank you. \ \ Roman TumaykinThe bug is found in the function jQuery.ajaxSetup, jquery-1.6.2.js - lines 6901 - 6919 \ \ Before v 1.5 function jQuery.ajaxSetup used to merge custom settings for each call with the standard jQuery.ajaxSettings using a shallow copy, for example: \ \ jQuery.extend( jQuery.ajaxSettings, settings ); \ \ Beginning with v 1.5 function jQuery.ajaxSetup started using deep copy (recursive) flag in it, for example: \ \ jQuery.extend( true, target, jQuery.ajaxSettings, settings ); \ \ There are 2 problems with this approach. \ \ First - when context is supplied within the settings, then if for example it is a "body" node of the document, then the deep copy would spend time making copy of an entire document into the target. And it is unnecessary, because one of the next steps in the original code goes and replaces the context and url with the references to the original objects. But until the copied data is not picked up by the garbage collector, it consumes memory, not even speaking of some extra resources spent for each ajax request. \ \ Second - it is possible that as a context, an object containing circular references could be passed. That will lead to out of stack space errors. \ \ I've created a simple jsfiddle case to demo the error. \ \ http://jsfiddle.net/MBwUx/3/ \ \ It produces errors on at least IE8, Firefox 5, Chrome12. I have not tested on other browsers but in my opinion the problem will 100% reproduce in any browser because it is not a browser-specific issue. \ \ In my jQuery copy I rewrote the function in the following way. Sorry I did not use GitHub: \ {{{#!js \ /* Creates a full fledged settings object into target \ with both ajaxSettings and settings fields. \ If target is omitted, writes into ajaxSettings. \ */ \ ajaxSetup: function ( target, settings ) { \ /* Create an empty object to hold an independent copy of \ jQuery.ajaxSettings if target is provided \ */ \ \ var _ajaxSettings = {}; \ \ /* no target provided --> assign settings and make jQuery.ajaxSettings a target */ \ \ if (!settings) { \ settings = target; \ target = jQuery.ajaxSettings; \ } \ /* else create a copy of jQuery.ajaxSettings so context and url \ could be deleted from it without modifying jQuery.ajaxSettings itself \ */ \ \ else { \ jQuery.extend(_ajaxSettings, jQuery.ajaxSettings); \ } \ /* Flatten fields we don't want deep extended */ \ \ for (var field in { context: 1, url: 1 }) { \ if (field in settings) { \ target[field] = settings[field]; \ } else if (field in _ajaxSettings) { \ target[field] = _ajaxSettings[field]; \ } \ /* drop properties so they don't get involved in the jQuery.extend step */ \ \ delete settings[field]; \ delete _ajaxSettings[field]; \ } \ \ jQuery.extend(true, target, _ajaxSettings, settings); \ \ return target; \ }, \ }}} \ \ Thank you. \ \ Roman Tumaykin

Changed July 22, 2011 01:51AM UTC by jaubourg comment:3

A couple observations:

  • ajaxSetup must not modify the settings object
  • if you really wish to gain in perf, you may want to avoid using target in the deep extend when it is not provided (because you basically deep extend ajaxSettings into itself)
  • also, dom nodes are never iterated by a deep extend (they're not plain objects), so the argument about the body element being iterated over is irrelevant since it will never happen

Also, some jsperf test would be nice together with the pull request.

Changed July 22, 2011 06:18PM UTC by rtumaykin comment:4

I will need to learn how to use github - will do this over this weekend. I will modify the code to not modify the original settings object. Regarding the target in the deep extend - my code actually keeps _ajaxSettings = {} if the target is jQuery.ajaxSettings itself.

Thanks for your comments. I will post here when I am done with the pull request.

Roman

Changed July 22, 2011 06:20PM UTC by dmethvin comment:5

#8464 is a duplicate of this ticket.

Changed July 22, 2011 11:31PM UTC by jaubourg comment:6

type: enhancementbug

Changed July 23, 2011 12:10AM UTC by jaubourg comment:7

resolution: → fixed
status: assignedclosed

Fixes #9887: ajax now supports circular references into objects passed as context. Prefilter and transport developpers should add their own custom option into flatOptions when needed. Unit test added.

Changeset: e6a99fdb0ee9e0fd7552d5de8bc4acbe982e98b7

Changed July 23, 2011 12:40AM UTC by jaubourg comment:8

milestone: 1.next1.6.3

Changed July 23, 2011 05:21AM UTC by rtumaykin comment:9

Very nice solution. Thank you!

Roman