cujojs / poly

Small, fast, awesome. The only ES5-ish set of polyfills (shims) you can mix-and-match because they're individual modules.
Other
139 stars 18 forks source link

array.js polyfill _iterate is missing hasOwnProperty check #9

Closed jaxley closed 11 years ago

jaxley commented 12 years ago

On line 142 of array.js, you are doing this:

    if (i in alo) {

Which is a no-no on an array because what happens on line 143 is you then include that "property" in the iteration. Which actually causes the polyfill to break the page because array indexing will include prototype properties (including all of the Array.prototype polyfills) in addition to the array values themselves. We were seeing array iterations going off the end of the array and including the actual function source as phantom array "values".

The fix is trivial:

if (i in alo && alo.prototype.hasOwnProperty(i)) {

You should add a unit test for this. Not sure if you run your unit tests in a browser like IE7 or IE8 where the polyfills actually are loaded to catch this.

jeffrose commented 12 years ago

FYI. This fiddle illustrates the problem.

http://jsfiddle.net/core24/q6qPU/

briancavalier commented 12 years ago

@jaxley, are you saying that using poly's version of Array.prototype.forEach (or map, reduce, etc. any iterator) exhibits the problem? If so, then that's def bad. Or, are you seeing the problem when using for..in on an Array? If it's the latter, then is it an option for you simply not to use for..in on Arrays? A numeric for/while or forEach() will be much faster in most cases (except for large sparse arrays).

Also, beyond the standard ES5 methods that poly adds to Array.prototype, are you augmenting Array.prototype with additional stuff?

Thanks for the info, b

unscriptable commented 12 years ago

We have unit tests that would expose this problem and they don't fail in IE6+. Are you actually seeing a problem when using poly/array in your code?

jaxley commented 11 years ago

Sorry for the long delay. We found that our specific issue was caused by some bad array iteration. So the poly code was not at fault.

Not sure if the poly array code iteration should be more defensive still or not.

unscriptable commented 11 years ago

I'd be open to having optional defensive features. Something to think about, for sure!