Bug 231202

Summary: [JSC] JSPropertyNameEnumerator should not have cached prototype chain since empty JSPropertyNameEnumerator is shared
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: c.jaentsch, eduard.comanici, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 231419    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch keith_miller: review+

Yusuke Suzuki
Reported 2021-10-04 17:58:14 PDT
[JSC] JSPropertyNameEnumerator should not have cached prototype chain since empty JSPropertyNameEnumerator is shared
Attachments
Patch (17.98 KB, patch)
2021-10-04 18:03 PDT, Yusuke Suzuki 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (30.67 KB, patch)
2021-10-04 18:42 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (30.67 KB, patch)
2021-10-04 18:48 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (30.95 KB, patch)
2021-10-04 18:52 PDT, Yusuke Suzuki 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (34.10 KB, patch)
2021-10-04 19:19 PDT, Yusuke Suzuki 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (34.11 KB, patch)
2021-10-04 19:30 PDT, Yusuke Suzuki 		keith_miller: review+
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2021-10-04 18:03:48 PDT
Created attachment 440132 [details] Patch WIP
Yusuke Suzuki
Comment 2 2021-10-04 18:03:51 PDT
<rdar://problem/83815122>
Yusuke Suzuki
Comment 3 2021-10-04 18:42:34 PDT
Created attachment 440140 [details] Patch WIP
Yusuke Suzuki
Comment 4 2021-10-04 18:48:28 PDT
Created attachment 440141 [details] Patch WIP
Yusuke Suzuki
Comment 5 2021-10-04 18:52:17 PDT
Created attachment 440142 [details] Patch WIP
Yusuke Suzuki
Comment 6 2021-10-04 19:19:52 PDT
Created attachment 440145 [details] Patch
Yusuke Suzuki
Comment 7 2021-10-04 19:30:32 PDT
Created attachment 440147 [details] Patch
Keith Miller
Comment 8 2021-10-05 10:31:36 PDT
Comment on attachment 440147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440147&action=review r=me with some nits > Source/JavaScriptCore/ChangeLog:11 > + invariant was broken since we now have shared empty JSPropertyNameEnumerator, which can Nit: shared empty => shared empty sentinel. Might be a bit clearer. > Source/JavaScriptCore/ChangeLog:27 > + While reviewing the code, we also found that watchpoint-based validation didn't care PolyProto. Nit: care => care about? > Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:107 > + // JSPropertyNameEnumerator is immutable data structure, which allows VM to cache empty one. Typo: cache empty one => cache the empty one. > Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:131 > + if (!(enumeratorAndFlag & StructureRareData::cachedPropertyNameEnumeratorIsValidatedViaTraversingFlag)) Nit: Maybe use OptionSet I personally prefer that but we use this idiom everywhere so idc too much.
Yusuke Suzuki
Comment 9 2021-10-05 10:48:55 PDT
Comment on attachment 440147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440147&action=review Thanks! >> Source/JavaScriptCore/ChangeLog:11 >> + invariant was broken since we now have shared empty JSPropertyNameEnumerator, which can > > Nit: shared empty => shared empty sentinel. Might be a bit clearer. Fixed. >> Source/JavaScriptCore/ChangeLog:27 >> + While reviewing the code, we also found that watchpoint-based validation didn't care PolyProto. > > Nit: care => care about? Fixed. >> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:107 >> + // JSPropertyNameEnumerator is immutable data structure, which allows VM to cache empty one. > > Typo: cache empty one => cache the empty one. Fixed. >> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:131 >> + if (!(enumeratorAndFlag & StructureRareData::cachedPropertyNameEnumeratorIsValidatedViaTraversingFlag)) > > Nit: Maybe use OptionSet I personally prefer that but we use this idiom everywhere so idc too much. Because this is uintptr_t including enumerator pointer itself, we cannot use OptionSet.
Yusuke Suzuki
Comment 10 2021-10-05 10:50:00 PDT
Committed r283556 (242521@main): <https://commits.webkit.org/242521@main>
Justin Michaud
Comment 11 2021-10-11 16:27:23 PDT
*** Bug 231419 has been marked as a duplicate of this bug. ***
Yusuke Suzuki
Comment 12 2022-07-12 21:08:40 PDT
*** Bug 231572 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.