Fixed a bug where core-js was modifying native collection iterator prototypes

[aickin]

First off, thanks for all the hard work that has gone into core-js!

I ran into a fairly obscure bug in core-js's handling of collection iterator prototype chains while running compat-table tests on various platforms. I found that core-js was causing collection iterator prototype chain tests to fail on platforms that already had native collection iterator support. The tests that were failing are for iterators on Set, Map, Array, and String, and they all look basically like this:

function test() {
        // Iterator instance
        var iterator = ''[Symbol.iterator]();
        // %StringIteratorPrototype%
        var proto1 = Object.getPrototypeOf(iterator);
        // %IteratorPrototype%
        var proto2 = Object.getPrototypeOf(proto1);

        return proto2.hasOwnProperty(Symbol.iterator) &&
          !proto1    .hasOwnProperty(Symbol.iterator) &&
          !iterator  .hasOwnProperty(Symbol.iterator) &&
          iterator[Symbol.iterator]() === iterator;
}

As this test shows, the iterator should not have a Symbol.iterator own property, and neither should the iterator's prototype. The iterator's prototype's prototype, however, should have a Symbol.iterator property, and that method should return the iterator. These tests pass in all modern browsers without any shims (Chrome, Firefox, Safari). They also pass when core-js is used in a browser that has no iterator support. These tests fail, however, when used in modern browsers in concert with core-js.

It looks to me like there is a method in _iter-define.js that attempts to patch the iterator prototype chain for native iterator implementations only, but the implementation puts a Symbol.iterator method on the iterator's prototype rather than the iterator's prototype's prototype. This is what causes the test failures.

This PR attempts to fix this by patching the native iterator's prototype's prototype instead. I added tests for Set, Map, Array, and String, although those tests didn't fail with the pre-existing code. I think this because npm run test runs the test suite in Phantom, which doesn't have native iterator support. As I said above, this bug only shows up in browsers that have native iterator support.

Apologies for any breaches of code style and such; I've never used LiveScript before and did not feel super confident about my changes there. Feedback is welcome, and thanks again for your work!

[zloirock]

You are right, it's incorrect behaviour, but initially it was added intentionally. Early iterators implementations haven't %IteratorPrototype%. Not all engines have a way of fixing it (setPrototypeOf, etc), so @@iterator property was added to all prototypes of iterators directly. Your fix will break those implementations. But yes, it should not be added in modern implementations. I'll think how to fix it.

[aickin]

That is extremely helpful feedback, @zloirock! I could tell that there was some sort of backwards compatibility reason, but it wasn't entirely clear to me.

With this feedback, I modified the PR to hopefully still patch old browsers but not interfere with newer, spec-compliant browsers in 7d54d72.

Instead of checking to see if the iterator's prototype has @@iterator, the new code now checks to see if the iterator's prototype's prototype has @@iterator. If the prototype's prototype doesn't have @@iterator (as in old, uncompliant browsers), it just does the same thing it used to: adds @@iterator to the iterator's prototype.

I think this should maintain existing core-js behavior in browsers that don't have %IteratorPrototype% while not modifying the prototype chain in spec-compliant browsers.

I also added a big wordy comment explaining this for folks who may come to the code and misunderstand it in the way that I did. That may conflict with your comment style, and I'll happily modify it if you wish. Just let me know.

Thanks again for your work on such a critical piece of web infrastructure!

[aickin]

Hey there!

I'm wondering if I can respectfully bump this PR a little. I think the new implementation addresses your concerns while allowing newer browsers to have the correct prototype chain. Any feedback on the PR? Thanks so much for your time, and again: thanks for all your work!

[zloirock]

Fixed in v3 branch.