|
Description
Caio Lima
2016-07-04 00:32:34 PDT
Created attachment 282740 [details]
Patch
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 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
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 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
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 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
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 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
Created attachment 283286 [details]
Patch
Comment on attachment 283286 [details]
Patch
Problem not solved.
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. (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. Created attachment 283499 [details]
Patch
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 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
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 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
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 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
Created attachment 283742 [details]
Patch
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 Created attachment 283766 [details]
Patch
(In reply to comment #23) > Created attachment 283766 [details] > Patch ping review 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. 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. 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. (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. Created attachment 284980 [details]
Patch
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? 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 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
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 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
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 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
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 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
(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. Created attachment 284985 [details]
Patch
Ping Review 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? 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. 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.
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.
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. 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
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. 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
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. 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
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. 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
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.
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. 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
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. 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
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. 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
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. 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
Created attachment 290344 [details]
Patch for landing
I changed some files but accident. Now I hope it is going to pass on all tests.
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 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
Created attachment 290429 [details]
Patch
Fixing index on GenericArguments<DirectArguments>::deleteProperty.
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.
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) Created attachment 290583 [details]
Patch
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. 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... Created attachment 291051 [details]
Patch
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. (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). Created attachment 291932 [details]
Patch
Created attachment 294029 [details]
Patch
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? 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. 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? (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? 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 Created attachment 295654 [details]
Patch
Created attachment 297500 [details]
Patch
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. 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! Created attachment 297526 [details]
Patch
(In reply to comment #86) > Created attachment 297526 [details] > Patch Patch for landing. Changed code according review. Created attachment 297747 [details]
Patch
Comment on attachment 297747 [details] Patch Clearing flags on attachment: 297747 Committed r210146: <http://trac.webkit.org/changeset/210146> All reviewed patches have been landed. Closing bug. |