Bug 208708 - @putByValDirect does not perform [[DefineOwnProperty]] correctly
Summary: @putByValDirect does not perform [[DefineOwnProperty]] correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Minor
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks: 163446
  Show dependency treegraph
 
Reported: 2020-03-06 05:49 PST by Alexey Shvayka
Modified: 2020-03-09 16:18 PDT (History)
10 users (show)

See Also:


Attachments
Patch (12.33 KB, patch)
2020-03-06 06:01 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (12.03 KB, patch)
2020-03-08 17:05 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (12.24 KB, patch)
2020-03-09 12:03 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2020-03-06 05:49:13 PST
CreateDataPropertyOrThrow (https://tc39.es/ecma262/#sec-createdatapropertyorthrow), called on property with descriptor:

1. {writable: false, configurable: true}
Expected: results in {value, writable: true, enumerable: true, configurable: true}
Actual: TypeError is thrown (seems like [[Set]] is performed instead)
ECMA262: https://tc39.es/ecma262/#sec-validateandapplypropertydescriptor (step 9.a)
Test262: https://test262.report/browse/built-ins/Array/of/does-not-use-set-for-indices.js

2. {writable: true, configurable: false} or {set: function() {}, configurable: false}
Expected: should throw TypeError
Actual: results in {value, writable: true, enumerable: true, configurable: true} (seems like descriptor validation is skipped)
ECMA262: https://tc39.es/ecma262/#sec-validateandapplypropertydescriptor (step 4.a)
Test262:
  https://test262.report/browse/built-ins/Array/prototype/filter/target-array-with-non-configurable-property.js
  https://test262.report/browse/built-ins/Array/prototype/map/target-array-with-non-configurable-property.js
Comment 1 Alexey Shvayka 2020-03-06 06:01:10 PST
Created attachment 392713 [details]
Patch
Comment 2 Yusuke Suzuki 2020-03-08 01:49:40 PST
Can you measure JetStream2 score (by 6 times runs) to see the effect of this patch?
Comment 3 Alexey Shvayka 2020-03-08 17:01:35 PDT
(In reply to Yusuke Suzuki from comment #2)
> Can you measure JetStream2 score (by 6 times runs) to see the effect of this patch?

Geometric mean of 6 hot JetStream2 runs (60°C CPU temp, MacBookAir8,1):

trunk: 109.810
patch: 110.567
Comment 4 Alexey Shvayka 2020-03-08 17:05:30 PDT
Created attachment 393001 [details]
Patch

Add note on JetStream2 results.
Comment 5 Yusuke Suzuki 2020-03-09 00:11:04 PDT
Comment on attachment 393001 [details]
Patch

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

r=me with ChangeLog change.

> Source/JavaScriptCore/ChangeLog:10
> +        This change fixes @putByValDirect to perform [[DefineOwnProperty]] per spec [1] while preserving
> +        existing behavior for Arguments exotic objects (thus the checks order in canDoFastPutDirectIndex),
> +        aligning JSC with V8 and SpiderMonkey.

Can you describe what is changed in detail?
Comment 6 Alexey Shvayka 2020-03-09 12:03:10 PDT
Created attachment 393060 [details]
Patch

Set reviewer and make ChangeLog more detailed.
Comment 7 WebKit Commit Bot 2020-03-09 16:17:47 PDT
Comment on attachment 393060 [details]
Patch

Clearing flags on attachment: 393060

Committed r258170: <https://trac.webkit.org/changeset/258170>
Comment 8 WebKit Commit Bot 2020-03-09 16:17:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2020-03-09 16:18:18 PDT
<rdar://problem/60248529>