Bug 203456 – Align JSGlobalObject::defineOwnProperty() with the spec and other runtimes

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

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

[Radar WebKit Bug Importer]

<rdar://problem/56658015>

[Robin Morisset]

*** Bug 207310 has been marked as a duplicate of this bug. ***

[Robin Morisset]

Created attachment 389902 [details]
Patch

[Robin Morisset]

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.

[Yusuke Suzuki]

Comment on attachment 389985 [details]
Patch

Looks like JSC tests etc. are failing. Can you review it?

[Yusuke Suzuki]

/review/revise/s

[Alexey Shvayka]

Created attachment 418998 [details]
WIP

[Alexey Shvayka]

*** Bug 220842 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Dupe bug 220842, and change assignee to Alexey.

[Alexey Shvayka]

Created attachment 419461 [details]
WIP

[Alexey Shvayka]

Created attachment 420024 [details]
Patch

[EWS Watchlist]

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

[Robin Morisset]

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.

[Alexey Shvayka]

(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().

[Alexey Shvayka]

Committed r274308 (235202@main): <https://commits.webkit.org/235202@main>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 223134

[Amir Mark Jr]

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

[Alexey Shvayka]

(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!