Bug Tracker

Ticket #8108 (closed bug: fixed)

Opened 4 years ago

Last modified 2 years ago

jQuery metadata is exposed on plain JS objects when serializing with JSON.stringify

Reported by: BorisMoore Owned by: dmethvin
Priority: blocker Milestone: 1.5.1
Component: data Version: 1.5
Keywords: Cc:
Blocking: Blocked by:

Description

New event binding code in 1.5 is adding jQuery expandos to plain objects that are not of type function, and therefore show up on JSON serialized objects.

See comment and suggested fix:

 https://github.com/jquery/jquery/commit/8e59a99e0ade75dec434f246f52e8b3f7393f359#src/data.js-P71

Change History

comment:1 Changed 4 years ago by BorisMoore

  • Keywords Events added
  • Component changed from unfiled to data

comment:2 Changed 4 years ago by john

  • Priority changed from undecided to blocker
  • Milestone changed from 1.next to 1.5.1

comment:3 Changed 4 years ago by rwaldron

  • Owner set to BorisMoore
  • Status changed from new to pending

Please provide a reduced jsFiddle test case to help us assess your ticket

comment:4 Changed 4 years ago by jitter

comment:5 Changed 4 years ago by dmethvin

ha you beat me jitter, here's my test:  http://jsfiddle.net/FFgdz/

comment:6 Changed 4 years ago by BorisMoore

  • Status changed from pending to new

This was assigned to me, but I think it would be better assigned to snover or similar, since I am not set up for committing fixes to jQuery itself.

I have made all necessary changes to DataLink to work against 1.5, given the new behavior of .data(). This bug concerns serialization of plain objects in general, and is a problem in the data component. It has no specific bearing on data link, and data link does not directly depend on this fix.

Should I simply reassign?

comment:7 Changed 4 years ago by snover

  • Keywords Events removed
  • Owner changed from BorisMoore to snover
  • Status changed from new to assigned

comment:8 Changed 4 years ago by Colin Snover

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

Update $.data to use a function instead of an object when attaching to JS objects in order to hide attached metadata from JSON.stringify. Remove event.js code that was doing this before specifically for events, which is now redundant. Fixes #8108. 1.5-stable

Changeset: 7acb141ed7f2dedd950bb65acf878098640d081e

comment:9 Changed 4 years ago by jitter

  • Status changed from closed to reopened
  • Resolution fixed deleted

Reopening to make sure we don't ship with this, as this fix fails the test suite in several browsers including:

  • Opera 11.01, 11.00, 10.63, 10.54
  • FF 3.5.11, 3.0.19, 2.0.0.20
  • Safari 4.0.5, 3.2.3, 3.1.2

Not all of those are officially supported (anymore) but many are.

Special attention is needed for the fail in Safari 4.0.5 which don't seem to related to prototype being an enumerable property. Safari 4.0.5 fails the test included for this ticket data: JSON serialization (#8108) -> Expando is hidden from JSON.stringify which probably means there is bug in the JSON.stringify implementation in Safari 4.0.5

comment:10 Changed 4 years ago by jitter

  • Status changed from reopened to open

comment:11 Changed 4 years ago by snover

  • Status changed from open to closed
  • Resolution set to wontfix

There’s no way to do this reliably; ES3 exposes enumerable properties by default on function instances. ES5 does not, but not all supported browsers have been updated to support ES5. Users that want to serialize JS objects that have been modified using $.data should pass a replacer function that filters out jQuery.expando properties by returning undefined.

comment:12 Changed 4 years ago by BorisMoore

In 1.4.4, before the change to the .data implementation, those test cases were presumably passing, right? At that point, events were being added to plain objects, as an expando of type function. If that is the case, then I have not understood why a modified .data implementation can't also work in the same way without breaking test cases.

Also I have not understood why a function instance exposing properties in ES3 is a problem. We want a plain object, which is itself a 'real' object, (not a function with added expando properties), to be able to have expandos of type property added to it, and for those expandos not to show up in the JSON serialization. Are you saying that stringify in Safari 4.0.5, does not hide expandos of type function from the serialization?

comment:13 Changed 4 years ago by snover

There were no test cases that applied in 1.4.4. JSON serialization was never tested explicitly. The only internal property that was hidden in this way was the internal events property, which was far from the only property that was potentially added to objects. The only test that existed in the 1.4.4 suite was “is events a function when added to a plain JS object?”. There was never an explicit test to see what a serialized object actually looked like until I added it along with this patch, and then it turned out that it only passed in IE and browsers that had started implementing ES5 (which changed the property to DontEnum).

It’s a problem for three reasons.

  1. ES3 exposes a prototype property by default as a non-deletable, enumerable value of the function instance. This means that it’s impossible to have a truly empty function instance in an ES3 environment: iterating through the instance always shows it as having a prototype property that cannot be deleted. This means that an “empty” data structure actually has properties, which means that a bunch of other tests start to fail, and the only way to work around those is to add more hacks in jQuery to deal with and ignore the potential existence of a prototype property.
  1. Using a function instance ignores the very real possibility of people extending Function.prototype with their own shit which ends up being enumerable on function instances, further breaking the ability to determine whether or not the data structure is empty. Unlike Object.prototype, modifying Function.prototype is not verboten, and occurs with relatively high frequency, especially with polyfills for ES5.
  1. Safari 4’s native JSON.stringify treats a Function like an Object and serializes it anyway.

There is a simple and much more viable alternative for people that want to serialize objects with attached metadata (replacer function). Please use it.

comment:14 Changed 4 years ago by BorisMoore

I am fine with using replacer in my code - that is not the issue :). DataLink does not use stringify. The issue is that if one plugin uses .data on plain objects, or binds events to plain objects - as datalink does, then every user of that plugin who wants to serialize objects needs to know that they should provide a replacer functions, and needs to implement that function.

Better have it work per the JSON spec with all browsers which comply to that spec, and with JSON2, than have it break for all browsers. In the not to far future, JSON implementations which are not compliant, and incorrectly serialize field of type function will be less and less relevant...

Why would we prefer to break serialization on all browsers, rather than breaking serialization on only a small percentage of browsers?

comment:15 Changed 4 years ago by snover

Users can ignore the metadata that shows up, or they can use a replacer function to remove it—there really is no third option here. This feature is impossible to implement across all browsers that jQuery supports and breaks completely in standards-compliant browsers that implement ES3. Trying to hide metadata using using a function instance is a bad hack that will never work sufficiently robustly.

If there is confusion among anyone about what this strange property is that shows up on their plain JS objects, then we can consider adding some documentation somewhere to address that confusion once it comes up.

Last edited 4 years ago by snover (previous) (diff)

comment:16 Changed 4 years ago by snover

  • Summary changed from JSON serialization broken: Event binding code expandos added to plain objects should be of type function. to jQuery metadata is exposed on plain JS objects when serializing with JSON.stringify

comment:17 Changed 4 years ago by dmethvin

  • Status changed from closed to reopened
  • Resolution wontfix deleted

I'm going to look at the trick that jitter suggested, override toJSON() on the cache object so it returns undefined.

comment:18 Changed 4 years ago by dmethvin

  • Owner changed from snover to dmethvin
  • Status changed from reopened to assigned

comment:19 Changed 4 years ago by BorisMoore

Great, thanks very much Dave. Sounds very promising.

Also, I agree with the modified description from Colin. The problem is not specific to events - it is with stringify and jQuery metadata in general...

comment:20 Changed 4 years ago by Colin Snover

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

Hide metadata when serializing JS objects using JSON.stringify via a toJSON hack. Fixes #8108.

Changeset: 2ed81708bdacfd4b97b77baef67ad8b75205dd20

comment:21 Changed 3 years ago by jeresig

Removing un-needed frameElement check as discussed in #8018. Fixes #8108.

Changeset: 7bfb6a7dd315f0eda3e5227f7d41e38f66f46549

Note: See TracTickets for help on using tickets.