OS#14101048: Fixes #249: built-in functions should not have caller/arguments (and related fixes) #4651

dilijev · 2018-02-07T20:44:38Z

Replaces #4639

Several tests and test baselines needed to be updated to reflect the change in behavior. This change improves caller/arguments behavior without regressing any previous Chakra behavior. Chakra still differs from the spec and other engines on some aspects of caller/arguments.

See: https://tc39.github.io/ecma262/#sec-forbidden-extensions

Fixes #249
Fixes OS#14101048

Test262 results strictly improved.

Using test262 commit tc39/test262@03da228:

Previously 44130 passed, now 44138 passed out of 57102. (Duplicates below are strict and non-strict versions of the tests.)

+PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js
+PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js
+PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js
+PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js
+PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js
+PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js
+PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js
+PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js

boingoing · 2018-02-07T22:06:41Z

lib/Runtime/Library/JavascriptFunction.cpp

            this->IsGeneratorFunction() ||
            this->IsBoundFunction() ||
-            this->IsStrictMode()
+            this->IsStrictMode() ||


Looks like this should also check to see if the function is a method, right? Seems like https://tc39.github.io/ecma262/#sec-forbidden-extensions implies this. This is a little complicated, though, because FunctionInfo doesn't have an IsMethod helper. To plumb the flag, you could pull it out of PnFnc::IsMethod in ByteCodeGenerator::StartBindFunction.

I think this->functionInfo->IsClassMethod() up at the top of this clause captures that.

I don't think that's true since kFunctionIsClassMember and kFunctionIsMethod are different flags on PnFnc.

I see. In any case the behavior seems to be correct (per comparison other engines on test/es6/ES6RestrictedProperties.js). Will look into this in the future.

In reply to: 166775893 [](ancestors = 166775893)

Oh I thought I saw a comment in one of the test files that said object-literal methods had different behavior on spidermonkey / v8.

boingoing

Looks good. 👍
I'd like to address handling methods correctly as well but that can be handled in a different issue as far as I'm concerned.

dilijev · 2018-02-07T22:11:40Z

lib/Runtime/Library/JavascriptFunction.cpp

            this->IsGeneratorFunction() ||
            this->IsBoundFunction() ||
-            this->IsStrictMode()
+            this->IsStrictMode() ||


I think this->functionInfo->IsClassMethod() up at the top of this clause captures that.

dilijev · 2018-02-07T22:15:20Z

lib/Runtime/Library/RuntimeFunction.cpp


+    bool RuntimeFunction::Is(Var func)
+    {
+        return JavascriptFunction::Is(func) && !JavascriptFunction::UnsafeFromVar(func)->GetFunctionInfo()->HasBody();


@curtisman does this look right?

For reference, ScriptFunction is defined similarly (without negating HasBody()):

bool ScriptFunction::Is(Var func) { return JavascriptFunction::Is(func) && JavascriptFunction::UnsafeFromVar(func)->GetFunctionInfo()->HasBody(); }

dilijev · 2018-02-07T22:38:39Z

Updated the description with test262 results. /cc @bterlson

dilijev · 2018-02-08T00:35:49Z

test/es6/ES6RestrictedProperties.js

            var p2 = Object.getOwnPropertyDescriptor(obj, 'arguments');
            assert.isFalse(p2.enumerable, "Function.prototype function has 'arguments' own property which is not enumerable");
-            assert.isFalse(p2.configurable, "Function.prototype function has 'arguments' own property which is not configurable");
+            assert.isTrue(p2.configurable, "Function.prototype function has 'arguments' own property which is is configurable");


is [](start = 110, length = 3)

whoops, typo

dilijev · 2018-02-08T00:58:34Z

See: https://tc39.github.io/ecma262/#sec-forbidden-extensions

…e caller/arguments (and related fixes) See: https://tc39.github.io/ecma262/#sec-forbidden-extensions Fixes chakra-core#249 Fixes OS#14101048

…er/arguments

dilijev · 2018-02-08T01:28:40Z

@boingoing

Oh I thought I saw a comment in one of the test files that said object-literal methods had different behavior on spidermonkey / v8.

Yes, that's right. I didn't realize we would call those object member function "methods". Getting that check right would fix that divergent behavior. But I think that can be outside the scope of this fix (which fixes the bug, no regressions).

curtisman

akroshg

…ould not have caller/arguments (and related fixes) Merge pull request #4651 from dilijev:func-restricted-props Replaces #4639 Several tests and test baselines needed to be updated to reflect the change in behavior. This change improves caller/arguments behavior without regressing any previous Chakra behavior. Chakra still differs from the spec and other engines on some aspects of caller/arguments. See: https://tc39.github.io/ecma262/#sec-forbidden-extensions Fixes #249 Fixes OS#14101048 --- Test262 results strictly improved. Using test262 commit tc39/test262@03da228: Previously 44130 passed, now 44138 passed out of 57102. (Duplicates below are strict and non-strict versions of the tests.) ``` +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js ```

…nctions should not have caller/arguments (and related fixes) Merge pull request #4651 from dilijev:func-restricted-props Replaces #4639 Several tests and test baselines needed to be updated to reflect the change in behavior. This change improves caller/arguments behavior without regressing any previous Chakra behavior. Chakra still differs from the spec and other engines on some aspects of caller/arguments. See: https://tc39.github.io/ecma262/#sec-forbidden-extensions Fixes #249 Fixes OS#14101048 --- Test262 results strictly improved. Using test262 commit tc39/test262@03da228: Previously 44130 passed, now 44138 passed out of 57102. (Duplicates below are strict and non-strict versions of the tests.) ``` +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js ```

… built-in functions should not have caller/arguments (and related fixes) Merge pull request #4651 from dilijev:func-restricted-props Replaces #4639 Several tests and test baselines needed to be updated to reflect the change in behavior. This change improves caller/arguments behavior without regressing any previous Chakra behavior. Chakra still differs from the spec and other engines on some aspects of caller/arguments. See: https://tc39.github.io/ecma262/#sec-forbidden-extensions Fixes #249 Fixes OS#14101048 --- Test262 results strictly improved. Using test262 commit tc39/test262@03da228: Previously 44130 passed, now 44138 passed out of 57102. (Duplicates below are strict and non-strict versions of the tests.) ``` +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js ```

dilijev changed the title ~~OS#14101048: gh-249: built-in functions should not have caller/arguments (and related fixes)~~ OS#14101048: Fixes #249: built-in functions should not have caller/arguments (and related fixes) Feb 7, 2018

boingoing reviewed Feb 7, 2018

View reviewed changes

boingoing approved these changes Feb 7, 2018

View reviewed changes

dilijev mentioned this pull request Feb 7, 2018

OS#14101048: For all built-in functions, func.hasOwnProperty('caller'|'arguments') === false #4639

Closed

dilijev requested a review from curtisman February 7, 2018 22:18

dilijev commented Feb 7, 2018

View reviewed changes

dilijev commented Feb 8, 2018

View reviewed changes

dilijev added 4 commits February 7, 2018 17:10

OS#14101048: Fixes chakra-core#249: built-in functions should not hav…

22c5c27

…e caller/arguments (and related fixes) See: https://tc39.github.io/ecma262/#sec-forbidden-extensions Fixes chakra-core#249 Fixes OS#14101048

Add test for chakra-core#249: built-in functions should not have call…

e86676c

…er/arguments

Update out-of-date tests.

40b757d

Update debugger baselines.

6a2e42d

dilijev force-pushed the func-restricted-props branch from b40e157 to 6a2e42d Compare February 8, 2018 01:13

dilijev changed the base branch from master to release/1.8 February 8, 2018 01:22

dilijev mentioned this pull request Feb 8, 2018

Object member functions handle caller/arguments incorrectly. #4655

Closed

curtisman approved these changes Feb 8, 2018

View reviewed changes

akroshg approved these changes Feb 8, 2018

View reviewed changes

chakrabot merged commit 6a2e42d into chakra-core:release/1.8 Feb 8, 2018

dilijev deleted the func-restricted-props branch February 8, 2018 23:14

rhuanjl mentioned this pull request Feb 13, 2018

Function.prototype.caller/arguments should be configurable #248

Closed

OS#14101048: Fixes #249: built-in functions should not have caller/arguments (and related fixes) #4651

OS#14101048: Fixes #249: built-in functions should not have caller/arguments (and related fixes) #4651

Uh oh!

Conversation

dilijev commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boingoing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilijev commented Feb 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilijev commented Feb 8, 2018

Uh oh!

dilijev commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

curtisman left a comment

Choose a reason for hiding this comment

Uh oh!

akroshg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dilijev commented Feb 7, 2018 •

edited

Loading

dilijev commented Feb 8, 2018 •

edited

Loading