Skip to main content

Bug Tracker

Side navigation

#12404 closed bug (notabug)

Opened August 27, 2012 06:33AM UTC

Closed October 16, 2012 07:01PM UTC

.post() submits expando

Reported by: crimar Owned by: crimar
Priority: low Milestone: None
Component: ajax Version: 1.8.0
Keywords: Cc:
Blocked by: Blocking:
Description

From a discussion about jsviews:

I looked into the issue with jQuery post, and you are right, they are including the jQuery expando property in the deep clone.

A quick fix is to change line 326:

if ( target === copy ) { 

to

if ( target === copy || copy && copy.toJSON && !copy.toJSON()) {

(https://github.com/BorisMoore/jsviews/issues/99#issuecomment-8011070)

Attachments (0)
Change History (13)

Changed August 27, 2012 01:24PM UTC by dmethvin comment:1

owner: → crimar
status: newpending

Can you provide a test case, preferably in jsFiddle.net or jsbin.com? I'd like to understand the use case and this may be affected by our deprecations/removals in 1.9 as well.

Changed August 27, 2012 04:21PM UTC by BorisMoore comment:2

_comment0: Hi Dave, thanks for looking at this. \ \ Hopefully crimar can provide a test case, - and if not I can look at doing so.) \ \ The use cases are when you use jQuery event binding or $.data(object, ...) on a plain JS object, and then submit that object as an AJAX POST. Depending on what data you set, and what objects are referenced from the event object (e.g, data passed in $(object).on(...) call) this at best pollutes the POST data, and at worst leads to a stack overflow, as a result of deep-copying the jQuery expando. \ \ This affects any app (or framework such as JsViews) that does event binding or jQuery data on plain objects. JsViews could if necessary use its own event subscription mechanism, rather than jQuery events, but even then it would need to store objects in a separate cache, (like jQuery does for elements) rather than referencing directly from an expando, since jQuery currently ignores the toJSON on attached object, during Ajax Post.1346084570441753

Hi Dave, thanks for looking at this.

Hopefully crimar can provide a test case. (If not I can look at doing so.)

The use cases are when you use jQuery event binding or $.data(object, ...) on a plain JS object, and then submit that object as an AJAX POST. Depending on what data you set, and what objects are referenced from the event object (e.g, data passed in $(object).on(...) call) this at best pollutes the POST data, and at worst leads to a stack overflow, as a result of deep-copying the jQuery expando.

This affects any app (or framework such as JsViews) that does event binding or jQuery data on plain objects. JsViews could if necessary use its own event subscription mechanism, rather than jQuery events, but even then it would need to store objects in a separate cache, (like jQuery does for elements) rather than referencing directly from an expando, since jQuery currently ignores the toJSON on attached object, during Ajax Post.

Changed August 27, 2012 05:17PM UTC by dmethvin comment:3

Hi Boris! We've never documented the whole .toJSON data hack, and the original reason we supported the .data API for plain objects was because we needed it indirectly for using events with the jquery-datalink plugin. As a result we *had* to document that it adds that expando (See "Working with plain objects" at http://api.jquery.com/jQuery/) but I would prefer not to formalize support beyond what we already do there because it is a rare specially special case.

Our goal with 1.9 is to remove all the special cases in the .data() method and we would no longer fire events on data modification. We never documented that behavior and it's very quirky. I don't know how that might affect your code but if it does we can discuss solutions.

Also, as far as serializing data there are a couple of open proposals regarding that to allow you to filter or change what gets sent. See #12134 and #12264. If they might fit a use case you should comment on them. Ticket #12227 proposes adding more overhead to $.extend to avoid cycles but it seems like such a shame to do that.

Changed August 28, 2012 06:59AM UTC by crimar comment:4

status: pendingnew

I couldn't find a way to show this with jsfiddle. Logging the submitted parameters on the server of the following shows the expando:

$(document).on("ready", function() {
	var data = { foo : "bar" };
	$(data).on("baz", function() {});
	$.post("test", data);
});

Changed August 28, 2012 12:11PM UTC by dmethvin comment:5

component: unfiledajax
priority: undecidedlow
status: newopen

That would definitely submit the expando, but again I don't think this is a common use case at all. Using the .toJSON hack on the object doesn't appeal to me at all. The proposed tickets above might provide a cleaner way to handle the expando, but for now you can just temporarily remove it before posting.

Changed August 28, 2012 12:22PM UTC by jaubourg comment:6

It seems very innefficient and potentially very dangerous to me to just push your live in-memory data on the wire like the use-case implies. At the very least, any decent data binding system with client-server communication will just push the differences over the wire.

Changed August 28, 2012 05:23PM UTC by BorisMoore comment:7

(Hi Julian!)

First, to clarify:

  • I wasn't suggesting the 'quick fix' above ('toJSON hack') was the correct fix. (It is literally just a quick fix).
  • On the scenario, the data you are posting may of course be something other than some data which you originally got from an ajax request, and may indeed be a 'delta' object containing only the diff of client changes on the original data. But the issue is that you had better at no point have attached any kind of event handler to that 'delta' object, or used $.data(obj,...) on it. Or if you do, you must know to remove the jQuery expando from the object before posting.

Here http://jsfiddle.net/BorisMoore/DF48v/ is a test case which shows stack overflow situations. In reality the circular reference through deep copying can occur in many subtle ways...

One problem here is that the user who is posting the data may be using a library (such as JsViews, or simply jquery.observable) and not realize that they are as a result adding jQuery expandos to their posted data. If the same developer owns the whole stack it is much more reasonable to expect them to avoid this situation.

On future plans to stop supporting $.data and event binding on objects: that will of course prevent jquery.observable (and any other equivalent library) from working, and so may affect the planned jQuery UI Grid and data-aware widget plans. But I can understand you feeling about 'quirkiness'. For JsViews, I would in that case be forced to create my own implementation for 'observable objects and arrays' using my own eventing or maybe building something using jQuery callbacks...

Changed August 29, 2012 02:38PM UTC by dmethvin comment:8

There are some great ES5/6 features that support these kind of scenarios well, so one obstruction is whether you need oldIE support or not. The tickets I mentioned above for filtering serialization might do the trick for this case though, and if so they'd let us avoid a bunch of special case code.

I've been going through quite a bit of public jQuery code lately and the "features" like data events are serious performance killers in many cases. Almost nobody uses them, but they impose a 10x penalty on $.fn.data versus $.data where they're not fired. As a result, the jQuery Mobile guys for example are having to go through and rewrite code to avoid it.

We'll be doing an early beta of 1.9 this fall, please do give it a try when it comes out so that we can ensure that we make it smooth for your users. The 1.9 compat plugin will probably smooth out a lot of bumps, but ideally you won't want to advise that people use it as a long term solution.

Changed August 30, 2012 07:47PM UTC by BorisMoore comment:9

Thanks Dave.

Unfortunately the ES5/6 features won't be sufficient for a while, since jQuery.observable and JsViews are platform pieces that certainly will be often used on sites that need a broad browser matrix. On $.fn.data, I hear you, (and indeed I personally always use $.data rather than $.fn.data whenever I can for that perf reason). I'll be glad to test out the beta of 1.9.

I'm still wondering whether jQuery UI are looking at data binding, down the road, and care about jQuery.observable scenarios. Did you loop in Jorn or Richard on your thinking about whether to remove support for $.data and event binding on plain objects?

Changed August 30, 2012 08:30PM UTC by scottgonzalez comment:10

Boris, there are no plans to stop supporting data and event bindings on plain objects. The plan is to stop triggering events via .data().

Changed September 05, 2012 07:43PM UTC by BorisMoore comment:11

OK, great, thanks Scott. That works better for me :)

Changed September 13, 2012 11:12AM UTC by jaubourg comment:12

A bit late but: hi Boris!

Changed October 16, 2012 07:01PM UTC by jaubourg comment:13

resolution: → notabug
status: openclosed