* SUMMARY ES6: Classes: Should be allowed to create a static method with name "arguments". jsc> class MyClass { static arguments() { } } Exception: TypeError: Attempting to change configurable attribute of unconfigurable property. * NOTES - Behaves fine in Firefox and Chrome. - See related bug 144281.
I think should be the same for name 'caller' and getter and setter. class A { static get caller() { return '';} }
Created attachment 277870 [details] Patch Patch
Comment on attachment 277870 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277870&action=review > Source/JavaScriptCore/ChangeLog:10 > + class A { static argumetns() { return 'value'; } } typo: 'argumetns' => 'arguments' > Source/JavaScriptCore/runtime/JSFunction.cpp:368 > + if (!(thisObject->jsExecutable()->isClass() && slot.internalMethodType() == PropertySlot::InternalMethodType::GetOwnProperty)) { Why do we only care to skip this if we're doing a GetOwnProperty access on a class? We will not skip if we're doing a normal Get or VMInquiry or HasProperty? > Source/JavaScriptCore/runtime/JSFunction.cpp:399 > + bool result = Base::getOwnPropertySlot(thisObject, exec, propertyName, slot); > + if (!result) { > + GetterSetter* errorGetterSetter = thisObject->jsExecutable()->isClassConstructorFunction() || thisObject->jsExecutable()->parseMode() == SourceParseMode::MethodMode > + ? thisObject->globalObject()->throwTypeErrorArgumentsAndCallerGetterSetter(exec->vm()) > + : thisObject->globalObject()->throwTypeErrorGetterSetter(exec->vm()); > + > + thisObject->putDirectAccessor(exec, propertyName, errorGetterSetter, DontDelete | DontEnum | Accessor); > + result = Base::getOwnPropertySlot(thisObject, exec, propertyName, slot); > + ASSERT(result); > + } > + return result; Can we write a helper for the common code?
Created attachment 277905 [details] Patch Fix comments
I see you are adding a new test. You should update the existing test that references this bug: > LayoutTests/js/script-tests/keyword-method-names.js:79 > + // FIXME: <https://webkit.org/b/152985> ES6: Classes: Should be allowed to create a static method with name "arguments" > + // static arguments() { }
I hope to upload soon new patch with fixes of your and Saam comments
Created attachment 278348 [details] Patch New patch for review
Created attachment 278358 [details] Patch Patch with rebase
Created attachment 278360 [details] Patch More tests
Comment on attachment 278360 [details] Patch I'm a little confused, if I have read the spec correctly (https://tc39.github.io/ecma262/#sec-forbidden-extensions), it looks like "arguments" and "caller" should not be on any non-strict function anyway. It seems like we should just get rid of those properties and make them act like normal for non-strict functions. I'm pretty sure that all class constructors are strict anyway so I think this would entirely fix this issue. Is there something I am missing?
(In reply to comment #10) > Comment on attachment 278360 [details] > Patch > > I'm a little confused, if I have read the spec correctly > (https://tc39.github.io/ecma262/#sec-forbidden-extensions), it looks like > "arguments" and "caller" should not be on any non-strict function anyway. It Hmm, I did not find that about non-strict function in this spec. Only about function in strict-mode and arrow function, class declaration and etc. > seems like we should just get rid of those properties and make them act like > normal for non-strict functions. I'm pretty sure that all class constructors > are strict anyway so I think this would entirely fix this issue. Is there > something I am missing? Currently my patch does not cover arrow function, GeneratorDeclaration, GeneratorExpression and functions created using the bind. So I'll add try to cover those cases
Created attachment 279291 [details] Patch New patch
Comment on attachment 279291 [details] Patch Attachment 279291 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1344770 New failing tests: fast/scrolling/ios/scrollTo-at-page-load.html
Created attachment 279313 [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.4
Created attachment 279579 [details] Patch Small fixes
Created attachment 279797 [details] Patch Rebase
(In reply to comment #11) > (In reply to comment #10) > > Comment on attachment 278360 [details] > > Patch > > > > I'm a little confused, if I have read the spec correctly > > (https://tc39.github.io/ecma262/#sec-forbidden-extensions), it looks like > > "arguments" and "caller" should not be on any non-strict function anyway. It > Hmm, I did not find that about non-strict function in this spec. Only about > function in strict-mode and arrow function, class declaration and etc. > > seems like we should just get rid of those properties and make them act like > > normal for non-strict functions. I'm pretty sure that all class constructors > > are strict anyway so I think this would entirely fix this issue. Is there > > something I am missing? > > Currently my patch does not cover arrow function, GeneratorDeclaration, > GeneratorExpression and functions created using the bind. So I'll add try to > cover those cases I think you might have misunderstood my comment. I think that all strict functions should not have an "arguments" or "caller" property. Instead those properties should be on %FunctionPrototype% and should throw a type error on get or set. For example, function foo() { "use strict"; return 1; } class bar {} Object.getOwnPropertyDescriptor(foo, "arguments") // undefined Object.getOwnPropertyDescriptor(bar, "arguments") // undefined Object.getOwnPropertyDescriptor(foo, "caller") // undefined Object.getOwnPropertyDescriptor(bar, "caller") // undefined Object.getOwnPropertyDescriptor(foo.__proto__, "arguments") // { get: () => { throw new TypeError(), set: () => { throw new TypeError() }, writable: false, enumerable: false } Object.getOwnPropertyDescriptor(foo.__proto__, "caller") // { get: () => { throw new TypeError(), set: () => { throw new TypeError() }, writable: false, enumerable: false } Object.getOwnPropertyDescriptor(bar.__proto__, "arguments") // { get: () => { throw new TypeError(), set: () => { throw new TypeError() }, writable: false, enumerable: false } Object.getOwnPropertyDescriptor(bar.__proto__, "caller") // { get: () => { throw new TypeError(), set: () => { throw new TypeError() }, writable: false, enumerable: false } Does my example seem correct to you based on the spec?
(In reply to comment #17) > (In reply to comment #11) > > (In reply to comment #10) > > > Comment on attachment 278360 [details] > > > Patch > > > > > > I'm a little confused, if I have read the spec correctly > > > (https://tc39.github.io/ecma262/#sec-forbidden-extensions), it looks like > > > "arguments" and "caller" should not be on any non-strict function anyway. It > > Hmm, I did not find that about non-strict function in this spec. Only about > > function in strict-mode and arrow function, class declaration and etc. > > > seems like we should just get rid of those properties and make them act like > > > normal for non-strict functions. I'm pretty sure that all class constructors > > > are strict anyway so I think this would entirely fix this issue. Is there > > > something I am missing? > > > > Currently my patch does not cover arrow function, GeneratorDeclaration, > > GeneratorExpression and functions created using the bind. So I'll add try to > > cover those cases > > I think you might have misunderstood my comment. I think that all strict > functions should not have an "arguments" or "caller" property. Instead those > properties should be on %FunctionPrototype% and should throw a type error on > get or set. > > For example, > function foo() { > "use strict"; > return 1; > } > > class bar {} > > > Object.getOwnPropertyDescriptor(foo, "arguments") // undefined > Object.getOwnPropertyDescriptor(bar, "arguments") // undefined > Object.getOwnPropertyDescriptor(foo, "caller") // undefined > Object.getOwnPropertyDescriptor(bar, "caller") // undefined > > Object.getOwnPropertyDescriptor(foo.__proto__, "arguments") // { get: () => > { throw new TypeError(), set: () => { throw new TypeError() }, writable: > false, enumerable: false } > Object.getOwnPropertyDescriptor(foo.__proto__, "caller") // { get: () => { > throw new TypeError(), set: () => { throw new TypeError() }, writable: > false, enumerable: false } > > Object.getOwnPropertyDescriptor(bar.__proto__, "arguments") // { get: () => > { throw new TypeError(), set: () => { throw new TypeError() }, writable: > false, enumerable: false } > Object.getOwnPropertyDescriptor(bar.__proto__, "caller") // { get: () => { > throw new TypeError(), set: () => { throw new TypeError() }, writable: > false, enumerable: false } > > Does my example seem correct to you based on the spec? I got it. I think you are right. It should be in this way.
Created attachment 281184 [details] Patch Refactored patch
Created attachment 281186 [details] Patch Fix build
Comment on attachment 281186 [details] Patch Attachment 281186 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1496045 Number of test failures exceeded the failure limit.
Created attachment 281189 [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 281186 [details] Patch Attachment 281186 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1496044 Number of test failures exceeded the failure limit.
Created attachment 281190 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 281186 [details] Patch Attachment 281186 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1496067 New failing tests: js/kde/function_arguments.html
Created attachment 281191 [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
Comment on attachment 281186 [details] Patch Attachment 281186 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1496051 Number of test failures exceeded the failure limit.
Created attachment 281193 [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
Created attachment 281274 [details] Patch Fix webkit tests
Created attachment 281276 [details] Patch Made rebase
Comment on attachment 281276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281276&action=review So I'm a little confused what this patch changed. It would be great if the changelog went into more detail on what the code you changed does. It might also be helpful if you explained why you made the changes you did. Also, is there a reason why caller and arguments are not just normal static, lazy properties? Then you can have JSFunction intercept the lookup of arguments and caller for non-strict functions and it will probably make this code much simpler. > Source/JavaScriptCore/runtime/FunctionPrototype.cpp:247 > + if (thisObject->inherits(JSFunction::info())) { > + JSFunction* function = jsCast<JSFunction*>(thisObject); You can just do if (JSFunction* function = jsDynamicCast<JSFunction*>(JSValue::decode(thisValue))) > Source/JavaScriptCore/runtime/FunctionPrototype.cpp:254 > + ASSERT(!function->isHostFunction()); Is this code reachable?
(In reply to comment #31) > Comment on attachment 281276 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281276&action=review > > So I'm a little confused what this patch changed. It would be great if the > changelog went into more detail on what the code you changed does. It might > also be helpful if you explained why you made the changes you did. Also, is > there a reason why caller and arguments are not just normal static, lazy > properties? Then you can have JSFunction intercept the lookup of arguments > and caller for non-strict functions and it will probably make this code much > simpler. OK. I'll try explain my changes in more details in ChangeLog. I think mostly my patch is moving all logic that is related to caller/arguments from JSFunction to FunctionPrototype with small adjustments. The main reason why I moved caller/arguments from JSFunction to FunctionPrototype class, because I decided to have all code that is related to caller/arguments in one place: So code that handle cases when lookup of arguments/caller of function and arguments/caller of function prototype in one place: var foo = function () {}; var args = foo.arguments; // case 1 var proto = foo.__proto__; var caller = proto.caller; // case 2 Do think it is better to split this logic between JSFunction and FunctionPrototype classes? > > > Source/JavaScriptCore/runtime/FunctionPrototype.cpp:247 > > + if (thisObject->inherits(JSFunction::info())) { > > + JSFunction* function = jsCast<JSFunction*>(thisObject); > > You can just do if (JSFunction* function = > jsDynamicCast<JSFunction*>(JSValue::decode(thisValue))) > > > Source/JavaScriptCore/runtime/FunctionPrototype.cpp:254 > > + ASSERT(!function->isHostFunction()); > > Is this code reachable?
Created attachment 286891 [details] Patch Rebase the patch
Created attachment 286904 [details] Patch Rebase the patch #2
Comment on attachment 286904 [details] Patch Attachment 286904 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1936681 New failing tests: js/class-method-and-constructor-properties.html inspector/model/remote-object-get-properties.html js/basic-strict-mode.html js/Object-getOwnPropertyNames.html
Created attachment 286917 [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 286904 [details] Patch Attachment 286904 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1936684 New failing tests: js/class-method-and-constructor-properties.html inspector/model/remote-object-get-properties.html js/basic-strict-mode.html js/Object-getOwnPropertyNames.html
Created attachment 286918 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 286904 [details] Patch Attachment 286904 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1936663 New failing tests: js/class-method-and-constructor-properties.html js/basic-strict-mode.html js/Object-getOwnPropertyNames.html
Created attachment 286920 [details] Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Comment on attachment 286904 [details] Patch Attachment 286904 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1936721 New failing tests: js/class-method-and-constructor-properties.html inspector/model/remote-object-get-properties.html js/basic-strict-mode.html js/Object-getOwnPropertyNames.html
Created attachment 286921 [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 286952 [details] Patch Fix brocken tests
Comment on attachment 286952 [details] Patch Attachment 286952 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1939137 New failing tests: js/class-method-and-constructor-properties.html inspector/model/remote-object-get-properties.html
Created attachment 286955 [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 286952 [details] Patch Attachment 286952 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1939142 New failing tests: js/class-method-and-constructor-properties.html inspector/model/remote-object-get-properties.html
Created attachment 286956 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 286952 [details] Patch Attachment 286952 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1939140 New failing tests: js/class-method-and-constructor-properties.html inspector/model/remote-object-get-properties.html
Created attachment 286957 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 286952 [details] Patch Attachment 286952 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1939154 New failing tests: js/class-method-and-constructor-properties.html
Created attachment 286958 [details] Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Comment on attachment 286952 [details] Patch F
Created attachment 286999 [details] Patch Refactoring of the patch
(In reply to comment #31) > Comment on attachment 281276 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281276&action=review > > So I'm a little confused what this patch changed. It would be great if the > changelog went into more detail on what the code you changed does. It might > also be helpful if you explained why you made the changes you did. Also, is > there a reason why caller and arguments are not just normal static, lazy > properties? Then you can have JSFunction intercept the lookup of arguments > and caller for non-strict functions and it will probably make this code much > simpler. > I've made refactoring of the Patch as you suggested, so it allow to decrease size of changes
Comment on attachment 286999 [details] Patch Attachment 286999 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1941656 New failing tests: js/class-method-and-constructor-properties.html
Created attachment 287017 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 286999 [details] Patch Attachment 286999 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1941659 New failing tests: js/class-method-and-constructor-properties.html
Created attachment 287018 [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 286999 [details] Patch Looks like EWS is angry that you deleted the class-method-and-constructor-properties.js file. Also, can you explain why "arguments" and "caller" are on Function prototype, I'm not sure why I follow that they need to be there?
Comment on attachment 286999 [details] Patch Attachment 286999 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1941670 New failing tests: js/class-method-and-constructor-properties.html
Created attachment 287020 [details] Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Comment on attachment 286999 [details] Patch Attachment 286999 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1942205 New failing tests: js/class-method-and-constructor-properties.html
Created attachment 287042 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 287129 [details] Patch
(In reply to comment #59) > Comment on attachment 286999 [details] > Patch > > Looks like EWS is angry that you deleted the > class-method-and-constructor-properties.js file. Also, can you explain why > "arguments" and "caller" are on Function prototype, I'm not sure why I > follow that they need to be there? I move this to the Function prototype to allow execute following code: var f = function () {} f.__proto__.caller; //Should raise type error I've checked in Chrome and FireFox: 1) Chrome returns TypeError: 'caller' and 'arguments' are restricted function properties and cannot be accessed in this context 2) FireFox return just null.
Also there are two tests that covered this case https://github.com/WebKit/webkit/blob/master/JSTests/test262/test/built-ins/Function/prototype/restricted-property-arguments.js and https://github.com/WebKit/webkit/blob/master/JSTests/test262/test/built-ins/Function/prototype/restricted-property-caller.js
Comment on attachment 287129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287129&action=review I think this is getting close but I still think there are a few things that need to be changed. > Source/JavaScriptCore/ChangeLog:62 > + * runtime/Executable.h: > + * runtime/FunctionPrototype.cpp: > + (JSC::FunctionPrototype::defineOwnProperty): > + (JSC::FunctionPrototype::getThrowTypeErrorOwnPropertySlot): > + (JSC::FunctionPrototype::getOwnPropertySlot): > + (JSC::FunctionPrototype::getOwnNonIndexPropertyNames): > + (JSC::functionProtoFuncBind): Deleted. > + * runtime/FunctionPrototype.h: > + * runtime/JSFunction.cpp: > + (JSC::JSFunction::getOwnPropertySlot): > + (JSC::JSFunction::getOwnNonIndexPropertyNames): > + (JSC::JSFunction::put): > + (JSC::JSFunction::deleteProperty): > + (JSC::JSFunction::defineOwnProperty): > + * runtime/JSFunction.h: > + * runtime/JSObject.cpp: > + (JSC::ordinarySetSlow): > + (JSC::JSObject::getOwnPropertyDescriptor): > + * runtime/PropertySlot.h: > + (JSC::PropertySlot::PropertySlot): > + (JSC::PropertySlot::setIsClassMethod): > + (JSC::PropertySlot::isClassMethod): > + (JSC::PropertySlot::isTaintedByOpaqueObject): Deleted. Delete. > Source/JavaScriptCore/runtime/FunctionPrototype.cpp:184 > + if (!object->inherits(JSFunction::info())) > + return Base::defineOwnProperty(object, exec, propertyName, descriptor, throwException); I don't think you need this check. object should always be FunctionPrototype*. > Source/JavaScriptCore/runtime/FunctionPrototype.cpp:204 > + JSFunction* thisObject = jsCast<JSFunction*>(object); > + if (thisObject->isHostOrBuiltinFunction()) > + return Base::defineOwnProperty(object, exec, propertyName, descriptor, throwException); > + > + if (propertyName == exec->propertyNames().arguments) { > + if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isArrowFunction()) { > + PropertySlot slot(thisObject, PropertySlot::InternalMethodType::VMInquiry); > + if (!Base::getOwnPropertySlot(thisObject, exec, propertyName, slot)) > + thisObject->putDirectAccessor(exec, propertyName, thisObject->globalObject()->throwTypeErrorArgumentsCalleeAndCallerGetterSetter(), DontDelete | DontEnum | Accessor); > + return Base::defineOwnProperty(object, exec, propertyName, descriptor, throwException); > + } > + } else if (propertyName == exec->propertyNames().caller) { > + if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isArrowFunction()) { > + PropertySlot slot(thisObject, PropertySlot::InternalMethodType::VMInquiry); > + if (!Base::getOwnPropertySlot(thisObject, exec, propertyName, slot)) > + thisObject->putDirectAccessor(exec, propertyName, thisObject->globalObject()->throwTypeErrorArgumentsCalleeAndCallerGetterSetter(), DontDelete | DontEnum | Accessor); > + return Base::defineOwnProperty(object, exec, propertyName, descriptor, throwException); > + } > + } I think this could just be FunctionPrototype* thisObject = jsCast<FunctionPrototype*>(object); You would need to add a reifyLazyPropertyIfNeeded function to FunctionPrototype that handles arguments and length like: void FunctionPrototype::reifyLazyPropertyIfNeeded(exec, propertyName) { if (propertyName == exec->propertyNames().arguments || propertyName == exec->propertyNames().caller) // Do reification of arguments/caller to type error getter/setter if needed. Base::reifyLazyPropertyIfNeeded(exec, propertyName); } > Source/JavaScriptCore/runtime/FunctionPrototype.cpp:231 > + if (propertyName == exec->propertyNames().caller || propertyName == exec->propertyNames().arguments) { > + // For non-host functions, don't let these properties by deleted - except by DefineOwnProperty. > + if (slot.thisValue().inherits(JSFunction::info())) { > + JSFunction* thisObject = jsDynamicCast<JSFunction*>(slot.thisValue()); > + if (!thisObject->isHostOrBuiltinFunction()) > + return getThrowTypeErrorOwnPropertySlot(object, exec, propertyName, slot); > + } else if (slot.thisValue().inherits(FunctionPrototype::info())) > + return getThrowTypeErrorOwnPropertySlot(object, exec, propertyName, slot); > + } This could just be (if you do my change above): FunctionPrototype* thisObject = jsCast<FunctionPrototype*>(object); thisObject->reifyLazyPropertyIfNeeded(exec, propertyName); > Source/JavaScriptCore/runtime/FunctionPrototype.cpp:246 > + VM& vm = exec->vm(); > + // Make sure prototype has been reified. > + PropertySlot slot(thisObject, PropertySlot::InternalMethodType::VMInquiry); > + thisObject->methodTable(vm)->getOwnPropertySlot(thisObject, exec, vm.propertyNames->prototype, slot); > + > + propertyNames.add(vm.propertyNames->arguments); > + propertyNames.add(vm.propertyNames->caller); Ditto. > Source/JavaScriptCore/runtime/JSFunction.cpp:402 > + if (!thisObject->jsExecutable()->isStrictMode()) { > + propertyNames.add(vm.propertyNames->arguments); > + propertyNames.add(vm.propertyNames->caller); > + } You might need to change this to work with my proposal for reification. > Source/JavaScriptCore/runtime/JSObject.cpp:517 > + // We do not update the value of caller & arguments properties in function Why do we not allow the user to set arguments on a strict function? > Source/JavaScriptCore/runtime/JSObject.cpp:518 > + if (object->inherits(JSFunction::info())) I think we usually just do this with jsDynamicCast<JSFunction>(object) > Source/JavaScriptCore/runtime/JSObject.cpp:2858 > + if (propertyName == exec->propertyNames().arguments || propertyName == exec->propertyNames().caller) { > + if (auto thisObject = jsDynamicCast<JSFunction*>(this)) { > + if (!thisObject->isHostFunctionNonInline() && thisObject->jsExecutable()->isClass() && !thisObject->jsExecutable()->isClassConstructorFunction()) > + slot.setIsClassMethod(); > + } > + } What does this do? It doesn't look like you use the isClassMethod bit anywhere?
(In reply to comment #66) > Also there are two tests that covered this case > https://github.com/WebKit/webkit/blob/master/JSTests/test262/test/built-ins/ > Function/prototype/restricted-property-arguments.js and > https://github.com/WebKit/webkit/blob/master/JSTests/test262/test/built-ins/ > Function/prototype/restricted-property-caller.js It would be great if you could also update the expected results for these tests in JSTests/test262.yaml too.
Created attachment 287871 [details] Patch Fix comments&refactoring
Comment on attachment 287129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287129&action=review >> Source/JavaScriptCore/ChangeLog:62 >> + (JSC::PropertySlot::isTaintedByOpaqueObject): Deleted. > > Delete. Done >> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:184 >> + return Base::defineOwnProperty(object, exec, propertyName, descriptor, throwException); > > I don't think you need this check. object should always be FunctionPrototype*. Remove whole method :-) >> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:204 >> + } > > I think this could just be FunctionPrototype* thisObject = jsCast<FunctionPrototype*>(object); > > You would need to add a reifyLazyPropertyIfNeeded function to FunctionPrototype that handles arguments and length like: > > void FunctionPrototype::reifyLazyPropertyIfNeeded(exec, propertyName) { > if (propertyName == exec->propertyNames().arguments || propertyName == exec->propertyNames().caller) > // Do reification of arguments/caller to type error getter/setter if needed. > Base::reifyLazyPropertyIfNeeded(exec, propertyName); > } Removed whole method >> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:231 >> + } > > This could just be (if you do my change above): > > FunctionPrototype* thisObject = jsCast<FunctionPrototype*>(object); > thisObject->reifyLazyPropertyIfNeeded(exec, propertyName); Removed whole method >> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:246 >> + propertyNames.add(vm.propertyNames->caller); > > Ditto. Removed whole method >> Source/JavaScriptCore/runtime/JSFunction.cpp:402 >> + } > > You might need to change this to work with my proposal for reification. I've just added small check, to add this method for function not in Strict mode >> Source/JavaScriptCore/runtime/JSObject.cpp:517 >> + // We do not update the value of caller & arguments properties in function > > Why do we not allow the user to set arguments on a strict function? Removed whole change >> Source/JavaScriptCore/runtime/JSObject.cpp:2858 >> + } > > What does this do? It doesn't look like you use the isClassMethod bit anywhere? Yes, you are right. It is not used anymore. Removed
Comment on attachment 287871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287871&action=review > Source/JavaScriptCore/runtime/JSFunction.cpp:404 > + if (!thisObject->jsExecutable()->isStrictMode()) { Don't you want !thisObject->jsExecutable()->isStrictMode() && !thisObject->jsExecutable()->isES6Function()? Also, does this work with Function.prototype when the arguments property has been deleted? i.e. delete Function.prototype.arguments; for (name in Function.prototype) { if (name !== "arguments") throw new Error("bad name"); }
Created attachment 288449 [details] Patch Small fix & rebase
Comment on attachment 287871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287871&action=review >> Source/JavaScriptCore/runtime/JSFunction.cpp:404 >> + if (!thisObject->jsExecutable()->isStrictMode()) { > > Don't you want !thisObject->jsExecutable()->isStrictMode() && !thisObject->jsExecutable()->isES6Function()? > > Also, does this work with Function.prototype when the arguments property has been deleted? i.e. > > delete Function.prototype.arguments; > for (name in Function.prototype) { > if (name !== "arguments") > throw new Error("bad name"); > } Yeah, I've add !thisObject->jsExecutable()->isES6Function() condition to the new patch. In case of 'delete Function.prototype.arguments;' we delete it from FunctionPrototyp(FunctionPrototype.cpp) where it added with property DontEnum, so it will not appear during for..in operation before and after delete operation.
(In reply to comment #73) > Comment on attachment 287871 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=287871&action=review > > >> Source/JavaScriptCore/runtime/JSFunction.cpp:404 > >> + if (!thisObject->jsExecutable()->isStrictMode()) { > > > > Don't you want !thisObject->jsExecutable()->isStrictMode() && !thisObject->jsExecutable()->isES6Function()? > > > > Also, does this work with Function.prototype when the arguments property has been deleted? i.e. > > > > delete Function.prototype.arguments; > > for (name in Function.prototype) { > > if (name !== "arguments") > > throw new Error("bad name"); > > } > > Yeah, I've add !thisObject->jsExecutable()->isES6Function() condition to the > new patch. > > In case of 'delete Function.prototype.arguments;' we delete it from > FunctionPrototyp(FunctionPrototype.cpp) where it added with property > DontEnum, so it will not appear during for..in operation before and after > delete operation. Ah, I was thinking that FunctionPrototype subclassed JSFunction but I see that it actually subclasses InternalFunction. Then, you're right, it should work for Function.prototype.
Created attachment 288454 [details] Patch Fix build
Comment on attachment 288454 [details] Patch Attachment 288454 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2044561 New failing tests: js/es6-function-properties.html
Created attachment 288469 [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 288454 [details] Patch Attachment 288454 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2044582 New failing tests: js/es6-function-properties.html
Created attachment 288471 [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 288454 [details] Patch Attachment 288454 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2044600 New failing tests: js/es6-function-properties.html
Comment on attachment 288454 [details] Patch Attachment 288454 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2044581 New failing tests: js/es6-function-properties.html
Created attachment 288474 [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 288454 [details] Patch Attachment 288454 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2045124 New failing tests: js/es6-function-properties.html
Created attachment 288480 [details] Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Created attachment 288483 [details] Patch Use debug intead of print in Layout tests
Comment on attachment 288483 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288483&action=review r=me if you fix the typo. Thanks, for powering through this patch! :D > Source/JavaScriptCore/ChangeLog:15 > + To implement this patch âcallerâ and âargumentsâ were put to the FunctionPrototype should be "caller" and "arguments".
All reviewed patches have been landed. Closing bug. Committed r205856: <https://trac.webkit.org/changeset/205856>