[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 #71 from Caio Lima <ticaiolima at gmail.com> ---
Comment on attachment 290583
  --> https://bugs.webkit.org/attachment.cgi?id=290583
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290583&action=review

>> Source/JavaScriptCore/ChangeLog:11
>> +        cannot be deleted when argument "i" is {configurable: false}.
> 
> I think you mean you're making it so that arguments[i] can't be deleted, correct? I would maybe write "fixing cases where" => "ensuring that"

OK.

>> Source/JavaScriptCore/ChangeLog:13
>> +        The current implementation is against to the specification for 2 reasons:
> 
> I would say "previous" instead of "current" if you're referring to the behavior that is ToT now and what will be the old behavior as your patch lands.

Ok.

>> Source/JavaScriptCore/runtime/DirectArguments.cpp:90
>> +    size_t configurablesSize = thisObject->m_configurableMap ? thisObject->m_length : 0;
> 
> Please make this * sizeof(bool). I would also argue that the overridesSize variable above should do the same. I'm not a fan of the style assuming what sizeof(bool) is.

Agreed.

>> Source/JavaScriptCore/runtime/GenericArguments.h:66
>> +    static bool canDeletePropertyByIndex(Type*, ExecState*, unsigned);
> 
> These aren't implemented anywhere. Please delete.

Nice catch. Sorry.

>> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:212
>> +                thisObject->setConfigurable(vm, index, descriptor.configurable());
> 
> We're guaranteed that it's already configurable before we call this? It seems like maybe we should bail out somewhere if the property is already non-configurable.
> If i had to guess, maybe this program exhibits the bug:
> function foo(x) {
>       Object.defineProperty(arguments, 0, {configurable:false, writable:true, value:20});
>       Object.defineProperty(arguments, 0, {configurable:true, writable:true, value:50}); // Does this succeed?
> }
> foo("foo")

My guess is that ```Object.defineProperty(arguments, 0, {configurable:true, writable:true, value:50});``` throws TypeError, since we call ```Base::defineOwnProperty``` afterwards. However, I suspect that 
```Object.defineProperty(arguments, 0, {configurable:true, writable:true, get: () => {return 50}});``` works and should throw a TypeError instead.

>> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:214
>> +            // Just overrride arguments (i.e finish aliasing) if its descriptor contains {writable: false}.
> 
> What do you mean by "finish aliasing" here?

I mean stop mapping. I am changing aliasing from all places that I put it.

>> JSTests/microbenchmarks/super-getter.js:1
>> +class B {
> 
> Please remove this file.

Sorry...

-- 
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/20161009/618b3aa4/attachment.html>