753402 - Harmony proxies: Weird behaviour on objects when Object.prototype.__proto__ is a proxy.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Noticed this behaviour when debugging a ASSERT in Ion.  Here's the test code:

CODE:
-----
var counter = 0;
var p = Proxy.create({
    has : function(id) {
        print("HAS: counter=" + counter + " id=" + id);
        ++counter;
        if (counter == (12)) {
            print("HAS: here");
            obj.__proto__ = null;
            print("HAS: here2");
            return true;
        }
    },
    get : function(id) {
        print("HAS: counter=" + counter + " id=" + id);
        if (id == 'xyz')
            return 10;
    }
});

obj = { xyz: 1};
for (var i = 0; i != 100; ++i) {
    print("before obj.xyz (i=" + i + ")");
    var s = obj.xyz;
    print("after obj.xyz (i=" + i + ")");
    if (i == 10) {
        print("HERE1");
        delete obj.xyz;
        Object.prototype.__proto__ = p;
    }
}
-----

OUTPUT:
-----
after obj.xyz (i=0)
before obj.xyz (i=1)
after obj.xyz (i=1)
before obj.xyz (i=2)
after obj.xyz (i=2)
before obj.xyz (i=3)
after obj.xyz (i=3)
before obj.xyz (i=4)
after obj.xyz (i=4)
before obj.xyz (i=5)
after obj.xyz (i=5)
before obj.xyz (i=6)
after obj.xyz (i=6)
before obj.xyz (i=7)
after obj.xyz (i=7)
before obj.xyz (i=8)
after obj.xyz (i=8)
before obj.xyz (i=9)
after obj.xyz (i=9)
before obj.xyz (i=10)
after obj.xyz (i=10)
HERE1
before obj.xyz (i=11)
HAS: counter=0 id=xyz
after obj.xyz (i=11)
before obj.xyz (i=12)
HAS: counter=1 id=xyz
after obj.xyz (i=12)
before obj.xyz (i=13)
HAS: counter=2 id=xyz
after obj.xyz (i=13)
before obj.xyz (i=14)
HAS: counter=3 id=xyz
after obj.xyz (i=14)
before obj.xyz (i=15)
HAS: counter=4 id=xyz
after obj.xyz (i=15)
before obj.xyz (i=16)
HAS: counter=5 id=xyz
after obj.xyz (i=16)
before obj.xyz (i=17)
HAS: counter=6 id=xyz
after obj.xyz (i=17)
before obj.xyz (i=18)
HAS: counter=7 id=xyz
after obj.xyz (i=18)
before obj.xyz (i=19)
HAS: counter=8 id=xyz
HAS: counter=9 id=xyz
after obj.xyz (i=19)
before obj.xyz (i=20)
HAS: counter=10 id=xyz
HAS: counter=11 id=xyz
HAS: here
HAS: here2
:/tmp/test.js:14: TypeError: can't convert id to primitive type
-----


There are a couple of issues here.  One, note that after Object.prototype.__proto__ is changed to point to proxy p, the 'has' function on the handler is invoked.  This seems out of sync with the Harmony spec.

Secondly, note that 'has' is invoked twice on iteration i=19 and i=20 of the loop.  I think this may potentially be a jaeger-compile trigger that's causing that behaviour.

Comment 1

[Jason Orendorff [:jorendorff]]

A short test for the first "has" bug:

  var p = Proxy.create({
      has: function (id) { throw "FAIL"; },
      get: function (id) { return 10; }
  });
  var obj = Object.create(p);
  assertEq(obj.x, 10);

In mozilla-central tip, 'obj.x' triggers a call to the 'has' trap, with this stack:

  js::Proxy::has
  proxy_LookupGeneric
  JSObject::lookupGeneric
  LookupPropertyWithFlagsInline
  js_GetPropertyHelperInline
  js::GetPropertyHelper
  js::GetPropertyOperation

Direct proxies will not fix this.

Comment 2

[Jason Orendorff [:jorendorff]]

And here is a short test for the second bug:

  // |jit-test| mjitalways
  var c = 0;
  var p = Proxy.create({has: function (id) { c++; }});
  var obj = Object.create(p);
  for (var i = 0; i < 4; ++i)
      obj.xyz;
  assertEq(c, 4);

This fails due to some code under GetPropMaybeCached in methodjit/PolyIC.cpp. Toward the bottom of that function there's this:

    if (!monitor.recompiled() && pic->shouldUpdate(f.cx)) {
        GetPropCompiler cc(f, script, obj, *pic, name, stub);
        if (!cc.update())
            THROW();
    }

    Value v;
    if (cached) {
        if (!GetPropertyOperation(f.cx, f.pc(), f.regs.sp[-1], &v))
            THROW();
    } else {
        if (!obj->getProperty(f.cx, name, &v))
            THROW();
    }

The hit trap can be called twice: once from cc.update(), which does a lookup; and then again from obj->getProperty().

I suspect cc.update() should be changed to avoid firing proxy traps, but I can't take this bug.