Bug 162530 - [JSC] ES6 Method functions should not have prototype
Summary: [JSC] ES6 Method functions should not have prototype
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caitlin Potter (:caitp)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-24 09:53 PDT by Caitlin Potter (:caitp)
Modified: 2016-10-18 05:54 PDT (History)
7 users (show)

See Also:


Attachments
Do not add prototype to MethodDefinitions (7.52 KB, patch)
2016-09-24 12:01 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (1006.63 KB, application/zip)
2016-09-24 13:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.02 MB, application/zip)
2016-09-24 13:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.52 MB, application/zip)
2016-09-24 13:09 PDT, Build Bot
no flags Details
Fix LayoutTests (12.52 KB, patch)
2016-09-24 13:13 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (870.24 KB, application/zip)
2016-09-24 14:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.03 MB, application/zip)
2016-09-24 14:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (1.51 MB, application/zip)
2016-09-24 14:23 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2 (8.93 MB, application/zip)
2016-09-24 14:27 PDT, Build Bot
no flags Details
Fix LayoutTests (12.82 KB, patch)
2016-09-24 14:29 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Add extra es6-function-properties test cases (21.08 KB, patch)
2016-09-24 19:26 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (880.50 KB, application/zip)
2016-09-24 20:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.04 MB, application/zip)
2016-09-24 20:32 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (1.51 MB, application/zip)
2016-09-24 20:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2 (9.10 MB, application/zip)
2016-09-24 20:41 PDT, Build Bot
no flags Details
Fix accessors too, and fix that one LayoutTest, but didn't try any other ones (28.38 KB, patch)
2016-09-25 10:59 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Fix accessors too, and fix that one LayoutTest, but didn't try any other ones --- and rebase on Trunk (26.60 KB, patch)
2016-09-25 11:05 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (932.06 KB, application/zip)
2016-09-25 12:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.52 MB, application/zip)
2016-09-25 12:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 (13.60 MB, application/zip)
2016-09-25 12:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-yosemite (890.11 KB, application/zip)
2016-09-25 15:35 PDT, Build Bot
no flags Details
Fix LayoutTests (30.74 KB, patch)
2016-09-26 03:37 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (30.72 KB, patch)
2016-10-05 17:52 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (33.42 KB, patch)
2016-10-07 06:26 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (113.99 KB, patch)
2016-10-08 19:20 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
also address nit in Executable.h (113.95 KB, patch)
2016-10-10 08:54 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (114.19 KB, patch)
2016-10-15 21:14 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (114.41 KB, patch)
2016-10-17 19:08 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caitlin Potter (:caitp) 2016-09-24 09:53:06 PDT
ES6 object literal and class literal Methods, which are not GeneratorMethods and not "constructor" (in classes) incorrectly have a "prototype" property.

Example:

```
({ foo() {} }).foo.prototype // produces an Object with an own property "constructor" whose value is the "foo" function. Should be `undefined`
```

- Per https://tc39.github.io/ecma262/#sec-method-definitions-runtime-semantics-propertydefinitionevaluation, the "prototype" property should not be added.

This causes test262 failure for test262/test/language/expressions/object/method-definition/name-prototype-prop.js, and likely others
Comment 1 Caitlin Potter (:caitp) 2016-09-24 12:01:45 PDT
Created attachment 289749 [details]
Do not add prototype to MethodDefinitions
Comment 2 Build Bot 2016-09-24 13:04:09 PDT
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
Comment 3 Build Bot 2016-09-24 13:04:12 PDT
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 4 Build Bot 2016-09-24 13:05:32 PDT
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
Comment 5 Build Bot 2016-09-24 13:05:35 PDT
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 6 Build Bot 2016-09-24 13:08:58 PDT
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
Comment 7 Build Bot 2016-09-24 13:09:01 PDT
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
Comment 8 Caitlin Potter (:caitp) 2016-09-24 13:13:34 PDT
Created attachment 289755 [details]
Fix LayoutTests
Comment 9 Build Bot 2016-09-24 14:14:57 PDT
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
Comment 10 Build Bot 2016-09-24 14:15:00 PDT
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 11 Build Bot 2016-09-24 14:17:40 PDT
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
Comment 12 Build Bot 2016-09-24 14:17:43 PDT
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 13 Build Bot 2016-09-24 14:23:00 PDT
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
Comment 14 Build Bot 2016-09-24 14:23:03 PDT
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 15 Build Bot 2016-09-24 14:27:34 PDT
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
Comment 16 Build Bot 2016-09-24 14:27:37 PDT
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
Comment 17 Caitlin Potter (:caitp) 2016-09-24 14:29:31 PDT
Created attachment 289761 [details]
Fix LayoutTests
Comment 18 Saam Barati 2016-09-24 16:09:25 PDT
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 19 Caitlin Potter (:caitp) 2016-09-24 16:18:58 PDT
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)
Comment 20 Saam Barati 2016-09-24 16:29:39 PDT
(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.
Comment 21 Caitlin Potter (:caitp) 2016-09-24 19:26:25 PDT
Created attachment 289766 [details]
Add extra es6-function-properties test cases
Comment 22 Caitlin Potter (:caitp) 2016-09-24 19:27:44 PDT
(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 23 Build Bot 2016-09-24 20:28:00 PDT
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
Comment 24 Build Bot 2016-09-24 20:28:05 PDT
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 25 Build Bot 2016-09-24 20:31:57 PDT
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
Comment 26 Build Bot 2016-09-24 20:32:01 PDT
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 27 Build Bot 2016-09-24 20:34:29 PDT
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
Comment 28 Build Bot 2016-09-24 20:34:33 PDT
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 29 Build Bot 2016-09-24 20:41:20 PDT
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
Comment 30 Build Bot 2016-09-24 20:41:24 PDT
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
Comment 31 Caitlin Potter (:caitp) 2016-09-25 03:02:20 PDT
realized after takeout the generator method expectations were wrong :( sorry bout the spam
Comment 32 Caitlin Potter (:caitp) 2016-09-25 08:57:53 PDT
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.
Comment 33 Caitlin Potter (:caitp) 2016-09-25 10:59:11 PDT
Created attachment 289779 [details]
Fix accessors too, and fix that one LayoutTest, but didn't try any other ones
Comment 34 Caitlin Potter (:caitp) 2016-09-25 11:05:11 PDT
Created attachment 289780 [details]
Fix accessors too, and fix that one LayoutTest, but didn't try any other ones --- and rebase on Trunk
Comment 35 Build Bot 2016-09-25 12:09:26 PDT
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
Comment 36 Build Bot 2016-09-25 12:09:29 PDT
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 37 Build Bot 2016-09-25 12:21:37 PDT
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
Comment 38 Build Bot 2016-09-25 12:21:41 PDT
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 39 Build Bot 2016-09-25 12:26:56 PDT
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
Comment 40 Build Bot 2016-09-25 12:27:00 PDT
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 41 Build Bot 2016-09-25 15:35:18 PDT
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
Comment 42 Build Bot 2016-09-25 15:35:22 PDT
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
Comment 43 Caitlin Potter (:caitp) 2016-09-26 03:37:02 PDT
Created attachment 289815 [details]
Fix LayoutTests
Comment 44 Caitlin Potter (:caitp) 2016-09-27 02:18:37 PDT
Would it be frowned upon to request EditBugs permissions so that I can obsolete all of the no-longer-relevant EWS failures? :)
Comment 45 Saam Barati 2016-09-27 09:20:29 PDT
(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.
Comment 46 Caitlin Potter (:caitp) 2016-09-27 09:23:03 PDT
(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 :)
Comment 47 Saam Barati 2016-09-28 00:02:08 PDT
(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
Comment 48 Caitlin Potter (:caitp) 2016-10-05 11:42:49 PDT
(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.
Comment 49 Caitlin Potter (:caitp) 2016-10-05 17:47:05 PDT
Okay, cleaned up the bug a bit. It would be fantastic if I could get a look at this
Comment 50 Caitlin Potter (:caitp) 2016-10-05 17:52:29 PDT
Created attachment 290767 [details]
Patch
Comment 51 Saam Barati 2016-10-05 18:36:11 PDT
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.
Comment 52 Caitlin Potter (:caitp) 2016-10-07 06:26:07 PDT
Created attachment 290925 [details]
Patch
Comment 53 Caitlin Potter (:caitp) 2016-10-07 06:27:17 PDT
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 54 Geoffrey Garen 2016-10-07 12:02:26 PDT
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 55 Caitlin Potter (:caitp) 2016-10-07 12:30:50 PDT
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 56 Caitlin Potter (:caitp) 2016-10-07 13:08:37 PDT
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 57 Caitlin Potter (:caitp) 2016-10-07 13:11:28 PDT
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 58 Caitlin Potter (:caitp) 2016-10-07 14:38:20 PDT
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.
Comment 59 Caitlin Potter (:caitp) 2016-10-08 19:20:23 PDT
Created attachment 291034 [details]
Patch
Comment 60 Caitlin Potter (:caitp) 2016-10-08 19:36:05 PDT
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.
Comment 61 Saam Barati 2016-10-09 00:43:05 PDT
(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 62 Saam Barati 2016-10-09 00:46:04 PDT
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 63 Saam Barati 2016-10-09 00:47:14 PDT
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 64 Caitlin Potter (:caitp) 2016-10-09 04:30:26 PDT
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.
Comment 65 Caitlin Potter (:caitp) 2016-10-10 08:54:19 PDT
Created attachment 291109 [details]
also address nit in Executable.h
Comment 66 Saam Barati 2016-10-11 20:27:26 PDT
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 67 Caitlin Potter (:caitp) 2016-10-12 07:52:13 PDT
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.
Comment 68 Saam Barati 2016-10-15 13:50:28 PDT
(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.
Comment 69 Caitlin Potter (:caitp) 2016-10-15 21:14:28 PDT
Created attachment 291733 [details]
Patch
Comment 70 Caitlin Potter (:caitp) 2016-10-15 21:15:53 PDT
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
Comment 71 Caitlin Potter (:caitp) 2016-10-17 04:31:35 PDT
I've updated the comment, but I guess I can't CQ. I'd prefer to CQ over dcommit, though.
Comment 72 Keith Miller 2016-10-17 11:02:52 PDT
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.
Comment 73 Keith Miller 2016-10-17 11:04:51 PDT
(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.
Comment 74 Saam Barati 2016-10-17 17:56:02 PDT
(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+
Comment 75 Saam Barati 2016-10-17 17:56:22 PDT
(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?
Comment 76 Caitlin Potter (:caitp) 2016-10-17 19:08:09 PDT
Created attachment 291910 [details]
Patch
Comment 77 Caitlin Potter (:caitp) 2016-10-17 19:08:50 PDT
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 78 Saam Barati 2016-10-18 02:38:13 PDT
Comment on attachment 291910 [details]
Patch

Did you mean to set r? again or did you mean to set cq?
Comment 79 Caitlin Potter (:caitp) 2016-10-18 05:34:36 PDT
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 80 WebKit Commit Bot 2016-10-18 05:54:16 PDT
Comment on attachment 291910 [details]
Patch

Clearing flags on attachment: 291910

Committed r207461: <http://trac.webkit.org/changeset/207461>
Comment 81 WebKit Commit Bot 2016-10-18 05:54:24 PDT
All reviewed patches have been landed.  Closing bug.