Bug Tracker

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#9887 closed bug (fixed)

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 (last modified by jaubourg)

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.


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

Change History (9)

comment:1 Changed 12 years ago by jaubourg

Component: unfiledajax
Description: modified (diff)
Milestone: None1.next
Owner: set to jaubourg
Priority: undecidedlow
Status: newassigned
Type: bugenhancement

Please make a pull request with this on github :)

comment:2 Changed 12 years ago by jaubourg

Description: modified (diff)

comment:3 Changed 12 years ago by jaubourg

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.

comment:4 Changed 12 years ago by rtumaykin

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.


comment:5 Changed 12 years ago by dmethvin

#8464 is a duplicate of this ticket.

comment:6 Changed 12 years ago by jaubourg

Type: enhancementbug

comment:7 Changed 12 years ago by jaubourg

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

comment:8 Changed 12 years ago by jaubourg

Milestone: 1.next1.6.3

comment:9 Changed 12 years ago by rtumaykin

Very nice solution. Thank you!


Note: See TracTickets for help on using tickets.