automatic unwrapping

[metamatt]

I believe this behavior was introduced in harmony-reflect v0.0.5 while fixing #13.

The auto-unwrapping assumes Object.prototype.toString will only be called on objects. But lots of code calls it on other types to figure out what type they're dealing with.

Here's what one would normally expect for this behavior:

magi@ubuntu ~/s/d/foo> node --harmony
> ({}).toString.apply(1)
'[object Number]'
> ({}).toString.apply(true)
'[object Boolean]'
> ({}).toString.apply('asdf')
'[object String]'
> ({}).toString.apply(null)
'[object Null]'
> ({}).toString.apply(undefined)
'[object Undefined]'
> ({}).toString.apply([])
'[object Array]'
> ({}).toString.apply({})
'[object Object]'

Here's what you get with harmony-proxies v0.0.5:

magi@ubuntu ~/s/d/foo> node --harmony
> require('harmony-reflect')
{ snip }
> ({}).toString.apply(1)
TypeError: Invalid value used as weak map key
    at WeakMap.get (native)
    at Number.builtin (/home/magi/node_modules/harmony-reflect/reflect.js:1500:34)
    at repl:1:16
> ({}).toString.apply(true)
TypeError: Invalid value used as weak map key
    at WeakMap.get (native)
    at Boolean.builtin (/home/magi/node_modules/harmony-reflect/reflect.js:1500:34)
    at repl:1:16
> ({}).toString.apply('asdf')
TypeError: Invalid value used as weak map key
    at WeakMap.get (native)
    at String.builtin (/home/magi/node_modules/harmony-reflect/reflect.js:1500:34)
    at repl:1:16
> ({}).toString.apply(null)
TypeError: Invalid value used as weak map key
    at WeakMap.get (native)
    at builtin (/home/magi/node_modules/harmony-reflect/reflect.js:1500:34)
    at repl:1:16
> ({}).toString.apply(undefined)
TypeError: Invalid value used as weak map key
    at WeakMap.get (native)
    at builtin (/home/magi/node_modules/harmony-reflect/reflect.js:1500:34)
    at repl:1:16
> ({}).toString.apply([])
'[object Array]'
> ({}).toString.apply({})
'[object Object]'

[metamatt]

The offending call to WeakMap.get is here

function makeUnwrapping0ArgMethod(primitive) {
  return function builtin() {
    var vHandler = directProxies.get(this);
    if (vHandler !== undefined) {
      return builtin.call(vHandler.target);
    } else {
      return primitive.call(this);
    }
  }
};

But there are a number of other identical var vHandler = directProxies.get(this); instances that are also reachable with this not being an object (Array.isArray, and Object.prototype.isPrototypeOf exposes makeUnwrapping1ArgMethod).

So I suggest a patch something like

-    var vHandler = directProxies.get(this);
+    var vHandler;
+    if (typeof this === 'object' && this !== null) {
+      vHandler = directProxies.get(this);
+    }

at each of those call sites, though perhaps you have a better way of specifying that test.

I'll send a PR which you are free to accept or improve upon.

[ghost]

You'll need to account for functions, since this is also used on Function.prototype.toString. Something like:

function isObject(o){
  var type = typeof o;
  return type === 'object' ? o !== null : type === 'function';
}

[metamatt]

Thank you -- that's right.

I don't have a super deep background in Javascript, so I'll ask: is there a known way of nesting these checks in the right order for best performance, or is it ok (from a performance perspective) to just call WeakMap.get and let it figure out what's an object and throw errors which we catch, like:

try {
   vHandler = directProxies.get(this);
} catch (err) {
   vHandler = undefined;
}

[tvcutsem]

@metamatt thanks for the PR. Couldn't have done it better myself. I merged it, added unit tests to testRegression.js and updated the NPM package.

[metamatt]

Thanks Tom!