Bug Tracker

Modify

Ticket #9887 (closed bug: fixed)

Opened 3 years ago

Last modified 2 years ago

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:
Blocking: Blocked by:

Description (last modified by jaubourg) (diff)

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

Change History

comment:1 Changed 3 years ago by jaubourg

  • Status changed from new to assigned
  • Description modified (diff)
  • Component changed from unfiled to ajax
  • Priority changed from undecided to low
  • Milestone changed from None to 1.next
  • Owner set to jaubourg
  • Type changed from bug to enhancement

Please make a pull request with this on github :)

comment:2 Changed 3 years ago by jaubourg

  • Description modified (diff)

comment:3 Changed 3 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 3 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.

Roman

comment:5 Changed 3 years ago by dmethvin

#8464 is a duplicate of this ticket.

comment:6 Changed 3 years ago by jaubourg

  • Type changed from enhancement to bug

comment:7 Changed 3 years ago by jaubourg

  • Status changed from assigned to closed
  • Resolution set to fixed

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 3 years ago by jaubourg

  • Milestone changed from 1.next to 1.6.3

comment:9 Changed 3 years ago by rtumaykin

Very nice solution. Thank you!

Roman

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.