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.
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 comment:1
component: | unfiled → ajax |
---|---|
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 → 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 Tumaykin |
milestone: | None → 1.next |
owner: | → jaubourg |
priority: | undecided → low |
status: | new → assigned |
type: | bug → enhancement |
Changed July 22, 2011 01:45AM UTC by 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 Tumaykin → 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 Tumaykin |
---|
Changed July 22, 2011 01:51AM UTC by 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 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 11:31PM UTC by comment:6
type: | enhancement → bug |
---|
Changed July 23, 2011 12:10AM UTC by comment:7
resolution: | → fixed |
---|---|
status: | assigned → closed |
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 comment:8
milestone: | 1.next → 1.6.3 |
---|
Changed July 23, 2011 05:21AM UTC by comment:9
Very nice solution. Thank you!
Roman
Please make a pull request with this on github :)