Chakra should throw TypeError when try to set property of an object which has only a getter

[igor-simoes]

Hi everyone, I found an inconsistency when we try to set a getter-only property of an object.

OS: Ubuntu 16.04 x86 Chakra: 1.9

Step to reproduce:

try{
    var a = {get "0"() {return 1;}, length: 1};
    [].fill.call(a, 2);
    throw new Error("should not reach this point")
}
catch(error){
    if (!(error instanceof TypeError)) throw new Error('Should throw TypeError')
}

Actual results: Error: Should throw TypeError

Expected results: pass without errors

V8, SpiderMonkey and JavascriptCore throws a TypeError when attempted to assign to readonly property.

cinfuzz

[MSLaguana]

Looks like the problem is we are considering the whole prototype chain to maybe find a setter, rather than discovering the non-settable property and stopping there (throwing an error).

[MSLaguana]

In fact, it looks like it's to do with how we handle the shorthand getter there. If you change your declaration of a to be var a = {length: 1}; Object.defineProperty(a, "0", {get: function () { return 1;}, enumerable: true, configurable: true}); then it works as expected.

[MSLaguana]

Actually, I had tweaked the case when I was playing around and I mislead myself. The actual difference here is that the Array.prototype.fill method is strict in v8, and non-strict in chakracore.

[akroshg]

In https://tc39.github.io/ecma262/#sec-ordinarysetwithowndescriptor at 6. If setter is undefined, return false. But we don't seem to do that at

ES5ArrayTypeHandlerBase<T>::SetItem
...
            else if (descriptor->Setter)
            {
                RecyclableObject* func = RecyclableObject::FromVar(descriptor->Setter);
                // TODO : request context
                JavascriptOperators::CallSetter(func, instance, value, NULL);
            }

            return true;

APIs other than .fill may exhibit the same problem.

[dilijev]

Confirmed repro on Windows x64

## Source
try{
    var a = {get "0"() {return 1;}, length: 1};
    [].fill.call(a, 2);
    throw new Error("should not reach this point")
}
catch(error){
    if (!(error instanceof TypeError)) throw new Error('Should throw TypeError')
}


#### jsvu-d8, jsvu-jsc, jsvu-sm


#### jsvu-ch
Error: Should throw TypeError

[rhuanjl]

I had a quick look at this, and found two issues to solve in order to resolve it, I don't have a fix to submit but thought I'd write up for now in case it's useful:

1. similar to #5046 the set instruction reaches this point and finds no code to handle the situation of there not being a setter so just returns true: https://github.com/Microsoft/ChakraCore/blob/fb2dbf1469189d42cf6859401b553d692a962566/lib/Runtime/Types/ES5ArrayTypeHandler.cpp#L401-L412

2. Even if you return false there the implementation of array.prototype.fill actually discards the return values of the set calls: https://github.com/Microsoft/ChakraCore/blob/master/lib/Runtime/Library/JavascriptArray.cpp#L9205-L9265

Point 1 is an easy fix; point 2 I'm not so sure wrapping every SetItem call in an if(!setItem){throw...} could be a noticeable de-optimisation. Alternatively calling the SetItem calls specifying strict mode (along with putting in a CantAssign call at the write place in the set) results in the wrong error message being thrown.

Other note I checked the spec - this is non-compliant: https://github.com/Microsoft/ChakraCore/blob/master/lib/Runtime/Library/JavascriptArray.cpp#L9205-L9265 point 7.b specifies that Set should be called with the 4th parameter being true then: https://github.com/Microsoft/ChakraCore/blob/master/lib/Runtime/Library/JavascriptArray.cpp#L9205-L9265 Shows that set with its 4th parameter being true should throw if the set fails.

[ptomato]

Here's another very simple reproducer:

Object.defineProperty(Object.prototype, 0, { get() {} });
var arr = new Array(10);
arr[1] = 123;
arr.shift();

There are actually two tests in the test suite that test for the wrong behaviour here: Array/Array_TypeConfusion_bugs.js and Array/protoLookupWithGetters.js