1395095 - MappedArgumentsObject::obj_defineProperty should not create slotful getters/setters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

I have a patch to remove JSPROP_SHARED and slotful getters/setters, but it fails some |arguments| tests.

The problem is that MappedArgumentsObject::obj_defineProperty unconditionally adds MappedArgGetter/MappedArgSetter, even if the descriptor has a different value. We were also missing a spec step (5.a.) - maybe that was added after we wrote this code?

With slotful getters/setters this currently just happens to work, but it's pretty complicated. This patch fixes all test failures.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 8902604 [details] [diff] [review]
Patch

Actually this is buggy still I think.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

This works better. I also added a test that failed with the previous patch.

Not all of the newArgDesc modifications are strictly necessary, but I think explicitly clearing the value or getter/setter makes it easier to understand.

Comment 3

[Tom S. (please needinfo tschuster)]

Comment on attachment 8902619 [details] [diff] [review]
Patch

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

I think this should work, but it's hard to be completely sure with this insanity.

::: js/src/vm/ArgumentsObject.cpp
@@ +656,4 @@
>      if (!desc.isAccessorDescriptor() && isMapped) {
> +        // Step 5.a.
> +        if (desc.hasWritable() && !desc.writable()) {
> +            if (!desc.hasValue()) {

This isn't really necessary in our implementation, but follows the spec.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3094f515e80
Fix MappedArgumentsObject::obj_defineProperty to not create slotful getters/setters. r=evilpie

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c3094f515e80