1175823 - Updating a mapped arguments property with [[DefineOwnProperty]] should also change the value of its corresponding formal parameter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[ziyunfei]

Attachment: JSC, SM, V8

(function(a) {
    Object.defineProperty(arguments, "0", {
        value: 2
    })
    console.log(a) // 1, should be 2
    Object.defineProperty(arguments, "0", {
        value: 3,
        writable: false,
    })
    console.log(a) // 1, should be 3
    Object.defineProperty(arguments, "0", {
        value: 4
    })
    console.log(a) // 1, should be 3
})(1)

Comment 1

[Tom S [:evilpie]]

Attachment: [[DefineProperty]] implementation that doesn't work

By overwriting the MappedArgSetter we fail all the following simple assignments. I tried adding back the setter/getter, but this obviously fails if the user just redefined the property as non-configurable.

Comment 2

[Tom S [:evilpie]]

A while ago Jason suggest to just replacing what is defined with MappedArgSetter/MappedArgGetter, however this doesn't directly work, because we have to check that descriptor the user wants to define is actually valid etc. 

I think we can either move some of that overwriting behavior to NativeDefineProperty directly or maybe copy paste some of the spec steps for verifying a descriptor to the new [[DefineProperty]] implementation.

Comment 3

[Tom S [:evilpie]]

Attachment: Import test262 mapped arguments tests

Thanks André! This really helped to show that my previous approach didn't work for some special cases.

Comment 4

[Tom S [:evilpie]]

Attachment: Update arguments correctly when using defineProperty

This patch updates the property descriptor before we actually change the shape with NativeObject::putProperty, this allows us to keep the MappedArgSetter/Getter in the cases where the arguments is supposed to stay mapped. We also need to actual set the bindings!

Comment 5

[Tom S [:evilpie]]

I think I just found a solution that doesn't involve changing NativeDefineProperty. We can instead just call setGetter/setSetter from MappedArgumentsObject::obj_defineProperty.

Reviewing is taking long anyway.

Comment 6

[Tom S [:evilpie]]

Attachment: Implement [[DefineOwnProperty]] for mapped arguments object

I will land a test for the SetIntegrityLevel change in bug 1286629.

This is a pretty straightforward port of 9.4.4.2. Two interesting bits: 5.a. seems to be unnecessary, because NativeDefineProperty will lookup the existing property value in the generic descriptor case.

The .setGetter/.setSetter call make basically no difference when executing NativeDefineProperty (in fact of course per ES, data properties should not have getters or setters). But if we forget to add those two explicitly, the shape change for the mapped properties will stop including those getters/setters, which includes the live updating behavior that is specified per [[Set]] in ES.

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8812882 [details] [diff] [review]
Implement [[DefineOwnProperty]] for mapped arguments object

Review of attachment 8812882 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/ArgumentsObject.cpp
@@ +590,4 @@
>      return true;
>  }
>  
> +/* static */ bool

would be nice to have spec section comment (ES 2017 draft 9.4.4.2) above.

@@ +600,5 @@
> +    // Steps 2-3.
> +    bool isMapped = false;
> +    if (JSID_IS_INT(id)) {
> +        unsigned arg = unsigned(JSID_TO_INT(id));
> +        isMapped = arg < argsobj->initialLength() && !argsobj->isElementDeleted(arg);

It would be nice to move this condition to ArgumentObject's method.
there are already several places with same condition, or inverted condition.
maybe in new bug, or followup patch.

@@ +612,5 @@
> +        newArgDesc.setGetter(MappedArgGetter);
> +        newArgDesc.setSetter(MappedArgSetter);
> +    }
> +
> +    // Step. 5-6. NativeDefineProperty will lookup [[Value]] for us.

Steps 5-6

@@ +624,5 @@
> +    if (isMapped) {
> +        RootedFunction callee(cx, &argsobj->callee());
> +        RootedScript script(cx, callee->getOrCreateScript(cx));
> +        if (!script)
> +            return false;

above 4 lines can be moved inside |if (desc.hasValue())| block

Comment 8

[André Bargull [:anba]]

Comment on attachment 8803051 [details] [diff] [review]
Import test262 mapped arguments tests

Review of attachment 8803051 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/189500d81aad
Implement [[DefineOwnProperty]] for mapped arguments object. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/3200aaa75fc2
Import test262 mapped arguments tests. r=anba

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/189500d81aad
https://hg.mozilla.org/mozilla-central/rev/3200aaa75fc2