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

$("*") fails with Maximum call stack size exceeded #3704

Closed
NN--- opened this issue Jun 25, 2017 · 7 comments
Closed

$("*") fails with Maximum call stack size exceeded #3704

NN--- opened this issue Jun 25, 2017 · 7 comments

Comments

@NN---
Copy link

NN--- commented Jun 25, 2017

Open this html file.

https://mega.nz/#!qphByIrJ!72ksYS6ceoQw8KYUwKjb03K2DiMfKjQNh6HKYoAuKJQ

Simple code of $("*") fails with:

jquery-3.2.1.js:2704 Uncaught RangeError: Maximum call stack size exceeded
    at Sizzle.select (jquery-3.2.1.js:2704)
    at Function.Sizzle [as find] (jquery-3.2.1.js:884)
    at jQuery.fn.init.find (jquery-3.2.1.js:2922)
    at jQuery.fn.init (jquery-3.2.1.js:3032)
    at jQuery (jquery-3.2.1.js:98)
    at jqqs.html:15

While document.querySelectorAll("*") works as expected.

@dmethvin
Copy link
Member

While document.querySelectorAll("*") works as expected.

However, [].push.apply([], document.querySelectorAll("*")) dies in the same way on Chrome.

On Edge it takes 2 minutes 15 seconds to give an answer, but it does return. On Firefox it runs almost instantaneously.

Seems like this is an issue independent of jQuery?

@NN---
Copy link
Author

NN--- commented Jun 26, 2017

Jquery should not fail with stack exceeded at least.
The function should be rewritten to not use recursion.

@dmethvin
Copy link
Member

Look at the repro case I gave above. This fails in Chrome as well, no jQuery at all. Type it into your console.

[].push.apply([], document.querySelectorAll("*"))

@timmywil
Copy link
Member

However, this seems to work...

var elems = document.querySelectorAll('*');
var arr = [];
for (var i = 0, len = elems.length; i < len; i++) {
  arr.push(elems[i]);
}

Let's talk in the meeting.

@timmywil
Copy link
Member

We're going to try dropping push.apply and just use the for loop (which is already in Sizzle as a IE<9 fallback). If perf isn't an issue, code will end up being shorter.

@timmywil
Copy link
Member

Moved to Sizzle issue: jquery/sizzle#403

@docluv
Copy link

docluv commented May 27, 2018

Using apply creates new arguments, which eventually exceed the size of the stack. The for loop does not place the results in the argument list and thus keeps the stack smaller :)

@lock lock bot locked as resolved and limited conversation to collaborators Nov 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

4 participants