649567 - Setting arguments.callee changes enumerability of callee property

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

http://samples.msdn.microsoft.com/ietestcenter/Javascript/ES10.6.html

This is from 10.6-13-a-1, and I'm pretty sure it applies both to strict arguments and to non-strict arguments.

Comment 1

[Jeff Walden [:Waldo]]

Also 10.6-7-1 at the same place, for arguments.length (strict and non-strict).

Comment 2

[Tom S [:evilpie]]

Attachment: v1 - propagate attributes

Instead of deleting the property, which fails if the property is made non-configurable before doing eg. arguments.length = 'test';
The last comment in ArgSettter makes me a bit curious, but I think it's correct , after all this is the same as doing Object.defineProperty(arguments, 'length', {value: 'Test'}). The second test is buggy, it explicitly checks toString on a function, without this it works.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 526505 [details] [diff] [review]
v1 - propagate attributes

Looks reasonable, but this needs tests:

* covering strict and non-strict arguments
* covering callee, length, a numeric property less than the number in the enclosing function, a numeric property greater than the number in the enclosing function, and a numeric property equal to the number in the enclosing function
* where the property in question is unaltered, redefined with different attributes, or redefined as immutable (non-configurable, non-writable)

Also, as a sanity check add JS_ASSERT(!(attrs & JSPROP_READONLY)) after the js_GetAttributes calls.

Comment 4

[Tom S [:evilpie]]

Attachment: v2 - propagate attributes

I added the test (had some test, but forgot to hg add it, needed to rewrite anyway). Also added the two asserts.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 527676 [details] [diff] [review]
v2 - propagate attributes

Canceling for now, I see "enumerbale" in here and am going to not look further until that's fixed and any lurking bugs from it smoked out.

Comment 7

[Tom S [:evilpie]]

Attachment: v3

Seems to be good

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 545025 [details] [diff] [review]
v3

Aargh, you lost the tests in this patch.  :-(  Otherwise looks good, just want to see the tests are full, complete, not missing any corner cases, etc.

Comment 9

[Paul Biggar]

Tom, are you still working on this? If not, I can pick it up.

Comment 10

[Tom S [:evilpie]]

Going to finish that, thanks for reminder.

Comment 11

[Tom S [:evilpie]]

So sorry, but i currently don't have the mental power to wrap my head around this stuff. But still believe the code at least is basically correct.

Comment 12

[Brian O'Keefe [:bokeefe]]

Attachment: v4

I had some free time, so I grabbed Tom's v3 patch and updated it to apply to tip. I added back the tests from the v2 patch, too.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 656185 [details] [diff] [review]
v4

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

Thanks for picking this up!  It'll be good to see this oddity fixed now.  In the slightly longer run, hopefully ongoing object/property representation work will eliminate the need for this special-case handling bizarreness.

I see, in contrast to previous patches here, you've preserved the setProperty call in the strict arguments case, rather than replace it with a defineProperty call.  Why is this?  To make that work you have to add a property attributes argument to property-setting.  But the action of setting a property, in the spec, doesn't require specifying property attributes.  Therefore you should go back to the way the previous patches did it -- by defining the property.  Please do this?  (And r- for this reason first and foremost, although the test bits could certainly use the requested loving too.)

::: js/src/jit-test/tests/arguments/args-attributes.js
@@ +1,2 @@
> +var strictArgs = (function (a, b, c) {'use strict'; return arguments; })(1, 2);
> +var normalArgs = (function (a, b, c) { return arguments; })(1, 2);

So.  If I remember right,the mechanism for how ArgSetter works is that it's called on the try-wrapped property assignment.  That replaces the property and triggers the tail defineProperty/setProperty in ArgSetter or StrictArgSetter.  So once the first check() has happened, the relevant property is no longer special.  But this change concerns specifically the special bits in those two methods.  So, really, the |if (orig.configurable)| block isn't testing the important bits at all here.

I think we're going to have to work upon a new object every time, not a single strictArgs object, or a single normalArgs object.  I think these two should be functions, returning the expressions currently used as the values here.  Then you can pass that function into checkProperty, call it to get an object to test against, and can perform each individual test on a fresh arguments object, ensuring you're testing the setter funkiness here each time.  Or perhaps make the argument be "strict" or "non-strict" and have that control whether strictArgs() or normalArgs() is used.

I guess that doesn't test sequences of multiple operations on one of these properties.  Sigh.  (This is why this stuff shouldn't be so special...)  If you want to add tests for such cases, go ahead (perhaps in a separate test file).  If not, I'm not going to put a foot down on requiring them, I think (although I almost want to).

@@ +2,5 @@
> +var normalArgs = (function (a, b, c) { return arguments; })(1, 2);
> +
> +function checkProperty(obj, prop, shouldThrow) {
> +    var desc, orig, threw = false;
> +    

Bugzilla's diff viewer is awesome now!  Trailing whitespace highlighting!  \o/

...ahem.  Please remove this.  :-)

@@ +8,5 @@
> +
> +    function check() {
> +        orig = Object.getOwnPropertyDescriptor(obj, prop);
> +
> +        try {

Set threw to false before this; right now there's contamination of this value from previous check calls.  Actually, why is threw even an outer variable?  Move it inward!  :-)

@@ +18,5 @@
> +        assertEq(threw, shouldThrow, objType + prop + " threw");
> +
> +        if (orig === undefined) {
> +            orig = {value: undefined, writable: true, configurable: true, enumerable: true};
> +        }

Okay, this is a bit screwy -- you're conflating a property with |undefined| as its value, and no such property to begin with.  Could you just return early if there was no property to begin with?  That's much more understandable.

@@ +21,5 @@
> +            orig = {value: undefined, writable: true, configurable: true, enumerable: true};
> +        }
> +
> +        desc = Object.getOwnPropertyDescriptor(obj, prop);
> +        if (orig.value) {

"value" in orig, since it's property presence that's important, not truthy value.  (It doesn't matter here, to be sure, since every value is truthy, but it's more understandable.)

Comment 14

[Brian O'Keefe [:bokeefe]]

Attachment: v5

Comment 15

[Brian O'Keefe [:bokeefe]]

Comment on attachment 657276 [details] [diff] [review]
v5

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> I see, in contrast to previous patches here, you've preserved the
> setProperty call in the strict arguments case, rather than replace it with a
> defineProperty call.  Why is this?

I evidently botched my rebasing of the previous patch. I've fixed this, so a few
extra files went away.

> ::: js/src/jit-test/tests/arguments/args-attributes.js
> @@ +1,2 @@
> > +var strictArgs = (function (a, b, c) {'use strict'; return arguments; })(1, 2);
> > +var normalArgs = (function (a, b, c) { return arguments; })(1, 2);
> 
> So.  If I remember right,the mechanism for how ArgSetter works is that it's
> called on the try-wrapped property assignment.  That replaces the property
> and triggers the tail defineProperty/setProperty in ArgSetter or
> StrictArgSetter.  So once the first check() has happened, the relevant
> property is no longer special.  But this change concerns specifically the
> special bits in those two methods.  So, really, the |if (orig.configurable)|
> block isn't testing the important bits at all here.
> 
> I think we're going to have to work upon a new object every time, not a
> single strictArgs object, or a single normalArgs object.  I think these two
> should be functions, returning the expressions currently used as the values
> here.  Then you can pass that function into checkProperty, call it to get an
> object to test against, and can perform each individual test on a fresh
> arguments object, ensuring you're testing the setter funkiness here each
> time.  Or perhaps make the argument be "strict" or "non-strict" and have
> that control whether strictArgs() or normalArgs() is used.

It didn't occur to me that that would be an issue, but it seems pretty obvious
now. The test now gets a new arguments object before each defineProperty().

> I guess that doesn't test sequences of multiple operations on one of these
> properties.  Sigh.  (This is why this stuff shouldn't be so special...)  If
> you want to add tests for such cases, go ahead (perhaps in a separate test
> file).  If not, I'm not going to put a foot down on requiring them, I think
> (although I almost want to).

I added tests for that case. I kept them in the same file, because most of the
logic was the same. I can move them if you'd prefer.

> Bugzilla's diff viewer is awesome now!  Trailing whitespace highlighting! 
> \o/
> 
> ...ahem.  Please remove this.  :-)

Sigh... I thought I got all of those. Fixed.

> @@ +8,5 @@
> > +
> > +    function check() {
> > +        orig = Object.getOwnPropertyDescriptor(obj, prop);
> > +
> > +        try {
> 
> Set threw to false before this; right now there's contamination of this
> value from previous check calls.  Actually, why is threw even an outer
> variable?  Move it inward!  :-)

Fixed.

> @@ +18,5 @@
> > +        assertEq(threw, shouldThrow, objType + prop + " threw");
> > +
> > +        if (orig === undefined) {
> > +            orig = {value: undefined, writable: true, configurable: true, enumerable: true};
> > +        }
> 
> Okay, this is a bit screwy -- you're conflating a property with |undefined|
> as its value, and no such property to begin with.  Could you just return
> early if there was no property to begin with?  That's much more
> understandable.

Fixed.

> @@ +21,5 @@
> > +            orig = {value: undefined, writable: true, configurable: true, enumerable: true};
> > +        }
> > +
> > +        desc = Object.getOwnPropertyDescriptor(obj, prop);
> > +        if (orig.value) {
> 
> "value" in orig, since it's property presence that's important, not truthy
> value.  (It doesn't matter here, to be sure, since every value is truthy,
> but it's more understandable.)

And, fixed.

Thanks for reviewing :)

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 657276 [details] [diff] [review]
v5

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

Excellent!  I'll add this to my patch queue and push it the next time I have a few things ready to go.  Thanks for picking this up!  Are you looking for anything else interesting to do, by chance?  http://www.joshmatthews.net/bugsahoy/?jseng=1 has various suggestions for more intro-ish JS engine work that needs doing, and the #jsapi channel on irc.mozilla.org stands ready to make further suggestions, or to answer any questions you might have.

Comment 17

[Brian O'Keefe [:bokeefe]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> Excellent!  I'll add this to my patch queue and push it the next time I have
> a few things ready to go.  Thanks for picking this up!

You're welcome! Also, assigned to myself in case I burn the tree.

> Are you looking for
> anything else interesting to do, by chance? 
> http://www.joshmatthews.net/bugsahoy/?jseng=1 has various suggestions for
> more intro-ish JS engine work that needs doing, and the #jsapi channel on
> irc.mozilla.org stands ready to make further suggestions, or to answer any
> questions you might have.

I'll have a look at the list. Thanks!

Comment 18

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a458c2ce253b

Thanks again!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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