Bug Tracker

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#11219 closed bug (invalid)

jquery hangs in $.param when passed a jquery object

Reported by: jsuder Owned by:
Priority: low Milestone: None
Component: core Version: 1.7.1
Keywords: Cc:
Blocked by: Blocking:

Description

I found that there is a very easy way to make the browser hang in an infinite loop using latest jQuery. To do that, you need to pass an object containing a jQuery collection to $.param.

The way I found it is that I wasn't aware that $.fn.map returns a jQuery collection, and I was passing it to $.post as data. This code has worked fine before on jQuery 1.4.2, and then stopped working when I've upgraded jQuery to 1.7.1.

Here's a JSFiddle test with an example: http://jsfiddle.net/gfzSU/ - if you run it as is and click the button, it will just hang. This must have been changed somewhere between 1.4.4 and 1.5.2, because it works on 1.4.4 and starts behaving like this on 1.5.2.

Change History (15)

comment:1 Changed 7 years ago by jsuder

Finally... guys, sorry to say this but this bug tracker software is a piece of sh*.*... First I tried to report this bug without logging in, but said it was spam and I had to fill a captcha. I did, but then it told me it was spam anyway. So I tried to log in but none of my passwords worked, so I filled the forgot password form and waited for an email. I never got the email. So today I've registered a second account, and it told me simultaneously that it didn't work because someone is using the same name *and* that it did work (I got two messages, one in green and one in red). And then when I logged in and re-submitted this, guess what? It told me I needed to fill a captcha because it was spam. Luckily, it worked this time after filling the captcha...

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

comment:2 Changed 7 years ago by Rick Waldron

Component: unfiledcore
Priority: undecidedlow
Resolution: invalid
Status: newclosed

If you need an array, use toArray(). Note the docs don't mention support for serializing jQuery objects http://api.jquery.com/jQuery.param/ - only array and object

http://jsfiddle.net/rwaldron/SV8eX/

comment:3 Changed 7 years ago by jsuder

Wouldn't e.g. throwing an error be a better solution than making the browser hang? Imagine parseInt() punished you with hanging the browser for forgetting to add 10 as the second argument...

comment:4 Changed 7 years ago by Rick Waldron

parseInt doesn't throw when radix is omitted. Additionally, it's well known that circular structures cannot be serialized. Please use the forum for support questions

Last edited 7 years ago by Rick Waldron (previous) (diff)

comment:5 Changed 7 years ago by gibson042

Throwing an error is a better response to circular input than hanging the browser. A fix is ready if you want it

https://github.com/jquery/jquery/pull/665 (-57/+113/+48)

comment:6 Changed 7 years ago by dmethvin

That is a big amount of code for a bug that should be found before the page is deployed. I agree that theoretically it would be great to diagnose and clearly explain every misuse of the API with a good error message, but it just has too high a cost.

comment:7 Changed 7 years ago by Rick Waldron

jQuery.param is meant to be used for serializing objects that will be sent to a server - it should be assumed that this means it is limited to circular reference safe values. It's smarter to write code that doesn't make this mistake, then to modify jQuery core and burden all it's other users with the additional file size.

comment:8 Changed 7 years ago by gibson042

Out of curiosity, would you accept it at half the size?

comment:9 Changed 7 years ago by dmethvin

I think it's more a philosophical thing. Why would we check for this condition and throw an error when we don't do it for many others? What is special about this error?

comment:10 Changed 7 years ago by jsuder

I might be biased here, but I'd say it's rather easy to cause this error, because jQuery collections behave just like arrays in most cases (if you print them in the console you can't see any difference), and you might not notice that the thing you got from map() and looks like an array of integers is not a real array. It wouldn't even have to check for every possible circular reference, just if there aren't any jQuery collections inside the passed object, if they are known to be a problem in this case.

Also, hanging the browser is a pretty brutal reaction, most errors that you can generate for using the API in a wrong way result in either an exception (with stack trace and/or other details) or nothing happening at all when it should have.

comment:11 Changed 7 years ago by gibson042

Replying to dmethvin:

I think it's more a philosophical thing. Why would we check for this condition and throw an error when we don't do it for many others? What is special about this error?

That running afoul of it hangs the client in an infinite loop, as opposed to other issues that just yield unexpected results.

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

comment:12 Changed 7 years ago by dmethvin

So we just need to break it so that passing a jQuery object won't cause a loop? How about this?

https://github.com/dmethvin/jquery/commit/90a5b4486fdcd0a90386723c49df660619bcd534

However, there are clearly an infinite number of other bad inputs that can cause similar problems. Should we guard against those as well? If so, why?

comment:13 Changed 7 years ago by gibson042

Like you said, it's a philosophical thing. I think API calls should stay on the good side of the halting problem and always terminate. You agreed above, but considered 48 bytes too steep a price for that guarantee. So now I'm asking about 24.

comment:14 Changed 7 years ago by Rick Waldron

Can't wait for the bugs on this one... "my object has a property called 'jquery' that $.param won't serialize". How about this, try being more accountable for your own code and stop burdening the rest of jQuery users with cruft to protect your programs from YOU.

I'm asking for zero bytes, in return I promise to write smarter programs that don't try to serialize lists of nodes.

comment:15 Changed 7 years ago by gibson042

jQuery.param already has special code for differentiating jQuery objects from objects with a ' jquery' property. Not that it's relevant anyway; my PR was for terminating on all input, not for specifically rejecting jQuery.

But if that's how you feel about bad input, why not go all the way? I bet I can save 100 bytes by getting rid ofjQuery.error.

Note: See TracTickets for help on using tickets.