[Webkit-unassigned] [Bug 159398] [test262] Fixing mapped arguments object property test case

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=159398

--- Comment #26 from Caio Lima <ticaiolima at gmail.com> ---
Hey Geoffrey, thanks for the review.

(In reply to comment #25)
> Comment on attachment 283766 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283766&action=review
> 
> It looks like you've invented a way for the arguments object to override an
> indexed property by storing a value in its JSObject storage. The arguments
> object already had a different way to indicate that an indexed property had
> been overridden: the "overrideArgument" mechanism. Did you consider using
> the overrideArgument mechanism? What's better about this new mechanism?
> That's a good thing to explain in your ChangeLog.
> 
> I suspect that it might be better to use the overrideArgument mechanism, or
> to augment it for this purpose.

Actually I think arguments mapping quite confusing. I didn't see a way to use the current overrideArgument, because what I want is avoid the override in some cases. Check the following test262 test case https://raw.githubusercontent.com/tc39/test262/master/test/language/arguments-object/mapped/mapped-arguments-nonconfigurable-nonwritable-3.js

The point is that before "Object.defineProperty(arguments, "0", {writable: false});" the arguments object is still aliased. It changed in ES6 comparing to ES5 (check the bug in specification https://bugs.ecmascript.org/show_bug.cgi?id=4371). In our current implementation, the arguments alias is overwritten even if writable isn't present. This way, I added comparison to check if writable is not present and if it isn't, don't override this property (which makes sense, since default writable is false).

Also, I added a check in deletePropertyByIndex if non-configurable, because if it is "delete"shall throw an error in strict mode or false in non-strict mode.

> > Source/JavaScriptCore/ChangeLog:9
> > +        mapped arguments object with non-configurable and no-writable property
> 
> non-writable
> 
> > Source/JavaScriptCore/ChangeLog:10
> > +        cases. Also it is fixing cases where arguments[i] cannot be deleted 
> 
> fixes
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        when argument "i" is not configurable. The test cases added are from
> 
> from the
> 
> > Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:222
> > +                    object->putDirectMayBeIndex(exec, ident, value);
> > +
> > +                    return Base::defineOwnProperty(object, exec, ident, descriptor, shouldThrow);
> 
> This looks wrong. You're assigning the property twice -- first by
> putDirectMayBeIndex and second by defineOwnProperty.

I agree with you that it looks ugly, however it happens without my Patch too. Check the return line of GenericArguments<Type>::defineOwnProperty and it calls defineOwnProperty after object->putDirectMayBeIndex(exec, ident, value). I need do this because of 2 reasons:

1. I need to update the property descriptor, mainly because it is used in GenericArguments<Type>::deletePropertyBy(Index/Id). As the descriptor maybe contains a new property, It is safe use defineOwnProperty IMHO;

2. If I don't set object->putDirectMayBeIndex(exec, ident, value) before defineOwnProperty it throws an error because of:

```
// 10.a.ii. If the [[Writable]] field of current is false, then
                // 10.a.ii.1. Reject, if the [[Value]] field of Desc is present and SameValue(Desc.[[Value]], current.[[Value]]) is false.
                if (descriptor.value() && !sameValue(exec, descriptor.value(), current.value()))
```

Is these things clear to you?

Sorry for the confusion and miss explaining.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160727/fdf91aea/attachment-0001.html>