Summary: | Align JSGlobalObject::defineOwnProperty() with the spec and other runtimes | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | yaohouyou | ||||||||||||
Component: | JavaScriptCore | Assignee: | Alexey Shvayka <ashvayka> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | amir_mark, ashvayka, cdumez, clopez, commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, nisl_grammarly1, rmorisset, saam, tzagallo, webkit-bug-importer, youennf, ysuzuki | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
See Also: |
https://github.com/web-platform-tests/wpt/pull/28037 https://bugs.webkit.org/show_bug.cgi?id=223134 |
||||||||||||||
Attachments: |
|
Description
yaohouyou
2019-10-26 03:12:10 PDT
*** Bug 207310 has been marked as a duplicate of this bug. *** Created attachment 389902 [details]
Patch
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 on attachment 389985 [details]
Patch
Looks like JSC tests etc. are failing. Can you review it?
/review/revise/s Created attachment 418998 [details]
WIP
*** Bug 220842 has been marked as a duplicate of this bug. *** Dupe bug 220842, and change assignee to Alexey. Created attachment 419461 [details]
WIP
Created attachment 420024 [details]
Patch
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 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. (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(). Committed r274308 (235202@main): <https://commits.webkit.org/235202@main> Re-opened since this is blocked by bug 223134 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 (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! |