Skip to main content

Bug Tracker

Side navigation

#11219 closed bug (invalid)

Opened January 24, 2012 10:55PM UTC

Closed January 25, 2012 01:27AM UTC

Last modified January 29, 2012 01:37PM UTC

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.

Attachments (0)
Change History (15)

Changed January 24, 2012 10:59PM UTC by jsuder comment:1

_comment0: 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... \ 1327446041009679

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

Changed January 25, 2012 01:27AM UTC by rwaldron comment:2

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/

Changed January 25, 2012 01:32AM UTC by jsuder comment:3

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

Changed January 25, 2012 01:34AM UTC by rwaldron comment:4

_comment0: parseInt doesn't throw when radix is omitted.1327455320833052

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

Changed January 26, 2012 04:26PM UTC by gibson042 comment:5

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)

Changed January 26, 2012 04:49PM UTC by dmethvin comment:6

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.

Changed January 26, 2012 05:22PM UTC by rwaldron comment:7

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.

Changed January 28, 2012 07:12PM UTC by gibson042 comment:8

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

Changed January 28, 2012 07:17PM UTC by dmethvin comment:9

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?

Changed January 28, 2012 07:42PM UTC by jsuder comment:10

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.

Changed January 28, 2012 07:44PM UTC by gibson042 comment:11

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

Replying to [comment:9 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.

Changed January 29, 2012 04:24AM UTC by dmethvin comment:12

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?

Changed January 29, 2012 05:31AM UTC by gibson042 comment:13

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.

Changed January 29, 2012 05:40AM UTC by rwaldron comment:14

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.

Changed January 29, 2012 01:37PM UTC by gibson042 comment:15

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.