Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stack overflow is triggered when .data() is called in the options #4014

Closed
hacktivista opened this issue Dec 11, 2015 · 21 comments
Closed

Stack overflow is triggered when .data() is called in the options #4014

hacktivista opened this issue Dec 11, 2015 · 21 comments

Comments

@hacktivista
Copy link

I don't know if it's a Select2 problem, or a Parsley problem. I couldn't identify the error :(

Here is the simplest fiddle which shows the problem:
http://jsfiddle.net/jew1mg89/1/

The error is not present using the previous version of Select2 (3.x)

@hacktivista hacktivista changed the title "Too much recursion / Maximum call stack size exceeded" error when using in conjunction with Parsley.js Stack overflow when using in conjunction with Parsley.js Dec 11, 2015
@vin-zhou
Copy link

Hi guys, I faced the same problem these days. Any progress ? @kevin-brown @hacktivista

@hacktivista
Copy link
Author

@vin-zhou As workaround you can initialize parsley after select2.

Eventually somebody will be able to fix the problem. I couldn't :(

@sercanvirlan
Copy link

same here :(

@kevin-brown
Copy link
Member

Here's an updated version of the jsfiddle that works out of the box, without any changes, to reproduce the issue that you appear to be seeing.

http://jsfiddle.net/jew1mg89/5/

And it's true that initializing Select2 before initializing parsely will fix the issue, though I'm not sure why.

http://jsfiddle.net/jew1mg89/6/

@kevin-brown
Copy link
Member

Tracking this down a little further, the issue is triggered within select2/option, which is what parses the configuration for Select2. Specifically in fromElement, where we parse out any data from the data-* attributes on the element.

You can reproduce the error without Select2 by trying to clone the data for an element using $.extend, but only with the deep option specified.

http://jsfiddle.net/jew1mg89/8/

I expected this to be an issue in Select2 as well, but apparently Select2 can be deep copied without issue.

http://jsfiddle.net/jew1mg89/9/

So that officially makes this a Parsley issue, triggered by Select2.

@marcandre
Copy link
Contributor

I feel that select2 should not assume anything about data() that isn't his. Since jquery's deep extend isn't safe on recursive structure, then it shouldn't be used on objects that aren't select2.

Can select2 write it's own deep clone function that is recursion safe? I'll provide a PR if needed.

@marcandre
Copy link
Contributor

Note: I didn't check why select2 wants to do a deep copy of data(). Maybe that's not actually needed, a shallow copy could suffice?

@kevin-brown
Copy link
Member

Maybe that's not actually needed, a shallow copy could suffice?

This was added in a9f6d64 because we needed to normalize the result from HTMLElement.dataset (see #2969). I know it's also needed because of our data-* attribute parsing, which expects that it can modify the set of data attributes, which is why we make a clone for each element (see 3c8366e for a similar issue).

Normally this isn't an issue because most plugins avoid circular references within their code, and instead store state like that in other areas.

@marcandre
Copy link
Contributor

Thanks for the explanations.

My impression now is that there's no need to call data() and that it is a bit misguided to do so. data() is a cute shortcut to get the data attributes but it is not the best way to do so.

In particular:

The data- attributes are pulled in the first time the data property is accessed and then are no longer accessed or mutated (all data values are then stored internally in jQuery).

So for example it would be possible that a library calls data() on a element, that the element's attributes are later modified by JS code, and you would not get the updated values when calling data().

The fact is that data() is used to store more than read data attributes. Am I missing something, or you only care about data attributes and not third party data? If so, you could lookup the attributes and do the equivalent what data() is doing when pulling the attributes the first time, no? BTW, this is what we do in parsley, so that we always use the actual DOM attribute value.

@krazyjakee
Copy link

Thanks for taking the time to look into this @marcandre as I really need a solution to this.

@kevin-brown
Copy link
Member

You're right, Select2 could probably avoid calling .data (and would definitely entertain better ways).

One possible way might be to do a shallow copy of the data object, and then do a deeper copy of it if we can confirm that the value is not a complex JS object.

So for example it would be possible that a library calls data() on a element, that the element's attributes are later modified by JS code, and you would not get the updated values when calling data().

We're less concerned about that, but yes it is a possible issue one could encounter. We've seen (in the wild) that changes to data-* attributes after the page is rendered usually happen through .data anyway.

Am I missing something, or you only care about data attributes and not third party data?

This is pretty much the only place in the code base which uses .data (read more about why we avoid it), and we only use it because we are looking for all data attributes set on the element. Unfortunately this also pulls in any private data that has been set on the element, which hasn't been a major issue up until now.

If so, you could lookup the attributes and do the equivalent what data() is doing when pulling the attributes the first time, no?

This would be ideal, but Select2 (unlike parsley) does not have a small number of options. It actually doesn't have a fixed number of options that are used at any particular time, as adapters can specify new options that they are looking for. As you can imagine, this forces us to use a bit of magic to convert data attributes into options, resulting in what we currently have.

so that we always use the actual DOM attribute value.

Just wanted to mention that we use .data here instead of Element.attributes because it automatically parses the values into JS data types. Again, this is important because Select2 has options taking booleans, integers, strings, etc, as well as nested options (which can be specified with JSON objects if people feel the need).

@marcandre
Copy link
Contributor

This would be ideal, but Select2 (unlike parsley) does not have a small number of options.

Actually, parsley has an unlimited number of options too. We parse anything that starts with data-parsley (or whatever the namespace as configured by the user).

because it automatically parses the values

Same with Parsley, actually. It's annoying that jQuery doesn't expose this functionality and that we both have to rewrite it ourselves. Maybe that (very small) library could be extracted.

Would you like me to write something up for select2?

@marcandre
Copy link
Contributor

BTW, in your blog post, you write:

jQuery gets the data-* attributes every time .data() is called

As I tried to explain before, this is incorrect. data() will get the data-* attributes only the first time it is called, to initialize the object holding the data. For data('foo'), it's more complicated, it will read the data-foo attribute, until the day where that data is set, either because of data-foo or via data('foo', value).

This can be quite confusing. In cases where the attribute is changed via JS, a reading call to data('foo') can actually modify the results of data()! See it here.

@sanderbrauwers
Copy link

+1

@krazyjakee
Copy link

@kevin-brown what is your last word on this, or have you not yet decided?

@kevin-brown
Copy link
Member

My tentative plan is to rewrite how we clone the field-level data to exclude anything that wouldn't normally come from jQuery or the DOM. It'd be slightly easier if Select2 had a prefix for the data attributes, but that's a mistake we are going to have to live with.

In any case, I'm going to reopen this ticket and give it a more relevant title.

@kevin-brown kevin-brown reopened this Jan 11, 2016
@kevin-brown kevin-brown changed the title Stack overflow when using in conjunction with Parsley.js Stack overflow is triggered when .data() is called in the options Jan 11, 2016
@AGPDev
Copy link

AGPDev commented Apr 14, 2016

+1

NadeemAfana pushed a commit to NadeemAfana/select2 that referenced this issue May 5, 2016
@SteffenTallieu
Copy link

+1

@alexweissman alexweissman added this to the 4.0.6 milestone Oct 26, 2017
@alexweissman
Copy link
Contributor

#4346 has been merged into develop for 4.0.6 release.

@india081947
Copy link

Possibly Bug:

I was migrating from Select2 4.0.3 to Select2 4.0.6-rc.1, I have noticed that it has stopped taking effects of data-attributes like placeholder, minimum-input-length, ajax--delay etc mentioned in element html.

Same bug continues to latest RC.1

Steps to reproduce:

  1. Use the below markup with Select2 4.0.5
<select id="s2" style="width:500px" class="form-control" data-minimum-input-length="3" data-ajax--delay="3000" data-placeholder="Step 1: Select Click Here" data-select-on-close="true">
<option></option>
</select>
  1. Initialize it with
    $("#s2").select2();

  2. It will show place holder, minimum length required & ajax delay as expected

  3. Now change the select2 version to select2 4.0.6-rc.0

This will not show placeholder, will not consider min length or delay

JS Fiddle(Select2 4.0.5): https://jsfiddle.net/gyf7fkmo/2/
JS Fiddle(select2 4.0.6-rc.0): https://jsfiddle.net/jL4vL5xq/1/

Resolution: Reverting to $.data resolves the problem.
I have replaced all the Utils.GetData & Utils.SetData with $.data and it seems working fine.

kevin-brown added a commit that referenced this issue Apr 24, 2019
There was a change made that switched us from using `$.data` and
`$.fn.data` internally to using an internal data store (managed
through internal utilities). This had the unfortunate side effect
of breaking the automatic loading of data-* options in versions of
jQuery other than 1.x, which included anything that would be
considered modern jQuery. While the change was made and approved
in good faith, all of the tests passed and the docs pages appeared
to be working, the tests really failed when running on newer versions
of jQuery. This was confirmed when we started running automated tests
against both versions, which confirmed the bug that others have been
seeing for a while.

The change was made becuase calling `$.fn.data` on an element which
contains a reference to itself in the internal jQuery data cache
would cause a stack overflow. This bug was well documented at the
following GitHub ticket and was resolved by no longer using
`$.fn.data`: #4014

Unfortunately because `$.fn.data` was no longer being called in a
way such that all of the data attributes would be dumped out, we
needed to find a replacement. The substitute that was given in the
original bug fix worked when the data cache was fully primed, but
we never primed it anywhere so it actually failed in the general
case. That meant we needed to find a way to manually prime it,
which is exactly what this change does.
kevin-brown added a commit that referenced this issue Apr 28, 2019
* Start running tests against jQuery 2.x

We were only running tests against jQuery 1.x before and they were
all passing. This was a problem because apparently all of the data-*
attribute tests fail in jQuery 2.x.  We are now running both the
integration and unit tests against both jQuery 1.x and jQuery 2.x.

Right now this resulted in a complete duplication of the test files
because there wasn't an obvious way to run the tests against both
versions. We're going to look into removing this duplication in the
future once the current issues are fixed.

We are also going to look into testing against jQuery 3.x in the
future, since that is also a supported line of jQuery.

* Force the data-* attributes to be parsed

There was a change made that switched us from using `$.data` and
`$.fn.data` internally to using an internal data store (managed
through internal utilities). This had the unfortunate side effect
of breaking the automatic loading of data-* options in versions of
jQuery other than 1.x, which included anything that would be
considered modern jQuery. While the change was made and approved
in good faith, all of the tests passed and the docs pages appeared
to be working, the tests really failed when running on newer versions
of jQuery. This was confirmed when we started running automated tests
against both versions, which confirmed the bug that others have been
seeing for a while.

The change was made becuase calling `$.fn.data` on an element which
contains a reference to itself in the internal jQuery data cache
would cause a stack overflow. This bug was well documented at the
following GitHub ticket and was resolved by no longer using
`$.fn.data`: #4014

Unfortunately because `$.fn.data` was no longer being called in a
way such that all of the data attributes would be dumped out, we
needed to find a replacement. The substitute that was given in the
original bug fix worked when the data cache was fully primed, but
we never primed it anywhere so it actually failed in the general
case. That meant we needed to find a way to manually prime it,
which is exactly what this change does.

* Clean up select2/utils
@javiertejero
Copy link

javiertejero commented Oct 23, 2019

I have replaced all the Utils.GetData & Utils.SetData with $.data and it seems working fine.

I can't reproduce your issue in 4.0.10, see https://jsfiddle.net/javier_tejero/vm7a94pL/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests