Bug 203456 - Align JSGlobalObject::defineOwnProperty() with the spec and other runtimes
Summary: Align JSGlobalObject::defineOwnProperty() with the spec and other runtimes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
: 207310 220842 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-10-26 03:12 PDT by yaohouyou
Modified: 2021-03-14 15:21 PDT (History)
17 users (show)

See Also:


Attachments
Patch (6.70 KB, patch)
2020-02-05 16:51 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (6.41 KB, patch)
2020-02-06 13:28 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
WIP (9.79 KB, patch)
2021-02-02 07:30 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
WIP (25.11 KB, patch)
2021-02-05 14:08 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (42.11 KB, patch)
2021-02-11 12:13 PST, Alexey Shvayka
rmorisset: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yaohouyou 2019-10-26 03:12:10 PDT
Version: d940b47

Testcase:
var NISLFuzzingFunc = function() {
    var tmp = Object.defineProperty(this, "undefined", {
        configurable: true,
        get: function() {
            return false;
        }
    });
    print(tmp);
};
NISLFuzzingFunc();

Run steps:
WebKitBuild/Release/bin/jsc testcase.js

Output:
[object global]

Expected output:
Throw TypeError exception

Descriptions:
According to ECMAScript-262, JSC should throw TypeError exception when run the testcase above. So I suspect it is a bug.
The references of ECMAScript-262 are as follows:
http://www.ecma-international.org/ecma-262/6.0/index.html#sec-undefined
http://www.ecma-international.org/ecma-262/6.0/index.html#sec-definepropertyorthrow
http://www.ecma-international.org/ecma-262/6.0/index.html#sec-validateandapplypropertydescriptor
Comment 1 Radar WebKit Bug Importer 2019-10-27 18:59:35 PDT
<rdar://problem/56658015>
Comment 2 Robin Morisset 2020-02-05 16:45:56 PST
*** Bug 207310 has been marked as a duplicate of this bug. ***
Comment 3 Robin Morisset 2020-02-05 16:51:07 PST
Created attachment 389902 [details]
Patch
Comment 4 Robin Morisset 2020-02-06 13:28:49 PST
Created attachment 389985 [details]
Patch

From EWS it appears that defineGetter and defineSetter are less dead than I thought. This patch is like the previous one except it does not remove them from JSGlobalObject.h.
Comment 5 Yusuke Suzuki 2020-03-10 01:37:13 PDT
Comment on attachment 389985 [details]
Patch

Looks like JSC tests etc. are failing. Can you review it?
Comment 6 Yusuke Suzuki 2020-03-10 13:58:34 PDT
/review/revise/s
Comment 7 Alexey Shvayka 2021-02-02 07:30:35 PST
Created attachment 418998 [details]
WIP
Comment 8 Alexey Shvayka 2021-02-02 07:39:27 PST
*** Bug 220842 has been marked as a duplicate of this bug. ***
Comment 9 Yusuke Suzuki 2021-02-02 10:45:04 PST
Dupe bug 220842, and change assignee to Alexey.
Comment 10 Alexey Shvayka 2021-02-05 14:08:16 PST
Created attachment 419461 [details]
WIP
Comment 11 Alexey Shvayka 2021-02-11 12:13:40 PST
Created attachment 420024 [details]
Patch
Comment 12 EWS Watchlist 2021-02-11 12:14:27 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 13 Robin Morisset 2021-02-17 13:11:11 PST
Comment on attachment 420024 [details]
Patch

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

This looks good to me overall, thanks for this patch!
r=me

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1345
> +    loadp CodeBlock[cfr], scratch

It is probably not very important, but the loads of CodeBlock[cfr] and then CodeBlock::m_globalObject are repeated here and in varInjectionCheck, they could probably be factored out of these macros.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:-1478
> -    slot.disallowVMEntry.reset();

We lose this slot.disallowVMEntry.reset() call. I don't understand why it is no longer necessary.
Comment 14 Alexey Shvayka 2021-03-11 15:47:04 PST
(In reply to Robin Morisset from comment #13)
> This looks good to me overall, thanks for this patch!
> r=me

Thank you for reviewing & investigating the stress test failure.

> It is probably not very important, but the loads of CodeBlock[cfr] and then
> CodeBlock::m_globalObject are repeated here and in varInjectionCheck, they
> could probably be factored out of these macros.

Yeah, it would be nice to pass a register with global object.

However, this would require a) changing signature of 32-bit varInjectionCheck() to accept a register and b) extracting varInjectionCheckWithGlobalObject() / varReadOnlyCheckWithGlobalObject() macros because double load happens only in .pGlobalVarWithVarInjectionChecks branch, which itself is rare.

Landing with FIXME comment for now.

> We lose this slot.disallowVMEntry.reset() call. I don't understand why it is
> no longer necessary.

`slot.disallowVMEntry.reset()` was necessary for VMInquiry PropertySlot (please see its constructor), which was removed in favor of using different symbolTableGet().
Comment 15 Alexey Shvayka 2021-03-11 16:08:14 PST
Committed r274308 (235202@main): <https://commits.webkit.org/235202@main>
Comment 16 WebKit Commit Bot 2021-03-12 12:48:11 PST
Re-opened since this is blocked by bug 223134
Comment 17 Amir Mark Jr 2021-03-12 12:51:53 PST
This is reverted because it may have caused an assertion failure with these three tests:

stress/global-object-define-own-property.js
stress/global-object-define-own-property-put-to-scope.js
stress/eval-func-decl-in-frozen-global.js
Comment 18 Alexey Shvayka 2021-03-14 15:21:38 PDT
(In reply to Amir Mark Jr from comment #17)
> This is reverted because it may have caused an assertion failure with these
> three tests:
> 
> stress/global-object-define-own-property.js
> stress/global-object-define-own-property-put-to-scope.js
> stress/eval-func-decl-in-frozen-global.js

r274406 fixed the failed assertions.
Thank you for triaging!