Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polyfilled Float32Array constructor cannot be stringified #300

Closed
bisubus opened this issue May 2, 2017 · 15 comments
Closed

Polyfilled Float32Array constructor cannot be stringified #300

bisubus opened this issue May 2, 2017 · 15 comments

Comments

@bisubus
Copy link

bisubus commented May 2, 2017

This

require('core-js'); 
console.log(typeof Float32Array, Float32Array);

results in error:

TypeError: Cannot convert undefined or null to object
    at toString (native)
    at Function.<anonymous> (...\node_modules\core-js\modules\_ctx.js:18:15)
    at exports.format (util.js:128:18)
    at Console.log (console.js:43:37)
    at repl:1:9
    at sigintHandlersWrap (vm.js:22:35)
    at sigintHandlersWrap (vm.js:96:12)
    at ContextifyScript.Script.runInThisContext (vm.js:21:12)
    at REPLServer.defaultEval (repl.js:313:29)
    at bound (domain.js:280:14)

While the same code without core-js works as expected:

console.log(typeof Float32Array, Float32Array);

and outputs

function function Float32Array() { [native code] }

Another problem is that Float32Array behaviour was changed for no obvious reason and started to make problems. This was totally uncalled for and wasn't easy to debug. Since current Node releases are considered virtually ES2015/ES2016-compliant, the one could reasonably assume that require('core-js') is the same thing as cherry-picking ES2017+ features - and this would be a mistake that may result in counter-intuitive issues like this one (#283 already addresses the problem with default polyfilling strategy).

core-js 2.4.1
node 7.2.0

@zloirock
Copy link
Owner

zloirock commented May 3, 2017

Seems the problem not in polyfilled Float32Array because it's not polyfilled in modern Node versions, most likely the problem in the Function#toString wrapper.

@bisubus
Copy link
Author

bisubus commented May 3, 2017

@zloirock Yes, it looks like Float32Array stays the same, but something is going on the prototype and probably TypedArray.

E.g.

const f32aCtor = (new Float32Array(0)).constructor;
require('core-js');
f32aCtor !== (new Float32Array(0)).constructor;

@fatcerberus
Copy link
Contributor

Did some digging on this in my debugger - ultimately core-js gets to here:

return fn.apply(that, arguments);

Where:

  • that is Float32Array.prototype.toString
  • fn is Function.prototype.call
  • arguments.length is 0 (!)

Which is ultimately equivalent to doing:

Float32Array.prototype.toString.call();

Which ends in a TypeError. Unfortunately I wasn't able to trace what lead to the function getting called with zero arguments in the first place. The callstack gave no clues; console.log() was the only thing above the offending call.

@bisubus
Copy link
Author

bisubus commented May 3, 2017

@fatcerberus Good one. Actually, I was able to replicate this with just

require('core-js'); 
Float32Array.toString();

It appears that console.log stringifies arguments in quirky way. If the first argument isn't a string, then everything is fine ( in this case it likely calls valueOf instead of toString on all arguments).

@fatcerberus
Copy link
Contributor

Found something else suspicious. The whole .toString() factory:

	module.exports = function(fn, that, length){
	  aFunction(fn);
	  if(that === undefined)return fn;
	  switch(length){
	    case 1: return function(a){
	      return fn.call(that, a);
	    };
	    case 2: return function(a, b){
	      return fn.call(that, a, b);
	    };
	    case 3: return function(a, b, c){
	      return fn.call(that, a, b, c);
	    };
	  }
	  return function(/* ...args */){
	    return fn.apply(that, arguments);
	  };
	};

length is undefined!

@fatcerberus
Copy link
Contributor

I tried setting a breakpoint in the outer factory function, but it didn't get hit. I'll see if I can get to the bottom of this later, it's quite a curious bug.

@fatcerberus
Copy link
Contributor

fatcerberus commented May 4, 2017

As an experiment, I changed the offending line to:

return fn.apply(that, [this].concat(Array.from(arguments)));

Which caused this error:

TypeError: this is not a typed array.
    at Function.join (native)
    at Function.<anonymous> (C:\temp\node_modules\core-js\modules\_ctx.js:18:15)
    at Function.toString (native)
    at Function.<anonymous> (C:\temp\node_modules\core-js\modules\_ctx.js:18:15)
    at Object.<anonymous> (C:\temp\test.js:2:14)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)

@fatcerberus
Copy link
Contributor

What I've found out so far:

  • Float32Array.toString() improperly calls Float32Array.prototype.toString
  • fn.apply(that, arguments) is surely wrong here because it calls Function.prototype.call with no arguments, which will never work

Unfortunately I've been unable to follow the logic that leads core-js to polyfill the function this way in the first place. 🙁

@bisubus
Copy link
Author

bisubus commented May 6, 2017

Regarding unwanted polyfill, I found that Float32Array seems to not be polyfilled in Node, but it is TypedArray that is likely polyfilled.

Without core-js the error message is

RangeError: start offset of Float32Array should be a multiple of 4

And with

require('core-js');

the error message becomes

RangeError: Wrong offset!

Much less informative.

@fatcerberus
Copy link
Contributor

fatcerberus commented May 7, 2017

Hm, can't reproduce that behavior in Node 7.10.0:

require('core-js');
new Float32Array(new ArrayBuffer(8), 3);
C:\temp>node pig
C:\temp\pig.js:2
new Float32Array(new ArrayBuffer(8), 3);
^

RangeError: start offset of Float32Array should be a multiple of 4
    at new Float32Array (native)
    at Object.<anonymous> (C:\temp\pig.js:2:1)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:427:7)
    at startup (bootstrap_node.js:151:9)

The only thing I can think of is that you're using an older version of Node that's not fully standards compliant, triggering the polyfill. I indeed get the "wrong offset" error under Duktape, but not Node.

@bisubus
Copy link
Author

bisubus commented May 7, 2017

@fatcerberus Yes, you are right, I ran this code in Node 6.10.3 (as opposed to the example listed in the OP which ran in 7.2.0), this is the reason why I've got different behaviour, but it still isn't clear why core-js decided that it should be polyfilled, node.green cannot explain that.

@fatcerberus
Copy link
Contributor

@zloirock What was ultimately the problem? This bug was really crazy and I couldn't figure out what the cause was from the diff.

@zloirock
Copy link
Owner

@fatcerberus the problem was in $export core-js logic. Typed arrays polyfill contains a fix for %TypedArray%.prorotype.toString, but because of a problem in $export, %TypedArray%.toString was replaced to something like () => Function.prototype.call.apply(%TypedArray%.prototype.toString, arguments).

@Anoxy
Copy link

Anoxy commented May 7, 2018

how can this be fixed ? still got error in ie11

@zloirock
Copy link
Owner

zloirock commented May 7, 2018

@Anoxy by using actual versions. If it does not help, please, create a new issue with a reproducible example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants