Closed Bug 1175823 Opened 5 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox41 --- affected
firefox53 --- fixed

People

(Reporter: 446240525, Assigned: evilpie)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [DocArea=JS])

Attachments

(3 files, 2 obsolete files)

Attached image 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)
Assignee: nobody → evilpies
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.
Attachment #8798891 - Attachment is patch: true
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.
Thanks André! This really helped to show that my previous approach didn't work for some special cases.
Attachment #8798891 - Attachment is obsolete: true
Attachment #8803051 - Flags: review?(jorendorff)
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!
Attachment #8803052 - Flags: review?(jorendorff)
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.
Attachment #8803052 - Flags: review?(jorendorff)
Attachment #8803051 - Flags: review?(jorendorff)
Blocks: test262
Attachment #8803052 - Attachment is obsolete: true
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.
Attachment #8812882 - Flags: review?(arai.unmht)
Attachment #8803051 - Flags: review?(andrebargull)
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
Attachment #8812882 - Flags: review?(arai.unmht) → review+
Comment on attachment 8803051 [details] [diff] [review]
Import test262 mapped arguments tests

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

\o/
Attachment #8803051 - Flags: review?(andrebargull) → review+
Pushed by evilpies@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/189500d81aad
https://hg.mozilla.org/mozilla-central/rev/3200aaa75fc2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1286629
Duplicate of this bug: 594495
Depends on: 1360277
You need to log in before you can comment on or make changes to this bug.