Bug Tracker

Opened 7 years ago

Closed 7 years ago

#12404 closed bug (notabug)

.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)

Change History (13)

comment:1 Changed 7 years ago by dmethvin

Owner: set to 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.

comment:2 Changed 7 years ago by BorisMoore

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.

Last edited 7 years ago by BorisMoore (previous) (diff)

comment:3 Changed 7 years ago by dmethvin

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.

comment:4 Changed 7 years ago by crimar

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);
});

comment:5 Changed 7 years ago by dmethvin

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.

comment:6 Changed 7 years ago by jaubourg

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.

comment:7 Changed 7 years ago by BorisMoore

(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...

comment:8 Changed 7 years ago by dmethvin

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.

comment:9 Changed 7 years ago by BorisMoore

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?

comment:10 Changed 7 years ago by scottgonzalez

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

comment:11 Changed 7 years ago by BorisMoore

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

comment:12 Changed 7 years ago by jaubourg

A bit late but: hi Boris!

comment:13 Changed 7 years ago by jaubourg

Resolution: notabug
Status: openclosed
Note: See TracTickets for help on using tickets.