Bug 183523 – ClassExprNode evaluation of computed property names differs observably from spec and other implementations

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caitlin Potter (:caitp)]

https://jsfiddle.net/kposmd21/ is an example reproduction.

WebKit produces:

```
name(2)
name(1)
name(3)
name1 called
name2 called
name3 called
```

while Chrome and FireFox produce:

```
name(1)
name(2)
name(3)
name1 called
name2 called
name3 called
```

----

The spec indicates that Chrome and FF's behaviour is correct here.

This becomes even more potentially weird with the introduction of class fields, in https://bugs.webkit.org/show_bug.cgi?id=174212 --- class elements should be evaluated in order, regardless of whether or not they are methods or class fields, and regardless of whether or not they are static or belong on the instance.

[Caitlin Potter (:caitp)]

Created attachment 335609 [details]
Patch

[Keith Miller]

Comment on attachment 335609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335609&action=review

r=me with some nits. 

Also, does this fix any test262 tests? I'm cool with doing some gardening after this patch lands too.

> Source/JavaScriptCore/parser/Nodes.h:728
> +        unsigned m_classElementTag: 2;

Can you add:

static_assert(1 << 2 > ClassElementTag::LastTag, "ClassElementTag shouldn't use more than two bits"); 

You'll also need to add a LastTag value to the ClassElementTag enum too.

> Source/JavaScriptCore/parser/Nodes.h:742
> +        RegisterID* emitBytecode(BytecodeGenerator& generator, RegisterID* dst = 0) override

Nit: This should be: RegisterID* dst = nullptr

[EWS Watchlist]

Comment on attachment 335609 [details]
Patch

Attachment 335609 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6918355

New failing tests:
stress/non-constructors.js.dfg-maximal-flush-validate-no-cjit
stress/non-constructors.js.ftl-no-cjit-small-pool
stress/non-constructors.js.dfg-eager
stress/non-constructors.js.ftl-eager-no-cjit-b3o1
stress/non-constructors.js.default
stress/non-constructors.js.ftl-eager
stress/non-constructors.js.no-cjit-validate-phases
stress/non-constructors.js.no-ftl
stress/non-constructors.js.no-cjit-collect-continuously
stress/non-constructors.js.ftl-no-cjit-no-inline-validate
stress/non-constructors.js.dfg-eager-no-cjit-validate
stress/non-constructors.js.ftl-no-cjit-b3o1
stress/non-constructors.js.no-llint
stress/non-constructors.js.ftl-no-cjit-validate-sampling-profiler
stress/non-constructors.js.ftl-eager-no-cjit
stress/non-constructors.js.ftl-no-cjit-no-put-stack-validate

[Caitlin Potter (:caitp)]

(In reply to Keith Miller from comment #2)
> Comment on attachment 335609 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335609&action=review
> 
> r=me with some nits. 
> 
> Also, does this fix any test262 tests? I'm cool with doing some gardening
> after this patch lands too.

After fixing the regression in JSTests/stress/non-constructors.js, I'm seeing the following test262 failures. I've also run the command on r229534 with the same failures. No test262 coverage affected by the change (but I intend to submit a new test to get some coverage for it).

```
run-jsc-stress-tests JSTests/test262.yaml 
Using the following jsc path: /Users/caitp/git/WebKit/WebKitBuild/Release/jsc


test262.yaml/test262/test/intl402/Collator/ignore-invalid-unicode-ext-values.js.default-strict: Exception: Test262Error: Locale en is affected by key co; value standard. Expected SameValue(«en», «en-CA») to be true
test262.yaml/test262/test/intl402/Collator/ignore-invalid-unicode-ext-values.js.default: Exception: Test262Error: Locale en is affected by key co; value standard. Expected SameValue(«en», «en-CA») to be true
test262.yaml/test262/test/intl402/Collator/ignore-invalid-unicode-ext-values.js.default-strict: ERROR: Unexpected exit code: 3
test262.yaml/test262/test/intl402/Collator/ignore-invalid-unicode-ext-values.js.default: ERROR: Unexpected exit code: 3
test262.yaml/test262/test/language/statements/for-await-of/async-func-decl-dstr-obj-id-put-unresolvable-no-strict.js.default: Error: ReferenceError: Can't find variable: iterCount
test262.yaml/test262/test/language/statements/for-await-of/async-func-decl-dstr-obj-prop-put-unresolvable-no-strict.js.default: Error: ReferenceError: Can't find variable: iterCount
test262.yaml/test262/test/language/statements/for-await-of/async-gen-decl-dstr-array-elem-iter-rtrn-close-null.js.default: Error: Promise incorrectly fulfilled.
test262.yaml/test262/test/language/statements/for-await-of/async-gen-decl-dstr-array-elem-iter-rtrn-close-null.js.default-strict: Error: Promise incorrectly fulfilled.
test262.yaml/test262/test/language/statements/for-await-of/async-gen-decl-dstr-obj-id-put-unresolvable-no-strict.js.default: Error: ReferenceError: Can't find variable: iterCount
test262.yaml/test262/test/language/statements/for-await-of/async-gen-decl-dstr-obj-prop-put-unresolvable-no-strict.js.default: Error: ReferenceError: Can't find variable: iterCount
56378/56378 (failed 2)
```

[Caitlin Potter (:caitp)]

Created attachment 335656 [details]
Patch

fix JSTests/stress/non-constructors.js regression

[Caitlin Potter (:caitp)]

Created attachment 335717 [details]
Patch

rebased for landing

[EWS Watchlist]

Comment on attachment 335717 [details]
Patch

Rejecting attachment 335717 [details] from review queue.

[e-mail] does not have reviewer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Caitlin Potter (:caitp)]

(In reply to Build Bot from comment #7)
> Comment on attachment 335717 [details]
> Patch
> 
> Rejecting attachment 335717 [details] from review queue.
> 
> [e-mail] does not have reviewer permissions according to
> https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/
> contributors.json.
> 
> - If you do not have reviewer rights please read
> http://webkit.org/coding/contributing.html for instructions on how to use
> bugzilla flags.
> 
> - If you have reviewer rights please correct the error in
> Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to
> the file (no review needed).  The commit-queue restarts itself every 2
> hours.  After restart the commit-queue will correctly respect your reviewer
> rights.

It seems like very time I get back to work on a WebKit patch, I forget the bugzilla workflow entirely D': Shouldn't flipping the CQ+ bit have done the job here, or do I need to manually update the changelog OOPS or something?

[Caitlin Potter (:caitp)]

Created attachment 335788 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 335788 [details]
Patch for landing

Clearing flags on attachment: 335788

Committed r229608: <https://trac.webkit.org/changeset/229608>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38470271>