Incorrect realization of SetItem() functions

[sunlili]

Hello, The following code behaves incorrectly (inconsistent with the standard and other engines) in Chakra due to incorrect realization of SetItems() functions:

var y = 0;
var t = [1,2,3];

var mp = new Proxy([], {
  get: function (oTarget, sKey) {
    print("get " + sKey.toString());
    y = y + 1;
    print("y = " + y);
    if(y == 2){
        var temp = [];
        t.length = 20;
        for (var i=0; i<20; i++)
            t[i] = 0x10;
        return 5;
    }
    if(y > 2) {
        return oTarget[sKey];
    }
    return oTarget[sKey] || undefined;
    //return oTarget[sKey] || oTarget.getItem(sKey) || undefined;
  },
  set: function (oTarget, sKey, vValue) {
    print("set " + sKey);
    if (sKey in oTarget) { return false; }
    return oTarget[sKey] = vValue;
  },

});

function f(a){
    print(a);
}

t.length = 4;
t.__proto__ = mp;

f(...t);

the 't[i] = 0x10;' in get() should call set() when i is undefined, however, in Chakra, t[i] is assigned directly.

[dilijev]

Confirmed difference in behavior:

>eshost -its issue3187.js
## Source
var y = 0;
var t = [1,2,3];

var mp = new Proxy([], {
  get: function (oTarget, sKey) {
    print("get " + sKey.toString());
    y = y + 1;
        print("y = " + y);
    if(y == 2){
        var temp = [];
                t.length = 20;
                for (var i=0; i<20; i++)
                        t[i] = 0x10;
        return 5;
    }
        if(y > 2) {
                return oTarget[sKey];
        }
        return oTarget[sKey] || undefined;
    //return oTarget[sKey] || oTarget.getItem(sKey) || undefined;
  },
  set: function (oTarget, sKey, vValue) {
    print("set " + sKey);
    if (sKey in oTarget) { return false; }
    return oTarget[sKey] = vValue;
  },

});

function f(a){
    print(a);
}

t.length = 4;
t.__proto__ = mp;

f(...t);

┌────────────┬─────────────────────────────┐
│ d8         │ get Symbol(Symbol.iterator) │
│ sm         │ y = 1                       │
│ node       │ get 3                       │
│            │ y = 2                       │
│            │ set 3                       │
│            │ set 4                       │
│            │ set 5                       │
│            │ set 6                       │
│            │ set 7                       │
│            │ set 8                       │
│            │ set 9                       │
│            │ set 10                      │
│            │ set 11                      │
│            │ set 12                      │
│            │ set 13                      │
│            │ set 14                      │
│            │ set 15                      │
│            │ set 16                      │
│            │ set 17                      │
│            │ set 18                      │
│            │ set 19                      │
│            │ get 4                       │
│            │ y = 3                       │
│            │ get 5                       │
│            │ y = 4                       │
│            │ get 6                       │
│            │ y = 5                       │
│            │ get 7                       │
│            │ y = 6                       │
│            │ get 8                       │
│            │ y = 7                       │
│            │ get 9                       │
│            │ y = 8                       │
│            │ get 10                      │
│            │ y = 9                       │
│            │ get 11                      │
│            │ y = 10                      │
│            │ get 12                      │
│            │ y = 11                      │
│            │ get 13                      │
│            │ y = 12                      │
│            │ get 14                      │
│            │ y = 13                      │
│            │ get 15                      │
│            │ y = 14                      │
│            │ get 16                      │
│            │ y = 15                      │
│            │ get 17                      │
│            │ y = 16                      │
│            │ get 18                      │
│            │ y = 17                      │
│            │ get 19                      │
│            │ y = 18                      │
│            │ 1                           │
├────────────┼─────────────────────────────┤
│ ch-1.5.0   │ get Symbol(Symbol.iterator) │
│ ch-1.3.2   │ y = 1                       │
│ ch-1.2.3   │ get 3                       │
│ ch-master  │ y = 2                       │
│ ch-1.4.4   │ 1                           │
│ node-ch    │                             │
├────────────┼─────────────────────────────┤
│ jsc        │ 1                           │
└────────────┴─────────────────────────────┘

(Note jsc is from Feb 2017 so it might be fixed there.)

[dilijev]

@bterlson could you track down the relevant spec sections and determine severity of this issue?

[bterlson]

Minimal-ish repro:

var obj = [];
var p = new Proxy({}, {
  get(t, k) {
    print('Getting', k);
    return Reflect.get(t, k);
  }, 
  set(t, k, v) {
    print('Setting', k);
    return Reflect.set(t, k, v);
  }
});
Object.setPrototypeOf(obj, p);
obj[0] = 3;

Only repros when target is an array. I'll do a spec dig if I get time. It seems like we're pretending that the target array has an own property already that we can set, rather than doing the proto walk looking for possible setters... it's possible one needs to do the right thing with has() as well for this to work.

[sunlili]

Can I get a bug number after report this bug?

[dilijev]

@sunlili We will track the status of this issue here on GitHub.

[bterlson]

My spec read:

12.15.4 AssignmentExpression Evaluation evaluates LHS to reference with bv of obj, name of 0 per 12.3.2.1 MemberExpressionEvaluation --> 6.2.4.9 PutValue step 6.b --> 9.1.9 ordinary [[Set]] (arrays don't have a specialization here) --> 9.1.9.1 Ordinary Set step 3.b.i --> 9.5.9 Proxy Set which should trigger the trap.

Hence, should be fixed :)

[kfarnung]

@dilijev Can you verify whether this is still an issue?

[dilijev]

There is still a spread in implementations but different from before.

## Source
var y = 0;
var t = [1,2,3];

var mp = new Proxy([], {
  get: function (oTarget, sKey) {
    print("get " + sKey.toString());
    y = y + 1;
        print("y = " + y);
    if(y == 2){
        var temp = [];
                t.length = 20;
                for (var i=0; i<20; i++)
                        t[i] = 0x10;
        return 5;
    }
        if(y > 2) {
                return oTarget[sKey];
        }
        return oTarget[sKey] || undefined;
    //return oTarget[sKey] || oTarget.getItem(sKey) || undefined;
  },
  set: function (oTarget, sKey, vValue) {
    print("set " + sKey);
    if (sKey in oTarget) { return false; }
    return oTarget[sKey] = vValue;
  },

});

function f(a){
    print(a);
}

t.length = 4;
t.__proto__ = mp;

f(...t);

#### ch-master-latest, jsvu-ch
get Symbol(Symbol.iterator)
y = 1
get 3
y = 2
1

#### jsvu-d8
get Symbol(Symbol.iterator)
y = 1
1

#### jsvu-jsc, jsvu-sm
get Symbol(Symbol.iterator)
y = 1
get 3
y = 2
set 3
set 4
set 5
set 6
set 7
set 8
set 9
set 10
set 11
set 12
set 13
set 14
set 15
set 16
set 17
set 18
set 19
get 4
y = 3
get 5
y = 4
get 6
y = 5
get 7
y = 6
get 8
y = 7
get 9
y = 8
get 10
y = 9
get 11
y = 10
get 12
y = 11
get 13
y = 12
get 14
y = 13
get 15
y = 14
get 16
y = 15
get 17
y = 16
get 18
y = 17
get 19
y = 18
1

[dilijev]

And the minimal repro:

## Source
var obj = [];
var p = new Proxy({}, {
  get(t, k) {
    print('Getting', k);
    return Reflect.get(t, k);
  },
  set(t, k, v) {
    print('Setting', k);
    return Reflect.set(t, k, v);
  }
});
Object.setPrototypeOf(obj, p);
obj[0] = 3;
print("DONE");

#### ch-master-latest, jsvu-ch
DONE

#### jsvu-d8, jsvu-jsc, jsvu-sm
Setting 0
DONE

[tcare]

Had a look at this. The fix seems simple but has some potential perf implications.

It's currently not easy to disable the optimization that bypasses the set trap on a per object basis. For prototype objects with getters/setters we disable this globally per scriptContext since it's simpler and we don't expect it to happen much in real code.

Proxies are different because we do expect this pattern to be common. Doing this globally would be easy but would affect perf. Doing this on a more granular level e.g. like nonwritable attributes in prototype chain is more difficult and not a bug level fix.

I will do some investigation to see the impact of the global fastpath disable on the web and then decide which way to go. Either way, this won't be done in time for 1.10.