Bug 159398 – [test262] Fixing mapped arguments object property test case

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caio Lima]

Make the test case https://raw.githubusercontent.com/tc39/test262/master/test/language/arguments-object/mapped/mapped-arguments-nonconfigurable-nonwritable-3.js pass.

[Caio Lima]

Created attachment 282740 [details]
Patch

[Build Bot]

Comment on attachment 282740 [details]
Patch

Attachment 282740 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1627648

New failing tests:
js/mozilla/strict/10.6.html

[Build Bot]

Created attachment 282742 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282740 [details]
Patch

Attachment 282740 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1627651

New failing tests:
js/mozilla/strict/10.6.html

[Build Bot]

Created attachment 282743 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282740 [details]
Patch

Attachment 282740 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1627652

New failing tests:
js/mozilla/strict/10.6.html

[Build Bot]

Created attachment 282744 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282740 [details]
Patch

Attachment 282740 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1627653

New failing tests:
js/mozilla/strict/10.6.html

[Build Bot]

Created attachment 282745 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Caio Lima]

Created attachment 283286 [details]
Patch

[Caio Lima]

Comment on attachment 283286 [details]
Patch

Problem not solved.

[Benjamin Poulain]

Can you please also extend the tests to cover what you fix?

Every patch should have as much test coverage as possible. Until Keith import test262 into WebKit, we have to write new tests.

[Caio Lima]

(In reply to comment #12)
> Can you please also extend the tests to cover what you fix?
> 
> Every patch should have as much test coverage as possible. Until Keith
> import test262 into WebKit, we have to write new tests.

Sure. I talked to him 1 hour ago about that.

[Caio Lima]

Created attachment 283499 [details]
Patch

[Build Bot]

Comment on attachment 283499 [details]
Patch

Attachment 283499 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1673498

New failing tests:
js/arguments.html
js/mozilla/strict/10.6.html

[Build Bot]

Created attachment 283504 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 283499 [details]
Patch

Attachment 283499 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1673502

New failing tests:
js/arguments.html
js/mozilla/strict/10.6.html

[Build Bot]

Created attachment 283505 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 283499 [details]
Patch

Attachment 283499 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1673530

New failing tests:
js/arguments.html
js/function-dot-arguments.html
js/mozilla/strict/10.6.html

[Build Bot]

Created attachment 283509 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[Caio Lima]

Created attachment 283742 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 283742 [details]
Patch

https://trac.webkit.org/changeset/203270 added the whole test262, please update your patch accordingly:
- remove tests from the patch
- update expectations in Source/JavaScriptCore/tests/test262.yaml
- don't add test262.yaml to Tools/Scripts/run-javascriptcore-tests, we don't run these tests by default

note: with the fix I proposed in bug159810, all test262 tests run as expected set in test262.yaml

[Caio Lima]

Created attachment 283766 [details]
Patch

[Caio Lima]

(In reply to comment #23)
> Created attachment 283766 [details]
> Patch

ping review

[Geoffrey Garen]

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.

> 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.

Also, you don't seem to invoke the "overrideArgument" mechanism, which is what we usually use to indicate that something strange has happened to an indexed property.

[Caio Lima]

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.

[Geoffrey Garen]

Comment on attachment 283766 [details]
Patch

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

I think I understand the logic here now: It used to be that a property either aliased *or* lived in JSObject property storage with attributes. You're adding a new feature that allows a property to alias *and* live in JSObject property storage with attributes. In this state, somewhat confusingly, the value stored in JSObject property storage can be bogus because it is not observed. You should explain this in your ChangeLog.

I think you missed a case where the new property descriptor is not an accessor, is writable, and is DontDelete. In that case, your code will return early without setting the DontDelete attribute. Is that right?

Can you simplify the logic and duplicated code in GenericArguments<Type>::defineOwnProperty?

The relevant cases inside canAccessIndexQuickly, as I understand them, are:

(1) If the property is not an accessor and has either no attributes or only the writable:true attribute, set the value and return.

Otherwise:

(2) getIndexQuickly and putDirectMayBeIndex.

(3) If the property has any attributes other than configurable:true or configurable:false, overrideArgument.

(4) Fall through to the standard call to defineOwnProperty.

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:158
> +    PropertySlot slot(thisObject, PropertySlot::InternalMethodType::GetOwnProperty);
> +    if (Base::getOwnPropertySlot(thisObject, exec, ident, slot)) {
> +        if (slot.attributes() & DontDelete)
> +            return false;
> +    }

Let's make this a helper function: GenericArguments<T>::canDeleteProperty(PropertyName).

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:179
> +    PropertySlot slot(thisObject, PropertySlot::InternalMethodType::GetOwnProperty);
> +    if (Base::getOwnPropertySlotByIndex(thisObject, exec, index, slot)) {
> +        if (slot.attributes() & DontDelete)
> +            return false;
> +    }

Ditto.

[Caio Lima]

(In reply to comment #27)
> Comment on attachment 283766 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283766&action=review
> 
> I think I understand the logic here now: It used to be that a property
> either aliased *or* lived in JSObject property storage with attributes.
> You're adding a new feature that allows a property to alias *and* live in
> JSObject property storage with attributes. In this state, somewhat
> confusingly, the value stored in JSObject property storage can be bogus
> because it is not observed. You should explain this in your ChangeLog.

Yes. My change is using the attributes just to check the attribute is DontDelete in deleteProperty*. One alternative is store configurable attribute in an auxiliary structure (such as m_overrides), however, IMHO, it doesn't look as intuitive as checking property attributes.

> I think you missed a case where the new property descriptor is not an
> accessor, is writable, and is DontDelete. In that case, your code will
> return early without setting the DontDelete attribute. Is that right?

You are right. Fixing this case.

> Can you simplify the logic and duplicated code in
> GenericArguments<Type>::defineOwnProperty?

Yes. I agree that it can be better implemented.

> The relevant cases inside canAccessIndexQuickly, as I understand them, are:
> 
> (1) If the property is not an accessor and has either no attributes or only
> the writable:true attribute, set the value and return.
> 
> Otherwise:
> 
> (2) getIndexQuickly and putDirectMayBeIndex.
> 
> (3) If the property has any attributes other than configurable:true or
> configurable:false, overrideArgument.
> 
> (4) Fall through to the standard call to defineOwnProperty.
>
> > Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:158
> > +    PropertySlot slot(thisObject, PropertySlot::InternalMethodType::GetOwnProperty);
> > +    if (Base::getOwnPropertySlot(thisObject, exec, ident, slot)) {
> > +        if (slot.attributes() & DontDelete)
> > +            return false;
> > +    }
> 
> Let's make this a helper function:
> GenericArguments<T>::canDeleteProperty(PropertyName).
> 
> > Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:179
> > +    PropertySlot slot(thisObject, PropertySlot::InternalMethodType::GetOwnProperty);
> > +    if (Base::getOwnPropertySlotByIndex(thisObject, exec, index, slot)) {
> > +        if (slot.attributes() & DontDelete)
> > +            return false;
> > +    }
> 
> Ditto.

[Caio Lima]

Created attachment 284980 [details]
Patch

[Darin Adler]

Comment on attachment 284980 [details]
Patch

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

> Source/JavaScriptCore/runtime/GenericArguments.h:59
> +private:
> +

WebKit coding style does not leave a blank line here.

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:165
> +    if (!canDeletePropertyById(thisObject, exec, ident))
> +        return false;

This means looking up every property twice when deleting it, once to see if it can be deleted and then a second time to actually delete it. Is there any way to get the correct behavior without adding the double lookup?

[Build Bot]

Comment on attachment 284980 [details]
Patch

Attachment 284980 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1789062

New failing tests:
js/arguments.html

[Build Bot]

Created attachment 284981 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 284980 [details]
Patch

Attachment 284980 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1789084

New failing tests:
js/arguments.html

[Build Bot]

Created attachment 284982 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 284980 [details]
Patch

Attachment 284980 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1789090

New failing tests:
js/arguments.html

[Build Bot]

Created attachment 284983 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 284980 [details]
Patch

Attachment 284980 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1789087

New failing tests:
js/arguments.html

[Build Bot]

Created attachment 284984 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[Caio Lima]

(In reply to comment #30)
> Comment on attachment 284980 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284980&action=review
> 
> > Source/JavaScriptCore/runtime/GenericArguments.h:59
> > +private:
> > +
> 
> WebKit coding style does not leave a blank line here.
> 
> > Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:165
> > +    if (!canDeletePropertyById(thisObject, exec, ident))
> > +        return false;
> 
> This means looking up every property twice when deleting it, once to see if
> it can be deleted and then a second time to actually delete it. Is there any
> way to get the correct behavior without adding the double lookup?

I can't see an clear way to do that, but I am totally open to discuss about a viable solution =). As I commented above, the lookup is necessary because I want to avoid create a new auxiliar data structure to store if the argument[i] is configurable. Since these properties can be sparse, this data structure need to be dynamic such as m_overrides. IMHO the complexity of this kind of optimization isn't worth enough when comparing with readability.

[Caio Lima]

Created attachment 284985 [details]
Patch

[Caio Lima]

Ping Review

[Saam Barati]

Comment on attachment 284985 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        property cases. Also it is fixing cases where arguments[i]
> +        cannot be deleted when argument "i" is not configurable.

Shouldn't being non-configurable mean you can't be deleted?

> Source/JavaScriptCore/ChangeLog:14
> +        aliased xor stored in a JSObject with attributes. This patch changes

what does it mean to be aliased in this situation?

> Source/JavaScriptCore/ChangeLog:18
> +        attribute is true. It is important realize that when the attribute is

Really? I thought it can't be deleted if configurable is false. That's how normal objects work, anyways.

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:145
> +    PropertySlot slot(thisObject, PropertySlot::InternalMethodType::GetOwnProperty);

Seems like this should be ::VMInquiry. ::GetOwnProperty is allowed to do arbitrary side effects.

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:179
> +    PropertySlot slot(thisObject, PropertySlot::InternalMethodType::GetOwnProperty);

Ditto.

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:228
> +                if (descriptor.writable() && descriptor.configurable())

Please change the above comment to reflect the new behavior.

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:238
> +            if ((descriptor.writablePresent() && !descriptor.writable()) || descriptor.isAccessorDescriptor())

Why do we care about it being non-writable? I thought for deleting a property, we care about it being configurable?

[Caio Lima]

Comment on attachment 284985 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:11
>> +        cannot be deleted when argument "i" is not configurable.
> 
> Shouldn't being non-configurable mean you can't be deleted?

Yes. The current behavior is not following this rule.

>> Source/JavaScriptCore/ChangeLog:14
>> +        aliased xor stored in a JSObject with attributes. This patch changes
> 
> what does it mean to be aliased in this situation?

Aliased I mean that for example:
```
((a) => {
  print(a); // prints 2
  print(arguments[0]); prints 2
  a = 3;
  print(a); // prints 3
  print(arguments[0]); // prints 3 because it is aliased with a 
})(2);

>> Source/JavaScriptCore/ChangeLog:18
>> +        attribute is true. It is important realize that when the attribute is
> 
> Really? I thought it can't be deleted if configurable is false. That's how normal objects work, anyways.

I think I was not very clear in this ChangeLog. What I am trying to say is that when {configurable: false}, the member can't be deleted. In the current behavior it is not true and when arguments object isn't aliases with parameters, delete operations succeed, which is against specification.

>> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:145
>> +    PropertySlot slot(thisObject, PropertySlot::InternalMethodType::GetOwnProperty);
> 
> Seems like this should be ::VMInquiry. ::GetOwnProperty is allowed to do arbitrary side effects.

I am going to change it now. Also, as Darin Adler commented above, it is also not optimized.

>> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:238
>> +            if ((descriptor.writablePresent() && !descriptor.writable()) || descriptor.isAccessorDescriptor())
> 
> Why do we care about it being non-writable? I thought for deleting a property, we care about it being configurable?

This logic here is not related with {configurable}. In this current Patch, the configurable case is checked on lines 192 - 180. Here we are controlling the aliasing of "arguments" object. Checking the specification (http://www.ecma-international.org/ecma-262/7.0/index.html#sec-createunmappedargumentsobject), all indexed elements from arguments are "writable: true", which means that they are aliased by default. To stop the aliasing, "descriptor.writable" need to be present and false.

[Caio Lima]

Created attachment 289955 [details]
WIP Fixing arguments aliasing

That is the new approach I am taking. It is still failing in a bunch of tests and I think it is GC related. I am going to figure out what is the problem. Running EWS to check failed tests.

[WebKit Commit Bot]

Attachment 289955 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/DirectArguments.cpp:172:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/runtime/DirectArguments.h:115:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/DirectArguments.h:116:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/DirectArguments.h:117:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/DirectArguments.h:118:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/ScopedArguments.cpp:38:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/ScopedArguments.cpp:183:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/runtime/ScopedArguments.h:118:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/ScopedArguments.h:119:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/ScopedArguments.h:120:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/ScopedArguments.h:121:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 11 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 289955 [details]
WIP Fixing arguments aliasing

Attachment 289955 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2155460

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289969 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289955 [details]
WIP Fixing arguments aliasing

Attachment 289955 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2155506

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289970 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289955 [details]
WIP Fixing arguments aliasing

Attachment 289955 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2155477

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289975 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 289955 [details]
WIP Fixing arguments aliasing

Attachment 289955 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2155581

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289979 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Caio Lima]

Created attachment 290296 [details]
Patch for landing

In this version I am avoiding use GetPropertyDescriptor because of performance. Also, I changed the ChangeLog and the comments.

[Build Bot]

Comment on attachment 290296 [details]
Patch for landing

Attachment 290296 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2173059

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 290303 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290296 [details]
Patch for landing

Attachment 290296 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2173091

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 290305 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290296 [details]
Patch for landing

Attachment 290296 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2173063

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 290306 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 290296 [details]
Patch for landing

Attachment 290296 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2174474

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 290326 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Caio Lima]

Created attachment 290344 [details]
Patch for landing

I changed some files but accident. Now I hope it is going to pass on all tests.

[Build Bot]

Comment on attachment 290344 [details]
Patch for landing

Attachment 290344 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2176317

New failing tests:
ietestcenter/Javascript/11.4.1-4.a-11.html
js/preventExtensions.html
sputnik/Conformance/10_Execution_Contexts/10.1_Definitions/10.1.8_Arguments_Object/S10.1.8_A3_T3.html
sputnik/Conformance/10_Execution_Contexts/10.1_Definitions/10.1.8_Arguments_Object/S10.1.8_A5_T3.html

[Build Bot]

Created attachment 290350 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Caio Lima]

Created attachment 290429 [details]
Patch

Fixing index on GenericArguments<DirectArguments>::deleteProperty.

[WebKit Commit Bot]

Attachment 290429 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:243:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 290429 [details]
Patch

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

> Source/JavaScriptCore/runtime/DirectArguments.h:117
> +    void initConfigurableMap(VM&);
> +    void initConfigurableMapIfNecessary(VM&);
> +    void setConfigurable(VM&, unsigned index, bool value);
> +    bool isConfigurable(VM&, unsigned index);

Why not make these functions on GenericArguments since this is defined the same way inside both?

> Source/JavaScriptCore/runtime/DirectArguments.h:157
> +    AuxiliaryBarrier<bool*> m_configurableMap;

Ditto here, why not make this a field inside GenericArguments?

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:226
> +

please revert.

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:243
> + ;       if (thisObject->canAccessIndexQuickly(i + offset))

Please revert

> Source/JavaScriptCore/runtime/ScopedArguments.cpp:182
> +    initConfigurableMapIfNecessary(vm);

Is this necessary? Can't you just do
if (!m_configurableMap)
   return true;
?

> Source/JavaScriptCore/runtime/ScopedArguments.h:152
> +    AuxiliaryBarrier<bool*> m_configurableMap;

Please move this field below the m_totalLength field for better alignment. (Or move it to GenericAruments as commented above)

[Caio Lima]

Created attachment 290583 [details]
Patch

[Saam Barati]

Comment on attachment 290583 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        property. Also it is fixing cases where arguments[i]
> +        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"

> 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.

> Source/JavaScriptCore/ChangeLog:18
> +           It means that we just stop aliasing an property index already
> +           defined if its property descriptor contains writable and its value

I'm confused by what you mean here.

> Source/JavaScriptCore/ChangeLog:22
> +        2. When a property is overriden it is always returning true. However

I would say "delete" instead of "it"

> 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.

> Source/JavaScriptCore/runtime/GenericArguments.h:66
> +    static bool canDeletePropertyById(Type*, ExecState*, PropertyName);
> +    static bool canDeletePropertyByIndex(Type*, ExecState*, unsigned);

These aren't implemented anywhere. Please delete.

> 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")

> 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?

> JSTests/microbenchmarks/super-getter.js:1
> +class B {

Please remove this file.

[Caio Lima]

Comment on attachment 290583 [details]
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...

[Caio Lima]

Created attachment 291051 [details]
Patch

[Saam Barati]

Comment on attachment 291051 [details]
Patch

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

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:194
> +        if (optionalIndex && !thisObject->isConfigurable(optionalIndex.value()) && !descriptor.justWritablePresent())

I'm not sure this is correct here. I think you want to be following:
https://tc39.github.io/ecma262/#sec-validateandapplypropertydescriptor
more or less here, with some extra rules defined in:
https://tc39.github.io/ecma262/#sec-arguments-exotic-objects-defineownproperty-p-desc

It looks like the code below falls back to normal object storage. Why not just fall back to normal object storage in every case instead of creating the isConfigurable map?

> JSTests/stress/arguments-non-configurable.js:27
> +function tryChangeWritableOfNonConfigurableDescriptor(x) {
> +    Object.defineProperty(arguments, 0, {configurable: false});
> +    Object.defineProperty(arguments, 0, {writable: true});
> +    Object.defineProperty(arguments, 0, {writable: false});
> +}

This is the specified behavior? This goes against normal object behavior:

>>> Object.defineProperty(o, "f", {configurable: false, value: 20})
[object Object]
>>> o.f
20
>>> o.f = 40
40
>>> o.f
20
>>> Object.defineProperty(o, "f", {writable: true, value:50})
Exception: TypeError: Attempting to change writable attribute of unconfigurable property.

[Caio Lima]

(In reply to comment #73)
> Comment on attachment 291051 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291051&action=review
> 
> > Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:194
> > +        if (optionalIndex && !thisObject->isConfigurable(optionalIndex.value()) && !descriptor.justWritablePresent())
> 
> I'm not sure this is correct here. I think you want to be following:
> https://tc39.github.io/ecma262/#sec-validateandapplypropertydescriptor
> more or less here, with some extra rules defined in:
> https://tc39.github.io/ecma262/#sec-arguments-exotic-objects-
> defineownproperty-p-desc

Yes. Actually, this patch is totally incorrect. I can reuse some code, but I need a totally different approach. The current Mapping is controlled by m_overrides. I think that finally I understood this mechanism. The idea is to override things as soon as we define a different descriptor from default and fallback operations like defineOwnProperty, getOwnProperty and delete to Base. The problem of this approach is that the ES6 mapping rules changed and this mechanism need to be fixed. I am with a new idea in mind that is separate fallBack operations to Base from mapping rules and I think it can work.

> It looks like the code below falls back to normal object storage. Why not
> just fall back to normal object storage in every case instead of creatina
> the isConfigurable map?
> 
> > JSTests/stress/arguments-non-configurable.js:27
> > +function tryChangeWritableOfNonConfigurableDescriptor(x) {
> > +    Object.defineProperty(arguments, 0, {configurable: false});
> > +    Object.defineProperty(arguments, 0, {writable: true});
> > +    Object.defineProperty(arguments, 0, {writable: false});
> > +}
> 
> This is the specified behavior? This goes against normal object behavior:
> 
> >>> Object.defineProperty(o, "f", {configurable: false, value: 20})
> [object Object]
> >>> o.f
> 20
> >>> o.f = 40
> 40
> >>> o.f
> 20
> >>> Object.defineProperty(o, "f", {writable: true, value:50})
> Exception: TypeError: Attempting to change writable attribute of
> unconfigurable property.

Also, this test case Is incorrect and I am going to remove it. I already created a test case validating spec behavior (https://tc39.github.io/ecma262/#sec-arguments-exotic-objects-defineownproperty-p-desc).

[Caio Lima]

Created attachment 291932 [details]
Patch

[Caio Lima]

Created attachment 294029 [details]
Patch

[Saam Barati]

Comment on attachment 294029 [details]
Patch

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

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:62
> +    if (index && thisObject->canAccessIndexQuickly(index.value()))

Why can we ever access an index quickly if we've modified an argument at that index? This seems weird to me.

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:232
> +            // property from arguments object are {writable: true, configurable: true, enumerable: true} by default

What if we modify configurable/enumuerable? Should we also stop mapping in such a scenario?
I'm confused why we have two vectors: modified/overrides. Can you explain why we need two of them?

[Caio Lima]

Comment on attachment 294029 [details]
Patch

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

>> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:62
>> +    if (index && thisObject->canAccessIndexQuickly(index.value()))
> 
> Why can we ever access an index quickly if we've modified an argument at that index? This seems weird to me.

I tried to explain it on ChangeLog.

In this new implementation, canAccessIndexQuickly actually returns if the index is still mapped. When it happens, we need to return property slot with value of "thisObject->getIndexQuickly(index.value())" and with Base::getOwnPropertySlot value. Dos it make sense?

I have 2 things in mind to make it clear:

1. Change canAccessIndexQuickly to isIndexMapped;
2. Explain it with a comment;

>> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:232
>> +            // property from arguments object are {writable: true, configurable: true, enumerable: true} by default
> 
> What if we modify configurable/enumuerable? Should we also stop mapping in such a scenario?
> I'm confused why we have two vectors: modified/overrides. Can you explain why we need two of them?

I also tried to explain it on ChangeLog. I am going to try make it more clear.

The point is that the old implementation is using overrides to keep track if the index is still mapped and also if its property descriptor was changed in some way. The problem of this approach is that Mapping rules aren't related with property descriptor changes and we need different mechanism to keep track of them, since a property can still be mapped, even its descriptor has changed. In other words, we stop mapping as soon as a property descriptor is changed, however there are some cases that this mapping should continue happening (e.g. when we change configurable or enumerable to false, for example). One test case that is keeping track of this behavior is JSTests/stress/arguments-bizarre-behaviour-disable-enumerability.js.

[Saam Barati]

So if we change enumerable/configurable, we still keep arguments aliased? Only changing writable makes it not aliased? However, if you do change eneumerable/configurable, you still store to JSObject's property mechanism just to keep a record of the descriptor? Am I understanding this correctly?

[Caio Lima]

(In reply to comment #79)
> So if we change enumerable/configurable, we still keep arguments aliased?

Yes.

> Only changing writable makes it not aliased?

No. It also happens if you use non Data Descriptor (i.e. descriptor contains accessor)

> However, if you do change
> eneumerable/configurable, you still store to JSObject's property mechanism
> just to keep a record of the descriptor? Am I understanding this correctly?

Yes. We use JSObject's to consult its descriptor. When the index is mapped, we need to get the value from mapping mechanism (now it is stored on m_overrides).

To understand the correct behavior I used these test cases: https://github.com/tc39/test262/tree/master/test/language/arguments-object/mapped

Also, https://tc39.github.io/ecma262/#sec-arguments-exotic-objects-defineownproperty-p-desc is presenting when the should stop mapping.

Here https://tc39.github.io/ecma262/#sec-arguments-exotic-objects-getownproperty-p we can check steps 5. and 6, that the desc.[[Value]] is updated from mapping when it is being mapped.

Does it make more sense now?

[Saam Barati]

Comment on attachment 294029 [details]
Patch

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

patch LGTM. Just a couple comments. Also, I think the names "modified" and "overrides" are somewhat confusing. I want us to try to pick a better name to indicate that they mean.
- Overrides => argument is mapped/unmapped
- Modified => the descriptor is now changed, however, the argument may or may not be mapped

> Source/JavaScriptCore/runtime/DirectArguments.cpp:106
> +    if (thisObject->m_modifiedArguments)
> +        visitor.markAuxiliary(thisObject->m_modifiedArguments.get());

This should be done in the GenericArguments visitChildren method since GenericArguments owns that field. You may need to write a visitCHildren for GenericArguments.

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:226
> +        if (optionalIndex && thisObject->canAccessIndexQuickly(optionalIndex.value())) {

Nit: You can move this branch into the above `if (optionalIndex)` statement and drop the "optionalIndex" part.

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:239
> +                thisObject->overrideArgument(vm, index);

We don't need to set modified here too?

> Source/JavaScriptCore/runtime/PropertyDescriptor.h:82
> +

please revert

> Source/JavaScriptCore/runtime/ScopedArguments.cpp:116
> +    if (thisObject->m_modifiedArguments)
> +        visitor.markAuxiliary(thisObject->m_modifiedArguments.get());

Ditto for this visitChildren

[Caio Lima]

Created attachment 295654 [details]
Patch

[Caio Lima]

Created attachment 297500 [details]
Patch

[Saam Barati]

Comment on attachment 297500 [details]
Patch

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

Nice. r=me with comments addressed.

> Source/JavaScriptCore/ChangeLog:29
> +        to store wich arguments[i] descriptor was changed from its default

Typo: wich => which

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:70
> +    bool result = Base::getOwnPropertySlot(thisObject, exec, ident, slot);

Is this just to get the attributes?

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:234
> +            if (thisObject->isMappedArgument(optionalIndex.value())) {

Nit: you can just use index since it's defined above.

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h.orig:1
> +/*

Oops. Please remove file.

[Caio Lima]

Comment on attachment 297500 [details]
Patch

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

>> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:70
>> +    bool result = Base::getOwnPropertySlot(thisObject, exec, ident, slot);
> 
> Is this just to get the attributes?

Yes! Do you think that there is another way to get them?

>> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:234
>> +            if (thisObject->isMappedArgument(optionalIndex.value())) {
> 
> Nit: you can just use index since it's defined above.

Nice catch. Thx!

[Caio Lima]

Created attachment 297526 [details]
Patch

[Caio Lima]

(In reply to comment #86)
> Created attachment 297526 [details]
> Patch

Patch for landing. Changed code according review.

[Caio Lima]

Created attachment 297747 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 297747 [details]
Patch

Clearing flags on attachment: 297747

Committed r210146: <http://trac.webkit.org/changeset/210146>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.