paulmillr / es6-shim

ECMAScript 6 compatibility shims for legacy JS engines
http://paulmillr.com
MIT License
3.11k stars 387 forks source link

Expected int32 as second argument in Firefox with Array.prototype.map #347

Closed TheSavior closed 8 years ago

TheSavior commented 9 years ago

When I'm running my test suite in Firefox, I get this error. I haven't been able to successfully track down where my code is calling map such that this would fail which has been quite frustrating. Have you guys seen this before or know what it might be? This error does not occur on Chrome or Safari.

Firefox 38.0.0 (Mac OS X 10.10) ERROR
  Error: Expected int32 as second argument
  at 
node_modules/es6-shim/es6-shim.js:928:0

If I remove the part of the shim for Array.prototype.map by commenting out:

if (!toLengthsCorrectly(Array.prototype.map)) {
    var originalMap = Array.prototype.map;
    overrideNative(Array.prototype, 'map', function map(callbackFn) {
      return _apply(originalMap, this.length >= 0 ? this : [], arguments);
    }, true);
  }

then my test suite runs perfectly in Firefox. Any ideas?

ljharb commented 9 years ago

I have v38.0.1 on my Mac and I don't get this error. Which version of the es6-shim are you using?

Also, presumably this isn't happening when the shim is loaded, but when your code runs later - how are you calling .map in your own code?

ljharb commented 9 years ago

Note that this error is actually what happens in an unshimmed firefox if you do Array.prototype.map.call({ 0: 1, length: -1}, function () {}) since it's improperly handling negative lengths.

Are you using Array.prototype.map with a non-array anywhere?

TheSavior commented 9 years ago

Hmm, I'll look closer at our codebase. I'm unsure why the shim itself is causing this problem. When I remove that section from the shim, our tests all pass in Firefox. That seems like it would imply it being an issue in the shim, right?

ljharb commented 9 years ago

It definitely means it's something in the shim conflicting with something in your codebase. However, you could probably remove the other conflict and fix it too :-)

Are you using anything that might override Array.prototype.map? Are you using the es5-shim, but loading it after es6-shim?

TheSavior commented 9 years ago

I'm still trying to track this down.

I changed the map part of the shim to:

if (!toLengthsCorrectly(Array.prototype.map)) {
    var originalMap = Array.prototype.map;
    overrideNative(Array.prototype, 'map', function map(callbackFn) {

      try {
        return _apply(originalMap, this.length >= 0 ? this : [], arguments);
      }
      catch (e) {
        console.error('failed', originalMap, this.length >= 0 ? this : []);
        throw e;
      }

    }, true);
  }

as that should give us the information we need about what might be failing.

Here is a snippit from my output:

Firefox 38.0.0 (Mac OS X 10.10.0) ERROR: 'failed', function map() { ... }, ['lala']
..
Firefox 38.0.0 (Mac OS X 10.10.0) [REDACTED] FAILED
  Expected int32 as second argument
  map@/var/folders/mg/64k6_7p16vn2jwbtlz41_f6w0000gp/T/6897b5f55636a5d20259f7caa3712929.browserify:16243:16 <- ../node_modules/es6-shim/es6-shim.js:931:0
  d3_selection_classed@/var/folders/mg/64k6_7p16vn2jwbtlz41_f6w0000gp/T/6897b5f55636a5d20259f7caa3712929.browserify:6514:12 <- ../node_modules/d3/d3.js:707:0
  require<[37]</</d3_selectionPrototype.classed@/var/folders/mg/64k6_7p16vn2jwbtlz41_f6w0000gp/T/6897b5f55636a5d20259f7caa3712929.browserify:6505:22 <- ../node_modules/d3/d3.js:698:0
  ...

Firefox 38.0.0 (Mac OS X 10.10.0) ERROR: 'failed', function map() { ... }, ['lala']
Firefox 38.0.0 (Mac OS X 10.10.0) [REDACTED] FAILED
  Expected int32 as second argument
  map@/var/folders/mg/64k6_7p16vn2jwbtlz41_f6w0000gp/T/6897b5f55636a5d20259f7caa3712929.browserify:16243:16 <- ../node_modules/es6-shim/es6-shim.js:931:0
  d3_selection_classed@/var/folders/mg/64k6_7p16vn2jwbtlz41_f6w0000gp/T/6897b5f55636a5d20259f7caa3712929.browserify:6514:12 <- ../node_modules/d3/d3.js:707:0
  require<[37]</</d3_selectionPrototype.classed@/var/folders/mg/64k6_7p16vn2jwbtlz41_f6w0000gp/T/6897b5f55636a5d20259f7caa3712929.browserify:6505:22 <- ../node_modules/d3/d3.js:698:0
[REDACTED].js

And here is a larger snippit with more examples (not just coming from d3). http://pastebin.com/PnK5xbev

Any thoughts? The inputs look reasonable relative to other calls that don't fail. I haven't been able to get to a individual failing test case though. When I run just the tests that fail, they pass. Still digging in.

FWIW, this is still Firefox only.

ljharb commented 9 years ago

In your console.error path, can you include the arguments also? That's the part triggering the error :-)

classiemilio commented 9 years ago

I was able to narrow it down further. All my tests pass when I don't call toLengthsCorrectly(Array.prototype.map).

 var toLengthsCorrectly = function (method, reversed) {
    var obj = { length: -1 };
    obj[reversed ? ((-1 >>> 0) - 1) : 0] = true;
    return valueOrFalseIfThrows(function () {
      _call(method, obj, function () {
        // note: in nonconforming browsers, this will be called
        // -1 >>> 0 times, which is 4294967295, so the throw matters.
        throw new RangeError('should not reach here');
      }, []);
    });
  };

That looks in line with what you're saying about an object having a length of -1.

Also, the RangeError is called in Firefox.

ljharb commented 8 years ago

fwiw, the latest Firefox Nightly made a change to Array#map that actually avoids this error from being thrown - so I suspect that once that lands in stable, this problem may go away. I've still been able to reproduce it, and from our offline discussion it seems only reproducible with karma + firefox :-/

TheSavior commented 8 years ago

I look forward to not needing our hack in front of the polyfill. :)

mikeazo commented 8 years ago

@ljharb Do you have a link to the Firefox changes? I'd like to track to see when it makes it into stable.

ljharb commented 8 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=924058 is the overall ToLength bug, but https://bugzilla.mozilla.org/show_bug.cgi?id=1200108 is specific to "map", and the specific one that fixed this issue.

timglaser commented 8 years ago

Is anyone aware of an existing fork of es6-shim that is modified to not cause these errors in Firefox 42 and prior? Ideally it would be a fork that is to be kept up to date with the latest of es6-shim but with the Firefox fix in place until Firefox 45 is released for those that follow the convention of supporting the latest two versions of browsers. So far for my needs are met by commenting out the if statement around the Array.prototype.map wrapping at https://github.com/paulmillr/es6-shim/blob/master/es6-shim.js#L1244. But I'd rather not include have to maintain a dependency in my own repo.

ljharb commented 8 years ago

@timglaser this specific error was only occurring with the combination of Karma and Firefox. Is it happening for you in FF 42 by itself? If so, I will absolutely fix it.

timglaser commented 8 years ago

@ljharb it is. I'm getting the "expected Int32 second argument" error in production where karma is not included in any form. I just did a double check to make sure I'm not lying :) I'm getting the error in FF 42, but not in FF43, and not in FF 42 if I apply the change I mentioned above (and so far I'm not finding any negative effects in any of our supported browsers due to the change).

ljharb commented 8 years ago

@timglaser on what OS? I can't reproduce it. Can you make a jsfiddle that repros it?

timglaser commented 8 years ago

@ljharb This is happening on OS X and Windows versions of Firefox 42 and Firefox ESR. I'll see if I can get a jsfiddle together. Not likely today. It could be that something in our package other than karma is interacting in a way that highlights this bug. Thanks for being responsive on this!

ljharb commented 8 years ago

@timglaser Absolutely - I will happily fix anything I can reproduce. The remainder of this issue is because with karma, try/catch simply wasn't working - which creates a scenario that's unfixable. Hopefully your problem isn't equally unfixable :-)

ljharb commented 8 years ago

@TheSavior @timglaser can you check this again as of c7ff2226198eb94177ee2839770762fb9c82f69d? I fixed a number of issues regarding toLengthsCorrectly in older Firefoxes, and I'm hoping it's fixed this issue.

cholewczuk commented 8 years ago

I had the same problem on prod with FF v29 - v42. I can confirm that https://github.com/paulmillr/es6-shim/commit/c7ff2226198eb94177ee2839770762fb9c82f69d helped. Thanks!

ljharb commented 8 years ago

Awesome! It's released as v0.34.0 so hopefully that will resolve this issue.

TheSavior commented 8 years ago

0.34.0 fixed it for us as well.

ljharb commented 8 years ago

Hooray, glad I finally found a solution, and was able to reproduce it without Karma!