Description
Caitlin Potter (:caitp)
2016-09-24 09:53:06 PDT
Created attachment 289749 [details]
Do not add prototype to MethodDefinitions
Comment on attachment 289749 [details] Do not add prototype to MethodDefinitions Attachment 289749 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2138349 New failing tests: js/es6-function-properties.html Created attachment 289752 [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 289749 [details] Do not add prototype to MethodDefinitions Attachment 289749 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2138347 New failing tests: js/es6-function-properties.html Created attachment 289753 [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 289749 [details] Do not add prototype to MethodDefinitions Attachment 289749 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2138348 New failing tests: js/es6-function-properties.html Created attachment 289754 [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 289755 [details]
Fix LayoutTests
Comment on attachment 289755 [details] Fix LayoutTests Attachment 289755 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2138611 New failing tests: js/es6-function-properties.html Created attachment 289757 [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 289755 [details] Fix LayoutTests Attachment 289755 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2138614 New failing tests: js/es6-function-properties.html Created attachment 289758 [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 289755 [details] Fix LayoutTests Attachment 289755 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2138615 New failing tests: js/es6-function-properties.html Created attachment 289759 [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
Comment on attachment 289755 [details] Fix LayoutTests Attachment 289755 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2138621 New failing tests: js/es6-function-properties.html Created attachment 289760 [details]
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 289761 [details]
Fix LayoutTests
Comment on attachment 289761 [details] Fix LayoutTests View in context: https://bugs.webkit.org/attachment.cgi?id=289761&action=review > Source/JavaScriptCore/runtime/JSFunction.cpp:356 > + bool isMethod = executable->isMethod() && !executable->isClass(); Why the "!isClass" here? We distinguish between methods on object literals and methods on classes? What's the behavior for object literals? Please add tests for that. Comment on attachment 289761 [details] Fix LayoutTests View in context: https://bugs.webkit.org/attachment.cgi?id=289761&action=review >> Source/JavaScriptCore/runtime/JSFunction.cpp:356 >> + bool isMethod = executable->isMethod() && !executable->isClass(); > > Why the "!isClass" here? > We distinguish between methods on object literals and methods on classes? > > What's the behavior for object literals? Please add tests for that. 'isClass' really means "class constructor", which is a special case (gets a prototype property during MakeConstructor (https://tc39.github.io/ecma262/#sec-makeconstructor) during step 16 of ClassDefinitionEvaluation (https://tc39.github.io/ecma262/#sec-runtime-semantics-classdefinitionevaluation). I'll add some extra tests either tomorrow or during the week) (In reply to comment #19) > Comment on attachment 289761 [details] > Fix LayoutTests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289761&action=review > > >> Source/JavaScriptCore/runtime/JSFunction.cpp:356 > >> + bool isMethod = executable->isMethod() && !executable->isClass(); > > > > Why the "!isClass" here? > > We distinguish between methods on object literals and methods on classes? > > > > What's the behavior for object literals? Please add tests for that. > > 'isClass' really means "class constructor", which is a special case (gets a > prototype property during MakeConstructor > (https://tc39.github.io/ecma262/#sec-makeconstructor) during step 16 of > ClassDefinitionEvaluation > (https://tc39.github.io/ecma262/#sec-runtime-semantics- > classdefinitionevaluation). Oh wow. Let's please rename that method. That's horribly misleading. > > I'll add some extra tests either tomorrow or during the week) Cool, thanks. Created attachment 289766 [details]
Add extra es6-function-properties test cases
(In reply to comment #20) > (In reply to comment #19) > > Comment on attachment 289761 [details] > > Fix LayoutTests > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=289761&action=review > > > > >> Source/JavaScriptCore/runtime/JSFunction.cpp:356 > > >> + bool isMethod = executable->isMethod() && !executable->isClass(); > > > > > > Why the "!isClass" here? > > > We distinguish between methods on object literals and methods on classes? > > > > > > What's the behavior for object literals? Please add tests for that. > > > > 'isClass' really means "class constructor", which is a special case (gets a > > prototype property during MakeConstructor > > (https://tc39.github.io/ecma262/#sec-makeconstructor) during step 16 of > > ClassDefinitionEvaluation > > (https://tc39.github.io/ecma262/#sec-runtime-semantics- > > classdefinitionevaluation). > Oh wow. Let's please rename that method. That's horribly misleading. > > > > > I'll add some extra tests either tomorrow or during the week) > Cool, thanks. Got it done while waiting in the airport instead, I guess :) I'm not positive the tests will pass (laptop is a bit low powered to check this at the moment), but if it doesn't, will obviously fix it up tomorrow or monday. Comment on attachment 289766 [details] Add extra es6-function-properties test cases Attachment 289766 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2139923 New failing tests: js/es6-function-properties.html Created attachment 289769 [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 289766 [details] Add extra es6-function-properties test cases Attachment 289766 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2139928 New failing tests: js/es6-function-properties.html Created attachment 289770 [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 289766 [details] Add extra es6-function-properties test cases Attachment 289766 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2139926 New failing tests: js/es6-function-properties.html Created attachment 289771 [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
Comment on attachment 289766 [details] Add extra es6-function-properties test cases Attachment 289766 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2139932 New failing tests: js/es6-function-properties.html Created attachment 289772 [details]
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.6
realized after takeout the generator method expectations were wrong :( sorry bout the spam Huh, I guess WebKit also gets part of https://tc39.github.io/ecma262/#sec-forbidden-extensions wrong, not forbidding caller/arguments for accessors in object literals. I guess I'll fix that in this patch as well. Created attachment 289779 [details]
Fix accessors too, and fix that one LayoutTest, but didn't try any other ones
Created attachment 289780 [details]
Fix accessors too, and fix that one LayoutTest, but didn't try any other ones --- and rebase on Trunk
Comment on attachment 289780 [details] Fix accessors too, and fix that one LayoutTest, but didn't try any other ones --- and rebase on Trunk Attachment 289780 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2143220 New failing tests: js/arrowfunction-prototype.html js/caller-property.html js/mozilla/strict/15.3.5.2.html Created attachment 289781 [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 289780 [details] Fix accessors too, and fix that one LayoutTest, but didn't try any other ones --- and rebase on Trunk Attachment 289780 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2143244 New failing tests: js/arrowfunction-prototype.html js/caller-property.html js/mozilla/strict/15.3.5.2.html Created attachment 289782 [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 289780 [details] Fix accessors too, and fix that one LayoutTest, but didn't try any other ones --- and rebase on Trunk Attachment 289780 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2143234 New failing tests: js/arrowfunction-prototype.html js/caller-property.html js/mozilla/strict/15.3.5.2.html Created attachment 289783 [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 289780 [details] Fix accessors too, and fix that one LayoutTest, but didn't try any other ones --- and rebase on Trunk Attachment 289780 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2143942 New failing tests: js/arrowfunction-prototype.html js/caller-property.html js/mozilla/strict/15.3.5.2.html Created attachment 289784 [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
Created attachment 289815 [details]
Fix LayoutTests
Would it be frowned upon to request EditBugs permissions so that I can obsolete all of the no-longer-relevant EWS failures? :) (In reply to comment #44) > Would it be frowned upon to request EditBugs permissions so that I can > obsolete all of the no-longer-relevant EWS failures? :) No, that would be great. Have you landed more than 10 patches in WebKit? (I would guess yes.) If so, we should get you committer access too. (In reply to comment #45) > (In reply to comment #44) > > Would it be frowned upon to request EditBugs permissions so that I can > > obsolete all of the no-longer-relevant EWS failures? :) > > No, that would be great. Have you landed more than 10 patches in WebKit? (I > would guess yes.) If so, we should get you committer access too. I'm a committer, but I do not yet have EditBugs, heh :) (In reply to comment #44) > Would it be frowned upon to request EditBugs permissions so that I can > obsolete all of the no-longer-relevant EWS failures? :) Ok I'll try to get you edit bugs privileges tomorrow when more folks are on IRC (In reply to comment #47) > (In reply to comment #44) > > Would it be frowned upon to request EditBugs permissions so that I can > > obsolete all of the no-longer-relevant EWS failures? :) > > Ok I'll try to get you edit bugs privileges tomorrow when more folks are on > IRC Hi, has there been any update on this? I can send a mail to webkit-dev to ask for this, but I prefer to avoid that if possible. Okay, cleaned up the bug a bit. It would be fantastic if I could get a look at this Created attachment 290767 [details]
Patch
Comment on attachment 290767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290767&action=review > Source/JavaScriptCore/ChangeLog:7 > + Reviewed by NOBODY (OOPS!). > + Please add some explanatory text here, and maybe a link to the spec. > Source/JavaScriptCore/runtime/Executable.h:673 > + ).contains(parseMode()) || isClass(); I'm still in favor of renaming isClass() to isClassConstructor() > Source/JavaScriptCore/runtime/JSFunction.cpp:342 > + bool isES6OrStrictMode = function->jsExecutable()->isStrictMode() || function->jsExecutable()->isES6Function(); > + if (function->isHostOrBuiltinFunction() || !isES6OrStrictMode) Please change the error message below since this no longer just throws for strict mode. Also, a note on style, I think this condition would be easier to read like so: if (function->isHostOrbuiltinFunction() || (!function->isStrictMode() && !function->isES6Function())) return caller; ... throw error > Source/JavaScriptCore/runtime/JSFunction.cpp:357 > + FunctionExecutable* executable = thisObject->jsExecutable(); > + bool hasPrototype = executable->hasInitialPrototypeProperty(); Nit: I don't think these temporary variables add any value. Created attachment 290925 [details]
Patch
Comment on attachment 290767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290767&action=review I think I've addressed this (except for the `isClass() -> isClassConstructor()` difference, for the reason mentioned --- happy to submit a followup to do that if it's important, though) >> Source/JavaScriptCore/runtime/Executable.h:673 >> + ).contains(parseMode()) || isClass(); > > I'm still in favor of renaming isClass() to isClassConstructor() Apparently I could use `isClassConstructorFunction()` instead, if you'd prefer that. But: a class is its constructor in ECMAScript, so I'm not sure the distinction really adds much. I think I'd prefer to make this change in a separate bug where it's the main issue being addressed. >> Source/JavaScriptCore/runtime/JSFunction.cpp:342 >> + if (function->isHostOrBuiltinFunction() || !isES6OrStrictMode) > > Please change the error message below since this no longer just throws for strict mode. Also, a note on style, I think this condition would be easier to read like so: > if (function->isHostOrbuiltinFunction() || (!function->isStrictMode() && !function->isES6Function())) > return caller; > ... throw error Would you prefer something like "Function.caller cannot be accessed in this context"? I'll try that. >> Source/JavaScriptCore/runtime/JSFunction.cpp:357 >> + bool hasPrototype = executable->hasInitialPrototypeProperty(); > > Nit: I don't think these temporary variables add any value. Done Comment on attachment 290925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290925&action=review > Source/JavaScriptCore/runtime/Executable.h:665 > + bool isES6Function() const > + { > + // Per https://tc39.github.io/ecma262/#sec-forbidden-extensions, special function forms which are forbidden from having legacy > + // Annex B properties > + return SourceParseModeSet( > + SourceParseMode::ArrowFunctionMode, > + SourceParseMode::GeneratorBodyMode, > + SourceParseMode::GeneratorWrapperFunctionMode, > + SourceParseMode::MethodMode, > + SourceParseMode::GetterMode, > + SourceParseMode::SetterMode).contains(parseMode()) || isClassConstructorFunction(); > + } I see a few problems with this function and its name: (1) Getters and setters are not ES6 features, so it's surprising that getters and setters claim to be ES6 functions. (2) The comment in the function, which describes its behavior, does not say "returns true for ES6 functions" -- implying that the name is incorrect or at least incomplete. (3) All of our clients seem to check isStrictMode() || isES6Function(). That implies an impedance mismatch, in which our clients would prefer a different function that built in the whole test for "do I support .arguments and .caller". I think I would change this function to incorporate the isStrictMode() test and then rename it to "hasRestrictedFunctionProperties" or "hasCallerAndArugmentsProperties". What do you think? > Source/JavaScriptCore/runtime/Executable.h:666 > + bool hasInitialPrototypeProperty() const I would just call this "hasPrototypeProperty" to match "hasRestrictedFunctionProperties" or "hasCallerAndArugmentsProperties". > Source/JavaScriptCore/runtime/Executable.h:668 > + // Return `true` if is a Function with an initial "prototype" property No need for this comment: The function name already says it. > Source/JavaScriptCore/runtime/JSFunction.cpp:344 > + bool isES6OrStrictMode = function->jsExecutable()->isStrictMode() || function->jsExecutable()->isES6Function(); > + if (function->isHostOrBuiltinFunction() || !isES6OrStrictMode) > return JSValue::encode(caller); > - return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller used to retrieve strict caller"))); > + return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context"))); getOwnPropertySlot does this for .caller: if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function()) return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot); Under what conditions can we invoke the callerGetter for a strict mode or ES6 function? Comment on attachment 290925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290925&action=review >> Source/JavaScriptCore/runtime/Executable.h:665 >> + } > > I see a few problems with this function and its name: > > (1) Getters and setters are not ES6 features, so it's surprising that getters and setters claim to be ES6 functions. > > (2) The comment in the function, which describes its behavior, does not say "returns true for ES6 functions" -- implying that the name is incorrect or at least incomplete. > > (3) All of our clients seem to check isStrictMode() || isES6Function(). That implies an impedance mismatch, in which our clients would prefer a different function that built in the whole test for "do I support .arguments and .caller". > > I think I would change this function to incorporate the isStrictMode() test and then rename it to "hasRestrictedFunctionProperties" or "hasCallerAndArugmentsProperties". What do you think? "hasRestricted...Properties" is the inverse condition, but sounds reasonable. While it's true that getters/setters predate ES6, they became Methods in ES6, and this was found to be web compatible (since they are almost never interacted with as function), hence why I think the currentname is "ok". >> Source/JavaScriptCore/runtime/Executable.h:666 >> + bool hasInitialPrototypeProperty() const > > I would just call this "hasPrototypeProperty" to match "hasRestrictedFunctionProperties" or "hasCallerAndArugmentsProperties". SGTM >> Source/JavaScriptCore/runtime/JSFunction.cpp:344 >> + return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context"))); > > getOwnPropertySlot does this for .caller: > > if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function()) > return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot); > > Under what conditions can we invoke the callerGetter for a strict mode or ES6 function? The ones in the caller-property LayoutTest, I suppose. > PASS strictCaller(nonStrictCallee) threw exception TypeError: Function.caller cannot be accessed in this context. when the callee is an accessor (or a different ES6 function) you get the different error from callerGetter, I guess. Theres no real reason for the tdistinct errors afaik, thats just the way it was to begin with. Comment on attachment 290925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290925&action=review >>> Source/JavaScriptCore/runtime/JSFunction.cpp:344 >>> + return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context"))); >> >> getOwnPropertySlot does this for .caller: >> >> if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function()) >> return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot); >> >> Under what conditions can we invoke the callerGetter for a strict mode or ES6 function? > > The ones in the caller-property LayoutTest, I suppose. > > > PASS strictCaller(nonStrictCallee) threw exception TypeError: Function.caller cannot be accessed in this context. > > when the callee is an accessor (or a different ES6 function) you get the different error from callerGetter, I guess. Theres no real reason for the tdistinct errors afaik, thats just the way it was to begin with. For what it's worth, as far as I can tell, there is nothing in the spec requiring the behaviour here (preventing ES6 callers from using the caller property of a non-strict function). Seems to be a WebKit-ism. As an example: ``` function f() { return [...g()]; } function* g() { yield f.caller; } function y() { return f(); } y() // returns `[ y ]` in V8 and SpiderMonkey, but throws in JSC ``` Comment on attachment 290925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290925&action=review >>>> Source/JavaScriptCore/runtime/JSFunction.cpp:344 >>>> + return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context"))); >>> >>> getOwnPropertySlot does this for .caller: >>> >>> if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function()) >>> return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot); >>> >>> Under what conditions can we invoke the callerGetter for a strict mode or ES6 function? >> >> The ones in the caller-property LayoutTest, I suppose. >> >> > PASS strictCaller(nonStrictCallee) threw exception TypeError: Function.caller cannot be accessed in this context. >> >> when the callee is an accessor (or a different ES6 function) you get the different error from callerGetter, I guess. Theres no real reason for the tdistinct errors afaik, thats just the way it was to begin with. > > For what it's worth, as far as I can tell, there is nothing in the spec requiring the behaviour here (preventing ES6 callers from using the caller property of a non-strict function). Seems to be a WebKit-ism. > > As an example: > > ``` > function f() { return [...g()]; } > function* g() { yield f.caller; } > function y() { return f(); } > > y() // returns `[ y ]` in V8 and SpiderMonkey, but throws in JSC > ``` Er, sorry, that's a bad example... ``` var f = () => [...g()]; function* g() { yield f.caller; } function y() { return f(); } y() ``` is more accurate, and V8 also throws in this case --- but I'm fairly sure this isn't required by anything. I'll double check. Comment on attachment 290925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290925&action=review >>>>> Source/JavaScriptCore/runtime/JSFunction.cpp:344 >>>>> + return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context"))); >>>> >>>> getOwnPropertySlot does this for .caller: >>>> >>>> if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function()) >>>> return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot); >>>> >>>> Under what conditions can we invoke the callerGetter for a strict mode or ES6 function? >>> >>> The ones in the caller-property LayoutTest, I suppose. >>> >>> > PASS strictCaller(nonStrictCallee) threw exception TypeError: Function.caller cannot be accessed in this context. >>> >>> when the callee is an accessor (or a different ES6 function) you get the different error from callerGetter, I guess. Theres no real reason for the tdistinct errors afaik, thats just the way it was to begin with. >> >> For what it's worth, as far as I can tell, there is nothing in the spec requiring the behaviour here (preventing ES6 callers from using the caller property of a non-strict function). Seems to be a WebKit-ism. >> >> As an example: >> >> ``` >> function f() { return [...g()]; } >> function* g() { yield f.caller; } >> function y() { return f(); } >> >> y() // returns `[ y ]` in V8 and SpiderMonkey, but throws in JSC >> ``` > > Er, sorry, that's a bad example... > > ``` > var f = () => [...g()]; > function* g() { yield f.caller; } > function y() { return f(); } > > y() > ``` > > is more accurate, and V8 also throws in this case --- but I'm fairly sure this isn't required by anything. I'll double check. After discussing this with others a bit, I've decided to just revert the changes to callerGetter, which correctly follows the restrictions from annex B. Created attachment 291034 [details]
Patch
Comment on attachment 291034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291034&action=review some self-review / notes: > Source/JavaScriptCore/runtime/Executable.h:658 > + return !isStrictMode() && parseMode() == SourceParseMode::NormalFunctionMode && !isClassConstructorFunction(); Unfortunately, the builtin default constructor has a strange parseMode() (NormalFunctionMode), so the `!isClassConstructorFunction()` is also needed in addition to the parse mode check. > Source/JavaScriptCore/runtime/JSFunction.cpp:355 > + if (propertyName == vm.propertyNames->prototype && thisObject->jsExecutable()->hasPrototypeProperty() && !thisObject->jsExecutable()->isClassConstructorFunction()) { The `!thisObject->jsExecutable()->isClassConstructorFunction()` is needed because the bytecode for the ClassExprNodes emits a DefineOwnProperty op to add the prototype, which fails if the default prototype is added here. Maybe it's better to just omit the `|| isClass();` in `hasPrototypeProperty()` to keep this simpler. Thoughts? > Source/JavaScriptCore/runtime/JSFunction.cpp:381 > + TODO: fix whitespace before landing > Source/JavaScriptCore/runtime/JSFunction.cpp:449 > + if (propertyName == exec->propertyNames().arguments || propertyName == exec->propertyNames().caller) { cleanup: remove duped branching > Source/JavaScriptCore/runtime/JSFunction.cpp:476 > + if (propertyName == exec->propertyNames().caller || propertyName == exec->propertyNames().arguments) cleanup: simplify this a bit > Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:865 > + return throwVMTypeError(exec, scope, "'arguments', 'callee', and 'caller' cannot be accessed in this context."); Was asked to update the error message --- I've reverted the change to the error message in callerGetter(), which is still accurate (will only throw when the caller is a strict function). The more generic "property is not accessible" error is updated to be a bit more generic and hopefully not misleading about the language mode of the receiver. (In reply to comment #56) > Comment on attachment 290925 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=290925&action=review > > >>> Source/JavaScriptCore/runtime/JSFunction.cpp:344 > >>> + return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context"))); > >> > >> getOwnPropertySlot does this for .caller: > >> > >> if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function()) > >> return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot); > >> > >> Under what conditions can we invoke the callerGetter for a strict mode or ES6 function? > > > > The ones in the caller-property LayoutTest, I suppose. > > > > > PASS strictCaller(nonStrictCallee) threw exception TypeError: Function.caller cannot be accessed in this context. > > > > when the callee is an accessor (or a different ES6 function) you get the different error from callerGetter, I guess. Theres no real reason for the tdistinct errors afaik, thats just the way it was to begin with. > > For what it's worth, as far as I can tell, there is nothing in the spec > requiring the behaviour here (preventing ES6 callers from using the caller > property of a non-strict function). Seems to be a WebKit-ism. > > As an example: > > ``` > function f() { return [...g()]; } > function* g() { yield f.caller; } > function y() { return f(); } > > y() // returns `[ y ]` in V8 and SpiderMonkey, but throws in JSC > ``` Why is this a bad example? I wouldn't expect this to throw. Comment on attachment 291034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291034&action=review LGTM still, but I'll also give some time for Geoff to comment before I r+ >> Source/JavaScriptCore/runtime/Executable.h:658 >> + return !isStrictMode() && parseMode() == SourceParseMode::NormalFunctionMode && !isClassConstructorFunction(); > > Unfortunately, the builtin default constructor has a strange parseMode() (NormalFunctionMode), so the `!isClassConstructorFunction()` is also needed in addition to the parse mode check. what is the SourceParseMode for the constructor here: class Foo { constructor() { } } I'm guessing it's MethodMode. Why not just make the default constructors also have that SourceParseMode? (This could be done in a follow up if it seems valid to change it.) >> Source/JavaScriptCore/runtime/JSFunction.cpp:355 >> + if (propertyName == vm.propertyNames->prototype && thisObject->jsExecutable()->hasPrototypeProperty() && !thisObject->jsExecutable()->isClassConstructorFunction()) { > > The `!thisObject->jsExecutable()->isClassConstructorFunction()` is needed because the bytecode for the ClassExprNodes emits a DefineOwnProperty op to add the prototype, which fails if the default prototype is added here. > > Maybe it's better to just omit the `|| isClass();` in `hasPrototypeProperty()` to keep this simpler. Thoughts? This seems confusing. Why did this ever work before? > Source/JavaScriptCore/runtime/JSFunction.cpp:387 > + if (!thisObject->jsExecutable()->hasCallerAndArgumentsProperties()) Can you ever have caller but not arguments? Or are those property names always either both present or both not present? >> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:865 >> + return throwVMTypeError(exec, scope, "'arguments', 'callee', and 'caller' cannot be accessed in this context."); > > Was asked to update the error message --- I've reverted the change to the error message in callerGetter(), which is still accurate (will only throw when the caller is a strict function). The more generic "property is not accessible" error is updated to be a bit more generic and hopefully not misleading about the language mode of the receiver. This might be worth a bugzilla bug and a FIXME since we could come up with variants of this function which specialize on what "this context" actually is. Regarding the phrasing "this context", I wonder if it would be better if we enumerated the places where these properties are disallowed. Not sure if that'd bloat the error message too much or not. Comment on attachment 291034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291034&action=review >>> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:865 >>> + return throwVMTypeError(exec, scope, "'arguments', 'callee', and 'caller' cannot be accessed in this context."); >> >> Was asked to update the error message --- I've reverted the change to the error message in callerGetter(), which is still accurate (will only throw when the caller is a strict function). The more generic "property is not accessible" error is updated to be a bit more generic and hopefully not misleading about the language mode of the receiver. > > This might be worth a bugzilla bug and a FIXME since we could come up with variants of this function which specialize on what "this context" actually is. > Regarding the phrasing "this context", I wonder if it would be better if we enumerated the places where these properties are disallowed. Not sure if that'd bloat the error message too much or not. Actually, scratch that, I think there are too many functions to enumerate in an error message. I'm ok with "this context" Comment on attachment 290925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290925&action=review >>>>>>> Source/JavaScriptCore/runtime/JSFunction.cpp:344 >>>>>>> + return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context"))); >>>>>> >>>>>> getOwnPropertySlot does this for .caller: >>>>>> >>>>>> if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function()) >>>>>> return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot); >>>>>> >>>>>> Under what conditions can we invoke the callerGetter for a strict mode or ES6 function? >>>>> >>>>> The ones in the caller-property LayoutTest, I suppose. >>>>> >>>>> > PASS strictCaller(nonStrictCallee) threw exception TypeError: Function.caller cannot be accessed in this context. >>>>> >>>>> when the callee is an accessor (or a different ES6 function) you get the different error from callerGetter, I guess. Theres no real reason for the tdistinct errors afaik, thats just the way it was to begin with. >>>> >>>> For what it's worth, as far as I can tell, there is nothing in the spec requiring the behaviour here (preventing ES6 callers from using the caller property of a non-strict function). Seems to be a WebKit-ism. >>>> >>>> As an example: >>>> >>>> ``` >>>> function f() { return [...g()]; } >>>> function* g() { yield f.caller; } >>>> function y() { return f(); } >>>> >>>> y() // returns `[ y ]` in V8 and SpiderMonkey, but throws in JSC >>>> ``` >>> >>> Er, sorry, that's a bad example... >>> >>> ``` >>> var f = () => [...g()]; >>> function* g() { yield f.caller; } >>> function y() { return f(); } >>> >>> y() >>> ``` >>> >>> is more accurate, and V8 also throws in this case --- but I'm fairly sure this isn't required by anything. I'll double check. >> >> After discussing this with others a bit, I've decided to just revert the changes to callerGetter, which correctly follows the restrictions from annex B. > > Why is this a bad example? I wouldn't expect this to throw. They're bad examples because the idea was to show "WebKit throws when the caller is an ES6 function", but n'either example really demonstrates that. It does throw when the caller is strict (which y implicitly is when declared in the web console). I misread callerGetter and thought it was looking at the callee's strictness/ES6ness rather than the caller's, and thus wrote a nonsense example. Hence reverting the change relating to this. While the spec does not say anything about throwing here (could just return null instead), it'sunspecified and good enough, IMO. Created attachment 291109 [details]
also address nit in Executable.h
Comment on attachment 291109 [details] also address nit in Executable.h View in context: https://bugs.webkit.org/attachment.cgi?id=291109&action=review r=me leaving cq? since I have just one question > Source/JavaScriptCore/runtime/JSFunction.cpp:456 > + if (!thisObject->jsExecutable()->hasCallerAndArgumentsProperties()) { > + bool okay = thisObject->hasProperty(exec, propertyName); > + ASSERT_UNUSED(okay, okay); > + scope.release(); > + return Base::put(thisObject, exec, propertyName, value, slot); > + } I know this is touching code that was here before, but I'm not sure why this is correct. Can you elaborate on this? If we don't own a specially handled "caller"/"arguments" property, how can we guarant7ee hasProperty doesn't have side effects? Why not just reify the property here before calling Base::put()? Comment on attachment 291109 [details] also address nit in Executable.h View in context: https://bugs.webkit.org/attachment.cgi?id=291109&action=review >> Source/JavaScriptCore/runtime/JSFunction.cpp:456 >> + } > > I know this is touching code that was here before, but I'm not sure why this is correct. Can you elaborate on this? If we don't own a specially handled "caller"/"arguments" property, how can we guarant7ee hasProperty doesn't have side effects? Why not just reify the property here before calling Base::put()? I'm not sure what reifying here really entails (or what the `hasProperty` call accomplishes), because the `!thisObject->jsExecutable()->hasCallerAndArgumentsProperties()` indicates that these are not virtual/lazy properties. AFAIK all that's really needed is the Base::put() call, but I left the `hasProperty()` just in case there's something I'm missing. I think it's safe to remove it and not replace it with anything, though. (In reply to comment #67) > Comment on attachment 291109 [details] > also address nit in Executable.h > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291109&action=review > > >> Source/JavaScriptCore/runtime/JSFunction.cpp:456 > >> + } > > > > I know this is touching code that was here before, but I'm not sure why this is correct. Can you elaborate on this? If we don't own a specially handled "caller"/"arguments" property, how can we guarant7ee hasProperty doesn't have side effects? Why not just reify the property here before calling Base::put()? > > I'm not sure what reifying here really entails (or what the `hasProperty` > call accomplishes), because the > `!thisObject->jsExecutable()->hasCallerAndArgumentsProperties()` indicates > that these are not virtual/lazy properties. > > AFAIK all that's really needed is the Base::put() call, but I left the > `hasProperty()` just in case there's something I'm missing. I think it's > safe to remove it and not replace it with anything, though. Interesting. I'm never a big fan of having code in the tree where we don't know what it does. Let me try to figure out what's going on here. That said, I don't want this to hold up your patch since this was here before your patch. Can you just land a version of this patch with a FIXME linked to something saying "why is this code necessary" or something similar. Created attachment 291733 [details]
Patch
Comment on attachment 291109 [details] also address nit in Executable.h View in context: https://bugs.webkit.org/attachment.cgi?id=291109&action=review >>>> Source/JavaScriptCore/runtime/JSFunction.cpp:456 >>>> + } >>> >>> I know this is touching code that was here before, but I'm not sure why this is correct. Can you elaborate on this? If we don't own a specially handled "caller"/"arguments" property, how can we guarant7ee hasProperty doesn't have side effects? Why not just reify the property here before calling Base::put()? >> >> I'm not sure what reifying here really entails (or what the `hasProperty` call accomplishes), because the `!thisObject->jsExecutable()->hasCallerAndArgumentsProperties()` indicates that these are not virtual/lazy properties. >> >> AFAIK all that's really needed is the Base::put() call, but I left the `hasProperty()` just in case there's something I'm missing. I think it's safe to remove it and not replace it with anything, though. > > Interesting. I'm never a big fan of having code in the tree where we don't know what it does. Let me try to figure out what's going on here. That said, I don't want this to hold up your patch since this was here before your patch. Can you just land a version of this patch with a FIXME linked to something saying "why is this code necessary" or something similar. sgtm I've updated the comment, but I guess I can't CQ. I'd prefer to CQ over dcommit, though. Comment on attachment 291733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291733&action=review > Source/JavaScriptCore/runtime/JSFunction.cpp:453 > + // FIXME: Investigate if the `hasProperty()` call is even needed, as in the `!hasCallerAndArgumentsProperties()` case, > + // these properties are not lazy and should not need to be reified. I think we generally try to have FIXMEs point to bugzillas these days. (In reply to comment #71) > I've updated the comment, but I guess I can't CQ. I'd prefer to CQ over > dcommit, though. You're a committer so the commit queue should take your patches. Maybe the patch can't have a review? though. (In reply to comment #71) > I've updated the comment, but I guess I can't CQ. I'd prefer to CQ over > dcommit, though. Usually what I do when using the commit queue after somebody has already reviewed the patch: 1. I fill in the "Reviewed by ..." on each changelog 2. Upload to bugzilla and cq+ (In reply to comment #72) > Comment on attachment 291733 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291733&action=review > > > Source/JavaScriptCore/runtime/JSFunction.cpp:453 > > + // FIXME: Investigate if the `hasProperty()` call is even needed, as in the `!hasCallerAndArgumentsProperties()` case, > > + // these properties are not lazy and should not need to be reified. > > I think we generally try to have FIXMEs point to bugzillas these days. Agreed. Can we put a link next to this FIXME? Created attachment 291910 [details]
Patch
Comment on attachment 291733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291733&action=review >>> Source/JavaScriptCore/runtime/JSFunction.cpp:453 >>> + // these properties are not lazy and should not need to be reified. >> >> I think we generally try to have FIXMEs point to bugzillas these days. > > Agreed. Can we put a link next to this FIXME? bug link added Comment on attachment 291910 [details]
Patch
Did you mean to set r? again or did you mean to set cq?
just uploaded with webkit-patch and waited for the tree to get fixed up, since RenderThemeEfl.cpp was broken a while ago. I'll cq now Comment on attachment 291910 [details] Patch Clearing flags on attachment: 291910 Committed r207461: <http://trac.webkit.org/changeset/207461> All reviewed patches have been landed. Closing bug. |